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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2024, 01:22 UTC
Message-ID: <xmqq4j44ggq0.fsf@gitster.g>
In-Reply-To: <20241118151755.756265-3-bence@ferdinandy.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.

Show 5 quoted lines

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

Show 5 quoted lines

> +/*
> + * 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.

Show 15 quoted lines

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