On Mon Nov 18, 2024 at 08:22, Patrick Steinhardt <ps@xxxxxx> wrote: > > 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. Well, the other way around (it's how files backend does it), but I guess that is irrelevant at this point. > > 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? 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? - 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. Thanks, Bence
