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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 23, 2024, 11:16 UTC
Message-ID: <Za-gF_Hp_lXViGWw@tanuki>
In-Reply-To: <xmqqplxtrucm.fsf@gitster.g>

On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:

Show 6 quoted lines

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

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.

Patrick