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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Nov 18, 2024 at 09:08:13AM +0100, Bence Ferdinandy wrote:
> On Mon Nov 18, 2024 at 08:22, Patrick Steinhardt <ps@pks.im> wrote:
> > 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