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