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.
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.
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;
}
