Is there any stacked-PR methodology consensus?

Project Infrastructure GitHub
1

This is intended as a continuation of previous discussions around code review on GitHub, in light of the GitHub support for the pull request stack as a reified object in their code review model.

It seems like GitHub has chosen to codify:

  • The requirement that stacked PR head branches be pushed to the target repo (i.e. not to user forks)
  • The requirement that the head branch of a “parent” PR in the stack is the same as the base branch of its “child” PR
    • This means, in practice, that force pushes are required for just about every operation with the new stacked PRs
    • Edit: as @jh7370 points out, there is also the option of letting the code of each PR diverge as changes are added to its “parents”, although it seems like (at least presently) there isn’t any way to automate this (or at the very least, the official gh stack cannot do this)

We currently discourage force pushes (LLVM GitHub User Guide - Rebasing Pull Requests and Force Pushes) and AFAICT the new stack support doesn’t come with significant improvements to the problems that force pushing can cause for reviewers:

  • By-line comments from the old versions of the PR branch generally lose their context, unless some narrow conditions are met (the line they reference not moving or being edited?)
  • The link to “view reviewed changes” for those unmoored comments doesn’t show them in their original “reviewed” context: it either shows them in the latest version or can’t show them at all
  • The “changes since you last reviewed” may break, and there is no way to compare versions of the PR
    • Even if the relevant versions have not been garbage-collected, if there was an intervening rebase there is no way to see only the diff relevant to the PR in question (unless you use something like @nhaehnle’s diff-modulo-base or git range-diff)

Gerrit

In light of what I consider to be a pretty poor showing from GitHub on implementing the stack feature, I took a look at other options again. The one which seems the most viable to me is to support (but not require) using a hosted Gerrit for review of patch series. See Code Review Process Update - #23 by jyknight for some background from when this has been discussed before. The basic idea boils down to:

  • PRs are still the “default”, and the on-ramp for new developers.
  • If a change is easier to review in parts (e.g. an NFC refactor, followed by the core change, followed by an NFC “strangling fig” type cleanup) it can be reviewed on GerritHub.
    • The GitHub PR can be “imported” into GerritHub, so the decisions doesn’t even need to be made ahead of time, the review can move to Gerrit as-needed.
    • Once the review is complete on GerritHub, the result can be merged back into the Github PR.
    • The PR can be merged in the usual way.

This flow would mean that there is still always a PR for every change going into the repo, it may just not contain all of the review comments and patch versions. I don’t think this really represents an issue, since GitHub tends to throw these artifacts away for stacks anyway. Also, if GerritHub is ever about to go away, we can always mirror the relevant reviews and host them read-only in the same way we have with Phabricator reviews.

In looking into this option, I came across CUE, which explicitly supports both GitHub PRs and GerritHub CLs.

It seems like LLVM could also get a subdomain just for our org; see https://cue.gerrithub.io/ where the CUE project works on GerritHub.

From looking at some Gerrit issues releated to GerritHub, along with what little discussion I could find online, it seems like support is very prompt and competent. I also think the risks are relatively small, in that the worst case scenario is that we just go back to exclusively PRs (where we are already).

The status quo on GitHub is abysmal in my experience, so I would be willing to take on the role of engaging with the GerritHub folks to get a better idea of how everything would work, if there is any interest in this.

3 Likes
[RFC] Upstreaming nanoMIPS support to LLVM
2

I haven’t yet used the official GitHub stacked PR flow, so I’m not particularly familiar with the terminology. What do you mean by “head branch” and “base branch”? In my manual stacking of PRs, I’d have one branch per PR, e.g. branch1, branch2 and branch3 with branch2 initially being branched off the head of branch1 at the time of PR creation of the PR for branch2 and similarly branch3 being based off of branch2. After that, fixup commits can be made to the individual branches without a problem. The downside with that is without a rebase/merge into branch2/branch3, they don’t get the fixups from the earlier branches, although in my experience this is usually not a problem.

Is the stacked PR feature still in public beta or has it been officially released now? If it’s in beta, can we feed back the pain points and see if we can get improvements made?

I’m against anything that requires people to review outside of native GitHub, in order to be able to properly review a piece of functionality, whether it’s in a stack or not. Doing so makes it harder for newcomers and for people used to the GitHub UI, due to the context switching if nothing else. I believe we already have workflows that enable stacking without requiring reviewing outside of GitHub. I’m also fairly opposed (but not quite so strongly) to anything which would mean a review can end up being done in two places, potentially due to different reviewers preferring different systems to review something. If the comments in one system are automatically migrated to the other, that’s fine, because then a reviewer in one system can see the comments made in the other.

