|
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. |
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.