3P by GN⁺ | ★ favorite | 댓글 1개
  • 작은 변경은 unified diff와 split diff 어느 쪽으로도 충분히 볼 수 있지만, 크고 복잡한 변경에서는 diff만 읽는 리뷰가 한계에 부딪힘
  • 큰 변경은 변경 이력보다 특정 시점의 코드베이스 전체 상태를 기준으로, 실제 코드를 수정하듯 탐색하고 검증해야 함
  • 이상적인 리뷰 화면은 현재 디스크상의 코드와 해당 영역의 unified diff를 나란히 보여줘, 코드 맥락과 변경 내용을 동시에 따라가게 해야 함
  • 기존 도구 지원이 부족해 PR을 로컬에 체크아웃한 뒤 커밋은 지우고 변경만 남기는 저기술 워크플로를 사용함
  • 이 방식으로 변경 파일 탐색과 reviewed hunk 표시는 가능하지만, 에디터의 현재 위치와 diff가 자동 동기화되지 않아 수동 조작이 남아 있음

diff보다 현재 코드 상태를 리뷰하기

  • split diff와 unified diff는 작고 단순한 변경에서는 모두 잘 작동함
  • 변경이 커지고 복잡해지면 단순히 diff를 읽는 것만으로는 충분하지 않음
  • 큰 변경에서는 특정 시점의 코드베이스를 제대로 살펴봐야 함
    • 최근 변경 영역에 주의를 기울이되, 기본적으로는 일반 코드 리뷰처럼 봐야 함
    • 테스트를 실행해야 함
    • goto definition 같은 에디터 탐색 기능을 사용해야 함
    • 로컬 변경을 적용해 다른 작성 방식이 가능했는지 확인해야 함
    • 더 넓은 문맥에서 바뀌었어야 할 부분을 찾아야 함
    • 변경 이력과 무관하게 현재 코드베이스에서 이상한 점을 발견할 수 있어야 함

원하는 리뷰 화면과 로컬 우회 방식

  • 이상적인 diff 뷰는 왼쪽에 현재 코드 상태를, 오른쪽에 현재 보이는 코드 영역의 unified diff를 함께 보여주는 형태임
    • 왼쪽 코드는 디스크에 있는 현재 상태와 같아야 함
    • 변경 사항은 여백에 은은하게 표시됨
    • 오른쪽 diff는 왼쪽에서 현재 보이는 코드 영역과 대응해야 함
  • 이런 리뷰 형식은 현재 도구 지원이 부족함
  • 실제 워크플로는 gpr script로 PR을 로컬에 체크아웃하는 방식임
    • 마지막 단계에서 PR의 커밋은 지우고 변경 사항만 남김
    • 이 상태에서 staging과 commit용 워크플로를 코드 리뷰에 활용함
  • edamagit으로 변경 파일 목록을 보고, 에디터에서 다음/이전 변경으로 이동할 수 있음
    • staging area를 사용해 이미 리뷰한 hunk를 표시할 수도 있음
  • 남는 불편은 magit status buffer와 에디터에서 현재 열린 파일 사이의 자동 동기화가 없다는 점임
    • 현재 파일과 diff를 나란히 보려면 diff를 수동으로 열고 현재 보고 있는 위치까지 직접 스크롤해야 함
  • 코드에 이 정도로 가까이 접근하는 리뷰를 하려면 현재는 임시 도구를 직접 만들어야 하는 상황임
  • 코드 리뷰의 주요 목표가 반드시 코드 검토만은 아니며, 관련 글로 Two Kinds of Code Review를 함께 볼 수 있음

댓글과 토론

