On Tue Nov 19, 2024 at 06:10, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes: > >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index 38eb14d591..60cb83f23a 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -830,7 +830,9 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, >> return ret; >> >> ret = reftable_stack_read_ref(stack, refname, &ref); >> - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) >> + if (!ret && (ref.value_type != REFTABLE_REF_SYMREF)) >> + ret = -2; >> + else if (!ret && (ref.value_type == REFTABLE_REF_SYMREF)) >> strbuf_addstr(referent, ref.value.symref); >> else >> ret = -1; > > The ref.value_type can be either equal to REFTABLE_REF_SYMREF or not > equal to it, and there is no other choice. Ah, ok, I didn't realize this. > > Wouldn't it be easier to reason about if the above code were written > more like this: > > if (ret) > ret = -1; > else if (ref.value_type == REFTABLE_REF_SYMREF) > strbuf_addstr(...); > else > ret = -2; > > I found it curious when I read it again while attempting to resolve > conflicts with 5413d69f (refs/reftable: refactor reading symbolic > refs to use reftable backend, 2024-11-05). The resolution has to > update this part of code to use the new implementation that asks > reftable_backend_read_ref() and becomes a lot simpler, so the way it > is written in your topic does not make much difference in the longer > term when both topics graduate. I'll update the patch with the above, > > IOW, if we were rebuilding your topic on top of Patrick's topoic > that includes 5413d69f, this part would read like so, I think. > > diff --git c/refs/reftable-backend.c w/refs/reftable-backend.c > index 6298991da7..b6bc3039a5 100644 > --- c/refs/reftable-backend.c > +++ w/refs/reftable-backend.c > @@ -920,8 +920,10 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, > return ret; > > ret = reftable_backend_read_ref(be, refname, &oid, referent, &type); > - if (type != REF_ISSYMREF) > + if (ret) > ret = -1; > + else if (type != REF_ISSYMREF) > + ret = -2; > return ret; > } > but I'll save this as well, I would not be completely surprised if Patrick's topic makes it in sooner :) Thanks, Bence
