Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes

From: Stefan Beller <sbeller@google.com>
Date: Dec 17, 2014, 23:58 UTC
Message-ID: <CAGZ79kZCTb5Uj34Kj-qnQEWmD+4=3U8LyeHxQWmLg-ozJr7Azg@mail.gmail.com>
In-Reply-To: <xmqqa92l97u1.fsf@gitster.dls.corp.google.com>

On Wed, Dec 17, 2014 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -1086,8 +1100,25 @@ static void execute_commands(struct command *commands,
>>
>>               if (cmd->skip_update)
>>                       continue;
>> -
>> +             if (!use_atomic) {
>> +                     transaction = ref_transaction_begin(&err);
>> +                     if (!transaction) {
>> +                             rp_error("%s", err.buf);
>> +                             strbuf_release(&err);
>> +                             cmd->error_string = "failed to start transaction";
>> +                             return;
>> +                     }
>> +             }
>>               cmd->error_string = update(cmd, si);
>> +             if (!use_atomic)
>> +                     if (ref_transaction_commit(transaction, &err)) {
>> +                             ref_transaction_free(transaction);
>> +                             rp_error("%s", err.buf);
>> +                             strbuf_release(&err);
>> +                             cmd->error_string = "failed to update ref";
>> +                             return;
>> +                     }
>
> Hmm, should the code even attempt to commit if update() returned a
> non NULL, signaling a failure?
>
> Or would we want to do this instead?

This would change the current behavior. In the case of !atomic we want to consider all commands and not stop early.

So maybe more
if (!cmd->error_string) {
        if (!use_atomic
            && ref_transaction_commit(...)) {
            ...
        }
} else {
        if (use_atomic)
             goto check_atomic_commit;
}

and the  check_atomic_commit label is replacing the loop to check:
-        for (cmd = commands; cmd; cmd = cmd->next)
-                if (cmd->error_string)
-                        break;
+ check_atomic_commit: