Re: [PATCH v8 17/44] fast-import.c: change update_branch to use ref transactions

From: Ronnie Sahlberg <sahlberg@google.com>
Date: May 15, 2014, 22:20 UTC
Message-ID: <CAL=YDWm-9Bq+bpqEieUuARzGO6+3ubbsLPg5cU+2g3OiYTdm8g@mail.gmail.com>
In-Reply-To: <20140515214745.GN26471@google.com>

On Thu, May 15, 2014 at 2:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 37 quoted lines

> Ronnie Sahlberg wrote:
>
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1679,39 +1679,45 @@ found_entry:
>>  static int update_branch(struct branch *b)
>>  {
>>       static const char *msg = "fast-import";
>> -     struct ref_lock *lock;
>> +     struct ref_transaction *transaction;
>>       unsigned char old_sha1[20];
>> +     struct strbuf err = STRBUF_INIT;
>>
>>       if (read_ref(b->name, old_sha1))
>>               hashclr(old_sha1);
>> +
>>       if (is_null_sha1(b->sha1)) {
>>               if (b->delete)
>>                       delete_ref(b->name, old_sha1, 0);
>>               return 0;
>>       }
>> -     lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
>> -     if (!lock)
>> -             return error("Unable to lock %s", b->name);
>>       if (!force_update && !is_null_sha1(old_sha1)) {
>>               struct commit *old_cmit, *new_cmit;
>>
>>               old_cmit = lookup_commit_reference_gently(old_sha1, 0);
>>               new_cmit = lookup_commit_reference_gently(b->sha1, 0);
>>               if (!old_cmit || !new_cmit) {
>> -                     unlock_ref(lock);
>>                       return error("Branch %s is missing commits.", b->name);
>>               }
>
> (style) Now that there's only one line in the "if" body, we can
> drop the braces.
>

Done.

Show 10 quoted lines

>>
>>               if (!in_merge_bases(old_cmit, new_cmit)) {
>> -                     unlock_ref(lock);
>>                       warning("Not updating %s"
>>                               " (new tip %s does not contain %s)",
>>                               b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));
>>                       return -1;
>
> (not about this patch, feel free to ignore) This could
> return warning("...")

Ignored.

Show 17 quoted lines

>
>>       }
>> -     if (write_ref_sha1(lock, b->sha1, msg) < 0)
>> -             return error("Unable to update %s", b->name);
>> +     transaction = ref_transaction_begin();
>> +     if ((!transaction ||
>> +         ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
>> +                                0, 1)) ||
>
> Would be more idiomatic to drop a layer of parentheses:
>
>         if (!transaction ||
>             ref_transaction_update(...) ||
>
>> +         (ref_transaction_commit(transaction, msg, &err) &&
>> +          !(transaction = NULL))) {
>

Done.

> Would be clearer if ref_transaction_commit didn't take care of the
> rollback (or in other words if patch 21 were earlier in the series).

True, but first making it correct, but ugly, here makes one appreciate patch 21 more. That patch makes it correct and not ugly :-) Leaving as is.

Show 16 quoted lines

>
>> +             ref_transaction_rollback(transaction);
>> +             error("Unable to update branch %s: %s", b->name, err.buf);
>> +             strbuf_release(&err);
>> +             return -1;
>> +     }
>
> Example old message:
>
>         error: Unable to lock refs/heads/master
>
> New message:
>
>         error: Unable to update branch refs/heads/master: Cannot lock the ref 'refs/heads/master'.
>
> So 'error("%s", err.buf)' would probably work better.

Done.

Show 14 quoted lines

>
> The only call site is dump_branches:
>
>         for (i = 0; i < branch_table_sz; i++) {
>                 for (b = branch_table[i]; b; b = b->table_next_branch)
>                         failure |= update_branch(b);
>         }
>
> Should these happen in a single transaction?  I haven't thought
> through whether it would be a good idea, if it should be optional, or
> what.
>
> Anyway, that would be a bigger behavior change, but it's interesting
> to think about.

I will try to add it to the next patch series.

>
> Thanks,
> Jonathan