Re: [PATCH v8 26/44] fast-import.c: use a ref transaction when dumping tags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 20, 2014 at 1:38 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Ronnie Sahlberg wrote:
>
>> [Subject: fast-import.c: use a ref transaction when dumping tags]
>
> This seems like an odd thing to do: either it would make sense to have
> a single transaction for all imported refs so all fail or succeed
> together, or there would be separate transactions for each ref.
>
> That said, I don't mind, particularly if it's a step on the way to
> using a single transaction for everything being dumped.

For now they are two transactions but I will merge them into a single one later.

>
> [...]
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1736,15 +1736,22 @@ static void dump_tags(void)
>>  {
>>       static const char *msg = "fast-import";
>>       struct tag *t;
>> -     struct ref_lock *lock;
>>       char ref_name[PATH_MAX];
>> +     struct strbuf err = STRBUF_INIT;
>> +     struct ref_transaction *transaction;
>>
>> +     transaction = ref_transaction_begin();
>>       for (t = first_tag; t; t = t->next_tag) {
>> -             sprintf(ref_name, "tags/%s", t->name);
>> +             sprintf(ref_name, "refs/tags/%s", t->name);
>
> Can this overflow the buffer?

Changed to snprint. (We probably should do this everywhere.)

>
>> -             lock = lock_ref_sha1(ref_name, NULL);
>> -             if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
>> -                     failure |= error("Unable to update %s", ref_name);
>> +
>> +             if (ref_transaction_update(transaction, ref_name, t->sha1,
>> +                                        NULL, 0, 0, &err)) {
>> +                     failure |= 1;
>> +                     break;
>> +             }
>>       }
>> +     if (failure || ref_transaction_commit(transaction, msg, &err))
>> +             failure |= error("Unable to update %s", err.buf);
>
> This 'if (failure || ...) failure |=' idiom seems strange.
> Is err.buf always set if failure is nonzero?
>
> Elsewhere failure is 0 or -1 but this introduces the possibility for
> it to be temporarily 1.
>
> dump_branches could have caused failure to be -1 before dump_tags
> is called.  I think the intent is
>
>                 if (ref_transaction_update(...)) {
>                         failure |= error(...);
>                         goto out;
>                 }
>         }
>         if (ref_transaction_commit(...))
>                 failure |= error(...);
>  out:
>         ref_transaction_free(transaction);
>         ...
>

Actually, since the recent change to make _commit fail if the
transaction has an error, what we want is something like this :

    ...
        if (ref_transaction_update(transaction, ref_name, t->sha1,
NULL, 0, 0, &err))
            break;
    }
    if (ref_transaction_commit(transaction, msg, &err))
        failure |= error("%s", err.buf);
    ...


Please see updated ref-transactions branch.


Thanks.

> 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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]