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.
[...]
> --- 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?
> - 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);
...
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