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 <oswald.buddenhagen@xxxxxx>
---
Cc: Stephan Beyer <s-beyer@xxxxxxx>
Cc: Johannes Schindelin <johannes.schindelin@xxxxxx>
Cc: Phillip Wood <phillip.wood123@xxxxxxxxx>
---
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 &&
