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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 3, 2023, 19:48 UTC
Message-ID: <fdf80c36-0e28-44f3-9cef-85d38d2d48f1@gmail.com>
In-Reply-To: <ZPTdmnHfDcTBqaSl@ugly>

On 03/09/2023 20:25, Oswald Buddenhagen wrote:

Show 12 quoted lines

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

Show 14 quoted lines

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

Show 10 quoted lines

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