Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD

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

Patrick Steinhardt <ps@pks.im> writes:

>> > Now there is a very particular edge case in this situation: when
>> > preparing an empty ref transacton, we end up returning whatever value
>> > `read_ref_without_reload()` returned to the caller. Under normal
>> > conditions this would be fine: "HEAD" should usually exist, and thus the
>> > function would return `0`. But if "HEAD" doesn't exist, the function
>> > returns a positive value which we end up returning to the caller.
>> >
>> > Fix this bug by resetting the return code to `0` and add a test.

So this _will_ surface as a bug when the other change in the series
is applied, but it nevertheless is worth fixing independently of the
other one, because ...

>> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>> >  				      &head_referent, &head_type);
>> >  	if (ret < 0)
>> >  		goto done;
>> > +	ret = 0;

... after "if the refs->err records an error already, skip
everything we do and return to the caller", we should take the
ownership of what we return (which will be in "ret") from now on.

So the current code uses "ret" as an uninitialized variable, even
not technically so because it is "initialized" to "refs->err"
upfront, and this is like a fix of uninitialized variable use.

>> So this is not really a problem in this function, it's more of that
>> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.
>
> Well, yes. I'd claim that it is a problem in this function because it
> returns positive even though the transaction was prepared successfully.
>
>> Nit: would be nice to have a comment about why overriding this value is
>> ok.
>
> True.

Yup.  It seems we will see a v2 for updating the test code as well,
so I'll assume that you'd explain this as an independent fix (as
well as a required preliminary fix for the other one).

Thanks, both.