3

What do you mean by “head branch” and “base branch”?

Think of a PR as a request to pull changes “from the head” and merge them “into the base”. So for a non-stacked PR the “base” is generally main. If you’re familiar with the various uses of head and HEAD in git you can also orient from that (i.e. the PRs “head” is the head of the feature branch).

branch1, branch2 and branch3 with branch2 initially being branched off the head of branch1 at the time of PR creation of the PR for branch2 and similarly branch3 being based off of branch2. After that, fixup commits can be made to the individual branches without a problem. The downside with that is without a rebase/merge into branch2/branch3, they don’t get the fixups from the earlier branches, although in my experience this is usually not a problem.

Do you also push a “clean” version of the patchset to somewhere so reviewers can see it? I frequently review changes locally, often building them and tinkering a bit before responding. It seems like your approach means that isn’t possible in general?

I think I’m uncomfortable with the inconsistency between reviews that you’re describing. It is maybe a personal issue of mine, but I don’t do well with “it is fundamentally broken, but usually that isn’t a problem” compromises.

I’m also curious how you manage this locally? Do you work directly out of the diverged branches?

Is the stacked PR feature still in public beta or has it been officially released now? If it’s in beta, can we feed back the pain points and see if we can get improvements made?

At the pace GitHub moves, and with their track-record for half-baked features, I am just not optimistic. They have fundamentally hinged the entire model around the core pain point that drives much of the developer demand for the feature to begin with, which is the requirement to force-push (or else review “the wrong code”).

As it stands, they effectively just added a <ul> to their UI and hacked a stack object into the API and said “there you go.”

Ultimately it seems they did not actually engage with many of the developers who were requesting the feature, nor any prior art (from something like Gerrit) and I’m not interested in asking them to “try again, but for real this time.”

I’m against anything that requires people to review outside of native GitHub, in order to be able to properly review a piece of functionality, whether it’s in a stack or not. Doing so makes it harder for newcomers and for people used to the GitHub UI, due to the context switching if nothing else.

Cheekily I would say that GitHub is the thing that makes it impossible to properly review a piece of functionality on GitHub, but I do take your point.

My argument here is that the fundamental shortcomings of GitHub make it harder for me to review code, and I’m not alone. I agree we should balance the “on ramp” for new contributors with the needs of established developers, and that is why I would propose encouraging PRs as the default, with Gerrit available for when changes outgrow them.

There has to be a goal to bringing in new developers. We should strive to get them to a point that they do outgrow the fundamentally flawed PR model, and start to wish they had better tools.

I believe we already have workflows that enable stacking without requiring reviewing outside of GitHub.

I think I just don’t agree here, which is maybe the fundamental issue I have. I think we have a (now official) hack with a lot of pain points, not a workflow.

I’m also fairly opposed (but not quite so strongly) to anything which would mean a review can end up being done in two places, potentially due to different reviewers preferring different systems to review something. If the comments in one system are automatically migrated to the other, that’s fine, because then a reviewer in one system can see the comments made in the other.

If there is any traction to the idea I will be sure to get all the details of how the synchronization works for comments. I do agree that this is the biggest issue I foresee, and will admit that I haven’t put in the time to understand quite how this works out of the box.

4

Let’s not do this. -inf from me.

1 Like
5

Noted, although I have no idea how to incorporate just “no” into the decision making process. If you have any arguments (even just bullet points of whatever is going through your head when you see “Gerrit”) that would help!

Edit: I didn’t mean this to come off as dismissive, I have just genuinely only had positive experiences with Gerrit in my past lives using it. If you have the time/energy, it would genuinely be a great help to collect the reasons why using Gerrit would be a bad idea.

6

Gerrit is a non-starter, let’s not restart that conversation.

There’s no benefit in discussing about “my favorite tool that has been discussed before” because every tool has pros and cons, and what we have today is the lowest global minimum efforts after years of discussion, not necessarily your or my local minima.

We’ve gone through that many times (since before we moved to Git / Github, 10 years ago), and Gerrit has been shot down at every turn. I don’t think renewing that conversation is going to do us any good. Let’s focus on how to make the best out of the tools we have.

