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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 4, 2023, 10:05 UTC
Message-ID: <4e0628ab-c39c-410d-864b-b7c74f9e04b1@gmail.com>
In-Reply-To: <ZPTqEIvW3zJ4eafT@ugly>

On 03/09/2023 21:18, Oswald Buddenhagen wrote:

Show 12 quoted lines

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

Show 11 quoted lines

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

Show 5 quoted lines

> 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

Show 9 quoted lines

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