Re: [PATCH v6 13/42] commit.c: use ref transactions for updates

[Ronnie Sahlberg <sahlberg@google.com>]

On Wed, May 14, 2014 at 6:11 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
> [...]
>> +++ b/builtin/commit.c
>> @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> [...]
>> @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>       strbuf_release(&author_ident);
>>       free_commit_extra_headers(extra);
>>
>> -     ref_lock = lock_any_ref_for_update("HEAD",
>> -                                        !current_head
>> -                                        ? NULL
>> -                                        : current_head->object.sha1,
>> -                                        0, NULL);
>> -     if (!ref_lock) {
>> -             rollback_index_files();
>> -             die(_("cannot lock HEAD ref"));
>> -     }
>> -
>>       nl = strchr(sb.buf, '\n');
>>       if (nl)
>>               strbuf_setlen(&sb, nl + 1 - sb.buf);
>>       else
>>               strbuf_addch(&sb, '\n');
>>       strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>>       strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>>
>> -     if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, "HEAD", sha1,
>> +                                current_head ?
>> +                                current_head->object.sha1 : NULL,
>> +                                0, !!current_head) ||
>> +         ref_transaction_commit(transaction, sb.buf, &err)) {
>>               rollback_index_files();
>> -             die(_("cannot update HEAD ref"));
>> +             die(_("HEAD: cannot update ref: %s"), err.buf);
>
> Same question about !transaction (it also applies to later patches but I
> won't mention it any more).
>
> The error message changed from
>
>         fatal: cannot lock HEAD ref
>
> to
>
>         fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.
>
> Does the message from ref_transaction_commit always say what ref
> was being updated when it failed?  If so, it's tempting to just use
> the message as-is:
>
>         fatal: Cannot lock the ref 'HEAD'
>
> If the caller should add to the message, it could say something about
> the context --- e.g.,
>
>         fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'
>
> Looking at that,
>
>         die("%s", err.buf)
>
> seems simplest since even if "git commit" was being called in a loop,
> it's already clear that git was trying to lock HEAD to advance it.

Changed it to
>         die("%s", err.buf)

as you suggested.



Many thanks for the reviews so far!

regards
ronnie sahlberg