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

[Phillip Wood <phillip.wood123@gmail.com>]

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