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

On Tue, May 20, 2014 at 1:38 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

[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.

[...]

quoted

--- 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.)

quoted

-             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