modularml / stack-pr

A tool for working with stacked PRs on github.
Other
257 stars 8 forks source link

Feature request: implement dry-run mode #12

Open mwichmann opened 1 month ago

mwichmann commented 1 month ago

Hint: maybe enable github Discussions, so folks can ask questions without filing them as issues?

Interested in this, but trying to figure out if it fits in a particular project's workflow. It's hard to tell, if you create a stack of PRs, if the maintainer can merge individual ones as s/he finishes with them through the web UI "as I've always done", or if it has to be done all in one go from the cli (presumably by the submitter, since others are unlikely to have the branches locally in the right form). There's a hint at the latter in the workflow description with this line:

instead of merging them you should use land.

It's also not entirely clear how the workflow works once submit happens and reviewers start asking for changes:

If we need to make changes to any of the PRs (e.g. to address the reviewfeedback), we simply amend the desired changes to the appropriate git commits and run submit again.

That implies force-pushing, which is generally considered a no-no.

If needed, we can rearrange commits or add new ones.

Does that imply new PRs will be created too? Or will they be included in existing branches/PRs?

To come down to something resembling a suggestion/feature request - while some of us aren't really sure what's going on under the covers and whether it will be okay for our particular needs, could the project implement a dry-run mode that shows what would be done without actually making the live changes?

ZolotukhinM commented 1 month ago

Awesome questions, let me try to cover them! I'll also check if I can enable discussions (thanks for the suggestion!), but in the meanwhile I'll reply here:

if you create a stack of PRs, if the maintainer can merge individual ones as s/he finishes with them through the web UI "as I've always done", or if it has to be done all in one go from the cli

The PRs in the created stack will be chained in a sense that only for the bottom PR the base branch (the branch where the PR would be merged to) is main. For the second PR in the stack the base branch would be the head branch of the 1st (bottom) PR. For the 3rd PR the base branch will be the head branch of the 2nd PR, etc.

If you use cli and will try to land the whole stack, it will find the bottom one, land it, and rebase the rest - so everything will be correct.

If you use web UI, go to, say, the 3rd PR, and click "merge", then what will happen is that it will be merged into the branch of the 2nd PR - and that is probably not what you want. This is why the recommended way is to use cli. An astute reader would notice that merging through web UI should still work for the bottom PR, and this is indeed correct, so all in all as long as you understand what you're merging and where to, you can use web UI just fine. Other people can land your PRs too (I did that a couple of times, but that said I won't be surprised if when doing that you'd expose some edge cases that's not currently handled correctly - that's a path less frequently taken, so less tested).

One other thing about merging through web UI vs cli - when you use cli before landing the tool also cleans up the commit message a bit. E.g. it removes the stack info (links to other PRs at the top). If you use web UI the default commit message would have those, so you need to edit it manually before landing if you don't want to have them in your git history.

It's also not entirely clear how the workflow works once submit happens and reviewers start asking for changes: That implies force-pushing, which is generally considered a no-no.

Force pushing to branches that you share with other people is a no-no, but it's usually ok to force push to your own branches. But yes - if that's not something you can allow in your repo, you won't be able to use the tool, it does force-push to branches it creates (but never to main).

If needed, we can rearrange commits or add new ones.

Does that imply new PRs will be created too? Or will they be included in existing branches/PRs?

If a commit already has a PR associated with it, then that PR will be used after reordering as well (when the tool creates a PR for a commit it adds a special line to the commit message that later is used to associate this commit with that PR). If you add new commits to your stack, and they don't have PRs associated with them (read: their commit messages don't have metainfo pointing to an existing PR), then new PRs will be created for such commits.

To come down to something resembling a suggestion/feature request - while some of us aren't really sure what's going on under the covers and whether it will be okay for our particular needs, could the project implement a dry-run mode that shows what would be done without actually making the live changes?

To some extent you can use stack-pr view as a "dry-run". It will show you 1) what commits are considered part of the stack 2) which of them are associated with existing PRs and which are not

And that command never mutates anything, so it that sense it's "dry".

Actual dry-run which would show real commands that are going to be executed is tricky to implement here because the commands depend on each other (e.g. until we actually run gh pr we don't know the PR number). But it should be possible to print an approximate plan for commands to be run, if that'd be helpful.

Personally, I recommend creating a toy repo and play with the tool on it - that in my experience is the fastest way to see what the tool does and how it works.

mwichmann commented 1 month ago

Thanks for those explanations, helps a lot.

Personally, I recommend creating a toy repo and play with the tool on it - that in my experience is the fastest way to see what the tool does and how it works

I was coming around to that conclusion too, seems a good way to get familiar without upsetting someone else :-)