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

From: "Bence Ferdinandy" <[email-masked]>
Date: Tue, 19 Nov 2024 11:04:21 +0100

On Tue Nov 19, 2024 at 06:10, Junio C Hamano <[email-masked]> wrote:
> Bence Ferdinandy <[email-masked]> 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