Hacker News Reader

Comment by danking00 | Hacker News Reader

I've heard this point of view many times, but cannot find an extensive explanation of it. Could anyone elaborate on the issue with the GitHub review UI/UX?

> I hate the GitHub review UI with a passion. At least, right now, GitHub PRs are not a viable option for Mozilla [...] the more general shortcomings in the review UI.

View on HN

I know people who express similar feelings. Usually it is shorthand for "I would prefer stacked diffs" or similar.

Two blog posts I've seen people point at:

* https://mitchellh.com/writing/github-changesets

* https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/

From the second article, a minor point but possibly helpful to other here, he contrasts doing everything in the terminal with stacked commits vs going to the Github UI. If people aren't aware, Github offers a cli tool[1]. I've been using it for a few months now and am finding it does make me more productive -- it's nice to be able to open up a PR directly from my terminal. I do still use the GH UI for a lot of things, but I'll often at least start in the terminal, and it also makes the transition from terminal to browser easy as many commands support the `--web` flag open up the right page for you (eg `gh repo view --web`).

[1] https://cli.github.com/

The CLI doesn't help with stacked commits, though. There's tools like spr[1] but none of them are anywhere as pleasant to use as Gerrit (or Phabricator, I guess).

[1]: https://github.com/ejoffe/spr

It doesn't -- hence my noting the GH cli as a minor point. It's a tool I've personally found helped increase my productivity, so just wanted to share as others may find it useful too.

I've found that Graphite [0] has solved this issue for me. It's makes the experience of shipping stacked diffs about the same as the CR tool within AWS

[0]: https://graphite.dev/

Thanks for this. I admittedly still don't really grok it, but maybe if I play around with the tooling, something will click.

I think that this page gives a decent overview of what the tool aims to accomplish: https://graphite.dev/docs/how-stacking-works

This page covers some of the more advanced functionality: https://graphite.dev/docs/manage-stacks

Either way, I hope you like it! I've tried many other tools that aim to implement stacked diffs for GitHub PRs, and Graphite is _by far_ the best. They have their own Web UI but I skip that completely and just use their CLI + GH's UI.

I watched their product overview video last night, but like, I don't grok what the difference between rebasing a branch and rebasing a stack is, and they said something like "if there are conflicts, this is when you'd fix them." but I thought the point was reducing conflicts. It sounds to me like short lived feature branches with a lot of extra terminology.

I really think I just need to actually try the tooling and then examine the git repo. I am very bottom-up brained when it comes to git, and so I need to visualize what commands actually do to the DAG to really grok what's going on.

What are these people talking about? What's the difference between "stacked diffs" and just adding commits to a branch? Who cares if the branch is called "master" or not? You could also force push updated commits (e.g. after fixup and rebase etc) and tools like Gitlab and GitHub can show you the diff between versions (or "changesets", as they call them). Gitlab at least makes it pretty clear when a change has been made and the order things were done in. Pretty sure GitHub does too.

As I alluded to, it’s their opinion, not mine. I don’t fully understand the differences myself. But they’re very passionate about this.

I once worked with a guy who hated Python. He also hated IntelliJ and did all of his programming in Eclipse. He hated MacOS and would only run old versions of Windows of Linux.

What I found was that he was just sensitive to change. It took him awhile to get comfortable with something but as soon as he did he would from then on resist any change. Anything that was different to what he was used to he hated.

In some sense, it makes sense. People should prefer processes over tools in many cases. If you have a process that works with one tool but doesn't work with a new tool then you might be better off sticking with the original tool. Broken processes can debilitate organizations.

When people say "I hate X" often what they are really saying is "I have a process to do Y that works for me but tool X makes that process too difficult". The problem is they just say "I hate X" so you never really hear about the process Y which provides the context.

GitHub PRs are both bad process (no stacking or per-commit review, incentivizes large changes, ...) and bad tool (no inter-diffs, noisy diffs, comments disappear through rebases, slow, ...).

> slow

but have you tried bitbucket? Omfg is it slow

> Anything that was different to what he was used to he hated.

I suspect he just hated the constant churn. That's kind of a rational frame of mind when you think about it.

I enjoy learning new things, but my bar for "is this actually better?" seems to be unconsciously higher than it used to be.

Mine too. I suspect it's the classic explore/exploit dilemma. We've learned enough that every new hotness just doesn't seem that novel or worth the effort to learn given we already see a path to our goals using what we already know.

https://en.wikipedia.org/wiki/Exploration-exploitation_dilem...

One point: if you are re-reviewing, other platforms (e.g. Phabricator, Gerrit) have much more developed ways to compare changes relative to one another.

Phabricator was awful. It loaded commit messages with irrelevant boilerplate and allowed people to post patches that you couldn’t build for testing because they were just diffs and not actual branches in the repo. Not sad to see it go.

Bias warning: I created CodeApprove to give GitHub reviews a better UI.

GitHub PR review UI is … fine. But it has very little depth. Unlike an IDE it’s hard to grow into a power user of it. It seems to be optimized for simple reviews with 1-2 rounds of a few comments each.

When you do dozens of code reviews every week you want something more. You want something snappy and dense with keyboard shortcuts. You want something that draws your attention to where it’s needed and something that helps you ensure all your conversations reach resolution.

So there’s a lot of room for improvement and GitHub hasn’t shown much interest in improving this area. So 3p UIs are stepping in (Graphite, CodeApprove, Reviewable, etc)

One issue I was discussing with a colleague just the other day is that you cannot leave a comment on lines that are not "part of the diff" (changed or context), which is sometimes really useful for "see this thing over here".

You definitely can comment on lines that haven't been changed. I don't think there's a way to comment on files that haven't been changed, though.

Actually you can’t comment on lines that haven’t been changed if they’re too far from the changed lines! It’s a weird limitation of GitHubs data model.

(source: that’s one of the features that people like most about the review tool I created, CodeApprove)

Annotations can be added by "checks" anywhere, though, I believe.

I like to use that to surface cross-references (annotating anchors whose references appear in changed or context lines, and references whose anchors do).

As the other commenter pointed out, only if they're close enough to a changed line. That's what I meant by "context" (borrowing terminology from diff, where you change the amount of it with -C).

See https://news.ycombinator.com/item?id=37836932 by a Mozilla employee

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.

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.

Github review is good for either zero or one round of comments. After that you can't tell what the hell is going on. It has clearly been written by people who do not themselves practice code review. Compare and contrast with Gerrit, which remains usable after many rounds of comments, in both directions, and changes. Compare also Phabricator.

bitbucket is worse, reviewees can resolve reviewer's comments (github has this too turns out), after file changes, comments disappear

What's wrong with reviewees marking comments resolved? That allows anyone reading to see what issues are "open" or already addressed at a glance.

Code review isn't adversarial, so it's not like the reviewee is closing comments to shove bad code through and they need approvals anyway.

It's when they resolve it without verifying if their solution was good

Not bad if you have a junior that knows they are a junior, bad otherwise.

My pet peeve with bitbucket was that when doing code review, you couldn't send all comments and overall review in a batch, every single comment was received in real time by the author. Sometimes I would be still reviewing code when the author pushed fixes to comments I've made before and that still might need other changes. After 15 years they improved that 2 weeks ago. I'm glad it's better, but I hate it with a burning passion.

Bitbucket is much better IME. It has a better handle on the current state of the review rather than offering you the whole diff to re-review whenever someone rebases, and it loses comments much less than github (indeed not at all, in my experience).

Reply on news.ycombinator.com