[PATCH v2 00/13] Use ref transactions from most callers

[PATCH v2 00/13] Use ref transactions from most callers

[Ronnie Sahlberg]

This patch series changes most of the places where the ref functions for
locking and writing refs to instead use the new ref transaction API. There
are still three more places where write_ref_sha1() is called from outside
of refs.c but those all will require more complex work and review so those
changes belong in a different patch series.

Version 2:
 - Add a patch to ref_transaction_commit to make it honor onerr even if the
   error triggered in ref_Transaction_commit itself rather than in a call
   to other functions (that already honor onerr).
 - Add a patch to make the update_ref() helper function use transactions
   internally.
 - Change ref_transaction_update to die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change ref_transaction_create to die() instead of error() if new_sha1
   is false but we pass it a null_sha1.
 - Change ref_transaction_delete die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change several places to do  if(!transaction || ref_transaction_update()
   || ref_Transaction_commit()) die(generic-message) instead of checking each
   step separately and having a different message for each failure.
   Most users are likely not interested in what step of the transaction
   failed and only whether it failed or not.
 - Change commit.c to only pass a pointer to ref_transaction_update
   iff current_head is non-NULL.
   The previous patch used to compute a garbage pointer for
   current_head->object.sha1 and relied on the fact that ref_transaction_update
   would not try to dereference this pointer if !!current_head was 0.
 - Updated commit message for the walker_fetch change to try to justify why
   the change in locking semantics should not be harmful.


Ronnie Sahlberg (13):
  refs.c: constify the sha arguments for
    ref_transaction_create|delete|update
  refs.c: use a single exit path from transaction commit and handle
    onerr
  refs.c: change ref_transaction_update() to do error checking and
    return status
  refs.c: change ref_transaction_create to do error checking and return
    status
  refs.c: ref_transaction_delete to check for error and return status
  tag.c: use ref transactions when doing updates
  replace.c: use the ref transaction functions for updates
  commit.c: use ref transactions for updates
  sequencer.c: use ref transactions for all ref updates
  fast-import.c: change update_branch to use ref transactions
  branch.c: use ref transaction for all ref updates
  walker.c: use ref transaction for ref updates
  refs.c: change update_ref to use a transaction

 branch.c             | 16 ++++++----
 builtin/commit.c     | 23 +++++++-------
 builtin/replace.c    | 13 ++++----
 builtin/tag.c        | 11 +++----
 builtin/update-ref.c | 19 +++++++-----
 fast-import.c        | 23 +++++++++-----
 refs.c               | 84 +++++++++++++++++++++++++++++++++++++++-------------
 refs.h               | 25 ++++++++--------
 sequencer.c          | 17 ++++++++---
 walker.c             | 45 +++++++++++++---------------
 10 files changed, 172 insertions(+), 104 deletions(-)

-- 
1.9.1.515.g3b87021

[PATCH v2 01/13] refs.c: constify the sha arguments for ref_transaction_create|delete|update

[Ronnie Sahlberg]

ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.

Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 7 ++++---
 refs.h | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 728a761..138ab70 100644
--- a/refs.c
+++ b/refs.c
@@ -3329,7 +3329,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 
 void ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
 	struct ref_update *update = add_update(transaction, refname);
