Hi Patrick

On 01/12/2023 06:43, Patrick Steinhardt wrote:

On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:

quoted

Hi Patrick

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

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

quoted

+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

quoted

quoted

+	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

quoted

quoted

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