3 pontos por GN⁺ 2023-10-25 | 1 comentários | Compartilhar no WhatsApp
  • Mudanças pequenas podem ser analisadas suficientemente bem tanto com unified diff quanto com split diff, mas, em mudanças grandes e complexas, uma revisão baseada apenas na leitura do diff encontra limites
  • Mudanças grandes devem ser exploradas e verificadas com base no estado completo da base de código em um determinado momento, como se você estivesse modificando o código real, e não apenas olhando o histórico de alterações
  • A tela ideal de revisão deveria mostrar lado a lado o código atualmente no disco e o unified diff daquela área, permitindo acompanhar ao mesmo tempo o contexto do código e o conteúdo alterado
  • Como o suporte das ferramentas existentes é insuficiente, usa-se um fluxo de trabalho de baixa tecnologia: fazer checkout do PR localmente, remover os commits e deixar apenas as alterações
  • Com essa abordagem, é possível navegar pelos arquivos alterados e marcar hunks revisados, mas a posição atual no editor e o diff não são sincronizados automaticamente, então ainda sobra manipulação manual

Revisar o estado atual do código, não o diff

  • Tanto split diff quanto unified diff funcionam bem para mudanças pequenas e simples
  • Quando a mudança fica maior e mais complexa, simplesmente ler o diff deixa de ser suficiente
  • Em mudanças grandes, é preciso examinar adequadamente a base de código em um determinado momento
    • Prestando atenção às áreas alteradas recentemente, mas basicamente encarando como uma revisão de código comum
    • É preciso executar os testes
    • É preciso usar recursos de navegação do editor, como goto definition
    • É preciso aplicar mudanças locais para verificar se havia outras formas possíveis de escrever aquilo
    • É preciso procurar partes que deveriam ter sido alteradas em um contexto mais amplo
    • É preciso conseguir encontrar pontos estranhos na base de código atual, independentemente do histórico de alterações

Tela de revisão desejada e workaround local

  • A visualização de diff ideal mostraria, à esquerda, o estado atual do código e, à direita, o unified diff da área de código atualmente visível
    • O código à esquerda deveria ser igual ao estado atual em disco
    • As alterações seriam indicadas de forma sutil na margem
    • O diff à direita deveria corresponder à área de código atualmente visível à esquerda
  • Atualmente, o suporte das ferramentas a esse formato de revisão é insuficiente
  • O fluxo de trabalho real consiste em fazer checkout do PR localmente usando o gpr script
    • Na etapa final, os commits do PR são removidos e apenas as alterações permanecem
    • Nesse estado, o fluxo de trabalho de staging e commit é aproveitado para a revisão de código
  • Com o edamagit, é possível ver a lista de arquivos alterados e navegar para a próxima/anterior alteração no editor
    • Também é possível usar a staging area para marcar hunks já revisados
  • O incômodo restante é que não há sincronização automática entre o magit status buffer e o arquivo atualmente aberto no editor
    • Para ver o arquivo atual e o diff lado a lado, é preciso abrir o diff manualmente e rolar diretamente até a posição que se está vendo
  • Para fazer uma revisão tão próxima do código, hoje é necessário criar suas próprias ferramentas provisórias
  • O principal objetivo de uma revisão de código não é necessariamente apenas examinar o código; como leitura relacionada, também há Two Kinds of Code Review

