Skip to content

Support modifying order of unapplied patches #144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
NonLogicalDev opened this issue Jul 28, 2021 · 7 comments
Closed

Support modifying order of unapplied patches #144

NonLogicalDev opened this issue Jul 28, 2021 · 7 comments

Comments

@NonLogicalDev
Copy link
Contributor

NonLogicalDev commented Jul 28, 2021

It would be very helpfult to some more advanced usecases to be able to modify the order of patches later in the stack, to bridge the gap between stg and git rebase -i a little more.

This way it would be possible to reorder future operations without any conflicts and the start going through the stack and taking care of issues as they come up.

I have not looked very closely, but I am thinking this should not be very difficult to implement, as stg delete already allows you to modify the future, albeit only by deleting patches from the future.

I propose modifications to two commands push and float by adding --noapply flag:

  • stg push --noapply [patches...]
  • stg float --noapply -s series-file

I believe this can make it a lot simpler to write a shell script to implement patch guards from #70

guards_pass() {
  SHA=$1
   # check if patch should be pushed (could check git notes for guard annotations or patch name)
}

# all patches for simple demo, but can check only unapplied + hidden patches in the future.
PATCHES=$(stg series --noprefix -a) 
GUARD_PASS=( )
GUARD_FAIL=( )
for patch in $PATCHES; do
  if guards_pass $(stg id $patch); then
     GUARD_PASS+=( $patch )
  else
     GUARD_FAIL+=( $patch )
  fi
done

stg pop -a
stg push --noapply "${GUARD_PASS[@]}"
stg hide "${GUARD_FAIL[@]}"

This is certainly not exactly the same idea as Mercurial Queue Patch guards, as it assumes that patch sets are not interleaving. But it can easily be extended to take care of interleaving patchsets also as long as you keep track of global patch order in a separate file along with constraints:

patch-list.txt (to be read into PATCHES var)

patch1 +guard1 -guard2
patch2 +guard2
patch3 +guard1
patch4
patch5

Example Workflow:

## Given this stack:
+ setB-p1
+ setB-p2
> setB-p3
- setB-p4
- setA-p1
- setA-p2
- setA-p3

## Work on patch set  1:
$ stg pop -a
$ stg push --noapply setA-p1 setA-p2 setA-p3

## Stack should look like this now: (* = stack changes for illustration)
- setA-p1 *
- setA-p2 *
- setA-p3 *
- setB-p1
- setB-p2
- setB-p3
- setB-p4

$ stg push # now on `setA-p1`
# ... resolve conflicts / adjust `setA-p1` 

$ stg push  # now on `setA-p2`
# ... resolve conflicts / adjust `setA-p2`

# NOTE: not addressing conflicts or changing `setA-p3` yet
## Stack should look like this now: 
+ setA-p1
> setA-p2
- setA-p3
- setB-p1
- setB-p2
- setB-p3
- setB-p4

## Switch to working on set 2:
$ stg pop -a
$ stg push --noapply setB-p1 setB-p2 setB-p3 setB-p4


## Stack should look like this now: (* = stack changes for illustration)
- setB-p1 *
- setB-p2 *
- setB-p3 *
- setB-p4 *
- setA-p1
- setA-p2
- setA-p3

$ stg push # now on `setB-p1`
# ... resolve conflicts / adjust `setB-p1`

## Stack should look like this now:
> setB-p1
- setB-p2
- setB-p3
- setB-p4
- setA-p1
- setA-p2
- setA-p3

# etc...
@NonLogicalDev
Copy link
Contributor Author

@topher200 / @jpgrayson Do you have any thoughts?

@topher200
Copy link
Contributor

@NonLogicalDev Thanks for tagging me! I hadn't seen this Issue.

I could see how this would be useful. I occasionally want to re-order non-applied patches in stg rebase --interactive but currently order is only observed for applied patches.

Quick question: would there be conflict resolution for unapplied patches which are re-ordered? ie if they don't merge cleanly on top of each other, would this case be detected? Intuitively I think they would not (because the patch is never applied, there's no chance for a conflict to occur)... I just want to sanity check and make sure I'm understanding the feature correctly.

@topher200
Copy link
Contributor

topher200 commented Aug 24, 2021

would there be conflict resolution for unapplied patches which are re-ordered?

I re-read your description and determined that conflict resolution would only occur when a patch is applied, as I expected. Question answered!


I have a new question! Your description left out any usecases where a stg push --noapply <patch> occurred on a stack with applied patches. Would it work by 'pushing' the patch after all the applied patches? For example:

$ stg series
+ setA-p1
> setA-p2
- setA-p3
- setB-p1

# push an unapplied patch
$ stg push --noapply setB-p1

# the pushed patch is now the first unapplied patch
$ stg series
+ setA-p1
> setA-p2
- setB-p1
- setA-p3

@NonLogicalDev
Copy link
Contributor Author

would there be conflict resolution for unapplied patches which are re-ordered?

I re-read your description and determined that conflict resolution would only occur when a patch is applied, as I expected. Question answered!

I have a new question! Your description left out any usecases where a stg push --noapply <patch> occurred on a stack with applied patches. Would it work by 'pushing' the patch after all the applied patches? For example:
...

I am somewhat split on whether this should be a float only option. But yes the way I was thinking about it those patches that you float will be "pushed" above the current one always.

@jpgrayson
Copy link
Collaborator

Sorry for the delay in joining the conversation. Thank you @NonLogicalDev for writing up this idea.

I think I see the value in having this capability. My concern is whether there are any corner cases that have not yet been considered that break this feature. But perhaps the best way to uncover that risk is by pursuing the feature.

I agree that it is an interesting design decision as to whether we should have push --noapply, float --noapply, or both? I think the right answer will be both. The operations would be nearly the same, but with the difference that float --noapply would unapply any of the specified patches that were already applied whereas push --noapply would abort if any applied patches were specified on the command line. I suspect both behaviors are useful.

jpgrayson added a commit that referenced this issue Aug 27, 2021
This option allows patches to be floated without also applying them. I.e.
patches can be reordered without modifying the worktree or immediately
resolving potential conflicts.

Addresses #144

Signed-off-by: Peter Grayson <[email protected]>
jpgrayson added a commit that referenced this issue Aug 27, 2021
The new --noapply option to push allows the order of unapplied patches to
be modified without actually applying those patches.

It may be helpful to some workflows to allow patches to be reordered
without either (a) having to immediately resolve merge conflicts; or (b)
having to modify the contents of the working tree.

Addresses #144.

Signed-off-by: Peter Grayson <[email protected]>
@jpgrayson
Copy link
Collaborator

I took a look at this today and as @NonLogicalDev indicated, it was pretty straightforward. So both stg float --noapply and stg push --noapply are now implemented.

I would greatly appreciate @NonLogicalDev giving this feature a try to see if it matches expectations. Also please take a look at the tests I wrote to make sure they cover the target use cases.

I'm going to close this for now. Please re-open if there are any issues with this feature.

@NonLogicalDev
Copy link
Contributor Author

Whoa! @jpgrayson thank you so much, that was really quick! I will get to testing right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants