How's it going with pull requests?

LLVM Project
1

Hi,

We’ve been using pull requests for almost 2 weeks now, and I wanted to try to get some feedback about how it’s been going. What’s been good? What’s been bad? Any urgent issues that need to be fixed?

1 Like
2

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

6 Likes
3

Sorry if this is discussed. I want to know how can I rerun the testing bot? Must I sign up into the buildkite.com?

4

I am not a prolific submitter or reviewer, but even in my limited role, I am just stoically holding on to the same hope.

About email filters, Github adds some useful headers to each email. Of these, I have been able to use “X-GitHub-Reason” with good results to differentiate between different reasons such as whether I am mentioned by name, or I am the PR author, or the email was generated by my own activity.

I do have one small thing that I need to figure out. In the manual command-line flow, I always reset the date on a commit before I push it. I don’t know if that’s possible in the web UI … at least it wasn’t easily discoverable.

5

Email notifications in particular:

I’m also finding the github UI for just handling / filtering lists of Pull Requests I’m involved with very poor, I’m hoping better use of labels will make this easier in the future, but afaict there’s no equivalent of Phab’s Queries sidebar to save the different filters?

1 Like
6

I tried to address one part of this in Skip sending stub mails for a number of PR events by mstorsjo · Pull Request #3 · llvm/llvm-admin · GitHub, trying to increase the signal-to-noise ratio of the mailing lists. But there’s still a long way to go to make the *-commits lists as useful as they were before for keeping track of patches that one isn’t directly involved with yet.

1 Like
7

Over at https://github.com/pulls you can do some pretty good filtering - I have something like:

is:pr review-requested:tru archived:false repo:llvm/llvm-project label:platform:windows to find all PR’s for me with the label platform:windows

This documentation describes the filter options available:

There is no native UI for saving these filters, but you can bookmark the URL since it contains all the info or you can use this chrome extension: Github Saved Filters - Chrome Web Store

8

I am just going to chime in and say that I see quite a few positives, that should be taken with the grain of salt that I never reviewed as many issues on Phab as some others do/did so I am coming at this from another perspective.

  • Writing and maintaining GitHub actions are much faster and easier than our former pre-merge setup. I expect us to enable much more automation regarding PRs in the future. I have already worked on a few and more are underway. If you have ideas for checks or tests that can be run on PRs, ensure you file an issue and we can look at adding action for it!
  • I have seen an influx of first-time contributors in just my areas of people stepping up and helping contribute to the project - that’s encouraging I think!
  • I don’t use email in my workflow so I have not been as affected by notifications-via-email problems as others. But I find Sign in to GitHub · GitHub and github.com/pulls a much better way to get an overview of the things that need my attention. As I replied to another topic above, there are ways to improve and filter notifications and pull requests in the web interface. Personal preference of course.

Also a huge thanks to @tstellar, @mehdi_amini, @cor3ntin and all the others that have been working a lot this last couple of weeks to improve our workflows and fix issues with different integrations.

3 Likes
9

Part of the problem is the github web UI. I recently discovered magit/forge, and hoping that it will be just as big effective as magit itself in my workflow. Or perhaps some other command-line tool that handles Github pull requests in a way that fits my brain.

10

Thanks! Before this patch, my email filter was diverting 40%+ of the llvm-commits/cfe-commits traffic. Too soon to say what the ratio is now, but just eyeballing the counts it’s definitely better. (The “labeled” emails were particularly useless, “user labeled PR” without reporting the actual label is complete noise.)

11

One thing I’ve noticed is that there’s been a number of commits that have username@users.noreply.github.com email addresses. This is causing buildbot failure emails to be sent to those addresses, and I’m not sure that goes anywhere (trying to send a test email returned NXDOMAIN delivery message failure). It’s not clear to me what workflow is creating this email address, but it’s definitely suboptimal.

1 Like
12

So far I feel like it has been a mixed bag.

As a reviewer, I find myself thinking so much more about what I am doing. I am hoping that will get better after I use it more. I am super frustrated that after I do a review it removes the group I am part of as a reviewer. There is still a lot more e-mail although it seems to have improved a bit. I find the diff e-mails to be a lot more verbose then Phab used to be and therefore a lot less useful.

I feel like the workflow for submitting a PR is more work then it used to be for Phab and I don’t think that will improve much because using Phab allowed for a lot of short cuts that avoided me having to think about git itself.

Overall I feel less efficient but it is too early to tell how much better that will get over time.

13

This happened to me when I recently severely broke the buildbots. I later found it’s because my github account had “Keep my email addresses private” checked, but I didn’t realize it.

14

For people who used arc that could be true. I didn’t, and I find the workflow much simpler.

