Re: [PATCH v8 25/44] receive-pack.c: use a reference transaction for updating the refs

> Ronnie Sahlberg wrote:
>
>> Wrap all the ref updates inside a transaction to make the update atomic.
>
> Interesting.
>
> [...]
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -46,6 +46,8 @@ static void *head_name_to_free;
>>  static int sent_capabilities;
>>  static int shallow_update;
>>  static const char *alt_shallow_file;
>> +static struct strbuf err = STRBUF_INIT;
>
> I think it would be cleaner for err to be local.  It isn't used for
> communication between functions.

>
> [...]
>> @@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>>                   update_shallow_ref(cmd, si))
>>                       return "shallow error";
>>
>> -             lock = lock_any_ref_for_update(namespaced_name, old_sha1,
>> -                                            0, NULL);
>> -             if (!lock) {
>> -                     rp_error("failed to lock %s", name);
>> -                     return "failed to lock";
>> -             }
>> -             if (write_ref_sha1(lock, new_sha1, "push")) {
>> -                     return "failed to write"; /* error() already called */
>> -             }
>> +             if (ref_transaction_update(transaction, namespaced_name,
>> +                                        new_sha1, old_sha1, 0, 1, &err))
>> +                     return "failed to update";
>
> The original used rp_error to send an error message immediately via
> sideband.  This drops that --- intended?

>
> The old error string shown on the push status line was was "failed to
> lock" or "failed to write" which makes it clear that the cause is
> contention or database problems or filesystem problems, respectively.
> After this change it would say "failed to update" which is about as
> clear as "failed".

>
> Would it be safe to send err.buf as-is over the wire, or does it
> contain information or formatting that wouldn't be suitable for the
> client?  (I haven't thought this through completely yet.)  Is there
> some easy way to distinguish between failure to lock and failure to
> write?  Or is there some one-size-fits-all error for this case?

>
> When the transaction fails, we need to make sure that all ref updates
> emit 'ng' and not 'ok' in receive-pack.c::report (see the example at
> the end of Documentation/technical/pack-protocol.txt for what this
> means).  What error string should they use?  Is there some way to make
> it clear to the user which ref was the culprit?
>
> What should happen when checks outside the ref transaction system
> cause a ref update to fail?  I'm thinking of
>
>  * per-ref 'update' hook (see githooks(5))
>  * fast-forward check
>  * ref creation/deletion checks
>  * attempt to push to the currently checked out branch
>
> I think the natural thing to do would be to put each ref update in its
> own transaction to start so the semantics do not change right away.
> If there are obvious answers to all these questions, then a separate
> patch could combine all these into a single transaction; or if there
> are no obvious answers, we could make the single-transaction-per-push
> semantics optional (using a configuration variable or protocol
> capability or something) to make it possible to experiment.