Subject: Re: [PATCH v12 2/8] refs: atomically record overwritten ref in update_symref

From: "Bence Ferdinandy" <[email-masked]>
Date: Fri, 15 Nov 2024 23:18:10 +0100

On Fri Nov 15, 2024 at 06:50, Junio C Hamano <[email-masked]> wrote:
> Bence Ferdinandy <[email-masked]> writes:
>
>>  int refs_update_symref(struct ref_store *refs, const char *ref,
>>  		       const char *target, const char *logmsg)
>> +{
>> +	return refs_update_symref_extended(refs, ref, target, logmsg, NULL);
>> +}
>
> OK.  As the enhanced and renamed function is also external, we do
> not have to worry about reordering the old one to come after the new
> one.

I guess this also decides that the name "_extended" is fine :)

>
>> +int refs_update_symref_extended(struct ref_store *refs, const char *ref,
>> +		       const char *target, const char *logmsg,
>> +		       struct strbuf *referent)
>>  {
>>  	struct ref_transaction *transaction;
>>  	struct strbuf err = STRBUF_INIT;
>> @@ -2122,13 +2129,20 @@ int refs_update_symref(struct ref_store *refs, const char *ref,
>>  
>>  	transaction = ref_store_transaction_begin(refs, &err);
>>  	if (!transaction ||
>> -	    ref_transaction_update(transaction, ref, NULL, NULL,
>> +		ref_transaction_update(transaction, ref, NULL, NULL,
>
> An unwanted patch noise?
>
>>  				   target, NULL, REF_NO_DEREF,
>>  				   logmsg, &err) ||
>> -	    ref_transaction_commit(transaction, &err)) {
>> +		ref_transaction_prepare(transaction, &err)) {
>
> Likewise, but the noise distracts from the real change made on this
> line, which is even worse.

Mea culpa, I'll get this cleaned up.

>
> The real change here is to only call _prepare(), which also asks the
> transaction hook if we are OK to proceed.  If we fail, we stop here
>
>>  		ret = error("%s", err.buf);
>> +		goto cleanup;
>
> This is also a real change.  We could instead make the additional
> code below into the else clause (see below).
>
>>  	}
>> +	if (referent)
>> +		refs_read_symbolic_ref(refs, ref, referent);
>
> And if we were asked to give the value of the symbolic ref, we make
> this call.  What should this code do when reading fails (I know it
> ignores, as written, but I am asking what it _should_ do)?

I think this should do _nothing_ if it fails (although should it stay this way,
I guess it should be marked with a comment that this is on purpose). My
reasoning is that running `git fetch` will be running this part of the code,
which means that should reading the symbolic ref fail for any reason, a `fetch`
that previously ran without error would now fail. We now pass up an empty
string as the previous which does mask that there was an error here. What
I think we could maybe do is pass up a special string that means there was an
error? Something that for sure cannot be a valid value for an existing
reference? I'm not sure how much sense that makes.

>
>> +	if (ref_transaction_commit(transaction, &err))
>> +		ret = error("%s", err.buf);
>
> And then we commit, or we fail to commit.
>
>> +cleanup:
>
> We could write the whole thing as a single "do these and leave as
> soon as we see any failure" ||-cascade,
>
> 	if (!(transaction = ref_store_transaction_begin(refs, &err)) ||
>
> 	    ref_transaction_update(transaction, ref, NULL, NULL,
> 				   target, NULL, REF_NO_DEREF,
> 				   logmsg, &err) ||
>
> 	    ref_transaction_prepare(transaction, &err)) ||
>
> 	    (referent
> 	    ? refs_read_symbolic_ref(refs, ref, referent)
> 	    : 0) ||
>
> 	    ref_transaction_commit(transaction, &err)) {
> 		if (!err.len)
> 			... stuff default error message to err ...;
> 		ret = error("%s", err.buf);
> 	}
>
> which may not necessarily easier to follow (and in fact it is rather
> ugly), but at least, the resulting code does not have to special
> case the "optionally peek into the symref" step.

As I said above, I don't think we actually want to fail the update even if the
symbolic ref reading fails, so I think the special casing should stay. I'll
wait here to see more clearly on what to do here.