Edited to add: I don’t mean to be rude, just that this kind of discussion goes on forever for no benefit, and tends to create schisms in the community. This is exactly what happened in every previous thread that mentioned Gerrit. I’m just trying to avoid another one.

5 Likes
7

Github has its limitations, but

  1. Storing reviews outside of Github is out of the question.
  2. Hosting our own instance of a third-party tool has been ruled out when it was considered for Phabricator. Besides the reasons already discussed, instead of one platform with issues we’d have two.

Nothing is perfect and a lot of people have different visions of what an ideal tool should look like. Sticking with Github will at least avoid fragmentation of the review/commit process, which IMO has a lot of value on its own.

1 Like
8

I would like to restart the conversation, but I definitely empathize with how these discussions tend to go.

I don’t feel like I am entitled to the time the discussion will take, but if others do have the energy I would like to at least understand the core problem with adding GerritHub as an option. We already encourage using both Graphite and Reviewable.io, which seem to have the same issues of not having perfect 2-way sync, and requiring signing into another service to engage with the review happening there directly. Do we just discourage actually doing any review there, or does that just not occur in practice?

I think another stable place I could see is to just accept force-push as a least-bad general option, at least as long as that is GitHub’s own position. Is there any appetite for discussing that? I have a tool which produces interdiff-like comments when force-pushing, and plan to add an option to automatically push tags (to the user’s fork, so it doesn’t clog up the main repo) to save old versions from garbage collection.

Then, the comment could look like:

Changes from [v1](<v1-commit-link>) to [v2](<v2-commit-link>) ([compare](link-to-compare-view)):
```diff
...
```

When a reviewer comes back, they can use the interdiff or native github compare/ view to orient themselves, and can see any previous version. They will have to map in “outdated” comments themselves, but the alternative is non-stacked, massive PRs that are unreviewable on their own merits, or ad-hoc stacks where changes either aren’t propagated down the stack at all, or are mixed in with the actual change under review.

I’m sorry for drudging this up again, I really am, but the current situation is slowly eroding my will to keep working on LLVM. That isn’t necessarily LLVM’s problem, but I posted here in hopes of understanding if anyone is in a similar position, and whether there is any light at the end of the proverbial tunnel.

9

That could be useful. Does the user need to do something when pushing or is this some local hook? Specifically, when a user does gh stack push, would this tool add the diff automatically?

I looked at the linked PR and it looks nice.

10

The user doesn’t use gh stack, as the tool is essentially an alternative to gh stack. The interdiff part could be integrated into to gh stack as a hook, though, at least in theory.

Assuming one does use praddle, you just work locally using one commit per “change” (where a change maps to a PR 1:1) on a single branch (where a branch maps to a GH stack 1:1), and then run praddle push to ask the tool to:

  • Fetch current remote copies of every change in the stack (if any)
  • Discover existing PRs for every change in the stack (if any)
  • Discover which stacks those PRs belong to (if any)
  • If everything looks OK, make all the edits to reflect your local stack using PRs and PR stacks:
    • Update remote branches (--force-with-lease and --atomic to avoid races/partial updates)
    • Update stack membership so the stack gets the new shiny UI
    • Update PR titles/bodies (so they reflect changes in the local commit messages)
    • Calculate and post the interdiff comment on your behalf
    • …

The goal was to make it completely unobtrusive locally (it never modifies any local refs, just remote tracking branches) and to be stateless, so e.g. working from another machine does not require synchronizing a state file (like gh stack seems to require). You can just pull down the last PR’s branch and immediately praddle push and it will work.

One big caveat is that it requires a commit-msg hook that ensures commit messages have the Change-Id trailer. Right now the hook is just lifted from Gerrit and baked into the praddle executable so it can be installed with praddle install-hook, but recently there has been the beginnings of an effort to standardize this, see Gerrit, GitButler, and Jujutsu projects collaborating on change-id commit footer. Fundamentally this is only really required to get the stateless part, but it is how praddle specifically works right now.

I also realized the link I sent wasn’t to a review made with the latest version where I threw together support for the new GitHub stack stuff. This is a sample of what that currently looks like. Note that the pinning of refs and enumerating them as “versions” that can be compared with the native GH compare/ view isn’t implemented there yet.

