Re: [PATCH v12 2/8] refs: atomically record overwritten ref in update_symref
- From
Bence Ferdinandy <bence@ferdinandy.com>
- Date
- Nov 15, 2024, 22:18 UTC
- Message-ID
- <D5N3N1RMMR9Q.28GSGYYBWZSOS@ferdinandy.com>
- In-Reply-To
- <xmqqr07d11wt.fsf@gitster.g>
On Fri Nov 15, 2024 at 06:50, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Bence Ferdinandy <bence@ferdinandy.com> 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 :)
Show 23 quoted lines
>
>> +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.
Show 17 quoted lines
>
> 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.
Show 32 quoted lines
>
>> + 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.