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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 30, 2023, 15:42 UTC
Message-ID: <15f67e21-c05f-4a72-9557-2a09a1311f25@gmail.com>
In-Reply-To: <0e38103114a206bedbbbd7ea97cb77fa05fd3c29.1701243201.git.ps@pks.im>

Hi Patrick

Thanks for working on this. I've left a couple of thought below.

On 29/11/2023 08:14, Patrick Steinhardt wrote:

Show 19 quoted lines

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

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

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

Best Wishes

Phillip