Also note that an old buggy version of the tool posted a bunch of useless comments that I didn’t notice at the time; that’s been fixed, but as I haven’t built a test suite yet I can’t claim I’m terribly confident the tool is not still broken in some way. If there were genuine interest in it, and it seemed possible it could be blessed as at least an acceptable tool to use for LLVM reviews, I would find a way to commit more time to polishing it.

11

While force-push are discouraged, I don’t believe they are banned. As part of transitioning from Phabricator to GitHub PR they were identified as necessary to match how we worked with “stacked PR” before even using GitHub, that is: having a local stack of commits, each one matching a PR/revision, and just rebasing and editing each commit, keep a clean linear history locally (this is my local workflow for over a decade so far).

1 Like
12

Maybe I have taken the suggestion on that page too literally, and it is OK for me to keep force-pushing for my stacked PRs?

It is still not ideal, but I can at least work in that world. I just don’t want to make the lives of reviewers any harder, and have been reminded on a couple reviews of the fact that force-push is discouraged.

13

Before gh stack, with manually created stacks the “update branch” option would do a merge commit. You had to do it by hand, and you could do it only for a part of the stack (some range of bottom PRs). So there was a way to manage a stack without force pushes. You just had to remember to pull the upsteam commits to your local repo (to avoid extra work to fix it later).

Now gh stack uses force pushes as a part of its function. It’s just how it works and if we use it, we have to accept force pushes as a part of the process. I’d still recommend avoiding them when not necessary (because of the known problems), but at this point I consider them “legitimate”.

1 Like
14

I’m not really sure I understand your query, but I’ll try to explain my thought process.

Until a branch has been posted for review (via a PR) and indeed even afterwards until somebody is actually reviewing it, I have no issue with force pushes to clean things up. However, I also have zero problem with lots of small commits in a branch to fix things. I almost never look at a PR as a sequence of commits and instead just consider the current tip as the thing that needs reviewing versus its base branch.

If I spot an issue that really needs to be in the rest of the stack, I often do have to do a mass rebase (which I do via the command-line). Typically this happens before the review takes place, but it’s certainly not impossible that there would be a problem that is found during review that needs to be reflected in the later patches (e.g. an interface change). However, a merge commit would typically be sufficient to update the branch (and in a manual process I’d then do that in each subsequent branch, which is obviously not ideal, but could be improved with tooling of some form - recently I’ve been using codex to help me with this sort of workflow internally).

I’m not sure what inconsistency you’re referring to here. I will admit that I have a high tolerance for less than perfect systems though.

I’m already ahead of you in this queue of people :smiley:. I was opposed to moving from Phabricator in the first place and still somewhat prefer that tool, but that ship sailed long ago (and admittedly GitHub has improved since then).

Not from my point of view. The diff versus previous review is my primary means of doing reviews after my first view of something. I have zero interest in having to go outside the existing UI to be able to view that diff and make comments on it. I also need the ability to easily switch between that diff and the whole picture, plus the conversation view, which I tend to have in a separate tab, so again tooling isn’t likely to help me here (barring an entirely new review system).

That being said, as others have already said, the force push prohibition is not a hard and fast rule, so if a mechanism requires it, I’ll cope (though if merge commits can be used instead, that would be far superior, as long as they disappear when merged into main). I also have no problem with scripts and tools being made available for others to use, if it helps them with their review workflow (without making mine worse…).

15

I think this is where my problem is. There is already plenty to keep in my head when posting code for review, or when reviewing others’ code, and “there may be some stuff missing here, it may matter, it may not” seems like a really really bad thing to add on top of that.

I think I will just go forward with the force-push-always flow, until someone who cares enough about it automates the merge-only flow to an acceptable level (i.e. without touching local branches). I will just be ignoring requests from people to not force-push, since it is not practical to do so, and there is no prohibition on it.

I’ll try to avoid it for non-stacked PRs, but I don’t often make those, and those are the easiest to review anyway.

1 Like
16

I think this solution is feasible, but people who are unfamiliar with the GerritHub platform may need some time to familiarize themselves with the new submission process.

Because many open-source contributors accustomed to GitHub’s maintenance processes need to adapt their basic code collaboration workflows when switching to GerritHub, it’s recommended that the corresponding Code of Conduct documentation be updated accordingly. This documentation should clearly explain the differences in collaboration processes between GitHub and GerritHub.

17

Well, thanks for raising the issue of coming to some consensus on what to use for stacked PRs, we need to have this conversation.

