Re: [PATCH v12 2/8] refs: atomically record overwritten ref in update_symref

[Patrick Steinhardt <ps@pks.im>]

On Mon, Nov 18, 2024 at 12:39:43AM +0100, Bence Ferdinandy wrote:
> >
> >>  	} +	if (referent) +		refs_read_symbolic_ref(refs, ref,
> >>  	referent);
> 
> 
> So I've been working on detecting a detached remote/HEAD, and it seems that
> "refs_read_symbolic_ref" behaves differently for the files and the reftables
> backend. These are the exit codes in the various states:
> 
> 
> 	     reftables files
> detached	-1	 1	
> doesn't exist   -1	-1
> 
> I would assume this is a bug in reftables? At least the behaviour of files is
> more useful for this case ...
> 
> This now works fine with the files backend:
> 
> 	if (referent && refs_read_symbolic_ref(refs, ref, referent) == 1) {
> 		struct object_id oid;
> 		refs_read_ref(refs, ref, &oid);
> 		strbuf_addstr(referent, oid_to_hex(&oid));
> 		ret = -1;
> 	}
> 
> And 4/8 can now also detect being detached, by checking the return value using
> the test you suggested, but this fails for reftables. Just in case it might be
> something about the test not being correct:

So from what I understand you try to execute `refs_read_symbolic_ref()`
on a non-symbolic-ref, and your expectation is:

  - It returns -1 when reading the ref has failed.

  - It returns 0 when reading the ref was successful and it was a
    symref.

  - It retuns 1 when reading the ref was successful, but it was a
    regular ref.

This behaviour isn't documented anywhere, so I wouldn't declare it a bug
in the reftable backend. But what is a bug is that the two backends
behave differently, and that should be fixed indeed.

I couldn't find any callsites of `refs_read_symbolic_ref()` where we
rely on the current behaviour of either of the backends. We do have a
check whether `refs_read_symbolic_ref()` returns negative in "refs.c" in
`migrate_one_ref()`, but that one should be mostly fine given that we
check for the type of the ref beforehand. "Mostly" though because it can
happen that we race with another writer that happened to convert the ref
we are about to migrate from a symbolic ref into a normal ref. Unlikely,
but it can happen in theory.

I think it's an easy mistake to make to check for a negative return
code. So maybe we should adapt both backends to return -1 for generic
failures and -2 in case the ref is a regular ref?

Patrick