search

drewdeponte / git-ps-rs

Official git-ps Rust implementation - the future of git-ps
https://git-ps.sh
MIT License
78 stars 8 forks source link

Add `--stack-prs` option to the `request-review` command #263

Closed drewdeponte closed 10 months ago

drewdeponte commented 10 months ago

This option should only be applicable when requesting reviewing of a patch series.

The default behavior of the command should be to create a single branch and cherry pick the entire patch series into that branch and then create a pull request from that branch.

However, if the user includes the --stack-prs option it should instead create a stack of branches, one for each of the patches, and then it should create a pull request for each of those branches. However, the pull requests should be to destined for the previous branch. To understand this lets look at an example where we have a patch series of A, B, C.

We would have to create a branch for A. Let's call it BA based on origin/main and a pull request destined for origin/main. Then we would have to create a branch for B. Let's call it BB based on BA, and a pull request destined for BA. Then we would have to create a branch for C. Let's call it BC based on BB, and a pull request destined for BB.

Note: Ideally the pull request names would have in index as part of them indicating to peer reviewers what order these pull requests need to be reviewed and integrated.

I do have some concerns about this approach as it seems brittle. If a peer reviewer reviews and integrates a patch in the wrong order what happens? It also seems like the once the first patch is merged in and that branch gets cleaned up that the second pull requests will be invalid and therefore it won't work. This needs further investigation, thinking about and discussion. But people are saying this is what graphite.dev does, which is a Git Stacked Branch tool rather than a Patch Stack tool. But, it is also worth noting that they have an extension for GitHub.

drewdeponte commented 10 months ago

Ok, I did some testing with this stacked pull request just in terms of GitHub.

It seems to work with GitHub at least when you merge via the Green Merge Button when you have it configured to create a Merge Commit.

However, if you use the Green Merge Button when you have it configuredto Rebase + Merge. After Rebasing + Merging the first patch in the series. The second patches PR ends up showing two commits (the commit that was previously rebased and merged and the commit tied to the current PR). It also has a conflict. Even worse, the correct way to resolve this would be to rebase that branch onto main. But instead GitHub just changes the base to main. Which makes sense where it would show the two commits. So the only option is to manually resolve the conflict in the web based editor, or rebase the branch on the command line and do a force push up to resolve the conflict. The problem is that you will have exactly the same issue again on the next patch PR, except you will have to deal with 2 layers of conflicts in that one. Because it will have a total of 3 commits. Two of which have already been merged in but GitHub isn't smart enough to know.

So it seems pretty clear that with GitHub this approach can work only if you do Green Button Merge configured to create a Merge Commit. Meaning you can't have a linear history with this approach.

These inconsistencies are make me even more skeptical of this approach. It feels like a hack and is extremly brittle.

drewdeponte commented 10 months ago

I think the other question I have is do all Git hosting providers work this way with their PR handling? If not it is yet another point of brittleness. Also it seems a bit like a support nightmare as well.

drewdeponte commented 10 months ago

Also if a reviewer merges a PR in the wrong order it breaks everything. Yet another point of brittleness.

Alizter commented 10 months ago

You could create the other PRs as draft which might address the issue with the wrong order.

drewdeponte commented 10 months ago

Draft PRs in GitHub have merge disabled. So you would have to manage switching them from Draft to not draft after each is merged. This seems like madness to me. It seems like you might as well just open the PRs after the dependent one has been merged upstream.

Alizter commented 10 months ago

There could be a hook when you integrate to undraft the next PR, but this sounds kind of convoluted.

drewdeponte commented 10 months ago

Another approach would potentially be to have each branch include the dependent patches in it, but have it be targeted for origin/main. This has the same problem of one the first patch PR is rebased and merged the following PRs have conflicts and have to be manually resolved. However, it does kind of solve the merging out of order issue because if someone reviewed and merged the 3rd PR it would include the commits of the first two.

I am not suggesting this is really any better. I think this is still fundamentally horrible in my mind.

It feels like we are fighting against the core model of the tooling instead of just accepting the constraints of the current modeling of the tooling.

jameswalmsley-cpi commented 10 months ago

I don't think this idea follows the core model of of git-ps "or stacked-diffs" in general. I think there should really be 2 main ways of interacting with e.g. github etc (as we currently support):

  1. Isolate a single patch, and push as a PR.
  2. Isolate a series of patches, and push as a single PR.

The use-case you describe above can be handled by 2.. or if the developer really wants to simply submitting each in order, and waiting until each have been integrated before submitting the next.

So if the aim is to break a larger PR into smaller reviewable units, simply use 2 (multiple) commits and encourage people to look at the commit breakdown to understand the PR.

drewdeponte commented 10 months ago

That is how it behaves today. At least with the version in main.

My instinct with this is that people who want stacked branch based PRs actually want a fundamentally different tool than GitHub and they should likely be looking at something else like graphite.dev which provides an experience similar to that.

jameswalmsley-cpi commented 10 months ago

That is how it behaves today. At least with the version in main.

Yes. Just noticed that recently, it makes the tool essentially perfect for my mental model / use-case.

Stacking branches to implement the "patch-stack / stacked-diff" model is a mistake IMHO. Unless you have some tool that magically manages github directly so all the issues you identified above don't exist.. then fine.. but that then becomes a rather custom github-only tool.. and I much prefer the way you have gone.. which implements this model really well for any "pull-request" based service.

drewdeponte commented 10 months ago

We have decided not to do this until we discover a way that works well with the model GitHub, Bitbucket, or GitLab have. At which point this issue can be reopened.