I’ve been meaning to start writing up the things I’ve been having prolems with reviewing on GitHub since moving, with a plan of posting a big long list in a month’s time once all the dust has settled a bit more.
From a high-level point of view, ignoring the actual “getting things set up” pain, I’m just as convinced that GitHub is a terrible experience for reviewers as I was before this whole thing began. If anything, I’d say I’m actually more convinced than before, especially for a large-scale project with many PRs, patch authors, contributors.
Here are a list of things I find painful, and how they compared to Phabricator:
- “Outdated” comments: GitHub has this concept of “outdated comments” i.e. any comment that was attached to a line of code that has changed in a later commit than the one on which the comment was made. Phabricator did a very good job of keeping these comments on the same, modified line, or at least somewhere near them. GitHub completely hides them from view in the “Files” panel, only making them available in the “Conversation” view (more on that later), which means I have to constantly have two windows open (one on each view) if I’m to meaningfully check against previous comments. Furthermore, because of this hiding of supposedly outdated comments, having ongoing comment threads on a specific bit of code that is in flux becomes a mess.
- The “Resolve Conversation” button: I started a thread about this elsewhere (see RFC: GitHub PR "Resolve Conversation" button), but I wasn’t able to get a consensus. This button has the result of completely collapsing a conversation. This adds an additional unnecessary click to every conversation where anybody has clicked it, to review that conversation, in case it’s relevant still. There’s no local version of the “hide this conversation for me only” option like that there was in Phabricator, which means we’ll never likely come to an agreement on what to do with that “Resolve Conversation” button that works for everybody (except possibly ban the use of it entirely).
- The “Conversation” view: this, in my opinion, is basically useless. Conversations hide behind a “Hidden Conversation” thing far more readily than in Phabricator, making it hard to review the full comment history; inline comment threads are scattered all over the place, with the same thread often appearing in multiple locations inexplicably (and in those cases, usually without the ability to actually reply to the thread); and the diff context given (admittedly more than Phabricator) for most comments is so minimal that it is basically useless. Phabricator had everything on the one page, so it wasn’t as painful to switch between viewing the files and viewing the out-of-line comments.
- Email notifications: there are way too many of them, and most of them are noise. Fortunately, I don’t need to use the llvm-commits list, so I don’t have to worry about setting up lots of different email filters. However, it’s still irritating that typicllly when anyone is replying to comments, without using a “Review” to group them, every single one is emailed about independently. This makes it harder, even in clients like gmail, to find the really interesting parts (the words the person wrote) in between the noise of email headers, diff context etc.
- The replacement for “Herald”: I accept that this has gone through a state of flux, and I am grateful to those who have been working to get it functioning properly. However, I don’t think we’ll ever likely get to a sensible point where every individual can have their own set of Herald-equivalent rules, i.e. where a user can subscribe to specific files/folders/… I’m working on the assumption that we’re not going to have a team/label for every single individual contributor after all…
- Stacked PRs: there still isn’t anything remotely resembling a viable proposal on how to make this work, let alone a consensus on this. I’m just glad I’m no longer regularly writing LLVM patches. I rely on stacked reviews in our internal GitHub instance all the time (and similarly did so on Phabricator when I used to still regularly contribute patches to LLVM). Frankly, I got the impression that those driving the switchover to PRs didn’t care about stacked reviews and ignored concerns about how there was no viable alternative yet in GitHub for this style of workflow, expecting the people who cared about them to do all the work, even though many of those same people were probably opposed to the migration in the first place.
- Forced pushes: even in the limited cases where we permit them, these are a nightmare. In particular, they prevent the use of the “changes since your last review” option in GitHub, break links in emails, and so on. This is a direct consequence of GitHub’s PR model being based on the actual commits in the repo, so I doubt there’ll ever be a fix for it.
Has the migration been a complete disaster? No. Am I finding it more of a burden to review patches? Yes. Will things improve? I expect small bits of our processes will over time, but I doubt some of the fundamental issues with the review experience in GitHub will ever change. Are there benefits of moving from Phabriactor? Aside from stability issues, I’ve yet to see any as a reviewer (I’m sure there are for other stakeholders though). Will I be reviewing as much in GitHub as I was on Phabricator? I doubt it - it’s hard to do a scientific comparison, but I’m fairly confident the time spent on similarly sized patches in the two systems points to Phabricator as the better tool. Note that this isn’t related to GitHub being new to me - it isn’t (I’ve been using it for several years internally as part of my employment). Do I think the migration was a bad idea still? Yes. Do I think the focus was placed too heavily on making things work for newcomers? Yes (don’t get me wrong, doing things that are good for newcomers is not a bad move, but it needs to be not at the expense of the existing people who are making valuable contributions to the project).