drewdeponte / git-ps

Patch Stack workflow CLI extension for Git
MIT License
50 stars 4 forks source link

Add ability to request review of a series of patches #29

Closed drewdeponte closed 3 years ago

drewdeponte commented 3 years ago

The idea behind this is to add the ability to be able to invoke git ps rr <start-patch-index>-<end-patch-index> (e.g. git ps rr 2-4) to request review of a series of patches rather than just an individual one.

There are a couple use cases that have popped up already where this would be useful.

  1. You are stuck working in a Feature Branch methodology but you still want to get as much of the Patch Stack value as you can using it locally within your workflow. So you do Patch Stack locally and then once you are ready to request review for a "Feature" you simply request review for the contiguous series of patches that make up your Feature Branch.
  2. You really can't stand the fact that GitHub hides comments when the line of code they are attached to changes. So, you require your developers request review for their logically chunked patch to get started. But after feedback comes in and changes are requested. You have your developers make those changes as individual patches on top of the previous one. Enabling them to then request review of the series of the series of patches. Note: I am not a big fan of this. I think it starts to drive the users out of the mindset of logically chunked patches a bit and likely adds additional overhead of the developer remembering to squash/fixup the patches back into a singular patch to prep for publishing. But at some point you have to trust people to make good decisions for themselves or not so I think we should still add this feature.

It is worth noting that this also conceptually lines up with a practice that is sometimes used in the Kernel team's workflow. Where in certain cases people can submit a series of patches for integration. Generally they submit the series of patches for review individually but if the feature is complex enough and dependent enough sometimes they do submit a series of patches for integration with additional context via email describing why it is a series of patches, the order of the patches, and how they fit together. So this could work as parallel for that.

hanx2cho commented 3 years ago

Re: scenario 2 - in the case that you request review on Commit / Patch 0, and then add another commit and request review on Commit / Patch 0 - 1, the pull request should be the same. As in, it should not open a new pull request due to the number of commits changing, so that the reviewer can continue looking at the previously opened pull request.

drewdeponte commented 3 years ago

@hanx2cho thinking about branch association with this feature gets interesting.

It seems like we are possed with a number of scenarios and we probably need to figure out what to do in each of the scenarios.

  1. none of the patches in the series have a branch associated with them
  2. only one of the patches in the series has a branch associated with it
  3. multiple of the patches in the series have branches associated with them

In scenario 1 we would have to somehow identify the patches as being part of a series of patches and automatically generate a branch for that and then record association between that ps-series-id and the branch name and possibly also the ps-ids that are part of that series. This assumes however that the contents of a series isn't likely to change (grow/shrink) in number of patches.

In scenario 2 we could piggy back on the individual ps-id's associated branch. However this starts to blur the conceptual lines of things. The actual use case for a series of patches in the Patch Stack Workflow is to originally have each patch reviewed independently and then submit the "series" of patches for integration. So, it seems like we shouldn't overload the individual patches branch with other patches.

In scenario 3 we would have to have some arbitrary ruling for deciding which patch's branch we overload with the series. Instead we would have to create a new concept of a series of patches, record association with the patches, and record association with the branch. However, this takes away from the ephemeral/dynamic nature of a series of patches from being able to flex (grow/shrink) over time.

This also gets interesting when thinking about branch association which is currently done 1-to-1 with a patch. We don't have an identifier for the concept of a patch series. We could add an additional id to the comments of the patches to group them all to some ps-series-id which we could then associate to a branch. But this is getting far more complex than I would hope for a feature that dosn't really go along with the principles & concepts of the Patch Stack workflow. Also as discussed above it takes away from the ephemeral/dynamic nature of a series of patches over time.

So I think we need to not try and automate branch association and leave that responsibility up to the user.

In fact this actually fits my use case the best where one of my clients requires Feature Branch based PRs and they also require specific branch naming based on automation they have built out.

So we could do something like git ps rr 0-2 someFeatureBranchName and then not worry about the automated branch association at all.

This would also have the added benefit of allowing individual patch request reviews to occur without interferience.

drewdeponte commented 3 years ago

This has been completed and released as v0.7.1