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