I don’t want to dogpile, but I agree with other folks that we shouldn’t fragment review communication spaces more than they already are. Review comments aren’t throwaway content. I regularly dig up old Phabricator reviews to understand why things are the way they are, and I wouldn’t want to split that information between Gerrit and GitHub.

It seems like the main downside of GitHub PRs is that it attaches comments to changesets and it isn’t capable of carrying them forward through a rebase/force-push operation. It’s crappy, but it’s honestly just not that crappy. Fragmenting communications just to work around this seems worse than coping with the status quo.

Someone someday might build a better code review tool that we’d want to switch to, but I don’t have confidence that enabling Gerrit today is the first step towards a wholesale Gerrit migration. If we really had a serious proposal to go all the way with more benefits, we could start a trial, but without that, I don’t think it’s worth it.

Everyone has their own experience, but I feel like “abysmal” is a much stronger adjective than I’d use.

I actually would like to reconsider that. Now that we have github PR stacks, I’d like to recommend that we migrate to github PR stacks. I have a pending draft RFC to refresh the committer policy that will try to make this possible by more freely granting access to users to write user branches so they can use stacked PRs without granting them unilateral write access to main, but that’s a separate topic.

It’s definitely OK. Everyone just force pushes. I use https://jj-vcs.dev to manage patches now, and while I have started to master the merge workflow, it makes it so simple to edit a change in the middle of the stack and then everything implicitly rebases without doing anything and all change ids and branch names (bookmarks) remain stable.

jj git push just implicitly does --force-with-lease under the hood, and it’s more or less fine. I try to avoid it, since I’m aware of what it can do to PR comments, but I try not to drive myself crazy.

If I was to use a merge workflow, I would have to ripple-merge changes up the stack and do a ton of branch (bookmark) motion.

1 Like
18

FWIW, it’s not the PR comments that I’m most concerned by (it’s annoying, but I’ve got used to having a tab with the file view and a separate tab with the conversation view open in GitHub). What makes force pushes fundamentally problematic for me is that it prevents “changes since your last review”, since especially in a larger review I rarely care about stuff I’ve long ago decided was fine in a file and am only interested in the changes that have been made since my last review. I can then look at the comments I made in that review and ensure they have all been addressed as expected.

image
image653×221 14.6 KB

19

Yes, this is really unfortunate. Force-pushes don’t need to cause problems, it’s just that the GitHub UI deals with them extremely poorly. Which was kinda-sorta acceptable in the past, because GitHub’s model has always been to prefer uploading new commits to your PR branch, instead of iterating on a single commit.

But, now, GitHub has gone and released its new stacked-PR feature. While they could have had it push updates to the stack as new commits on the PR branch (as some previous tooling like spacedentist/spr did!)…they did not do so. Instead, it requires force-pushes. But they didn’t fix any of the long-standing issues which force-pushes cause in the GitHub review UI.

As a contrast: gerrit code-review has been able to deal with this perfectly well for its entire existence. (Gerrit’s model requires rebasing, because it was built from the start for a workflow like the Linux Kernel development model: in that model, you’re expected to send a large stack of small well-crafted commits for review, and iterate on the stack, together, to respond to review comments, in order to finally get a “perfect” stack of commits which can be applied.) So when Gerrit’s diff/comment viewer shows you the changes between the latest version of a particular patch in the series and a previous version of that patch, it doesn’t get confused by changes to the baseline of the PR. Instead, it effectively shows the diff of the diffs, and distinguishes changes that were just due to a differing baseline. And so on, through the rest of the UI…

1 Like
20

I’m trying to create my first official GitHub stack right now, and I agree that they seem to have picked the worst of both worlds. Either you stick to the commit-per-PR model of Sapling, jj, Gerrit, etc., with something like a PR URL or Gerrit’s Change-Id trailer or jj’s reverse-hex change IDs to serve as a stable identifier, or you do branch-per-PR but allow manipulating those branches freely. Requiring separate branches and mandating rebases for them is just confusing.

That being said, `gh stack sync` always rebases, which breaks PR review history · Issue #354 · github/gh-stack · GitHub claims to have found a technique which allows you to do merges on stacked PR branches without running into extraneous conflicts. Idk if it’ll break stacked PR lands (such that you’ll have to merge PRs one-by-one instead). I’m going to try it for my stack, and I can report how it ends up working.

1 Like