From: Phillip Wood Date: Sun, 03 Sep 2023 18:40:00 GMT Subject: Re: [PATCH] sequencer: update abort safety file more sparingly 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. > 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 > Signed-off-by: Oswald Buddenhagen > > --- > Cc: Stephan Beyer > Cc: Johannes Schindelin > Cc: Phillip Wood > --- > 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 &&