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

> On Tue, May 20, 2014 at 12:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> 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.
>
> Done.
>
>>
>> [...]
>>> @@ -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?
>
> Oops. I misread it as a normal error()
>
>>
>> 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".
>
> I changed it to return xstrdup(err.buf) which should be as detailed as
> we can get.
>
>>
>> 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?
>
> I think err.buf is what we want.
>
>>
>> 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.
>
> I changed it to use one transaction per ref for now.
>
> Please see the update ref-transactions branch.