Re: [PATCH v13 2/9] refs: standardize output of refs_read_symbolic_ref

[Junio C Hamano <gitster@pobox.com>]

Bence Ferdinandy <bence@ferdinandy.com> 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;