[PATCH] sequencer: update abort safety file more sparingly

[PATCH] sequencer: update abort safety file more sparingly

[Oswald Buddenhagen]

The only situation where the file's content matters is --continue'ing
(after a multi-cherry-pick merge conflict). This means that it is
sufficient to write it in a single place, when we are prematurely
exiting the main workhorse. 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.

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 &&
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] sequencer: update abort safety file more sparingly

[Phillip Wood]

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@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 &&

Re: [PATCH] sequencer: update abort safety file more sparingly

[Oswald Buddenhagen]

On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>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, 
>
duh, obvious blunder.

>it only matters for "cherry-pick --skip"
>
that doesn't seem right. a --skip is just a --continue with a prior 
reset, more or less.

>and "cherry-pick --abort".
>
that one, of course.

>> 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.
>
true, there is indeed this second entry point.
i'll try to find a better "choke point".

>> 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.
>
if you did a fresh commit before or after the single pick, you'd lose 
it.
also, the feature doesn't actually prevent aborting, only the automatic 
reset.

regards

Re: [PATCH] sequencer: update abort safety file more sparingly

[Phillip Wood]

On 03/09/2023 20:25, Oswald Buddenhagen wrote:
> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>> 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,
> duh, obvious blunder.
> 
>> it only matters for "cherry-pick --skip"
>>
> that doesn't seem right. a --skip is just a --continue with a prior 
> reset, more or less.

sequencer_skip() calls rollback_is_safe() which checks the abort safety 
file.

>> and "cherry-pick --abort".
>>
> that one, of course.
> 
>>> 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.
>>
> true, there is indeed this second entry point.
> i'll try to find a better "choke point".

I think that is probably tricky, I'm not really clear what the 
aim/purpose of this refactoring is.

>>> 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.
>>
> if you did a fresh commit before or after the single pick, you'd lose it.
> also,

Oh, I can see that you'd lose a commit made before a single pick but I 
don't see how you'd lose a commit made after it. I'm still not convinced 
it is a particularly helpful change.

> the feature doesn't actually prevent aborting, only the automatic 
> reset.

Oh right, it removes the state directory but leaves HEAD untouched if it 
does not match the commit recorded in the abort safety file.

Best Wishes

Phillip

> regards

Re: [PATCH] sequencer: update abort safety file more sparingly

[Oswald Buddenhagen]

On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote:
>On 03/09/2023 20:25, Oswald Buddenhagen wrote:
>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>>> it only matters for "cherry-pick --skip"
>>>
>> that doesn't seem right. a --skip is just a --continue with a prior 
>> reset, more or less.
>
>sequencer_skip() calls rollback_is_safe() which checks the abort safety 
>file.
>
that's weird. can you think of a good reason for doing that?

>> i'll try to find a better "choke point".
>
>I think that is probably tricky,
>
yeah

>I'm not really clear what the aim/purpose of this refactoring is.
>
to make my head not explode.
more specifically, to get it out of the way of the rebase path, which is 
what i'm actually concerned with.

generally, i think this whole ad-hoc state management is a nightmare, 
and i'd be surprised if there weren't some more loose ends.
i think i'd aim for an object-oriented-ish design with an encapsulated 
state, lazy loading getters, lazy setters, and a commit entry point (or 
maybe several partial ones). no idea how that would play out.

>> if you did a fresh commit before or after the single pick, you'd lose 
>> it.
>
>Oh, I can see that you'd lose a commit made before a single pick but I 
>don't see how you'd lose a commit made after it.
>
right. thinko. it's a bit late here. ^^

regards

Re: [PATCH] sequencer: update abort safety file more sparingly

[Phillip Wood]

On 03/09/2023 21:18, Oswald Buddenhagen wrote:
> On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote:
>> On 03/09/2023 20:25, Oswald Buddenhagen wrote:
>>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>>>> it only matters for "cherry-pick --skip"
>>>>
>>> that doesn't seem right. a --skip is just a --continue with a prior 
>>> reset, more or less.
>>
>> sequencer_skip() calls rollback_is_safe() which checks the abort 
>> safety file.
>>
> that's weird. can you think of a good reason for doing that?

I think it is clear from the code - so it does not reset changes after 
the user has committed the conflict resolution.

>>> i'll try to find a better "choke point".
>>
>> I think that is probably tricky,
>>
> yeah
> 
>> I'm not really clear what the aim/purpose of this refactoring is.
>>
> to make my head not explode.
> more specifically, to get it out of the way of the rebase path, which is 
> what i'm actually concerned with.

rebase and cherry-pick share the same code path most of the time. In 
particular "cherry-pick --continue" and "rebase --continue" both use 
sequencer_continue() as their entry point so I think the best you can do 
is guard the calls to update_abort_safety_file() with "if 
(!is_rebase_i(opts))" or add "if (is_rebase_i(opts)) return" to the 
start of update_abort_safety_file().

> generally, i think this whole ad-hoc state management is a nightmare, 
> and i'd be surprised if there weren't some more loose ends.
> i think i'd aim for an object-oriented-ish design with an encapsulated 
> state, lazy loading getters, lazy setters, and a commit entry point (or 
> maybe several partial ones). no idea how that would play out.

