Re: [PATCH v6 13/42] commit.c: use ref transactions for updates

On Wed, May 14, 2014 at 6:11 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

[...]

quoted

+++ b/builtin/commit.c

@@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

[...]

quoted

@@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
      strbuf_release(&author_ident);
      free_commit_extra_headers(extra);

-     ref_lock = lock_any_ref_for_update("HEAD",
-                                        !current_head
-                                        ? NULL
-                                        : current_head->object.sha1,
-                                        0, NULL);
-     if (!ref_lock) {
-             rollback_index_files();
-             die(_("cannot lock HEAD ref"));
-     }
-
      nl = strchr(sb.buf, '\n');
      if (nl)
              strbuf_setlen(&sb, nl + 1 - sb.buf);
      else
              strbuf_addch(&sb, '\n');
      strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
      strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);

-     if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+     transaction = ref_transaction_begin();
+     if (!transaction ||
+         ref_transaction_update(transaction, "HEAD", sha1,
+                                current_head ?
+                                current_head->object.sha1 : NULL,
+                                0, !!current_head) ||
+         ref_transaction_commit(transaction, sb.buf, &err)) {
              rollback_index_files();
-             die(_("cannot update HEAD ref"));
+             die(_("HEAD: cannot update ref: %s"), err.buf);

Same question about !transaction (it also applies to later patches but I
won't mention it any more).

The error message changed from

        fatal: cannot lock HEAD ref

to

        fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.

Does the message from ref_transaction_commit always say what ref
was being updated when it failed?  If so, it's tempting to just use
the message as-is:

        fatal: Cannot lock the ref 'HEAD'

If the caller should add to the message, it could say something about
the context --- e.g.,

        fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'

Looking at that,

        die("%s", err.buf)

seems simplest since even if "git commit" was being called in a loop,
it's already clear that git was trying to lock HEAD to advance it.

Changed it to

        die("%s", err.buf)

as you suggested.



Many thanks for the reviews so far!

regards
ronnie sahlberg