Re: [PATCH] fix cherry-pick/revert status when doing multiple commits
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 27, 2023, 13:13 UTC
- Message-ID
- <adbeb217-a30b-8193-4dad-b3efa0b4da05@gmail.com>
- In-Reply-To
- <20230621220754.126704-1-jacob.e.keller@intel.com>
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()); > }