I've been working on something similar to only write the state to disc 
when the sequencer stops for user interaction. I'm hoping to have the 
first set of patches ready to submit in the next development cycle. You 
can see the branch at [1]. It is very much a work in progress at the 
moment, the code is mostly OK (I'm running it in my git build) but some 
commits are empty, others need splitting and the commit messages need a 
lot of work. The basic idea is to add a private struct that holds the 
state and write that to disc when pick_commits() returns.

Best Wishes

Phillip

[1] https://github.com/phillipwood/git/commits/wip/sequencer-context

>>> if you did a fresh commit before or after the single pick, you'd lose 
>>> it.
>>
>> Oh, I can see that you'd lose a commit made before a single pick but I 
>> don't see how you'd lose a commit made after it.
>>
> right. thinko. it's a bit late here. ^^
> 
> regards

Re: [PATCH] sequencer: update abort safety file more sparingly

[Oswald Buddenhagen]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

On Mon, Sep 04, 2023 at 11:05:22AM +0100, Phillip Wood wrote:
>On 03/09/2023 21:18, Oswald Buddenhagen wrote:
>> On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote:
>>> On 03/09/2023 20:25, Oswald Buddenhagen wrote:
>>>> On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
>>>>> it only matters for "cherry-pick --skip"
>>>>>
>>>> that doesn't seem right. a --skip is just a --continue with a prior 
>>>> reset, more or less.
>>>
>>> sequencer_skip() calls rollback_is_safe() which checks the abort 
>>> safety file.
>>>
>> that's weird. can you think of a good reason for doing that?
>
>I think it is clear from the code - so it does not reset changes after 
>the user has committed the conflict resolution.
>
yeah, i've researched that meanwhile.
the background is that the machinery that was originally introduced for 
abort safety was later (ab-)used for checking whether a --skip makes 
sense (de81ca3f36, "cherry-pick/revert: add --skip option", 2019-07-02).  
this made the state file a misnomer (not that "abort-safety" was a 
particularly good name to start with - i'd have used "latest-head" or 
some such).

but the implementation made no sense to me, so i read the mailing list 
archive. the result is the attached patch.
however, even so, it seems kinda wrong to me: going by HEAD means that 
dropping commits would also trigger it, which would make the given 
advice misleading.
in fact, the situation this code path is covering is fundamentally 
different from the normal merge conflict: rather than letting the user 
resolve it and us finishing the commit, we are rescheduling the pick.  
but that means that --skip needs to skip whatever the next command is?  
that doesn't sound right.
also, i just tried --continue after a path conflict, and it apparently 
did the same as --skip, so something is really wrong.
also, when we have no _HEAD, actually attempting to `reset --merge` is 
pointless, no?

oh, and i just noticed that the git-prompt is buggy: it doesn't tell me 
about the interrupted multi-pick nested into an interrupted rebase.

ugh, and rebase lets me continue despite still being in the multi-pick.

and the path conflict check is made ineffective by the file in question 
being in .gitignore?! (i force-added config.mak.autogen for testing, and 
cherry-picking over it goes through just fine.)

>> i think i'd aim for an object-oriented-ish design with an 
>> encapsulated state, lazy loading getters, lazy setters, and a commit 
>> entry point (or maybe several partial ones). no idea how that would 
>> play out.
>
>I've been working on something similar
>
awesome!
(well, except for the rebase nightmare in my own series i expect because 
of this.)

>to only write the state to disc when the sequencer stops for user 
>interaction.
>
note that this must cover ctrl-c as well, because the sequencer state 
must be consistent with HEAD. of course one could also delay updating 
HEAD, but that hinges on no relevant hooks being present, i think?  
git-replay has a huge advantage here ...

regards

[-- Attachment #2: 0001-sequencer-improve-comment-in-sequencer_skip.patch --]
[-- Type: text/x-diff, Size: 1823 bytes --]

From eb81dc1d5ecb7d9d3cf4608b93c30250392f6fc7 Mon Sep 17 00:00:00 2001
From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Mon, 4 Sep 2023 13:07:04 +0200
Subject: [PATCH] sequencer: improve comment in sequencer_skip()

It wasn't clear under which circumstances the described path would be
relevant.

Change-Id: Ie9fd8a619dad4daf163c5efdb2f9a9eccf17307d
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sequencer.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c0ff165b83..11d2368ab1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3359,12 +3359,13 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
 	 * If the corresponding .git/<ACTION>_HEAD exists, we know that the
 	 * action is in progress and we can skip the commit.
 	 *
-	 * Otherwise we check that the last instruction was related to the
-	 * particular subcommand we're trying to execute and barf if that's not
-	 * the case.
-	 *
-	 * Finally we check that the rollback is "safe", i.e., has the HEAD
-	 * moved? In this case, it doesn't make sense to "reset the merge" and
+	 * But if the action would have overwritten an untracked file, no
+	 * corresponding _HEAD file exists.
+	 * In this case we fall back to checking that the last instruction was
+	 * related to the particular subcommand we're trying to execute and barf
+	 * if that's not the case.
+	 * We also check that the rollback is "safe", i.e., whether the HEAD
+	 * moved. If it did, it doesn't make sense to "reset the merge" and
 	 * "skip the commit" as the user already handled this by committing. But
 	 * we'd not want to barf here, instead give advice on how to proceed. We
 	 * only need to check that when .git/<ACTION>_HEAD doesn't exist because
-- 
2.42.0.324.gb1ea313d68