On Mon Nov 18, 2024 at 09:24, Patrick Steinhardt <ps@xxxxxx> wrote: > 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. Thanks, it was surprisingly easy to do, I'll do a touchup of the other patches and will send a v13 today with the fix. Best, Bence
