Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

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

Hi Patrick

On 23/01/2024 11:16, Patrick Steinhardt wrote:
> On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>> The list of hard coded exceptions also looks quite short, I
>>> can see MERGE_AUTOSTASH and BISECT_START are missing and there are
>>> probably others I've not thought of.
>>
>> I agree that it is something we need to fix.
> 
> I've taken a deeper look at BISECT_START because I previously missed it
> in my conversion to make special refs become normal pseudo refs. But as
> it turns out, BISECT_START is not a ref at all.
> > Depending on how you execute git-bisect(1), it will either contain the
> object ID of the detached HEAD or the branch you're starting the bisect
> from. This information is used to switch back to that state when you
> abort the bisect. So far this sounds like a proper ref indeed. But in
> case you're starting from a branch it will not be a symref that points
> to this branch, but it will just contain the branch name. This is not a
> valid format that could be read as a loose ref, and thus this file is
> not a proper ref at all (except that sometimes it behaves like one when
> starting from a detached HEAD).

Thank you, I'd missed that

> My first hunch was to convert it so that it indeed always is a proper
> ref. But thinking about it a bit more I'm less convinced that this is
> sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> represents its internal state. I thus came to the conclusion that it is
> more similar to the sequencer state that we have in ".git/rebase-merge"
> and ".git/rebase-apply" than anything else.
> 
> So if we wanted to rectify this, I think the most sensible way to
> address this would be to introduce a new ".git/bisect-state" directory
> that contains all of git-bisect(1)'s state:
> 
>      - BISECT_TERMS -> bisect-state/terms
>      - BISECT_LOG -> bisect-state/log
>      - BISECT_START -> bisect-state/start
>      - BISECT_RUN -> bisect-state/run
>      - BISECT_FIRST_PARENT -> bisect-state/first-parent
>      - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
> 
> I think this would make for a much cleaner solution overall as things
> are neatly contained. Cleaning up after a bisect would thus only require
> a delete of ".git/bisect-state/" and we're done.
> 
> Of course, this would be a backwards-incompatible change. We could
> transition to that newer schema by having newer Git versions recognize
> both ways to store the state, but only ever write the new schema. But
> I'm not sure whether it would ultimately be worth it.

I think that is a really good suggestion, it would bring bisect into 
line with am, rebase, cherry-pick etc. which keep their state in a 
subdirectory rather than cluttering up .git.

Best Wishes

Phillip

> Patrick