On Mon, Nov 18, 2024 at 09:08:13AM +0100, Bence Ferdinandy wrote:

On Mon Nov 18, 2024 at 08:22, Patrick Steinhardt [off-list ref] wrote:

quoted

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?

I've been wondering about this when writing other parts of the series and now
is a good a time as any to ask: I've already seen this pattern of returning
various negative integers as error codes, but never quite got the logic behind
it. Why not just return the same numbers but positive?

It's a matter of style, I guess. Many functions use the return value as
both an indicator for error and as the actual returned value. Think e.g.
function calls like open(3p), where a negative value indicates an error
and everything else is an actual file descriptor. This carries over into
our codebase for many functions, but we're not consistent.

Anyhow, the proposed solution sounds good and as far as I see how things are
done in the code. I guess if I want the series to land I should just fix that
as well, there are already a couple of not-entirely-related fixes in there :)

Two questions about that:

- what would be the ideal place to document this behaviour? In refs.c with
  `refs_read_symbolic_ref` or with the `struct ref_storage_be` in
  refs/refs-internal.h?

I'd document this in "refs.h", where the user-facing function is
declared, and in "refs-internal.h", where the callback is defined.

- should I look into adding specific tests for this? Since the rest of the
  series will depend on this behaviour it will be implicit tested anyway, so
  I don't particularly think it would be necessary, but I don't know what the
  general approach is.

I had a look and couldn't find another way to test the behaviour because
we use `refs_read_symbolic_ref()` sparingly, only. So I think it's okay
to implicitly test this, only.

Patrick