Non overlapping PRs

[spence-novata]

Would people be favorable to a PR strategy where each PR's base is the head of the previous PR?

So instead of

pr1 -> master
pr2 -> master
pr3 -> master
pr4 -> master

You have

pr1 -> master pr2 -> pr1 pr3 -> p2 pr4 -> p3

This presents a few benefits over the current strategy of overlapping PRs:

• PRs can be merged on GitHub (you still wouldn't want to merge out of order, and draft status PRs would help manage this)
• As you merge previous PRs and delete their branches, GitHub automatically updates the base of the next PR to the branch the previous PR was merged into (so after you merge pr1, GitHub will magically make your PRs pr2 -> master, pr3 -> pr2, pr4 -> p3)
• The changes of each commit can be reviewed in isolation on GitHub. The work on ReviewStack is admirable, but it doesn't feel as mature as the Sapling CLI and hasn't reached parity with GitHub PRs regarding features or experience.

[bolinfest]

To be clear, I don't want to offer overlapping PRs: it exists as a "best effort" workaround to limitations in GitHub. To the best of my knowledge, they are the only option for stacks when you do not have write access to the repo.

This is because GitHub requires that the base commit is a named branch in the target repo. If you do not have write access to the target repo, then you cannot create arbitrary named branches there.

Someone has complied a list of trade-offs for the various stacked diff approaches for GitHub: https://github.com/gitext-rs/git-stack/blob/main/docs/comparison.md

[spence-novata]

Is it not possible to offer a non-overlapping strategy behind configuration for those that do have the required access?

[bolinfest]

@spence-novata that is sort of, but not quite, what ghstack is. Though admittedly, ghstack does some extra tricks to avoid force-pushing (some GitHub projects have a "policy" of "no force pushes" and the GitHub UI doesn't always deal with them well), which admittedly creates some unnatural branches.

If you're OK with force-pushing, then I suppose there is another avenue like the one you describe, which I take it is what you're asking for?

[spence-novata]

Yeah the whole base/head branch thing that ghstack does doesn't really leverage the way GitHub automatically changes the base as branches are merged and deleted and still means you can't merge in the GitHub UI

I'm happy to force push as this is what I already do manually.

[bolinfest]

Honestly, I think I would prefer this, as well, particularly for personal projects. Part of the reason force-pushing causes problems in GitHub today is that GitHub periodically reaps commits that are not part of any named branch, so force-pushing "abandons" your old branch. I added an almost comical workaround for this in sl pr:

https://github.com/facebook/sapling/issues/302

Though even if we support this new mode of stacked pull requests, I tend to think sl pr still needs to be default because it's the only thing that works everywhere because it does not require write access.

[spence-novata]

Yeah that makes sense, tbh I had imagined this being either an opt in arg on sl pr and/or a config setting

[zengk95]

+1 to this. I think I suggested it in here https://github.com/facebook/sapling/issues/284

My use case is on a GHE instance and review stack is not supported. This makes stacked PRs hard to review cause PRs later on just contain all the changes prior.

[bolinfest]

@spence-novata @zengk95 as noted in 166e2640d3, I have experimental support for this workflow. If you're willing to build from source, you can try it today, or you can wait for the next release? I would love to get feedback on this approach!

[FirelightFlagboy]

Hello, does this experimental workflow still available on 0.2.20231113 ?

I would like to try it but the configuration github.pr_workflow isn't listed in sl config --verbose and the workflow provided by ghstack isn't something I want.

Regards

[oleg-codaio]

This works now (running 0.2.20240219); run: sl config --local github.pr_workflow single.

However, merging the bottom-most PR in the stack results in a merge conflict on the next PR:

Even if it didn't, the next PR still has commits from downstack PRs. To fully support this workflow there might be two solutions:

1. When merging a stack at a given PR, change the base of that PR to main and merge that, then close all the PRs below it in the stack. This is the approach that https://github.com/ejoffe/spr takes with git spr merge. The downside is it's harder to git blame changes to the respective PR where code was actually reviewed. Another issue is that the list of PRs in the body in upstack PRs will exclude prior PRs merged in the stack.
2. Alternatively, add some sl pr merge command that starts from the bottom of the stack, merges the PR, then rebases the stack, then repeats for all the remaining PRs in the stack (waiting for status checks as needed). With a local-only approach this is bottlenecked on CI; Graphite has done something similar but that process happens on their end so it's much more convenient.