patchsequencer: update abort safety file more sparingly

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

>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, 
>

>> 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.
>

>> 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.
>

> 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,

>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.
>

>> 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.
>

> 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

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