Re: [PATCH 3/4] refs: complete list of special refs

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

Hi Patrick

On 01/12/2023 06:43, Patrick Steinhardt wrote:
> On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:
>> Hi Patrick
>>
>> Thanks for working on this. I've left a couple of thought below.
>>
>> On 29/11/2023 08:14, Patrick Steinhardt wrote:
>>> +static int is_special_ref(const char *refname)
>>> +{
>>> +	/*
>>> +	 * Special references get written and read directly via the filesystem
>>> +	 * by the subsystems that create them. Thus, they must not go through
>>> +	 * the reference backend but must instead be read directly. It is
>>> +	 * arguable whether this behaviour is sensible, or whether it's simply
>>> +	 * a leaky abstraction enabled by us only having a single reference
>>> +	 * backend implementation. But at least for a subset of references it
>>> +	 * indeed does make sense to treat them specially:
>>> +	 *
>>> +	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
>>> +	 *   carries additional metadata like where it came from.
>>> +	 *
>>> +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
>>> +	 *   heads.
>>> +	 *
>>> +	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
>>> +	 *   rebases, where keeping it closely together feels sensible.
>>
>> I'd really like to get away from treating these files as refs. I think their
>> use as refs is purely historic and predates the reflog and possibly
>> ORIG_HEAD. These days I'm not sure there is a good reason to be running
>>
>>      git rev-parse rebase-merge/orig-head
>>
>> One reason for not wanting to treat them as refs is that we do not handle
>> multi-level refs that do not begin with "refs/" consistently.
>>
>>      git update-ref foo/bar HEAD
>>
>> succeeds and creates .git/foo/bar but
>>
>>      git update-ref -d foo/bar
>>
>> fails with
>>
>>      error: refusing to update ref with bad name 'foo/bar'
>>
>> To me it would make sense to refuse to create 'foo/bar' but allow an
>> existing ref named 'foo/bar' to be deleted but the current behavior is the
>> opposite of that.
>>
>> I'd be quite happy to see us refuse to treat anything that fails
>>
>>      if (starts_with(refname, "refs/") || refname_is_safe(refname))
>>
>> as a ref but I don't know how much pain that would cause.
> 
> Well, we already do use these internally as references, but I don't
> disagree with you.

I should have been clearer that I was talking about the refs starting 
"rebase-*" rather than FETCH_HEAD and MERGE_HEAD. As a user find it 
convenient to be able to run "git fetch ... && git log -p FETCH_HEAD" 
even if the implementation is a bit ugly. As far as I can see we do not 
use "rebase-(apply|merge)/(orig-head|amend|autostash)" as a ref in our 
code or tests.

> I think the current state is extremely confusing,
> which is why my first approach was to simply document what falls into
> the category of these "special" references.

That's certainly a good place to start

> In my mind, this patch series here is a first step towards addressing
> the problem more generally. For now it is more or less only documenting
> _what_ is a special ref and why they are special, while also ensuring
> that these refs are compatible with the reftable backend. But once this
> lands, I'd certainly want to see us continue to iterate on this.
> 
> Most importantly, I'd love to see us address two issues:
> 
>    - Start to refuse writing these special refs via the refdb so that
>      the rules I've now layed out are also enforced. This would also
>      address your point about things being inconsistent.
> 
>    - Gradually reduce the list of special refs so that they are reduced
>      to a bare minimum and so that most refs are simply that, a normal
>      ref.

That sounds like a good plan

>>> +	const char * const special_refs[] = {
>>> +		"AUTO_MERGE",
>>
>> Is there any reason to treat this specially in the long term? It points to a
>> tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it is
>> effectively a "normal" ref.
> 
> No, I'd love to see this and others converted to become a normal ref
> eventually. The goal of this patch series was mostly to document what we
> already have, and address those cases which are inconsistent with the
> new rules. But I'd be happy to convert more of these special refs to
> become normal refs after it lands.

That's great

>>> +		"BISECT_EXPECTED_REV",
>>> +		"FETCH_HEAD",
>>> +		"MERGE_AUTOSTASH",
>>
>> Should we be treating this as a ref? I thought it was written as an
>> implementation detail of the autostash implementation rather than to provide
>> a ref for users and scripts.
> 
> Yes, we have to in the context of the reftable backend. There's a bunch
> of tests that exercise our ability to parse this as a ref, and they
> would otherwise fail with the reftable backend.

Ah, looking at the the man page for "git merge" it seems we do actually 
document the existence of MERGE_AUTOSTASH so it is not just an 
implementation detail after all.

Best Wishes

Phillip