1 comentários

 
GN⁺ 2023-10-25
Opiniões do Hacker News
  • Dizer que, em mudanças grandes, você não quer fazer uma revisão de diff, mas sim revisar corretamente a base de código em um determinado momento e prestar atenção às áreas alteradas recentemente, pessoalmente, não parece combinar com uma revisão de código comum
    Depende da equipe e do ticket, mas, se o revisor não estiver em um papel como o de principal engineer, responsável principalmente pelo código como um todo, em geral vejo a revisão de código mais como uma simples verificação de consistência feita por um segundo par de olhos
    Se os commits do PR estiverem bem organizados ou tiverem sido squashados, também fica relativamente fácil revisar as mudanças incrementais separadamente
    No fim, a reclamação parece mais uma diferença entre tipos de revisão. Há a revisão comum de verificação de consistência e a revisão pre-merge profunda de arquitetura/funcionalidade, e a revisão de código em ferramentas web parece, em sua maioria, mais próxima da primeira

    • Ver revisão de código como uma simples verificação de consistência é uma perspectiva pobre demais. Revisão de código é um mecanismo central para reduzir a posse individual do código, disseminar convenções e fazer transferência de conhecimento
      Uma boa revisão de código começa com um bom PR. O PR deve explicar o objetivo e como esse objetivo foi alcançado
      O primeiro item de uma boa revisão é verificar se o código cumpre o objetivo descrito; o segundo, se há testes que validem a funcionalidade alegada; o terceiro, se segue as convenções arquiteturais do sistema existente; o quarto, se o estilo está de acordo com o esperado; e, por fim, se há sugestões para melhorar o uso da API
      Revisões de código que fazem apenas verificação de consistência são pouco úteis, pois isso é algo que o CI poderia fazer. Revisão de código é um processo humano, então deve aproveitar ao máximo o que só humanos conseguem fazer e deixar o restante para as máquinas
      Nos itens acima, está implícita a pergunta: “Esta mudança será um bom exemplo para contribuições futuras?”
    • Acho que revisões de código superficiais só produzem resultados superficiais. É por isso que tantos bugs chegam à produção apesar de revisões de código serem comuns
      Sempre fiz revisão de código com uma perspectiva de testador. Compilo e executo o código para ver se ele funciona como pretendido, procuro pontos fracos e tento quebrá-lo
      O diff não é o fim, é o começo. É apenas uma pista de onde cutucar o código; no fim, a prova vem do comportamento real
      Alguém poderia dizer: “isso é trabalho de testador, não de engenheiro”, mas quem poderia testar melhor do que alguém que entende o código e sabe onde olhar?
      Sei que essa postura é minoritária. Muitos engenheiros parecem ter uma resistência estranhamente forte a executar o código diretamente, e até a usar um debugger. Constroem uma infraestrutura enorme para automatizar tudo, mas automação, no fim, também é apenas código que pode ter bugs. Quem testa os testes, Quis custodiet ipsos custodes?
    • Concordo com o que se espera de uma revisão de código, mas, para funcionar direito, é preciso primeiro existir uma forma específica de trabalho
      A revisão de código é a última linha de defesa, excluindo o CI, mas nas equipes em que trabalhei a direção geral introduzida pelo PR já tinha sido discutida com várias pessoas antes desse ponto. Pode ser pair programming, explicações aprofundadas, conversas no café, qualquer forma adequada à complexidade
      Assim, o PR vira um espaço para revisar as táticas que implementam a estratégia, não a estratégia em si, como separar um novo módulo, introduzir uma grande dependência ou retrabalhar a superfície de uma API
      Se você apenas verifica a consistência de uma parte do código isoladamente, sem comunicação prévia, há o risco de fragmentar a posse do código. Acaba virando algo como: “O que aquele módulo faz?” “Não sei, pergunte ao Bob”
      Separadamente disso, seja qual for o modo de revisão, gostei da ideia de visualização de diff proposta pelo autor
    • Há pontos bons, mas, quando você passa por isso repetidamente, revisão de diff muitas vezes simplesmente fica incompreensível, então se torna meio sem sentido
      É difícil fazer qualquer revisão com um bloco misturando linhas vermelhas e verdes. Não é uma visualização útil do código e, no mínimo, não é adequada como visualização padrão
      Ainda assim, como é trabalho, a gente vai empurrando, mas concordo 100% com o autor que não é uma visualização muito útil
    • A cultura das empresas modernas de tecnologia se afastou demais de processos mais formais para gestão de desenvolvimento
      Sem um processo básico para lidar com defeitos, funcionalidades etc., cada engenheiro precisa inventar, a cada vez, como lançar uma funcionalidade por meio de colaboração
      A maioria acaba fazendo quase nada, e a revisão de código se torna a única colaboração feita em um nível de detalhe significativo sobre o assunto. Por isso, tudo que poderia ser uma intervenção acaba sendo enfiado em um único momento
  • Como terceira, ou quarta, opção, também vale mencionar o difftastic. Ele usa um diff “estrutural” em vez de diff por linha, destacando mudanças de forma mais granular
    https://github.com/Wilfred/difftastic

    • Como quarta, ou quinta, opção, também há o patdiff: https://opensource.janestreet.com/patdiff/
      Pelo que me lembro, às vezes ele deixava cerca de 35% dos diffs mais fáceis de ler; em geral, em 60% não fazia diferença; e, raramente, em 5% os tornava mais difíceis de ler
      Usei há alguns anos, então não me lembro bem dos problemas específicos. O único motivo de eu ter parado foi que comecei a usar magit para Git diff
    • É uma abordagem realmente boa. Só é uma pena que não esteja amplamente integrada. Pelo que sei, uma saída JSON estruturada adequada para outras ferramentas usarem só surgiu recentemente
    • O ediff do Emacs também faz isso. O recurso que destaca palavras diferentes é o Refine
    • git diff --word-diff também tem uma funcionalidade parecida e é bem boa
  • Ao revisar no Vim, a sensação é parecida
    Tenho um pequeno script que abre o PR e, para os arquivos alterados, faz algo mais ou menos como vimdiff <(git show baseref:file) file. As abas do Vim são, na prática, views, então é bom poder manter buffers individuais abertos ao mesmo tempo em estados diferentes
    Na view padrão, o scroll lock funciona como esperado e, se necessário, é possível evitá-lo em uma aba separada
    [c e ]c navegam entre hunks com base no conjunto de alterações existente no PR, não no diretório de trabalho
    Com dp e dg, dá para fazer push/pull de um hunk para o buffer de diff somente leitura e marcá-lo como “concluído”; com isso, ele fica oculto entre as alterações destacadas do buffer ativo
    Uma alteração feita no buffer ativo pode ser commitada imediatamente ou enviada como comentário
    Recursos normais do editor, como go to definition, integração com build e documentação em pop-up, continuam funcionando no estado atual da árvore
    Esse método permite ver as alterações do PR em relação ao branch-base e, ao mesmo tempo, manter o diretório de trabalho limpo. Parece muito melhor do que a solução do matklad, que vê as alterações deixando o diretório de trabalho sujo
    No ambiente em que trabalho, isso é especialmente bom porque as alterações muitas vezes são adicionadas como commits --fixup, e a ferramenta as recombina no momento do merge de modo que, por meio de uma chamada a git-interpret-trailers, o autor do commit de fixup seja atribuído ao commit original. Ela também extrai comentários de texto do PR e, quando apropriado, adiciona um trailer Reviewed-by, ou um trailer Acked-by quando equivale a um +1

    • Fiquei curioso sobre o que exatamente significam “enviar como comentário” e “extrai comentários de texto”. É algum tipo de lógica customizada interna da empresa?
  • Em bases de código muito grandes e complexas, com muitos engenheiros envolvidos, isso é um grande problema
    Code review é difícil porque o diff sempre parece plausível, os testes sempre passam e o básico está sempre verificado
    Mas, mesmo quando a alteração parece razoável, muitas vezes ela está errada na prática
    Uma única alteração ruim que pareça plausível pode fazer toda a arquitetura derivar
    Como sempre, estritamente falando, isso não é um problema de ferramenta; está mais para um problema de cultura e compartilhamento de conhecimento. Ferramentas melhores, sozinhas, não resolvem

    • É difícil aceitar ao mesmo tempo as duas frases “os testes sempre passam” e “mesmo quando a alteração parece razoável, muitas vezes ela está errada”
      Ao fazer code review, é raro encontrar o bug em si. Em vez disso, olho se existem testes que detectariam esse tipo de bug
  • Acho que estou deixando passar alguma coisa. O autor diz que split diff não funciona para ele, mas não explica por quê
    Um diff ideal pareceria quase igual a um split diff com o contexto duplicado removido. Em vez de o contexto se repetir dos dois lados, ele ficaria só à esquerda
    Fico me perguntando qual é a utilidade de remover o contexto duplicado do painel da direita

    • O autor escreve que precisa executar testes, usar go to definition e outros recursos de navegação do editor, aplicar alterações localmente para verificar se poderia ter escrito de outro jeito, olhar um contexto mais amplo para encontrar partes que também deveriam ter mudado e perceber pontos estranhos na base de código atual independentemente do caminho histórico das alterações
      Nos editores/IDEs que usei, o suporte a esses recursos dentro de um diff costuma ser básico. Go to definition às vezes só funciona dentro do mesmo arquivo, autocomplete geralmente não funciona bem para itens recém-adicionados, e muitas vezes não há recursos de refatoração
    • O lado esquerdo mostra o código atual, sem modificações. Se um lado mostra o contexto completo com recursos de navegação de código e o outro mostra as alterações, dá para ver quais mudanças se aplicam em determinado contexto
      Pelo que entendo, o autor quer inverter o diff. Em vez de olhar a alteração em si, quer olhar o código e verificar se há mudanças relacionadas a ele
      O texto não tem muitos detalhes, mas parece que ele quer navegar até implementações ou pontos de chamada e verificar se há alterações adequadas
    • Não sou o autor, mas me identifico
      O destaque de diff pode distrair mais do que parece e, quando desligado, às vezes a legibilidade melhora bastante. Eu também às vezes desligo isso no visualizador de diff para olhar o código com olhos novos, mas não há opção de mostrar o diff unificado junto em um split pane
  • Ao pressionar . no GitHub, você entra em uma IDE completa dentro do navegador
    Achei muito útil para revisão. Em vez de ver só fragmentos, dá para ver as alterações dentro do contexto do arquivo inteiro, o que ajuda muito mais a encontrar problemas sutis de design

    • Isso é realmente ótimo. Eu só tinha entrado nessa IDE ao editar arquivos diretamente no GitHub; não sabia que havia um atalho para ir direto a partir do diff de um commit
      Aliás, esse atalho também funciona no GitLab
  • Sinto falta de alguns recursos do p4merge. Por exemplo, há um recurso que, no espaço entre os split diffs, mostra qual parte de um arquivo corresponde a qual parte do outro, exibindo também o impacto do diff nos dois lados: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    Também gosto do recurso de merge em 3 vias: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    Esses recursos não chegaram às ferramentas de diff baseadas na web mais usadas. Acho que o diff em 3 vias é uma boa maneira de mostrar o resultado de um merge difícil

  • Se você usa GitHub, basta pressionar . na página do pull request em github.com ou mudar o domínio para github.dev
    Isso abre o VSCode no navegador, e o pull request é exibido em uma view de diff
    A vantagem é que dá para ver o arquivo inteiro e, como o algoritmo de diff é diferente do github.com, às vezes fica mais legível em diffs complexos

    • A desvantagem é que é lento e cheio de bugs
      A view de diff lado a lado que mostra comentários do PR em uma lista fora das linhas pode ser muito útil, mas é uma pena precisar abrir uma instância inteira do VSCode no navegador
      Na verdade, isso é tão útil que o GitHub já teve uma split view assim antigamente, sem precisar empurrar as pessoas para uma salada de buzzwords de ambiente de desenvolvimento AI/ML/crypto/blockchain/cloud-enabled
      A ideia é boa, mas a execução é péssima. Talvez isso também resuma o GitHub como um todo hoje em dia
  • Meld se encaixa muito bem nesse tipo de uso
    https://meldmerge.org/

    • https://kdiff3.sourceforge.net/ também tem muitas visualizações boas, então vale mencionar junto
    • Parece bastante com o FileMerge, incluído nas ferramentas de desenvolvedor da Apple
  • Minha ferramenta favorita é o diff2html-cli: https://diff2html.xyz/
    https://www.npmjs.com/package/diff2html-cli
    Permite visualizar diffs em HTML e suporta os formatos side-by-side e unified