On Thu, May 15, 2014 at 2:47 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
>>
>> 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.
>
>> }
>> - 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.
>
>> + 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.
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html