FTR I did one-time setup the clone of my fork to pull from llvm-project but push to my fork.

git config --add remote.origin.pushurl git@github.com:<githubid>/llvm-project.git
git config --unset remote.origin.url
git config --add remote.origin.url https://github.com/llvm/llvm-project

This means I can checkout main, then git pull to refresh from llvm-project, create a branch, and then git push on the branch goes to my fork. Creating a PR from there through the web UI is straightforward. I find it much simpler than generating a diff and navigating the Phab UI to crank up a review.

8 Likes
15

Just some quick advice regarding email filtering, you are going to have a much better experience ignoring llvm-commits and filtering the notifications that come directly from github. There is a lot more useful information in the github notifications that you can use to filter on.

16

Just some off-the-cuff thoughts for my two cents. I find the GitHub interface usable but wholly inferior to the old Phabricator pipeline. Phabricator had a much nicer UI for doing active code reviews. There’s external tools, but I feel like i shouldn’t need to rely on third party software to have readable diffs. I found doing incremental changes much nicer in phabricator because we didn’t need to worry about a force push to a remote branch, it was just a diff. I commonly used features like stacked reviews and in-line code suggestions so I’m sad to see those go as well. In general the email spam is much worse like others have mentioned, I also find it a lot more difficult to identify which reviews are still pending, but maybe I’ll get more familiar with the github filtering. I use the GitHub CLI tool and it’s a sufficiently close replacement for arc when working in the CLI. One small annoyance is that it takes ages to manually add reviewers through the CLI, presumably because there’s so many to search through.

Thank you for bringing this up, it explains why I wasn’t getting any email updates anymore.

17

Thank you for asking for feedback on how this is going! And thank you (and Tobias and plenty of others) for all of your efforts to try to make this transition a successful one.

Good:

  • The transition is in progress and we’re still able to get work done while it happens (it wasn’t a “stop the world, set things up, restart the world and hope for the best” kind of transition).
  • We’re still being flexible as issues arise, like with extending the grace period for Phabricator use based on community needs.
  • The community is working together to try to resolve issues as they come up; it’s great to see this level of collaboration!

Bad (trying to avoid rehashing points already raised by others, so not a complete list):

  • The timing of this transition was unfortunate. We were in the middle of trying to get 17.0.0 out the door and moved forward with the transition regardless. This adds extra pressure on folks who are involved in the release process and that could have been avoided by changing the date to “once 17.0.0 is out the door”. A minor delay would have meant more time for release managers and code owners to focus on the release instead of splitting attention between that and the transition.
  • Speaking of timing, I was not left with good feelings about the fact that we moved forward with this transition without having resolved many of the most pressing concerns the community identified when we were asked what we needed for a successful transition. Some of those needs can be delayed (like static HTML export of phab reviews), but not having workflows in place for daily work, a replacement for Herald that generates so many difficult-to-filter emails, no stacked reviews, etc is disruptive and causing problems. Note: not resolving everything up front happens and itself isn’t an issue; but I expected clear and explicit communication of what hadn’t been resolved and someone to ask the community “is it still okay to move ahead in spite of this?”
  • Code review is now harder at the expense of it being easier to push changes. Phabricator, for all its faults, was dedicated to solving the problem of “how do you review code?”. GitHub is a jack-of-all-trades where code review is one minor component rather than a focus. Some of this is related to us not having documented workflows for GitHub, but some of this is the choice of tool. We should be investigating things like reviewable.io or others so we have a process focused on code review needs.

So far, my impression and the impression I’ve had from talking to folks in Clang (DMs, Discord conversations, Discourse, office hours, etc) is that this transition has been more painful than expected, but there’s optimism that things will continue to improve over time and the pain will subside. Transitions of this nature always have bumps along the way, and I think the community is positioned to address the bumps we can – but I think the real question is: what are we planning to do about the bumps the community has no control over (deficiencies with GitHub as a code review tool)? Are we going to solve that with process documentation, with different review tools, paying Microsoft for feature support, ignore the problem, etc?

11 Likes
18

Github actually does have that, see step 6 on this page

In that case, maybe the thing to do is un-subscribe from llvm-commits, and change your github notifications to get notified about everything. (As usual, I can’t find where to set this, although I know I saw it earlier today.)

1 Like
19

It is worth giving it a try. What is pushurl and url here?

20

remote.origin.pushurl and remote.origin.url are git configuration parameters within the clone. pushurl says, when you do git push, where does it go? and url is where a fetch/pull comes from. For example, in the clone of my fork, I literally did

git config --add remote.origin.pushurl git@github.com:pogo59/llvm-project.git

and so on. Hope that answers the question.