threads / patch / 59893

patchfix cherry-pick/revert status when doing multiple commits

Subject: [PATCH] fix cherry-pick/revert status when doing multiple commits

## tl;dr

2 messages between Jun 21, 2023 and Jun 27, 2023. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Jacob Keller· Jun 21, 2023, 22:07 UTC · lore
From: Jacob Keller <jacob.keller@gmail.com>

The status report for an in-progress cherry-pick does not show the current commit if the cherry-pick happens as part of a series of multiple commits:

 $ git cherry-pick <commit1> <commit2>
 < one of the cherry-picks fails to merge clean >
 Cherry-pick currently in progress.
  (run "git cherry-pick --continue" to continue)
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)
 $ git status
 On branch <branch>
 Your branch is ahead of '<upstream>' by 1 commit.
   (use "git push" to publish your local commits)
 Cherry-pick currently in progress.
   (run "git cherry-pick --continue" to continue)
   (use "git cherry-pick --skip" to skip this patch)
   (use "git cherry-pick --abort" to cancel the cherry-pick operation)

The show_cherry_pick_in_progress() function prints "Cherry-pick currently in progress". That function does have a more verbose print based on whether the cherry_pick_head_oid is null or not. If it is not null, then a more helpful message including which commit is actually being picked is displayed.

The introduction of the "Cherry-pick currently in progress" message comes from 4a72486de97b ("fix cherry-pick/revert status after commit", 2019-04-17). This commit modified wt_status_get_state() in order to detect that a cherry-pick was in progress even if the user has used `git commit` in the middle of the sequence.

The check used to detect this is the call to sequencer_get_last_command. If the sequencer indicates that the lass command was a REPLAY_PICK, then the state->cherry_pick_in_progress is set to 1 and the cherry_pick_head_oid is initialized to the null_oid. Similar behavior is done for the case of REPLAY_REVERT.

It happens that this call of sequencer_get_last_command will always report the action even if the user hasn't interrupted anything. Thus, during a range of cherry-picks or reverts, the cherry_pick_head_oid and revert_head_oid will always be overwritten and initialized to the null oid.

This results in status always displaying the terse message which does not include commit information.

Fix this by adding an additional check so that we do not re-initialize the cherry_pick_head_oid or revert_head_oid if we have already set the cherry_pick_in_progress or revert_in_progress bits. This ensures that git status will display the more helpful information when its available. Add a test case covering this behavior.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 wt-status.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to wt-status.c +3 −2
diff --git a/wt-status.c b/wt-status.c
index 068b76ef6d96..1e2daca73024 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1790,10 +1790,11 @@ void wt_status_get_state(struct repository *r,
 		oidcpy(&state->revert_head_oid, &oid);
 	}
 	if (!sequencer_get_last_command(r, &action)) {
-		if (action == REPLAY_PICK) {
+		if (action == REPLAY_PICK && !state->cherry_pick_in_progress) {
 			state->cherry_pick_in_progress = 1;
 			oidcpy(&state->cherry_pick_head_oid, null_oid());
-		} else {
+		}
+		if (action == REPLAY_REVERT && !state->revert_in_progress) {
 			state->revert_in_progress = 1;
 			oidcpy(&state->revert_head_oid, null_oid());
 		}

-- 
2.41.0.1.g9857a21e0017.dirty
Phillip Wood· Jun 27, 2023, 13:13 UTC · re: Jacob Keller · lore

Re: [PATCH] fix cherry-pick/revert status when doing multiple commits

Hi Jacob
On 21/06/2023 23:07, Jacob Keller wrote:
Show 55 quoted lines
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> The status report for an in-progress cherry-pick does not show the
> current commit if the cherry-pick happens as part of a series of
> multiple commits:
> 
>   $ git cherry-pick <commit1> <commit2>
>   < one of the cherry-picks fails to merge clean >
>   Cherry-pick currently in progress.
>    (run "git cherry-pick --continue" to continue)
>    (use "git cherry-pick --skip" to skip this patch)
>    (use "git cherry-pick --abort" to cancel the cherry-pick operation)
> 
>   $ git status
>   On branch <branch>
>   Your branch is ahead of '<upstream>' by 1 commit.
>     (use "git push" to publish your local commits)
> 
>   Cherry-pick currently in progress.
>     (run "git cherry-pick --continue" to continue)
>     (use "git cherry-pick --skip" to skip this patch)
>     (use "git cherry-pick --abort" to cancel the cherry-pick operation)
> 
> The show_cherry_pick_in_progress() function prints "Cherry-pick
> currently in progress". That function does have a more verbose print
> based on whether the cherry_pick_head_oid is null or not. If it is not
> null, then a more helpful message including which commit is actually
> being picked is displayed.
> 
> The introduction of the "Cherry-pick currently in progress" message
> comes from 4a72486de97b ("fix cherry-pick/revert status after commit",
> 2019-04-17). This commit modified wt_status_get_state() in order to
> detect that a cherry-pick was in progress even if the user has used `git
> commit` in the middle of the sequence.
> 
> The check used to detect this is the call to sequencer_get_last_command.
> If the sequencer indicates that the lass command was a REPLAY_PICK, then
> the state->cherry_pick_in_progress is set to 1 and the
> cherry_pick_head_oid is initialized to the null_oid. Similar behavior is
> done for the case of REPLAY_REVERT.
> 
> It happens that this call of sequencer_get_last_command will always
> report the action even if the user hasn't interrupted anything. Thus,
> during a range of cherry-picks or reverts, the cherry_pick_head_oid and
> revert_head_oid will always be overwritten and initialized to the null
> oid.
> 
> This results in status always displaying the terse message which does
> not include commit information.
> 
> Fix this by adding an additional check so that we do not re-initialize
> the cherry_pick_head_oid or revert_head_oid if we have already set the
> cherry_pick_in_progress or revert_in_progress bits. This ensures that
> git status will display the more helpful information when its available.
> Add a test case covering this behavior.

Thanks for the detailed explanation, I agree with your diagnosis and fix. The test case you mention seems to be missing though. I've left one small comment below

Show 20 quoted lines
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>   wt-status.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index 068b76ef6d96..1e2daca73024 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1790,10 +1790,11 @@ void wt_status_get_state(struct repository *r,
>   		oidcpy(&state->revert_head_oid, &oid);
>   	}
>   	if (!sequencer_get_last_command(r, &action)) {
> -		if (action == REPLAY_PICK) {
> +		if (action == REPLAY_PICK && !state->cherry_pick_in_progress) {
>   			state->cherry_pick_in_progress = 1;
>   			oidcpy(&state->cherry_pick_head_oid, null_oid());
> -		} else {
> +		}
> +		if (action == REPLAY_REVERT && !state->revert_in_progress) {
I think this would be clearer as

- } else { + } else if ((action == REPLAY_PICK && !state->cherry_pick_in_progress) {

not worth a re-roll on its own, but I think it is worth considering when you re-roll with the missing test.

Thanks for working on this,
Phillip
>   			state->revert_in_progress = 1;
>   			oidcpy(&state->revert_head_oid, null_oid());
>   		}

← back to recent threads