@@ -3343,7 +3344,7 @@ void ref_transaction_update(struct ref_transaction *transaction,
 
 void ref_transaction_create(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1,
+			    const unsigned char *new_sha1,
 			    int flags)
 {
 	struct ref_update *update = add_update(transaction, refname);
@@ -3357,7 +3358,7 @@ void ref_transaction_create(struct ref_transaction *transaction,
 
 void ref_transaction_delete(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *old_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
 	struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 0f08def..892c5b6 100644
--- a/refs.h
+++ b/refs.h
@@ -239,7 +239,8 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
  */
 void ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old);
 
 /*
@@ -250,7 +251,7 @@ void ref_transaction_update(struct ref_transaction *transaction,
  */
 void ref_transaction_create(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1,
+			    const unsigned char *new_sha1,
 			    int flags);
 
 /*
@@ -260,7 +261,7 @@ void ref_transaction_create(struct ref_transaction *transaction,
  */
 void ref_transaction_delete(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *old_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old);
 
 /*
-- 
1.9.1.515.g3b87021

[PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr

[Ronnie Sahlberg]

Update ref_transaction_commit to have a single exit path and process onerr
if an error occured during hte commit. This does mean that in case of an error
occuring for UPDATE_REFS_MSG_ON_ERR during the calls to update_ref_lock or
update_ref_write we will log errors from both those functions as well as a
generic message from ref_transaction_commit.

I thought a while to make the MSG_ON_ERR message in ref_transaction_commit
conditional to only trigger if the error was not triggered by the two functions
we call that also take onerr, and which would already have logger an error
already for this case, but the code would just look too awful. I think it
is acceptable to log two error messages for those two cases than to badify
the commit code.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 138ab70..9daf89e 100644
--- a/refs.c
+++ b/refs.c
@@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			   const char *msg, enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	const char **delnames = NULL;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
 	if (!n)
-		return 0;
+		goto cleanup;
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
@@ -3481,6 +3481,14 @@ cleanup:
 			unlock_ref(updates[i]->lock);
 	free(delnames);
 	ref_transaction_free(transaction);
+	if (ret) {
+		const char *str = "Cannot commit transaction.";
+		switch (onerr) {
+		case UPDATE_REFS_MSG_ON_ERR: error(str); break;
+		case UPDATE_REFS_DIE_ON_ERR: die(str); break;
+		case UPDATE_REFS_QUIET_ON_ERR: break;
+		}
+	}
 	return ret;
 }
 
-- 
1.9.1.515.g3b87021

[PATCH v2 03/13] refs.c: change ref_transaction_update() to do error checking and return status

[Ronnie Sahlberg]

Update ref_transaction_update() do some basic error checking and return
true on error. Update all callers to check ref_transaction_update() for error.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 10 ++++++----
 refs.c               |  9 +++++++--
 refs.h               | 10 +++++-----
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..811e0b0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("update %s: extra input: %s", refname, next);
 
-	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+				   update_flags, have_old))
+		die("failed transaction update for %s", refname);
 
 	update_flags = 0;
 	free(refname);
@@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+				   update_flags, have_old))
+		die("failed transaction update for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 9daf89e..8c02779 100644
--- a/refs.c
+++ b/refs.c
@@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 	return update;
 }
 
-void ref_transaction_update(struct ref_transaction *transaction,
+int ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
 			    const unsigned char *new_sha1,
 			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
+
+	if (have_old && !old_sha1)
+		die("have_old is true but old_sha1 is NULL");
 
+	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
 	update->have_old = have_old;
 	if (have_old)
 		hashcpy(update->old_sha1, old_sha1);
+	return 0;
 }
 
 void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 892c5b6..00e4f7b 100644
--- a/refs.h
+++ b/refs.h
@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
  */
-void ref_transaction_update(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value
-- 
1.9.1.515.g3b87021

[PATCH v2 04/13] refs.c: change ref_transaction_create to do error checking and return status

[Ronnie Sahlberg]

Do basic error checking in ref_transaction_create() and make it return
status. Update all callers to check the result of ref_transaction_create()

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  4 +++-
 refs.c               | 17 +++++++++++------
 refs.h               |  8 ++++----
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 811e0b0..7d1e0c0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	ref_transaction_create(transaction, refname, new_sha1, update_flags);
+	if (ref_transaction_create(transaction, refname, new_sha1,
+				   update_flags))
+		die("failed transaction create for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 8c02779..8c728c6 100644
--- a/refs.c
+++ b/refs.c
@@ -3347,18 +3347,23 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	return 0;
 }
 
-void ref_transaction_create(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   int flags)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
+
+	if (!new_sha1 || is_null_sha1(new_sha1))
+		die("create ref with null new_sha1");
+
+	update = add_update(transaction, refname);
 
-	assert(!is_null_sha1(new_sha1));
 	hashcpy(update->new_sha1, new_sha1);
 	hashclr(update->old_sha1);
 	update->flags = flags;
 	update->have_old = 1;
+	return 0;
 }
 
 void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 00e4f7b..8799e15 100644
--- a/refs.h
+++ b/refs.h
@@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
  * null SHA-1.  It is verified that the reference does not exist
  * already.
  */
-void ref_transaction_create(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   int flags);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then
-- 
1.9.1.515.g3b87021

[PATCH v2 05/13] refs.c: ref_transaction_delete to check for error and return status

[Ronnie Sahlberg]

Change ref_transaction_delete() to do basic error checking and return
status. Update all callers to check the return for ref_transaction_delete()

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  5 +++--
 refs.c               | 15 ++++++++++-----
 refs.h               |  8 ++++----
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7d1e0c0..b65445d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,8 +257,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("delete %s: extra input: %s", refname, next);
 
-	ref_transaction_delete(transaction, refname, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_delete(transaction, refname, old_sha1,
+				   update_flags, have_old))
+		die("failed transaction delete for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 8c728c6..878948a 100644
--- a/refs.c
+++ b/refs.c
@@ -3366,19 +3366,24 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	return 0;
 }
 
-void ref_transaction_delete(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
 
+	if (have_old && !old_sha1)
+		die("have_old is true but old_sha1 is NULL");
+
+	update = add_update(transaction, refname);
 	update->flags = flags;
 	update->have_old = have_old;
 	if (have_old) {
 		assert(!is_null_sha1(old_sha1));
 		hashcpy(update->old_sha1, old_sha1);
 	}
+	return 0;
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 8799e15..7050da2 100644
--- a/refs.h
+++ b/refs.h
@@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction *transaction,
  * old_sha1 holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
  */
-void ref_transaction_delete(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old);
 
 /*
  * Commit all of the changes that have been queued in transaction, as
-- 
1.9.1.515.g3b87021

[PATCH v2 06/13] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

Change tag.c to use ref transactions for all ref updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/tag.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..3c40957 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf ref = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
-	struct ref_lock *lock;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int annotate = 0, force = 0, lines = -1;
@@ -496,6 +495,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
+	struct ref_transaction *transaction;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -641,10 +641,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (annotate)
 		create_tag(object, tag, &buf, &opt, prev, object);
 
-	lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
-	if (!lock)
-		die(_("%s: cannot lock the ref"), ref.buf);
-	if (write_ref_sha1(lock, object, NULL) < 0)
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref.buf, object, prev,
+				   0, !is_null_sha1(prev)) ||
+	    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
 		die(_("%s: cannot update the ref"), ref.buf);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
-- 
1.9.1.515.g3b87021

[PATCH v2 07/13] replace.c: use the ref transaction functions for updates

[Ronnie Sahlberg]

Update replace.c to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/replace.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..b91e550 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -129,7 +129,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	unsigned char object[20], prev[20], repl[20];
 	enum object_type obj_type, repl_type;
 	char ref[PATH_MAX];
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
 
 	if (get_sha1(object_ref, object))
 		die("Failed to resolve '%s' as a valid ref.", object_ref);
@@ -157,11 +157,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	else if (!force)
 		die("replace ref '%s' already exists", ref);
 
-	lock = lock_any_ref_for_update(ref, prev, 0, NULL);
-	if (!lock)
-		die("%s: cannot lock the ref", ref);
-	if (write_ref_sha1(lock, repl, NULL) < 0)
-		die("%s: cannot update the ref", ref);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref, repl, prev,
+				   0, !is_null_sha1(prev)) ||
+	    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
+		die(_("%s: failed to replace ref"), ref);
 
 	return 0;
 }
-- 
1.9.1.515.g3b87021

[PATCH v2 08/13] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/commit.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..1b3c764 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	const char *index_file, *reflog_msg;
 	char *nl;
 	unsigned char sha1[20];
-	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
 	struct stat statbuf;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
+	struct ref_transaction *transaction;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1667,12 +1667,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);
-
 	nl = strchr(sb.buf, '\n');
 	if (nl)
 		strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1681,14 +1675,23 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
 	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
 
-	if (!ref_lock) {
+	transaction = ref_transaction_begin();
+	if (!transaction) {
 		rollback_index_files();
-		die(_("cannot lock HEAD ref"));
+		die(_("HEAD: cannot start transaction"));
 	}
-	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+	if (ref_transaction_update(transaction, "HEAD", sha1,
+				   current_head ? 
+				   current_head->object.sha1 : NULL,
+				   0, !!current_head)) {
 		rollback_index_files();
 		die(_("cannot update HEAD ref"));
 	}
+	if (ref_transaction_commit(transaction, sb.buf,
+				   UPDATE_REFS_QUIET_ON_ERR)) {
+		rollback_index_files();
+		die(_("cannot commit HEAD ref"));
+	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
-- 
1.9.1.515.g3b87021

[PATCH v2 09/13] sequencer.c: use ref transactions for all ref updates

[Ronnie Sahlberg]

Change to use ref transactions for all updates to refs.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 sequencer.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bde5f04..fa14ac0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,17 +272,26 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 			int unborn, struct replay_opts *opts)
 {
-	struct ref_lock *ref_lock;
+	struct ref_transaction *transaction;
 	struct strbuf sb = STRBUF_INIT;
 	int ret;
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
 		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
-					   0, NULL);
+
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return 1;
+	if (ref_transaction_update(transaction, "HEAD", to, from,
+				   0, !unborn)) {
+		ref_transaction_rollback(transaction);
+		return 1;
+	}
+
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-	ret = write_ref_sha1(ref_lock, to, sb.buf);
+	ret = ref_transaction_commit(transaction, sb.buf,
+				     UPDATE_REFS_MSG_ON_ERR);
 	strbuf_release(&sb);
 	return ret;
 }
-- 
1.9.1.515.g3b87021

[PATCH v2 10/13] fast-import.c: change update_branch to use ref transactions

[Ronnie Sahlberg]

Change update_branch() to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 fast-import.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index fb4738d..466dfe3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,36 +1678,43 @@ found_entry:
 static int update_branch(struct branch *b)
 {
 	static const char *msg = "fast-import";
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
 	unsigned char old_sha1[20];
 
 	if (is_null_sha1(b->sha1))
 		return 0;
 	if (read_ref(b->name, old_sha1))
 		hashclr(old_sha1);
-	lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
-	if (!lock)
-		return error("Unable to lock %s", b->name);
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return error("Unable to begin transaction for %s", b->name);
 	if (!force_update && !is_null_sha1(old_sha1)) {
 		struct commit *old_cmit, *new_cmit;
 
 		old_cmit = lookup_commit_reference_gently(old_sha1, 0);
 		new_cmit = lookup_commit_reference_gently(b->sha1, 0);
 		if (!old_cmit || !new_cmit) {
-			unlock_ref(lock);
+			ref_transaction_rollback(transaction);
 			return error("Branch %s is missing commits.", b->name);
 		}
 
 		if (!in_merge_bases(old_cmit, new_cmit)) {
-			unlock_ref(lock);
+			ref_transaction_rollback(transaction);
 			warning("Not updating %s"
 				" (new tip %s does not contain %s)",
 				b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));
 			return -1;
 		}
 	}
-	if (write_ref_sha1(lock, b->sha1, msg) < 0)
-		return error("Unable to update %s", b->name);
+	if (ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+				   0, 1)) {
+		ref_transaction_rollback(transaction);
+		return error("Unable to update transaction for %s", b->name);
+	}
+	if (ref_transaction_commit(transaction, msg,
+				   UPDATE_REFS_QUIET_ON_ERR))
+		return error("Unable to commit transaction for %s", b->name);
+
 	return 0;
 }
 
-- 
1.9.1.515.g3b87021

[PATCH v2 11/13] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

Change branch.c to use ref transactions when doing updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..45c7766 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,7 @@ void create_branch(const char *head,
 		   int force, int reflog, int clobber_head,
 		   int quiet, enum branch_track track)
 {
-	struct ref_lock *lock = NULL;
+	struct ref_transaction *transaction;
 	struct commit *commit;
 	unsigned char sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
@@ -286,9 +286,12 @@ void create_branch(const char *head,
 	hashcpy(sha1, commit->object.sha1);
 
 	if (!dont_change_ref) {
-		lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
-		if (!lock)
-			die_errno(_("Failed to lock ref for update"));
+		transaction = ref_transaction_begin();
+		if (!transaction)
+			die_errno(_("Failed to begin transaction"));
+		if (ref_transaction_update(transaction, ref.buf, sha1, NULL,
+				   0, 0))
+			die_errno(_("Failed to update transaction"));
 	}
 
 	if (reflog)
@@ -305,8 +308,9 @@ void create_branch(const char *head,
 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
 	if (!dont_change_ref)
-		if (write_ref_sha1(lock, sha1, msg) < 0)
-			die_errno(_("Failed to write ref"));
+		if (ref_transaction_commit(transaction, msg,
+				     UPDATE_REFS_DIE_ON_ERR))
+			die_errno(_("Failed to commit transaction"));
 
 	strbuf_release(&ref);
 	free(real_ref);
-- 
1.9.1.515.g3b87021

[PATCH v2 12/13] walker.c: use ref transaction for ref updates

[Ronnie Sahlberg]

Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.

This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.

Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 walker.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/walker.c b/walker.c
index 1dd86b8..fa02d09 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,16 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
 int walker_fetch(struct walker *walker, int targets, char **target,
 		 const char **write_ref, const char *write_ref_log_details)
 {
-	struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+	struct ref_transaction *transaction;
 	unsigned char *sha1 = xmalloc(targets * 20);
-	char *msg;
-	int ret;
+	char *msg = NULL;
 	int i;
 
 	save_commit_buffer = 0;
 
-	for (i = 0; i < targets; i++) {
-		if (!write_ref || !write_ref[i])
-			continue;
-
-		lock[i] = lock_ref_sha1(write_ref[i], NULL);
-		if (!lock[i]) {
-			error("Can't lock ref %s", write_ref[i]);
-			goto unlock_and_fail;
-		}
-	}
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return -1;
 
 	if (!walker->get_recover)
 		for_each_ref(mark_complete, NULL);
@@ -276,14 +268,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (interpret_target(walker, target[i], &sha1[20 * i])) {
 			error("Could not interpret response from server '%s' as something to pull", target[i]);
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 		}
 		if (process(walker, lookup_unknown_object(&sha1[20 * i])))
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 	}
 
 	if (loop(walker))
-		goto unlock_and_fail;
+		goto rollback_and_fail;
 
 	if (write_ref_log_details) {
 		msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +286,22 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (!write_ref || !write_ref[i])
 			continue;
-		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
-		lock[i] = NULL;
-		if (ret)
-			goto unlock_and_fail;
+		if (ref_transaction_update(transaction, write_ref[i],
+					   &sha1[20 * i], NULL,
+					   0, 0))
+			goto rollback_and_fail;
 	}
-	free(msg);
 
+	if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+				   UPDATE_REFS_QUIET_ON_ERR))
+		goto rollback_and_fail;
+
+	free(msg);
 	return 0;
 
-unlock_and_fail:
-	for (i = 0; i < targets; i++)
-		if (lock[i])
-			unlock_ref(lock[i]);
+rollback_and_fail:
+	free(msg);
+	ref_transaction_rollback(transaction);
 
 	return -1;
 }
-- 
1.9.1.515.g3b87021

[PATCH v2 13/13] refs.c: change update_ref to use a transaction

[Ronnie Sahlberg]

Change the update_ref helper function to use a ref transaction internally.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 878948a..e52b6bf 100644
--- a/refs.c
+++ b/refs.c
@@ -3390,11 +3390,29 @@ int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
 	       int flags, enum action_on_err onerr)
 {
-	struct ref_lock *lock;
-	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
-	if (!lock)
-		return 1;
-	return update_ref_write(action, refname, sha1, lock, onerr);
+	struct ref_transaction *t;
+	const char *str = "update_ref failed for ref '%s'.";
+
+	t = ref_transaction_begin();
+	if (!t)
+		goto error_return;
+	if (ref_transaction_update(t, refname, sha1, oldval, flags,
+				   !!oldval)) {
+		ref_transaction_rollback(t);
+		goto error_return;
+	}
+	if (ref_transaction_commit(t, action, onerr))
+		goto error_return;
+
+	return 0;
+
+error_return:
+	switch (onerr) {
+	case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
+	case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
+	case UPDATE_REFS_QUIET_ON_ERR: break;
+	}
+	return 1;
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
-- 
1.9.1.515.g3b87021

Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> diff --git a/refs.c b/refs.c
> index 138ab70..9daf89e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  			   const char *msg, enum action_on_err onerr)
> ...
> +	if (ret) {
> +		const char *str = "Cannot commit transaction.";
> +		switch (onerr) {
> +		case UPDATE_REFS_MSG_ON_ERR: error(str); break;
> +		case UPDATE_REFS_DIE_ON_ERR: die(str); break;
> +		case UPDATE_REFS_QUIET_ON_ERR: break;
> +		}
> +	}
>  	return ret;
>  }

I think this is a response to Michael's earlier review "do different
callers want to give different error messages more suitable for the
situation?".  I suspect that the ideal endgame may end up all
callers passing QUIET_ON_ERR and doing the error message themselves,
e.g. branch.c::craete_branch() may end something like this:

    ...
    if (!dont_change_ref)
        if (ref_transaction_commit(..., UPDATE_REFS_QUIET_ON_ERR))
                die_errno(_("Failed to write branch '%s'"),
                          skip_prefix(ref.buf, "refs/heads/));

And in the meantime until the callers are converted, we may end up
showing the fallback "Cannot commit transaction." but that would be
OK during the development and polishing of this series.

Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> @@ -3481,6 +3481,14 @@ cleanup:
>  			unlock_ref(updates[i]->lock);
>  	free(delnames);
>  	ref_transaction_free(transaction);
> +	if (ret) {
> +		const char *str = "Cannot commit transaction.";
> +		switch (onerr) {
> +		case UPDATE_REFS_MSG_ON_ERR: error(str); break;
> +		case UPDATE_REFS_DIE_ON_ERR: die(str); break;
> +		case UPDATE_REFS_QUIET_ON_ERR: break;
> +		}
> +	}
>  	return ret;
>  }

Also on top of this part:

 - avoid complier warning for printf-like functions getting a non
   literal format string as their format argument;

 - style: case label and each statement on its own line.

 - Allow localizing the error message.

diff --git a/refs.c b/refs.c
index e52b6bf..35ce61a 100644
--- a/refs.c
+++ b/refs.c
@@ -3515,11 +3515,16 @@ cleanup:
 	free(delnames);
 	ref_transaction_free(transaction);
 	if (ret) {
-		const char *str = "Cannot commit transaction.";
+		const char *str = _("Cannot commit transaction.");
 		switch (onerr) {
-		case UPDATE_REFS_MSG_ON_ERR: error(str); break;
-		case UPDATE_REFS_DIE_ON_ERR: die(str); break;
-		case UPDATE_REFS_QUIET_ON_ERR: break;
+		case UPDATE_REFS_MSG_ON_ERR:
+			error("%s", str);
+			break;
+		case UPDATE_REFS_DIE_ON_ERR:
+			die("%s", str);
+			break;
+		case UPDATE_REFS_QUIET_ON_ERR:
+			break;
 		}
 	}
 	return ret;
-- 
2.0.0-rc0-187-g5842ffa

Re: [PATCH v2 00/13] Use ref transactions from most callers

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> This patch series changes most of the places where the ref functions for
> locking and writing refs to instead use the new ref transaction API. There
> are still three more places where write_ref_sha1() is called from outside
> of refs.c but those all will require more complex work and review so those
> changes belong in a different patch series.
>
> Version 2:
>  - Add a patch to ref_transaction_commit to make it honor onerr even if the
>    error triggered in ref_Transaction_commit itself rather than in a call
>    to other functions (that already honor onerr).
>  - Add a patch to make the update_ref() helper function use transactions
>    internally.
>  - Change ref_transaction_update to die() instead of error() if we pass
>    if a NULL old_sha1 but have have_old == true.
>  - Change ref_transaction_create to die() instead of error() if new_sha1
>    is false but we pass it a null_sha1.
>  - Change ref_transaction_delete die() instead of error() if we pass
>    if a NULL old_sha1 but have have_old == true.
>  - Change several places to do  if(!transaction || ref_transaction_update()
>    || ref_Transaction_commit()) die(generic-message) instead of checking each
>    step separately and having a different message for each failure.
>    Most users are likely not interested in what step of the transaction
>    failed and only whether it failed or not.
>  - Change commit.c to only pass a pointer to ref_transaction_update
>    iff current_head is non-NULL.
>    The previous patch used to compute a garbage pointer for
>    current_head->object.sha1 and relied on the fact that ref_transaction_update
>    would not try to dereference this pointer if !!current_head was 0.
>  - Updated commit message for the walker_fetch change to try to justify why
>    the change in locking semantics should not be harmful.

Will queue, but when applied on top of mh/ref-transaction and tested
standalone, it appears to fail test #14 of t5550-http-fetch-dumb.sh
for me.

It seems that the culprit is that this step:

    git http-fetch -a -w heads/master-new $HEAD $(git config remote.origin.url) &&

creates ".git/heads/master-new" when what it was asked to create was
".git/refs/heads/master-new".  Perhaps there is something fishy in
the conversion in walker.c?  We used to do lock_ref_sha1() on
"heads/master-new", which prepended "refs/" prefix before calling
lock_ref_sha1_basic() that expects the full path from $GIT_DIR/, but
ref_transaction_update(), which wants to see the full path, is still
fed "heads/master-new" after this series.

Re: [PATCH v2 00/13] Use ref transactions from most callers

[Ronnie Sahlberg]

I will look at this once i finish the next respin.

On Tue, Apr 22, 2014 at 12:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> This patch series changes most of the places where the ref functions for
>> locking and writing refs to instead use the new ref transaction API. There
>> are still three more places where write_ref_sha1() is called from outside
>> of refs.c but those all will require more complex work and review so those
>> changes belong in a different patch series.
>>
>> Version 2:
>>  - Add a patch to ref_transaction_commit to make it honor onerr even if the
>>    error triggered in ref_Transaction_commit itself rather than in a call
>>    to other functions (that already honor onerr).
>>  - Add a patch to make the update_ref() helper function use transactions
>>    internally.
>>  - Change ref_transaction_update to die() instead of error() if we pass
>>    if a NULL old_sha1 but have have_old == true.
>>  - Change ref_transaction_create to die() instead of error() if new_sha1
>>    is false but we pass it a null_sha1.
>>  - Change ref_transaction_delete die() instead of error() if we pass
>>    if a NULL old_sha1 but have have_old == true.
>>  - Change several places to do  if(!transaction || ref_transaction_update()
>>    || ref_Transaction_commit()) die(generic-message) instead of checking each
>>    step separately and having a different message for each failure.
>>    Most users are likely not interested in what step of the transaction
>>    failed and only whether it failed or not.
>>  - Change commit.c to only pass a pointer to ref_transaction_update
>>    iff current_head is non-NULL.
>>    The previous patch used to compute a garbage pointer for
>>    current_head->object.sha1 and relied on the fact that ref_transaction_update
>>    would not try to dereference this pointer if !!current_head was 0.
>>  - Updated commit message for the walker_fetch change to try to justify why
>>    the change in locking semantics should not be harmful.
>
> Will queue, but when applied on top of mh/ref-transaction and tested
> standalone, it appears to fail test #14 of t5550-http-fetch-dumb.sh
> for me.
>
> It seems that the culprit is that this step:
>
>     git http-fetch -a -w heads/master-new $HEAD $(git config remote.origin.url) &&
>
> creates ".git/heads/master-new" when what it was asked to create was
> ".git/refs/heads/master-new".  Perhaps there is something fishy in
> the conversion in walker.c?  We used to do lock_ref_sha1() on
> "heads/master-new", which prepended "refs/" prefix before calling
> lock_ref_sha1_basic() that expects the full path from $GIT_DIR/, but
> ref_transaction_update(), which wants to see the full path, is still
> fed "heads/master-new" after this series.
>

Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr

[Ronnie Sahlberg]

On Tue, Apr 22, 2014 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> diff --git a/refs.c b/refs.c
>> index 138ab70..9daf89e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                          const char *msg, enum action_on_err onerr)
>> ...
>> +     if (ret) {
>> +             const char *str = "Cannot commit transaction.";
>> +             switch (onerr) {
>> +             case UPDATE_REFS_MSG_ON_ERR: error(str); break;
>> +             case UPDATE_REFS_DIE_ON_ERR: die(str); break;
>> +             case UPDATE_REFS_QUIET_ON_ERR: break;
>> +             }
>> +     }
>>       return ret;
>>  }
>
> I think this is a response to Michael's earlier review "do different
> callers want to give different error messages more suitable for the
> situation?".  I suspect that the ideal endgame may end up all
> callers passing QUIET_ON_ERR and doing the error message themselves,
> e.g. branch.c::craete_branch() may end something like this:
>
>     ...
>     if (!dont_change_ref)
>         if (ref_transaction_commit(..., UPDATE_REFS_QUIET_ON_ERR))
>                 die_errno(_("Failed to write branch '%s'"),
>                           skip_prefix(ref.buf, "refs/heads/));
>
> And in the meantime until the callers are converted, we may end up
> showing the fallback "Cannot commit transaction." but that would be
> OK during the development and polishing of this series.
>

That is a good idea.
I will try to address that in the next respin.