Hacker News new | ask | show | jobs
by aseipp 946 days ago
Most of the projects that dislike GitHub's review UI want the functional equivalent of `git range-diff`. Code review systems like Phabricator and Gerrit basically revolve around this as basis of thinking about diffs, their evolution, and how code review progresses.

You want to write 3 patches to a project, that are committed in series, based off of `X`

         A ---> B ---> C
    X---/
Let's say A cleans up some code, getting it ready for B; it is not mandatory but was just naturally done as part of the change; maybe it's 50 lines. B then adds 500 lines of new code to be reviewed. Finally, C integrates the new code from B; maybe you migrate something to use it, changing a few internal API calls. So C might be a diff of only 5 or 10 lines.

The code reviewer will want to start by reading A, and leaving comments on A. And so on for B, and so on for C. Let's say they leave code review comments on each commit. So now you have a set of things to do.

GitHub encourages you to add a new set of patches on top of the previous 3, so you might publish add commits on top:

         A ---> B ---> C --- > D ---> E ---> ...
    X---/
Where each change after C incrementally addresses review comments.

In contrast, tools like Gerrit and Phabricator say instead that you should publish new versions of A, B, and C, wholesale. So now we have a new series of 3 patches:

         A' ---> B' ---> C'
    X---/
This branch might exist in parallel to your old one. So your full commit graph might look like this, now:

           A  ---> B  ---> C
          /
         /-A' ---> B' ---> C'
    X---/
So there's the original series of changes A,B,C, and the new series of changes that respond to all the comments. Think of that as "version 1" of the series and then "version 2"

Now here's the question: how does the code reviewer know that you addressed their comments? Answer: they need to do a range diff between the original version 1 series and the version 2 series:

    A  ---> B  ---> C
    |       |       |
  d(A,A') d(B,B') d(C,C')
    |       |       |
    A' ---> B' ---> C'
Where d(x,y) = diff(x,y). You're looking at the diff between the two versions of one patch. So instead you view the changes between version 1 of A, and version 2 of A. And so on and so forth for all 3 patches.

This is very useful for example because B might be 500 lines, but responding to review comments may only take 50 lines of fixes. It would be very annoying to re-read the entire 500 line patch, as opposed to just the 50 line incremental patch. This has very big effects as the review cycle goes forth.

People mostly like this review style because it keeps needless "fix review comment" commits out of the history and it "localizes" the unit of code review to each individual patch rather than the whole aggregate. Note that the final version of the series A,B,C will just contain those 3 logical changes, not 3 logical changes + 1 dozen fixup changes.

This not only makes changes more "dense", it improves the ability to navigate the history, and do things like `git blame`; and it means you don't commit things like "fix failing test" which would break bisection.

Note that most of the systems that implement the above review style do not literally use `git range-diff` in their implementation; rather, git range-diff is simply an implementation of this idea that you should review each version of a patch as a diff from the previous version. The tools themselves have their own lifecycle, patch management, APIs, etc that are wholly different from Git's.

Finally, there are lots of things GitHub's UX is just slacking on, functionality aside. You can't comment "anywhere" in a code review, just on changed lines. An annoying one I hate is that you can batch review comments but not batch review resolutions; if you leave 5 comments on a diff, you submit them as a batch. But if you resolve 5 comments, you do it one at a time, which is annoying and easy to lose track of. The UX has too many tabs you have to swap between, which is pointless when you could just make the page a little longer and things like Ctrl+F would work better. And so on, and so forth.

2 comments

Yes, this, but you've made a very long and amazing and intricate description of something that basically amounts to an essential workflow thing that those of us who worked at e.g. Google or on Chromium just found intuitive and normal: the comments I make are on the specific code/diff at the point in time that the comment was made and when I see you've resolved or updated it, I as a reviewer can see exactly what changed.

GitHub's PR tool sucks for this. It's also clunky, wastes boatloads of screen real estate, and lacks keyboard shortcuts.

These days I work in GitLab's MR system, and it's meh, but slightly better. But I still miss Gerritt. So much better, even if takes a while to get used to.

Yes, it's long-winded, and unfortunately, it's really hard to get across with the above description that the workflow described therein actually is quite simple with the correct tools, and (at least in my opinion) also results in a technically superior outcome. Everything I described above is very intuitive and easy once you get into the groove with something like Phab or Gerrit.

Unfortunately, I think it's sort of lost on people today, because basically everyone is so stubborn about GitHub and you basically have to drag them kicking and screaming to most other tools, even if they have clearly superior UX, much less fundamental design. I used to run an open-source project and we used Phabricator and most of the regular contributors ended up liking it, but the initial hurdle for people was often like pulling teeth (and it felt like people who didn't contribute because of it never missed an opportunity to tell you so, though that may just be sour grapes on my part from hearing it so often.)

Absolutely GitHub is so mediocre. It's fine for most open source project workflows, but when I worked for a company that used it as their main tool I was blown away how badly it managed professional-level code reviews.

Gerrit isn't beautiful, and it also had a lot of knobs that could be tweaked in weird ways. But I do miss aspects of it.

I have been wanting to check out JetBrain's code review tool and see how it looks.

This is an incredible workflow as someone who has only ever used git and GitHub (and poorly at that).

I presume there isn’t really any way to have this kind of workflow using git and GitHub? I’d be interested in something like this at my company but we’re sorta locked in to GitHub already.

There are third party tools like Graphite, CodeApprove, and Reviewable that will basically add these workflows on top of the existing GitHub API.

There's a much more detailed and complex answer to this question (TL;DR you can kind of fake it in various weird ways without too much effort) but that's probably the best approach if you're willing to fork over some money. Graphite in particular seems pretty well reviewed.

You may also want to look into Sapling and tools like git-branchless, which will change your workflow to better accommodate this kind of thinking.