Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes:
> Subject: Re: [PATCH v13 2/9] refs: standardize output of refs_read_symbolic_ref
"output" -> "return values", or something.
> When the symbolic reference we want to read with refs_read_symbolic_ref
> is actually not a symbolic reference, the files and the reftable
> backends return different values (1 and -1 respectively). Standardize
> the returned values so that 0 is success, -1 is a generic error and -2
> is that the reference was actually non-symbolic.
Are all the existing callers OK with this switch from 1 to -2?
IOW, if a caller using the ref-files backend start behaving
differently with this change upon seeing a return value of -2 (where
it previously was expecting to see 1), that would not be nice.
Because "reftable was already broken" is not a good excuse to
introduce a separate regression to ref-files users, we'd want to be
careful if we want to do this kind of "while at it" change.
> +/*
> + * Return 0 if the symbolic reference could be read without error.
> + * Return -1 for generic errors.
> + * Return -2 if the reference was actually non-symbolic.
> + */
As this is an implementation of ref_stroage_be.read_symbolic_ref,
the above comment must stay in sync with the comment over there (and
a comment before the corresponding function in the other backend).
I personally would not add the above comment for that reason, but as
long as we are consistent, I am OK either way. So if we add this,
we should do the same to the reftable side as well.
By the way, it is arguable that it is an error when a caller asks
"here is a ref, please read it as a symbolic ref" and the ref turns
out to be not a symbolic ref. I'd say that it is a valid view that
the caller asked the question to find out if the ref was a symbolic
before making the call, and "that ref is not symbolic" is one of the
valid and expected results. So if you wanted to change the value
from 1 to -2 only because "you called read_symbolic_ref() without
checking if it is a symbolic to begin with, which is your fault and
you deserve to get an error", I am not sure if I agree with that
view and reasoning.
In any case, this "not a symbolic ref" may want to have a symbolic
constant. With NOT_A_SYMREF being a non-error, you could structure
the caller like so:
status = read_symref()
if (status < 0)
... we got an error so we stop here ...
... we do not behave differently on error type ...
switch (status) {
case 0:
... everything is peachy ...
break ;;
case NOT_A_SYMREF:
... we want to handle non-symref here ...
break ;;
default:
BUG("unhandled case");
}
Also, even if we decide that we want to treat this as an error,
lumping everything else as "generic error" might make it awkward to
evolve the API, I suspect.
> 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;
