Re: [PATCH] sequencer: update abort safety file more sparingly
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 3, 2023, 18:40 UTC
- Message-ID
- <29fb7a38-1e92-457a-93ff-0e64ac09b907@gmail.com>
- In-Reply-To
- <20230903151132.739166-1-oswald.buddenhagen@gmx.de>
Hi Oswald
On 03/09/2023 16:11, Oswald Buddenhagen wrote:
> The only situation where the file's content matters is --continue'ing > (after a multi-cherry-pick merge conflict).
I don't think "cherry-pick --continue" consults the abort safety file, it only matters for "cherry-pick --skip" and "cherry-pick --abort".
> This means that it is > sufficient to write it in a single place, when we are prematurely > exiting the main workhorse.
I think this introduces a regression because the safety file will not get updated when "cherry-pick --continue" stops for the user to resolve conflicts.
Show 6 quoted lines
> This is much easier to reason about than the
> three dispersed calls originally introduced in 1e41229d ("sequencer:
> make sequencer abort safer", 2016-12-07). We now can also remove the
> inefficient file-based check whether the file needs writing, which
> wasn't even reliable: a single pick executed during an interrupted
> sequence would bypass the safety.An alternate view is that the abort safety file exists to prevent the user losing commits that have not been cherry-picked and it is desirable to be able to abort after cherry-picking a single pick in the middle of a sequence of cherry-picks.
Best Wishes
Phillip
Show 73 quoted lines
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> Cc: Stephan Beyer <s-beyer@gmx.net>
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
> sequencer.c | 9 ++-------
> t/t3510-cherry-pick-sequence.sh | 9 +++++++++
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab2..716384cc7b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -575,10 +575,6 @@ static void update_abort_safety_file(void)
> {
> struct object_id head;
>
> - /* Do nothing on a single-pick */
> - if (!file_exists(git_path_seq_dir()))
> - return;
> -
> if (!repo_get_oid(the_repository, "HEAD", &head))
> write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
> else
> @@ -618,7 +614,6 @@ static int fast_forward_to(struct repository *r,
> strbuf_release(&sb);
> strbuf_release(&err);
> ref_transaction_free(transaction);
> - update_abort_safety_file();
> return 0;
> }
>
> @@ -2435,7 +2430,6 @@ static int do_pick_commit(struct repository *r,
> free_message(commit, &msg);
> free(author);
> strbuf_release(&msgbuf);
> - update_abort_safety_file();
>
> return res;
> }
> @@ -5269,8 +5263,9 @@ int sequencer_pick_revisions(struct repository *r,
> return -1;
> if (save_opts(opts))
> return -1;
> - update_abort_safety_file();
> res = pick_commits(r, &todo_list, opts);
> + if (todo_list.current < todo_list.nr)
> + update_abort_safety_file();
> todo_list_release(&todo_list);
> return res;
> }
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 3b0fa66c33..170b664c33 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -318,6 +318,15 @@ test_expect_success '--abort does not unsafely change HEAD' '
> test_cmp_rev base HEAD
> '
>
> +test_expect_success '--abort after single pick does not unsafely change HEAD' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick picked anotherpick &&
> + git reset --hard &&
> + git cherry-pick unrelatedpick &&
> + git cherry-pick --abort 2>actual &&
> + test_i18ngrep "You seem to have moved HEAD" actual
> +'
> +
> test_expect_success 'cherry-pick --abort to cancel multiple revert' '
> pristine_detach anotherpick &&
> test_expect_code 1 git revert base..picked &&