Hacker News 의견들
  • 큰 변경에서 diff 리뷰가 아니라 특정 시점의 코드베이스를 제대로 리뷰하고 최근 변경 영역에 주의를 기울이고 싶다는 말은, 개인적으로 일반적인 코드 리뷰와는 맞지 않아 보임
    팀과 티켓마다 다르겠지만, 리뷰어가 코드 전체를 주로 책임지는 principal engineer 같은 역할이 아니라면 보통 코드 리뷰는 두 번째 눈으로 하는 간단한 정합성 확인에 가깝다고 봄
    PR의 커밋이 잘 정리되거나 squash되어 있으면 증분 변경을 따로 검토하기도 비교적 쉬움
    결국 불만은 리뷰 유형의 차이에 가까워 보임. 흔한 정합성 확인 리뷰와 깊은 아키텍처/기능 pre-merge 리뷰가 있는데, 웹 도구의 코드 리뷰는 대부분 전자에 가까운 듯함

    • 코드 리뷰를 단순한 정합성 확인으로 보는 건 너무 빈약한 관점임. 코드 리뷰는 개인 코드 소유권을 줄이고, 관례를 전파하며, 기술 이전을 하는 핵심 메커니즘임
      좋은 코드 리뷰는 좋은 PR에서 시작함. PR은 목표와 그 목표를 어떻게 달성했는지를 설명해야 함
      좋은 리뷰의 첫 항목은 코드가 설명한 목표를 달성하는지 확인하는 것, 둘째는 주장한 기능을 검증하는 테스트가 있는지, 셋째는 기존 시스템의 아키텍처 관례를 따르는지, 넷째는 스타일이 기대와 맞는지, 마지막으로 API 사용을 더 낫게 할 제안이 있는지임
      정합성 확인밖에 안 하는 코드 리뷰는 CI가 해도 되는 일이라 쓸모가 적음. 코드 리뷰는 인간 과정이므로 인간만 할 수 있는 일을 최대한 활용해야 하고, 나머지는 기계에 맡겨야 함
      위 항목들에는 “이 변경이 미래 기여에 좋은 예시가 될까?”라는 질문이 암묵적으로 들어 있음
    • 피상적인 코드 리뷰는 피상적인 결과만 낳는다고 봄. 코드 리뷰가 흔한데도 많은 버그가 배포되는 이유가 여기에 있음
      항상 테스터 관점으로 코드 리뷰를 해왔음. 코드를 빌드하고 실행해서 의도대로 동작하는지 보고, 약점을 찾고, 깨뜨려 보려고 함
      diff는 끝이 아니라 시작임. 코드 어디를 찔러봐야 할지 알려주는 단서일 뿐이고, 결국 증명은 실제 동작에서 나옴
      “그건 테스터가 할 일이지 엔지니어가 할 일이 아니다”라고 할 수도 있지만, 코드를 이해하고 어디를 봐야 하는지 아는 사람보다 더 잘 테스트할 수 있는 사람이 누가 있겠나 싶음
      이런 태도가 소수파라는 건 알고 있음. 많은 엔지니어는 직접 코드를 실행하는 것, 심지어 디버거를 쓰는 것에도 이상할 정도로 거부감이 있어 보임. 모든 걸 자동화하려고 거대한 인프라를 만들지만, 자동화도 결국 버그가 있을 수 있는 코드일 뿐임. 테스트는 누가 테스트하나, Quis custodiet ipsos custodes?
    • 코드 리뷰에 기대할 내용에는 동의하지만, 제대로 작동하려면 특정한 작업 방식이 먼저 자리 잡아야 함
      코드 리뷰는 CI를 제외하면 마지막 방어선이지만, 내가 일한 팀들에서는 PR이 도입하는 큰 방향이 그 시점 전에 여러 사람과 이미 논의되어 있었음. 페어링, 깊은 설명, 커피챗 등 복잡도에 맞는 방식이면 됨
      그러면 PR은 새 모듈 분리, 큰 의존성 도입, API 표면 재작업 같은 전략 자체가 아니라 그 전략을 구현한 전술을 리뷰하는 자리가 됨
      사전 커뮤니케이션 없이 코드 일부만 따로 정합성 확인하면 코드 소유권이 쪼개질 위험이 있음. “저 모듈은 뭐 하는 거야?” “몰라, Bob에게 물어봐” 같은 상태가 됨
      그와 별개로, 리뷰 모드가 무엇이든 글쓴이가 제안한 diff 뷰 아이디어는 마음에 듦
    • 좋은 말도 있지만, diff 리뷰가 반복적으로 겪어보면 그냥 이해 불가능한 경우가 많아서 다소 무의미해짐
      빨간 줄과 초록 줄이 뒤섞인 덩어리로는 어떤 리뷰도 하기 어려움. 코드의 유용한 뷰가 아니고, 적어도 기본 뷰로는 적절하지 않음
      그래도 일이라서 꾸역꾸역 하긴 하지만, 글쓴이 말처럼 별로 유용한 뷰가 아니라는 데는 100% 동의함
    • 현대 기술 회사 문화는 개발 관리를 위한 좀 더 형식적인 프로세스에서 너무 멀어졌음
      결함, 기능 등을 다루는 기본 프로세스가 없으니, 각 엔지니어가 협업을 통해 기능을 어떻게 배포할지 매번 직접 발명해야 함
      대부분은 거의 아무것도 하지 않게 되고, 코드 리뷰만이 그 주제에 대해 의미 있는 세부 수준으로 이뤄지는 유일한 협업이 됨. 그래서 가능한 모든 개입을 한 순간에 밀어 넣으려는 상태가 됨
  • 세 번째, 혹은 네 번째 선택지로 difftastic도 언급할 만함. 줄 단위 diff가 아니라 “구조적” diff를 사용해서 더 세밀한 변경 강조를 해줌
    https://github.com/Wilfred/difftastic

    • 네 번째, 혹은 다섯 번째 선택지로 patdiff도 있음: https://opensource.janestreet.com/patdiff/
      기억상으로는 가끔 35% 정도 diff를 더 읽기 쉽게 만들었고, 보통 60%는 차이가 없었으며, 드물게 5%는 더 읽기 어렵게 만들었음
      몇 년 전에 써서 구체적인 문제는 잘 기억나지 않음. 그만 쓴 유일한 이유는 Git diff에 magit을 쓰기 시작했기 때문임
    • 정말 좋은 접근임. 다만 널리 통합되지 않은 게 아쉬움. 다른 도구가 사용할 수 있는 제대로 된 구조화 JSON 출력도 최근에야 생긴 것으로 알고 있음
    • Emacs의 ediff도 이걸 함. 다른 단어를 강조하는 기능은 Refine임
    • git diff --word-diff에도 이와 비슷한 기능이 있고 꽤 좋음
  • Vim으로 리뷰할 때도 비슷하게 느껴짐
    PR을 여는 약간의 스크립트가 있고, 변경 파일에 대해 대략 vimdiff <(git show baseref:file) file 같은 동작을 수행함. Vim의 탭은 사실상 뷰라서, 개별 버퍼를 서로 다른 상태로 동시에 열어둘 수 있어 좋음
    기본 뷰에서는 스크롤 잠금이 기대대로 작동하고, 필요하면 별도 탭에서는 피할 수 있음
    [c]c는 작업 디렉터리가 아니라 PR에 존재하는 변경 묶음 기준으로 hunk 사이를 이동함
    dpdg로 hunk를 읽기 전용 diff 버퍼에 push/pull해서 “완료” 표시할 수 있고, 그러면 라이브 버퍼의 강조된 변경에서 숨겨짐
    라이브 버퍼에서 한 변경은 바로 커밋하거나 코멘트로 push할 수 있음
    현재 트리 상태에서 go to definition, 빌드 통합, 팝업 문서 같은 일반 편집기 기능도 그대로 동작함
    이런 방식은 PR의 기준 브랜치 대비 변경을 보면서도 작업 디렉터리는 깨끗하게 유지함. 작업 디렉터리를 더럽힌 상태로 변경을 보는 matklad의 해법보다 훨씬 나아 보임
    내가 일하는 환경에서는 변경이 종종 --fixup 커밋으로 추가되고, 도구가 merge 시점에 git-interpret-trailers 호출로 fixup 커밋 작성자를 원래 커밋에 귀속시키도록 다시 합쳐주기 때문에 특히 좋음. PR의 텍스트 코멘트를 뽑아 적절하면 Reviewed-by trailer를 붙이거나, +1에 해당하면 Acked-by trailer를 붙이기도 함

    • “코멘트로 push”와 “텍스트 코멘트를 뽑는다”가 정확히 무슨 뜻인지 궁금함. 회사 내부 전용 커스텀 로직 같은 건가?
  • 엔지니어가 많이 붙은 아주 크고 복잡한 코드베이스에서는 이게 큰 문제임
    코드 리뷰가 어려운 이유는 diff는 항상 그럴듯해 보이고, 테스트는 항상 통과하며, 기본적인 것들은 늘 체크되어 있기 때문임
    그런데 변경이 합리적으로 보여도 실제로는 틀린 경우가 자주 있음
    그럴듯해 보이는 나쁜 변경 하나로 전체 아키텍처가 표류할 수 있음
    늘 그렇듯 이건 엄밀히 말해 도구 문제가 아니라 문화와 지식 공유의 문제에 가까움. 더 나은 도구만으로는 해결되지 않음

    • “테스트는 항상 통과한다”와 “변경이 합리적으로 보여도 틀린 경우가 자주 있다”는 두 문장을 함께 받아들이기 어렵다
      코드 리뷰할 때 버그 자체를 찾는 경우는 드묾. 대신 그런 버그를 잡아낼 테스트가 있는지를 봄
  • 뭔가 놓치고 있는 것 같음. 글쓴이는 split diff가 자신에게 맞지 않는다고 하지만 이유를 말하지 않음
    이상적인 diff는 중복 문맥을 제거한 split diff와 거의 같아 보임. 문맥이 양쪽에 중복되는 대신 왼쪽에만 있는 형태임
    오른쪽 패널에서 중복 문맥을 없애면 어떤 효용이 생기는지 궁금함

    • 글쓴이는 테스트 실행, go to definition과 다른 편집기 탐색 기능 사용, 다른 방식으로 쓸 수 있었는지 확인하기 위한 로컬 변경 적용, 더 넓은 문맥을 보고 함께 바뀌었어야 할 부분 찾기, 그리고 현재 코드베이스에서 역사적 변경 경로와 무관하게 이상한 부분을 알아차리는 것이 필요하다고 씀
      내가 써본 편집기/IDE들은 보통 diff 안에서 이런 기능 지원이 기초적임. go to definition도 같은 파일 안에서만 되거나, 자동완성도 새로 추가된 항목에는 잘 안 되며, 리팩터링 기능은 없는 경우가 많음
    • 왼쪽은 수정 없는 현재 코드를 보여줌. 한쪽에는 코드 탐색 기능이 있는 전체 문맥을, 다른 쪽에는 변경을 보여주면 특정 문맥에서 어떤 변경이 적용되는지 볼 수 있음
      내가 이해하기로 글쓴이는 diff를 뒤집고 싶어 함. 변경 자체를 보는 대신 코드를 보고, 거기에 관련 변경이 있는지 확인하고 싶은 것임
      글은 세부가 부족하지만, 구현이나 호출 지점으로 탐색하면서 적절한 변경이 있는지 확인하고 싶은 것으로 보임
    • 글쓴이는 아니지만 공감됨
      diff 강조는 생각보다 산만할 수 있고, 꺼두면 가독성이 확실히 나아질 때가 있음. 나도 diff 뷰어에서 가끔 꺼서 코드를 새 눈으로 보지만, 통합 diff를 split pane에 함께 보여주는 선택지는 없음
  • GitHub에서 .을 누르면 브라우저 안의 전체 IDE로 들어감
    리뷰할 때 매우 유용했음. 조각만 보는 대신 전체 파일 문맥 속에서 변경을 볼 수 있어서 미묘한 설계 문제를 훨씬 잘 잡게 됨

    • 이거 정말 좋음. GitHub에서 파일을 직접 수정할 때만 이 IDE에 들어가 봤지, commit diff에서 바로 가는 단축키가 있는 줄은 몰랐음
      참고로 이 단축키는 GitLab에서도 동작함
  • p4merge의 기능 몇 가지가 그리움. 예를 들어 split diff 사이 여백에 파일의 어떤 부분이 다른 파일의 어떤 부분과 대응되는지 보여주면서, 양쪽에서 diff 영향도 같이 보여주는 기능이 있음: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    3-way merge 기능도 마음에 듦: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    이런 기능들은 널리 쓰이는 웹 기반 diff 도구에는 들어오지 않았음. 어려운 merge의 결과를 보여주기에는 3-way diff가 좋은 방법이라고 봄

  • GitHub를 쓴다면 github.com의 pull request 페이지에서 .을 누르거나 도메인을 github.dev로 바꾸면 됨
    그러면 브라우저에서 VSCode가 열리고, pull request가 diff 뷰로 표시됨
    장점은 전체 파일을 볼 수 있고, github.com과 diff 알고리즘이 달라 복잡한 diff에서 더 읽기 쉬울 때가 있다는 것임

    • 단점은 느리고, 버그가 많다는 것임
      PR 코멘트를 라인 밖 목록으로 보여주는 side-by-side diff 뷰는 매우 유용할 수 있는데, 브라우저에서 VSCode 인스턴스 전체를 띄워야 한다는 게 아쉬움
      사실 너무 유용해서 GitHub에는 예전에 이런 split view가 있었고, 사람들을 AI/ML/crypto/blockchain/cloud-enabled 개발 환경 같은 유행어 덩어리로 밀어 넣지 않아도 됐음
      아이디어는 좋지만 실행은 형편없음. 요즘 GitHub 전반을 요약하는 말 같기도 함
  • Meld가 이런 용도에 꽤 잘 맞음
    https://meldmerge.org/

  • 가장 좋아하는 도구는 diff2html-cli임: https://diff2html.xyz/
    https://www.npmjs.com/package/diff2html-cli
    diff를 HTML로 볼 수 있고, side-by-side나 unified 형태를 지원함