Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags

On Wed, May 28, 2014 at 3:17 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

On Wed, May 28, 2014 at 12:47 PM, Jonathan Nieder [off-list ref] wrote:

quoted

quoted

--- i/fast-import.c
+++ w/fast-import.c

@@ -1735,21 +1735,28 @@ static void dump_tags(void)
 {
        static const char *msg = "fast-import";
        struct tag *t;
-       char ref_name[PATH_MAX];
+       struct strbuf ref_name = STRBUF_INIT;
        struct strbuf err = STRBUF_INIT;
        struct ref_transaction *transaction;

+       strbuf_addstr(&ref_name, "refs/tags/");
+
        transaction = ref_transaction_begin(&err);
        for (t = first_tag; t; t = t->next_tag) {
-               snprintf(ref_name, PATH_MAX, "refs/tags/%s", t->name);
+               strbuf_setlen(&ref_name, strlen("refs/tags/"));
+               strbuf_addstr(&ref_name, t->name);

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

Changed to strbuf.  Thanks.

Thanks.  The semantics when ref_transaction_update() fail seem weird ---
see above.  (refs.h tells me "A failure to update means that the
transaction as a whole has failed and will need to be rolled back", so I
assume that the function should be rolling back instead of calling
_commit at that point.)

diff --git a/fast-import.c b/fast-import.c
index 4a7b196..e24c7e4 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -1735,15 +1735,24 @@ 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 ref_name = STRBUF_INIT;
+       struct strbuf err = STRBUF_INIT;
+       struct ref_transaction *transaction;

+       transaction = ref_transaction_begin(&err);
        for (t = first_tag; t; t = t->next_tag) {
-               sprintf(ref_name, "tags/%s", t->name);
-               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);
+               strbuf_reset(&ref_name);
+               strbuf_addf(&ref_name, "refs/tags/%s", t->name);
+
+               if (ref_transaction_update(transaction, ref_name.buf, t->sha1,
+                                          NULL, 0, 0, &err))
+                       break;
        }
+       if (ref_transaction_commit(transaction, msg, &err))
+               failure |= error("%s", err.buf);
+       ref_transaction_free(transaction);
+       strbuf_release(&ref_name);
+       strbuf_release(&err);
 }

I rely on the fact that if the transaction has failed then it is safe
to call ref_transaction_commit since it is guaranteed to return an
error too.