github: one PR, multiple commits. supported?

[AndrewSB]

is it possible to craft and upload a PR to github with multiple commits in it? or is that fundamentally diametric to saplings "one commit per diff" workflow?

i'd love to use sapling for my stacked PRs, but i need to be able to have multiple commits per PR

[elegios]

It's a bit hidden, but it seems like sl pr follow can be used to do what you (and I, for that matter) want.

sl pr follow -r .  # Mark the current commit as a 'follower'

This means it'll join a later PR, instead of creating its own. Note that follow (and unlink, which undoes follow) seem to silently do nothing if -r isn't given.

---

As an aside, it feels like sl pr submit --stack should do something like this, but I can't seem to figure out what it actually does. I would very much like it if sl pr just synced already created PRs, and sl pr submit created a PR for the current commit and automatically marked previous commits (that aren't already part of a PR) as follower, instead of automatically creating one PR for each commit. That, and/or a confirm dialog or something that shows what PRs will be created and how many commits they contain, before they're created and submitted.

[bolinfest]

Right now, there are four ways I know of that folks would like to create pull requests:

1. The ghstack way (supported by sl ghstack).
2. The "overlapping pull requests" way (supported by sl pr).
3. The "non-overlapping pull requests way" (i.e., "just force push"): https://github.com/facebook/sapling/issues/302
4. The "traditional" way on GitHub where one PR contains multiple commits.

I can see the merits for all four of these, and I would like to support them, but I would like to avoid having four subcommands for Sapling for creating pull requests. It turns out that implementing support for all four is not actually that difficult (particularly since 1 and 2 already work), but coming up with an UI that makes sense is the hard part.

There's also a secondary issue of ensuring https://sapling-scm.com/docs/addons/isl lets you choose which of the four schemes you want to use and making sure the UI reflects that choice, as appropriate.

/cc @spence-novata

[spence-novata]

@bolinfest does ISL currently support creating pull requests? I don't see anything in the docs or the UI

[bolinfest]

@spence-novata It does! ISL is documented on its own page, not the VS Code one, because it is also accessible via sl web: https://sapling-scm.com/docs/addons/isl#interacting-with-code-review.

[AndrewSB]

i'm all for the force push approach. is it possible to do update a stack with that approach today? @spence-novata are you force pushing to update your stacks somehow?

[spence-novata]

I'm afraid not

I'm doing the equivalent workflow manually

Also due to the fact that our ci can't cope with a bunch of builds landing at exactly the same time yet

[spence-novata]

@bolinfest reading the docs, isn't this a solved problem. Ie it's just a additional option on top of the existing pr and ghstack strategies?

[bolinfest]

@spence-novata Sort of...the sl pr follow thing makes things possible, but it seems clunky. Again, suppose you have A,B,C where:

• A is the top of the stack
• B has a PR associated with it
• C has been tagged as "follow"

Now you run sl pr from A. Should it:

• Create a new PR
• Declare itself the new head of the PR associated with B
• Not create or update any PRs.

I tend to think that, for an individual feature branch, it should be either a "one PR for the whole branch" or "one PR for each commit on the branch" situation. The user should probably opt into their preferred strategy for each feature branch they create in the repo, but it should be possible to change the strategy for an individual branch.

Though every configuration option like this arguably needs to be exposed in ISL, so it's important to find the right balance.

[AndrewSB]

being able to specify some commits as follows, and only create/update PRs for the non-followed commits is what i'm looking for. what i miss most from fb was being able to hg rebase -s 'draft()', to fix all my merge conflicts, then push all draft()s that were updated

i can already manually do this workflow with git (checkout A, rebase main, push --force A, checkout B, rebase A, push --force B, ... repeat for each PR in stack) being able to support this workflow would make switching to sl super attractive. both for me and many other developers who structure their work as stacked PRs on github

i think positioning sl as the best way to work with stacked PRs would be a super smart direction, at least until edenfs is released. sure the ISL is great too, but this would make it way more attractive, and much easier to pitch to other engineers i know

🙏

[AndrewSB]

the behavior i want from running sl pr from A. i'd expect it to create a new PR, which points to B's PR as it's base