[PATCH v2 00/12] Allow reference values to be checked in a transaction

[PATCH v2 00/12] Allow reference values to be checked in a transaction

[Michael Haggerty]

This is v2 of the patch series, with only minor changes from v1 [1].
Thanks to Stefan Beller and Junio for their feedback about v1. I think
I have addressed all of the points that were raised.

Changes since v1:

* Rebased onto new upstream master (because it now includes the
  dependencies, mh/reflog-expire and sb/atomic-push). The rebase was
  conflict-free.

* Tweaked some commit messages and added some Reviewed-by lines.

* Patch 06/11: Remove copyright line from t7516. Use write_script
  function to write the fake editor.

* Patch 12/11: New commit: consolidate comments for the four reference
  transaction update functions.

This branch is also available in my GitHub account [2] as branch
"refs-have-new".

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/263522
[2] https://github.com/mhagger/git

Michael Haggerty (12):
  refs: move REF_DELETING to refs.c
  refs: remove the gap in the REF_* constant values
  struct ref_update: move "have_old" into "flags"
  ref_transaction_update(): remove "have_old" parameter
  ref_transaction_delete(): remove "have_old" parameter
  commit: add tests of commit races
  commit: avoid race when creating orphan commits
  ref_transaction_create(): check that new_sha1 is valid
  ref_transaction_delete(): check that old_sha1 is not null_sha1
  ref_transaction_verify(): new function to check a reference's value
  update_ref(): improve documentation
  refs.h: Remove duplication in function docstrings

 branch.c                |   5 +-
 builtin/commit.c        |   4 +-
 builtin/fetch.c         |   6 ++-
 builtin/receive-pack.c  |   5 +-
 builtin/replace.c       |   2 +-
 builtin/tag.c           |   2 +-
 builtin/update-ref.c    |  17 +++----
 fast-import.c           |   6 +--
 refs.c                  | 130 +++++++++++++++++++++++++++++++++---------------
 refs.h                  | 111 ++++++++++++++++++++++++++++-------------
 sequencer.c             |   2 +-
 t/t7516-commit-races.sh |  33 ++++++++++++
 walker.c                |   2 +-
 13 files changed, 227 insertions(+), 98 deletions(-)
 create mode 100755 t/t7516-commit-races.sh

-- 
2.1.4

[PATCH v2 01/12] refs: move REF_DELETING to refs.c

[Michael Haggerty]

It is only used internally now. Document it a little bit better, too.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 6 ++++++
 refs.h | 4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index ab2f2a9..5e6355c 100644
--- a/refs.c
+++ b/refs.c
@@ -35,6 +35,12 @@ static unsigned char refname_disposition[256] = {
 };
 
 /*
+ * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
+ * refs (i.e., because the reference is about to be deleted anyway).
+ */
+#define REF_DELETING	0x02
+
+/*
  * Used as a flag to ref_transaction_delete when a loose ref is being
  * pruned.
  */
diff --git a/refs.h b/refs.h
index afa3c4d..9bf2148 100644
--- a/refs.h
+++ b/refs.h
@@ -183,12 +183,10 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
  * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
- * REF_DELETING: tolerate broken refs
  *
- * Flags >= 0x100 are reserved for internal use.
+ * Other flags are reserved for internal use.
  */
 #define REF_NODEREF	0x01
-#define REF_DELETING	0x02
 
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
-- 
2.1.4

[PATCH v2 02/12] refs: remove the gap in the REF_* constant values

[Michael Haggerty]

There is no reason to "reserve" a gap between the public and private
flags values.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 5e6355c..4de1383 100644
--- a/refs.c
+++ b/refs.c
@@ -44,7 +44,8 @@ static unsigned char refname_disposition[256] = {
  * Used as a flag to ref_transaction_delete when a loose ref is being
  * pruned.
  */
-#define REF_ISPRUNING	0x0100
+#define REF_ISPRUNING	0x04
+
 /*
  * Try to read one refname component from the front of refname.
  * Return the length of the component found, or -1 if the component is
-- 
2.1.4

[PATCH v2 03/12] struct ref_update: move "have_old" into "flags"

[Michael Haggerty]

Instead of having a separate have_old field, record this boolean value
as a bit in the "flags" field.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index 4de1383..6cfc07f 100644
--- a/refs.c
+++ b/refs.c
@@ -41,12 +41,18 @@ static unsigned char refname_disposition[256] = {
 #define REF_DELETING	0x02
 
 /*
- * Used as a flag to ref_transaction_delete when a loose ref is being
+ * Used as a flag in ref_update::flags when a loose ref is being
  * pruned.
  */
 #define REF_ISPRUNING	0x04
 
 /*
+ * Used as a flag in ref_update::flags when old_sha1 should be
+ * checked.
+ */
+#define REF_HAVE_OLD	0x08
+
+/*
  * Try to read one refname component from the front of refname.
  * Return the length of the component found, or -1 if the component is
  * not legal.  It is legal if it is something reasonable to have under
@@ -3563,16 +3569,20 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 }
 
 /**
- * Information needed for a single ref update.  Set new_sha1 to the
- * new value or to zero to delete the ref.  To check the old value
- * while locking the ref, set have_old to 1 and set old_sha1 to the
- * value or to zero to ensure the ref does not exist before update.
+ * Information needed for a single ref update. Set new_sha1 to the new
+ * value or to null_sha1 to delete the ref. To check the old value
+ * while the ref is locked, set (flags & REF_HAVE_OLD) and set
+ * old_sha1 to the old value, or to null_sha1 to ensure the ref does
+ * not exist before update.
  */
 struct ref_update {
 	unsigned char new_sha1[20];
 	unsigned char old_sha1[20];
-	int flags; /* REF_NODEREF? */
-	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+	/*
+	 * One or more of REF_HAVE_OLD, REF_NODEREF,
+	 * REF_DELETING, and REF_IS_PRUNING:
+	 */
+	int flags;
 	struct ref_lock *lock;
 	int type;
 	char *msg;
@@ -3666,10 +3676,11 @@ int ref_transaction_update(struct ref_transaction *transaction,
 
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
-	update->flags = flags;
-	update->have_old = have_old;
-	if (have_old)
+	if (have_old) {
 		hashcpy(update->old_sha1, old_sha1);
+		flags |= REF_HAVE_OLD;
+	}
+	update->flags = flags;
 	if (msg)
 		update->msg = xstrdup(msg);
 	return 0;
@@ -3785,13 +3796,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (is_null_sha1(update->new_sha1))
 			flags |= REF_DELETING;
-		update->lock = lock_ref_sha1_basic(update->refname,
-						   (update->have_old ?
-						    update->old_sha1 :
-						    NULL),
-						   NULL,
-						   flags,
-						   &update->type);
+		update->lock = lock_ref_sha1_basic(
+				update->refname,
+				((update->flags & REF_HAVE_OLD) ?
+				 update->old_sha1 : NULL),
+				NULL,
+				flags,
+				&update->type);
 		if (!update->lock) {
 			ret = (errno == ENOTDIR)
 				? TRANSACTION_NAME_CONFLICT
-- 
2.1.4

[PATCH v2 04/12] ref_transaction_update(): remove "have_old" parameter

[Michael Haggerty]

Instead, verify the reference's old value if and only if old_sha1 is
non-NULL.

ref_transaction_delete() will get the same treatment in a moment.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 branch.c               |  5 +++--
 builtin/commit.c       |  2 +-
 builtin/fetch.c        |  6 ++++--
 builtin/receive-pack.c |  2 +-
 builtin/replace.c      |  2 +-
 builtin/tag.c          |  2 +-
 builtin/update-ref.c   |  7 ++++---
 fast-import.c          |  6 +++---
 refs.c                 | 18 ++++++++----------
 refs.h                 |  6 +++---
 sequencer.c            |  2 +-
 walker.c               |  2 +-
 12 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/branch.c b/branch.c
index 4bab55a..b002435 100644
--- a/branch.c
+++ b/branch.c
@@ -284,8 +284,9 @@ void create_branch(const char *head,
 
 		transaction = ref_transaction_begin(&err);
 		if (!transaction ||
-		    ref_transaction_update(transaction, ref.buf, sha1,
-					   null_sha1, 0, !forcing, msg, &err) ||
+		    ref_transaction_update(transaction, ref.buf,
+					   sha1, forcing ? NULL : null_sha1,
+					   0, msg, &err) ||
 		    ref_transaction_commit(transaction, &err))
 			die("%s", err.buf);
 		ref_transaction_free(transaction);
diff --git a/builtin/commit.c b/builtin/commit.c
index 7f46713..8afb0ff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,7 +1767,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	    ref_transaction_update(transaction, "HEAD", sha1,
 				   current_head
 				   ? current_head->object.sha1 : NULL,
-				   0, !!current_head, sb.buf, &err) ||
+				   0, sb.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		rollback_index_files();
 		die("%s", err.buf);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7b84d35..719bf4f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -416,8 +416,10 @@ static int s_update_ref(const char *action,
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old, msg, &err))
+	    ref_transaction_update(transaction, ref->name,
+				   ref->new_sha1,
+				   check_old ? ref->old_sha1 : NULL,
+				   0, msg, &err))
 		goto fail;
 
 	ret = ref_transaction_commit(transaction, &err);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e0ce78e..0be50e9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -971,7 +971,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		if (ref_transaction_update(transaction,
 					   namespaced_name,
 					   new_sha1, old_sha1,
-					   0, 1, "push",
+					   0, "push",
 					   &err)) {
 			rp_error("%s", err.buf);
 			strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index 85d39b5..54bf01a 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -172,7 +172,7 @@ static int replace_object_sha1(const char *object_ref,
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref, repl, prev,
-				   0, 1, NULL, &err) ||
+				   0, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 6dc85a9..4194b9a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -733,7 +733,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
-				   0, 1, NULL, &err) ||
+				   0, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2497ba4..beb583e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -198,8 +198,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("update %s: extra input: %s", refname, next);
 
-	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, msg, &err))
+	if (ref_transaction_update(transaction, refname,
+				   new_sha1, have_old ? old_sha1 : NULL,
+				   update_flags, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -297,7 +298,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 		die("verify %s: extra input: %s", refname, next);
 
 	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, 1, msg, &err))
+				   update_flags, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
diff --git a/fast-import.c b/fast-import.c
index d0bd285..1e72bfb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1716,7 +1716,7 @@ static int update_branch(struct branch *b)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
-				   0, 1, msg, &err) ||
+				   0, msg, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
@@ -1756,8 +1756,8 @@ static void dump_tags(void)
 		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, msg, &err)) {
+		if (ref_transaction_update(transaction, ref_name.buf,
+					   t->sha1, NULL, 0, msg, &err)) {
 			failure |= error("%s", err.buf);
 			goto cleanup;
 		}
diff --git a/refs.c b/refs.c
index 6cfc07f..95ebd35 100644
--- a/refs.c
+++ b/refs.c
@@ -3654,7 +3654,7 @@ 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, const char *msg,
+			   int flags, const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");
 
-	if (have_old && !old_sha1)
-		die("BUG: have_old is true but old_sha1 is NULL");
-
 	if (!is_null_sha1(new_sha1) &&
 	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		strbuf_addf(err, "refusing to update ref with bad name %s",
@@ -3676,7 +3673,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
-	if (have_old) {
+	if (old_sha1) {
 		hashcpy(update->old_sha1, old_sha1);
 		flags |= REF_HAVE_OLD;
 	}
@@ -3693,7 +3690,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
 	return ref_transaction_update(transaction, refname, new_sha1,
-				      null_sha1, flags, 1, msg, err);
+				      null_sha1, flags, msg, err);
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
@@ -3702,8 +3699,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   int flags, int have_old, const char *msg,
 			   struct strbuf *err)
 {
-	return ref_transaction_update(transaction, refname, null_sha1,
-				      old_sha1, flags, have_old, msg, err);
+	return ref_transaction_update(transaction, refname,
+				      null_sha1, have_old ? old_sha1 : NULL,
+				      flags, msg, err);
 }
 
 int update_ref(const char *action, const char *refname,
@@ -3715,8 +3713,8 @@ int update_ref(const char *action, const char *refname,
 
 	t = ref_transaction_begin(&err);
 	if (!t ||
-	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval, action, &err) ||
+	    ref_transaction_update(t, refname, sha1, oldval,
+				   flags, action, &err) ||
 	    ref_transaction_commit(t, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
diff --git a/refs.h b/refs.h
index 9bf2148..f05ac89 100644
--- a/refs.h
+++ b/refs.h
@@ -265,8 +265,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
 /*
  * Add a reference update to transaction.  new_sha1 is the value that
  * the reference should have after the update, or null_sha1 if it should
- * be deleted.  If have_old is true, then old_sha1 holds the value
- * that the reference should have had before the update, or zeros if
+ * be deleted.  If old_sha1 is non-NULL, then it the value
+ * that the reference should have had before the update, or null_sha1 if
  * it must not have existed beforehand.
  * Function returns 0 on success and non-zero on failure. A failure to update
  * means that the transaction as a whole has failed and will need to be
@@ -276,7 +276,7 @@ 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, const char *msg,
+			   int flags, const char *msg,
 			   struct strbuf *err);
 
 /*
diff --git a/sequencer.c b/sequencer.c
index 77a1266..32aa05e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -252,7 +252,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD",
 				   to, unborn ? null_sha1 : from,
-				   0, 1, sb.buf, &err) ||
+				   0, sb.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
diff --git a/walker.c b/walker.c
index 483da4e..58ffeca 100644
--- a/walker.c
+++ b/walker.c
@@ -299,7 +299,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 		strbuf_reset(&refname);
 		strbuf_addf(&refname, "refs/%s", write_ref[i]);
 		if (ref_transaction_update(transaction, refname.buf,
-					   &sha1[20 * i], NULL, 0, 0,
+					   &sha1[20 * i], NULL, 0,
 					   msg ? msg : "fetch (unknown)",
 					   &err)) {
 			error("%s", err.buf);
-- 
2.1.4

[PATCH v2 05/12] ref_transaction_delete(): remove "have_old" parameter

[Michael Haggerty]

Instead, verify the reference's old value if and only if old_sha1 is
non-NULL.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/receive-pack.c |  3 +--
 builtin/update-ref.c   |  5 +++--
 refs.c                 | 11 ++++++-----
 refs.h                 |  6 +++---
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0be50e9..70e9ce5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -953,8 +953,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		if (ref_transaction_delete(transaction,
 					   namespaced_name,
 					   old_sha1,
-					   0, old_sha1 != NULL,
-					   "push", &err)) {
+					   0, "push", &err)) {
 			rp_error("%s", err.buf);
 			strbuf_release(&err);
 			return "failed to delete";
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index beb583e..e9632d5 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -265,8 +265,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (*next != line_termination)
 		die("delete %s: extra input: %s", refname, next);
 
-	if (ref_transaction_delete(transaction, refname, old_sha1,
-				   update_flags, have_old, msg, &err))
+	if (ref_transaction_delete(transaction, refname,
+				   have_old ? old_sha1 : NULL,
+				   update_flags, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
diff --git a/refs.c b/refs.c
index 95ebd35..ddb8bca 100644
--- a/refs.c
+++ b/refs.c
@@ -2576,7 +2576,7 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING, 1, NULL, &err) ||
+				   REF_ISPRUNING, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
@@ -2753,8 +2753,9 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
-	    ref_transaction_delete(transaction, refname, sha1, delopt,
-				   sha1 && !is_null_sha1(sha1), NULL, &err) ||
+	    ref_transaction_delete(transaction, refname,
+				   (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,
+				   delopt, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
@@ -3696,11 +3697,11 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
-			   int flags, int have_old, const char *msg,
+			   int flags, const char *msg,
 			   struct strbuf *err)
 {
 	return ref_transaction_update(transaction, refname,
-				      null_sha1, have_old ? old_sha1 : NULL,
+				      null_sha1, old_sha1,
 				      flags, msg, err);
 }
 
diff --git a/refs.h b/refs.h
index f05ac89..ee612da 100644
--- a/refs.h
+++ b/refs.h
@@ -295,8 +295,8 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   struct strbuf *err);
 
 /*
- * Add a reference deletion to transaction.  If have_old is true, then
- * old_sha1 holds the value that the reference should have had before
+ * Add a reference deletion to transaction.  If old_sha1 is non-NULL, then
+ * it holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
  * Function returns 0 on success and non-zero on failure. A failure to delete
  * means that the transaction as a whole has failed and will need to be
@@ -305,7 +305,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
-			   int flags, int have_old, const char *msg,
+			   int flags, const char *msg,
 			   struct strbuf *err);
 
 /*
-- 
2.1.4

[PATCH v2 06/12] commit: add tests of commit races

[Michael Haggerty]

Committing involves the following steps:

1. Determine the current value of HEAD (if any).
2. Create the new commit object.
3. Update HEAD.

Please note that step 2 can take arbitrarily long, because it might
involve the user editing a commit message.

If a second process sneaks in a commit during step 2, then the first
commit process should fail. This is usually done correctly, because
step 3 verifies that HEAD still points at the same commit that it
pointed to during step 1.

However, if there is a race when creating an *orphan* commit, then the
test in step 3 is skipped.

Add tests for proper handling of such races. One of the new tests
fails. It will be fixed in a moment.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t7516-commit-races.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100755 t/t7516-commit-races.sh

diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
new file mode 100755
index 0000000..08e6a6c
--- /dev/null
+++ b/t/t7516-commit-races.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='git commit races'
+. ./test-lib.sh
+
+test_tick
+
+test_expect_success 'set up editor' '
+	write_script editor <<-\EOF
+	git commit --allow-empty -m hare
+	echo tortoise >"$1"
+	EOF
+'
+
+test_expect_failure 'race to create orphan commit' '
+	test_must_fail env EDITOR=./editor git commit --allow-empty &&
+	git show -s --pretty=format:%s >subject &&
+	grep -q hare subject &&
+	test -z "$(git show -s --pretty=format:%P)"
+'
+
+test_expect_success 'race to create non-orphan commit' '
+	git checkout --orphan branch &&
+	git commit --allow-empty -m base &&
+	git rev-parse HEAD >base &&
+	test_must_fail env EDITOR=./editor git commit --allow-empty &&
+	git show -s --pretty=format:%s >subject &&
+	grep -q hare subject &&
+	git rev-parse HEAD^ >parent &&
+	test_cmp base parent
+'
+
+test_done
-- 
2.1.4

[PATCH v2 07/12] commit: avoid race when creating orphan commits

[Michael Haggerty]

If HEAD doesn't point at anything during the initial check, then we
should make sure that it *still* doesn't point at anything when we are
ready to update the reference. Otherwise, another process might commit
while we are working (e.g., while we are waiting for the user to edit
the commit message) and we will silently overwrite it.

This fixes a failing test in t7516.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 builtin/commit.c        | 2 +-
 t/t7516-commit-races.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8afb0ff..682f922 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1766,7 +1766,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD", sha1,
 				   current_head
-				   ? current_head->object.sha1 : NULL,
+				   ? current_head->object.sha1 : null_sha1,
 				   0, sb.buf, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		rollback_index_files();
diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
index 08e6a6c..dd44ef2 100755
--- a/t/t7516-commit-races.sh
+++ b/t/t7516-commit-races.sh
@@ -12,7 +12,7 @@ test_expect_success 'set up editor' '
 	EOF
 '
 
-test_expect_failure 'race to create orphan commit' '
+test_expect_success 'race to create orphan commit' '
 	test_must_fail env EDITOR=./editor git commit --allow-empty &&
 	git show -s --pretty=format:%s >subject &&
 	grep -q hare subject &&
-- 
2.1.4

[PATCH v2 08/12] ref_transaction_create(): check that new_sha1 is valid

[Michael Haggerty]

Creating a reference requires a new_sha1 that is not NULL and not
null_sha1.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.c b/refs.c
index ddb8bca..3f78fac 100644
--- a/refs.c
+++ b/refs.c
@@ -3690,6 +3690,8 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   int flags, const char *msg,
 			   struct strbuf *err)
 {
+	if (!new_sha1 || is_null_sha1(new_sha1))
+		die("BUG: create called without valid new_sha1");
 	return ref_transaction_update(transaction, refname, new_sha1,
 				      null_sha1, flags, msg, err);
 }
-- 
2.1.4

[PATCH v2 09/12] ref_transaction_delete(): check that old_sha1 is not null_sha1

[Michael Haggerty]

It makes no sense to delete a reference that is already known not to
exist.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.c b/refs.c
index 3f78fac..9dd7932 100644
--- a/refs.c
+++ b/refs.c
@@ -3702,6 +3702,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   int flags, const char *msg,
 			   struct strbuf *err)
 {
+	if (old_sha1 && is_null_sha1(old_sha1))
+		die("BUG: delete called with old_sha1 set to zeros");
 	return ref_transaction_update(transaction, refname,
 				      null_sha1, old_sha1,
 				      flags, msg, err);
-- 
2.1.4

[PATCH v2 10/12] ref_transaction_verify(): new function to check a reference's value

[Michael Haggerty]

If NULL is passed to ref_transaction_update()'s new_sha1 parameter,
then just verify old_sha1 (under lock) without trying to change the
new value of the reference.

Use this functionality to add a new function ref_transaction_verify(),
which checks the current value of the reference under lock but doesn't
change it.

Use ref_transaction_verify() in the implementation of "git update-ref
--stdin"'s "verify" command to avoid the awkward need to "update" the
reference to its existing value.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c |  7 ++-----
 refs.c               | 47 +++++++++++++++++++++++++++++++++++++++--------
 refs.h               | 34 ++++++++++++++++++++++++++--------
 3 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e9632d5..a75a098 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -282,7 +282,6 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
-	unsigned char new_sha1[20];
 	unsigned char old_sha1[20];
 
 	refname = parse_refname(input, &next);
@@ -293,13 +292,11 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 			    PARSE_SHA1_OLD))
 		hashclr(old_sha1);
 
-	hashcpy(new_sha1, old_sha1);
-
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, msg, &err))
+	if (ref_transaction_verify(transaction, refname, old_sha1,
+				   update_flags, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
diff --git a/refs.c b/refs.c
index 9dd7932..489eec9 100644
--- a/refs.c
+++ b/refs.c
@@ -47,10 +47,16 @@ static unsigned char refname_disposition[256] = {
 #define REF_ISPRUNING	0x04
 
 /*
+ * Used as a flag in ref_update::flags when the reference should be
+ * updated to new_sha1.
+ */
+#define REF_HAVE_NEW	0x08
+
+/*
  * Used as a flag in ref_update::flags when old_sha1 should be
  * checked.
  */
-#define REF_HAVE_OLD	0x08
+#define REF_HAVE_OLD	0x10
 
 /*
  * Try to read one refname component from the front of refname.
@@ -3577,10 +3583,17 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
  * not exist before update.
  */
 struct ref_update {
+	/*
+	 * If (flags & REF_HAVE_NEW), set the reference to this value:
+	 */
 	unsigned char new_sha1[20];
+	/*
+	 * If (flags & REF_HAVE_OLD), check that the reference
+	 * previously had this value:
+	 */
 	unsigned char old_sha1[20];
 	/*
-	 * One or more of REF_HAVE_OLD, REF_NODEREF,
+	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
 	 * REF_DELETING, and REF_IS_PRUNING:
 	 */
 	int flags;
@@ -3665,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");
 
-	if (!is_null_sha1(new_sha1) &&
+	if (new_sha1 && !is_null_sha1(new_sha1) &&
 	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		strbuf_addf(err, "refusing to update ref with bad name %s",
 			    refname);
@@ -3673,7 +3686,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	}
 
 	update = add_update(transaction, refname);
-	hashcpy(update->new_sha1, new_sha1);
+	if (new_sha1) {
+		hashcpy(update->new_sha1, new_sha1);
+		flags |= REF_HAVE_NEW;
+	}
 	if (old_sha1) {
 		hashcpy(update->old_sha1, old_sha1);
 		flags |= REF_HAVE_OLD;
@@ -3709,6 +3725,19 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 				      flags, msg, err);
 }
 
+int ref_transaction_verify(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags,
+			   struct strbuf *err)
+{
+	if (!old_sha1)
+		die("BUG: verify called with old_sha1 set to NULL");
+	return ref_transaction_update(transaction, refname,
+				      NULL, old_sha1,
+				      flags, NULL, err);
+}
+
 int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
 	       int flags, enum action_on_err onerr)
@@ -3797,7 +3826,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 		int flags = update->flags;
 
-		if (is_null_sha1(update->new_sha1))
+		if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
 			flags |= REF_DELETING;
 		update->lock = lock_ref_sha1_basic(
 				update->refname,
@@ -3819,8 +3848,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	/* Perform updates first so live commits remain referenced */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
+		int flags = update->flags;
 
-		if (!is_null_sha1(update->new_sha1)) {
+		if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {
 			if (write_ref_sha1(update->lock, update->new_sha1,
 					   update->msg)) {
 				update->lock = NULL; /* freed by write_ref_sha1 */
@@ -3836,14 +3866,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	/* Perform deletes now that updates are safely completed */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
+		int flags = update->flags;
 
-		if (update->lock) {
+		if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) {
 			if (delete_ref_loose(update->lock, update->type, err)) {
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
 			}
 
-			if (!(update->flags & REF_ISPRUNING))
+			if (!(flags & REF_ISPRUNING))
 				string_list_append(&refs_to_delete,
 						   update->lock->ref_name);
 		}
diff --git a/refs.h b/refs.h
index ee612da..02d6e39 100644
--- a/refs.h
+++ b/refs.h
@@ -263,14 +263,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  */
 
 /*
- * Add a reference update to transaction.  new_sha1 is the value that
- * the reference should have after the update, or null_sha1 if it should
- * be deleted.  If old_sha1 is non-NULL, then it the value
- * that the reference should have had before the update, or null_sha1 if
- * it must not have existed beforehand.
- * Function returns 0 on success and non-zero on failure. A failure to update
- * means that the transaction as a whole has failed and will need to be
- * rolled back.
+ * Add a reference update to transaction. new_sha1 is the value that
+ * the reference should have after the update, or null_sha1 if it
+ * should be deleted. If new_sha1 is NULL, then the reference is not
+ * changed at all. old_sha1 is the value that the reference must have
+ * before the update, or null_sha1 if it must not have existed
+ * beforehand. The old value is checked after the lock is taken to
+ * prevent races. If the old value doesn't agree with old_sha1, the
+ * whole transaction fails. If old_sha1 is NULL, then the previous
+ * value is not checked.
+ *
+ * Return 0 on success and non-zero on failure. Any failure in the
+ * transaction means that the transaction as a whole has failed and
+ * will need to be rolled back.
  */
 int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
@@ -309,6 +314,19 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 			   struct strbuf *err);
 
 /*
+ * Verify, within a transaction, that refname has the value old_sha1,
+ * or, if old_sha1 is null_sha1, then verify that the reference
+ * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on
+ * success and non-zero on failure. A failure to verify means that the
+ * transaction as a whole has failed and will need to be rolled back.
+ */
+int ref_transaction_verify(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags,
+			   struct strbuf *err);
+
+/*
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.
  *
-- 
2.1.4

[PATCH v2 11/12] update_ref(): improve documentation

[Michael Haggerty]

Add a docstring for update_ref(), emphasizing its similarity to
ref_transaction_update(). Rename its parameters to match those of
ref_transaction_update().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Stefan Beller <sbeller@google.com>
---
 refs.c |  8 ++++----
 refs.h | 15 +++++++++++----
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 489eec9..bea918d 100644
--- a/refs.c
+++ b/refs.c
@@ -3738,8 +3738,8 @@ int ref_transaction_verify(struct ref_transaction *transaction,
 				      flags, NULL, err);
 }
 
-int update_ref(const char *action, const char *refname,
-	       const unsigned char *sha1, const unsigned char *oldval,
+int update_ref(const char *msg, const char *refname,
+	       const unsigned char *new_sha1, const unsigned char *old_sha1,
 	       int flags, enum action_on_err onerr)
 {
 	struct ref_transaction *t;
@@ -3747,8 +3747,8 @@ int update_ref(const char *action, const char *refname,
 
 	t = ref_transaction_begin(&err);
 	if (!t ||
-	    ref_transaction_update(t, refname, sha1, oldval,
-				   flags, action, &err) ||
+	    ref_transaction_update(t, refname, new_sha1, old_sha1,
+				   flags, msg, &err) ||
 	    ref_transaction_commit(t, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
diff --git a/refs.h b/refs.h
index 02d6e39..08abd2c 100644
--- a/refs.h
+++ b/refs.h
@@ -344,10 +344,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  */
 void ref_transaction_free(struct ref_transaction *transaction);
 
-/** Lock a ref and then write its file */
-int update_ref(const char *action, const char *refname,
-		const unsigned char *sha1, const unsigned char *oldval,
-		int flags, enum action_on_err onerr);
+/**
+ * Lock, update, and unlock a single reference. This function
+ * basically does a transaction containing a single call to
+ * ref_transaction_update(). The parameters to this function have the
+ * same meaning as the corresponding parameters to
+ * ref_transaction_update(). Handle errors as requested by the `onerr`
+ * argument.
+ */
+int update_ref(const char *msg, const char *refname,
+	       const unsigned char *new_sha1, const unsigned char *old_sha1,
+	       int flags, enum action_on_err onerr);
 
 extern int parse_hide_refs_config(const char *var, const char *value, const char *);
 extern int ref_is_hidden(const char *);
-- 
2.1.4

[PATCH v2 12/12] refs.h: Remove duplication in function docstrings

[Michael Haggerty]

Add more information to the comment introducing the four reference
transaction update functions, so that each function's docstring
doesn't have to repeat it. Add a pointer from the individual
functions' docstrings to the introductory comment.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.h | 66 +++++++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/refs.h b/refs.h
index 08abd2c..4c5e3d2 100644
--- a/refs.h
+++ b/refs.h
@@ -255,11 +255,31 @@ enum action_on_err {
 struct ref_transaction *ref_transaction_begin(struct strbuf *err);
 
 /*
- * The following functions add a reference check or update to a
- * ref_transaction.  In all of them, refname is the name of the
- * reference to be affected.  The functions make internal copies of
- * refname and msg, so the caller retains ownership of these parameters.
- * flags can be REF_NODEREF; it is passed to update_ref_lock().
+ * Reference transaction updates
+ *
+ * The following four functions add a reference check or update to a
+ * ref_transaction.  They have some common similar parameters:
+ *
+ *     transaction -- a pointer to an open ref_transaction, obtained
+ *         from ref_transaction_begin().
+ *
+ *     refname -- the name of the reference to be affected.
+ *
+ *     flags -- flags affecting the update, passed to
+ *         update_ref_lock(). Can be REF_NODEREF, which means that
+ *         symbolic references should not be followed.
+ *
+ *     msg -- a message describing the change (for the reflog).
+ *
+ *     err -- a strbuf for receiving a description of any error that
+ *         might have occured.
+ *
+ * The functions make internal copies of refname and msg, so the
+ * caller retains ownership of these parameters.
+ *
+ * The functions return 0 on success and non-zero on failure. A
+ * failure means that the transaction as a whole has failed and needs
+ * to be rolled back.
  */
 
 /*
@@ -273,9 +293,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  * whole transaction fails. If old_sha1 is NULL, then the previous
  * value is not checked.
  *
- * Return 0 on success and non-zero on failure. Any failure in the
- * transaction means that the transaction as a whole has failed and
- * will need to be rolled back.
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
@@ -285,13 +304,13 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   struct strbuf *err);
 
 /*
- * Add a reference creation to transaction.  new_sha1 is the value
- * that the reference should have after the update; it must not be the
- * null SHA-1.  It is verified that the reference does not exist
+ * Add a reference creation to transaction. new_sha1 is the value that
+ * the reference should have after the update; it must not be
+ * null_sha1. It is verified that the reference does not exist
  * already.
- * Function returns 0 on success and non-zero on failure. A failure to create
- * means that the transaction as a whole has failed and will need to be
- * rolled back.
+ *
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
@@ -300,12 +319,12 @@ int ref_transaction_create(struct ref_transaction *transaction,
 			   struct strbuf *err);
 
 /*
- * Add a reference deletion to transaction.  If old_sha1 is non-NULL, then
- * it holds the value that the reference should have had before
- * the update (which must not be the null SHA-1).
- * Function returns 0 on success and non-zero on failure. A failure to delete
- * means that the transaction as a whole has failed and will need to be
- * rolled back.
+ * Add a reference deletion to transaction. If old_sha1 is non-NULL,
+ * then it holds the value that the reference should have had before
+ * the update (which must not be null_sha1).
+ *
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
@@ -316,9 +335,10 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 /*
  * Verify, within a transaction, that refname has the value old_sha1,
  * or, if old_sha1 is null_sha1, then verify that the reference
- * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on
- * success and non-zero on failure. A failure to verify means that the
- * transaction as a whole has failed and will need to be rolled back.
+ * doesn't exist. old_sha1 must be non-NULL.
+ *
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_verify(struct ref_transaction *transaction,
 			   const char *refname,
-- 
2.1.4

Re: [PATCH v2 04/12] ref_transaction_update(): remove "have_old" parameter

[Junio C Hamano]

On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Instead, verify the reference's old value if and only if old_sha1 is
> non-NULL.
>
>...
> @@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
>         if (transaction->state != REF_TRANSACTION_OPEN)
>                 die("BUG: update called for transaction that is not open");
>
> -       if (have_old && !old_sha1)
> -               die("BUG: have_old is true but old_sha1 is NULL");
> -

In the old world, old_sha1 here used to be one of
 (1) NULL, (2) null_sha1[], or (3) a real object name.
What is the rule in the new world?

Does it still allow NULL, and if so what does NULL mean?
The same thing as null_sha1[] or something else?

Or is it an error to pass NULL to this function after this change
(which I think is a very sensible rule, by the way)?

If the former, perhaps "if (!old_sha1) old_sha1 = null_sha1;"
here might be a prudent measure to simplify the code.

If the latter, "assert(old_sha1);" may be appropriate.

>         if (!is_null_sha1(new_sha1) &&
>             check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
>                 strbuf_addf(err, "refusing to update ref with bad name %s",

Re: [PATCH v2 03/12] struct ref_update: move "have_old" into "flags"

[Stefan Beller]

On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> -       int flags; /* REF_NODEREF? */
> -       int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
> +       /*
> +        * One or more of REF_HAVE_OLD, REF_NODEREF,
> +        * REF_DELETING, and REF_IS_PRUNING:
> +        */
> +       int flags;

Nit:
I'd find it more readable if it would be:
    /*
     * One or more of
     * REF_HAVE_OLD,
     * REF_NODEREF,
     * REF_DELETING,
     * REF_IS_PRUNING:
     * whose definition is found at the top of this file.
     */

With or without the nit:
Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH v2 06/12] commit: add tests of commit races

[Stefan Beller]

On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Committing involves the following steps:
>
> 1. Determine the current value of HEAD (if any).
> 2. Create the new commit object.
> 3. Update HEAD.
>
> Please note that step 2 can take arbitrarily long, because it might
> involve the user editing a commit message.
>
> If a second process sneaks in a commit during step 2, then the first
> commit process should fail. This is usually done correctly, because
> step 3 verifies that HEAD still points at the same commit that it
> pointed to during step 1.
>
> However, if there is a race when creating an *orphan* commit, then the
> test in step 3 is skipped.
>
> Add tests for proper handling of such races. One of the new tests
> fails. It will be fixed in a moment.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t7516-commit-races.sh | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100755 t/t7516-commit-races.sh
>
> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
> new file mode 100755
> index 0000000..08e6a6c
> --- /dev/null
> +++ b/t/t7516-commit-races.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='git commit races'
> +. ./test-lib.sh
> +
> +test_tick

So I am wondering why we need to have a test_tick here.
In case we need to pass simulation time after loading the
test-lib, we should rather have it inside the test-lib.

If we need to pass time specifically for this test (I just don't
understand why at this point of execution), then we should
move it more to the relevant place inside the &&-chain as all
other tests have test_tick inside a test chained up with &&.


> +
> +test_expect_success 'set up editor' '
> +       write_script editor <<-\EOF
> +       git commit --allow-empty -m hare
> +       echo tortoise >"$1"
> +       EOF
> +'
> +
> +test_expect_failure 'race to create orphan commit' '
> +       test_must_fail env EDITOR=./editor git commit --allow-empty &&
> +       git show -s --pretty=format:%s >subject &&
> +       grep -q hare subject &&
> +       test -z "$(git show -s --pretty=format:%P)"
> +'
> +
> +test_expect_success 'race to create non-orphan commit' '
> +       git checkout --orphan branch &&
> +       git commit --allow-empty -m base &&
> +       git rev-parse HEAD >base &&
> +       test_must_fail env EDITOR=./editor git commit --allow-empty &&
> +       git show -s --pretty=format:%s >subject &&
> +       grep -q hare subject &&
> +       git rev-parse HEAD^ >parent &&
> +       test_cmp base parent
> +'
> +
> +test_done
> --
> 2.1.4
>

Re: [PATCH v2 10/12] ref_transaction_verify(): new function to check a reference's value

[Stefan Beller]

On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH v2 03/12] struct ref_update: move "have_old" into "flags"

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> -       int flags; /* REF_NODEREF? */
>> -       int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>> +       /*
>> +        * One or more of REF_HAVE_OLD, REF_NODEREF,
>> +        * REF_DELETING, and REF_IS_PRUNING:
>> +        */
>> +       int flags;
>
> Nit:
> I'd find it more readable if it would be:
>     /*
>      * One or more of
>      * REF_HAVE_OLD,
>      * REF_NODEREF,
>      * REF_DELETING,
>      * REF_IS_PRUNING:
>      * whose definition is found at the top of this file.
>      */

I wouldn't do either, though, as you would have to keep repeating
yourself here and over there.  Wouldn't it be sufficient to:

 - Have a header that says "these are bits meant to go to struct
   ref_update.flags" at the beginning of series of #define's;

 - Say "ref_update.flags bits are defined above" here.  The phrasing
   can be "One or more of REF_HAVE_OLD, etc. defined above." as long
   as it is clear that this is not meant to be an exhausitive list.

Also, unless we are taking advantage of the fact that MSB is special
in signed integral types [*1*], I would prefer to see us use an
unsigned type to store these bits in a variable of an integral type.
That would let the readers assume that they have fewer tricky things
possibly going on in the code.


[Footnote]

*1* For example, you can give the MSB to the REF_ERROR bit, and say

	if (structure->flags < 0)
        	there is an error;
	else
        	other things;

only if flags is a signed type.  Also if you are relying on the fact
that MSB is special in this kind of thing:

	structure->flags >>= 3;

then you obviously cannot use unsigned type for collection of bits.

Re: [PATCH v2 06/12] commit: add tests of commit races

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
> new file mode 100755
> index 0000000..08e6a6c
> --- /dev/null
> +++ b/t/t7516-commit-races.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='git commit races'
> +. ./test-lib.sh
> +
> +test_tick
> +
> +test_expect_success 'set up editor' '
> +	write_script editor <<-\EOF
> +	git commit --allow-empty -m hare
> +	echo tortoise >"$1"
> +	EOF
> +'
> +
> +test_expect_failure 'race to create orphan commit' '
> +	test_must_fail env EDITOR=./editor git commit --allow-empty &&
> +	git show -s --pretty=format:%s >subject &&
> +	grep -q hare subject &&

Why "grep -q" in the test?  Normal invocation of the tests will hide
the output anyway, no?

Wouldn't letting "sh tDDDD-name-of-test.sh -v" show the output
better for those who are hunting for breakages to see at which step
of the &&-chain things break?

> +	test -z "$(git show -s --pretty=format:%P)"
> +'

> +test_expect_success 'race to create non-orphan commit' '
> +	git checkout --orphan branch &&
> +	git commit --allow-empty -m base &&
> +	git rev-parse HEAD >base &&
> +	test_must_fail env EDITOR=./editor git commit --allow-empty &&
> +	git show -s --pretty=format:%s >subject &&
> +	grep -q hare subject &&

Can we use a token different from hare and tortoise here?  If the
previous one worked correctly, the main "commit" process would have
failed to add 'tortoise' on top of 'hare' that raced from sideways
(which is simulated by making 'hare' from the editor), so the tip of
the history would be 'hare' when this test starts.  Expecting 'hare'
here makes it unclear if you are expecting _both_ of the competing
processes to fail (i.e. the main 'commit' fails to add 'tortoise'
and the racing 'commit' fails to do 'hare'), leaving the 'hare' the
previous test left at the tip of the history, or if you are expecting
that the competing one that tries to create the second 'hare' on top
of the existing 'hare' to win.

> +	git rev-parse HEAD^ >parent &&
> +	test_cmp base parent
> +'
> +
> +test_done

Re: [PATCH v2 07/12] commit: avoid race when creating orphan commits

[Junio C Hamano]

Good.

Re: [PATCH v2 00/12] Allow reference values to be checked in a transaction

[Junio C Hamano]

Modulo minor nits, this round looks nicely done.

Re: [PATCH v2 04/12] ref_transaction_update(): remove "have_old" parameter

[Michael Haggerty]

On 02/12/2015 06:32 PM, Junio C Hamano wrote:
> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Instead, verify the reference's old value if and only if old_sha1 is
>> non-NULL.
>>
>> ...
>> @@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>         if (transaction->state != REF_TRANSACTION_OPEN)
>>                 die("BUG: update called for transaction that is not open");
>>
>> -       if (have_old && !old_sha1)
>> -               die("BUG: have_old is true but old_sha1 is NULL");
>> -
> 
> In the old world, old_sha1 here used to be one of
>  (1) NULL, (2) null_sha1[], or (3) a real object name.
> What is the rule in the new world?

The new world is explained in the docstring in refs.h, which was updated
in this same commit:

    If old_sha1 is non-NULL, then it the value
    that the reference should have had before the update, or null_sha1
    if it must not have existed beforehand.

The docstring is further revised later in the patch series to

    old_sha1 is the value that the reference must have
    before the update, or null_sha1 if it must not have existed
    beforehand. The old value is checked after the lock is taken to
    prevent races. If the old value doesn't agree with old_sha1, the
    whole transaction fails. If old_sha1 is NULL, then the previous
    value is not checked.

The new rule is extended to ref_transaction_delete() in the subsequent
commit. I like the new semantics because it removes redundancy in the
interpretation of parameters.

Is that explanation adequate?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 04/12] ref_transaction_update(): remove "have_old" parameter

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 02/12/2015 06:32 PM, Junio C Hamano wrote:
>> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> Instead, verify the reference's old value if and only if old_sha1 is
>>> non-NULL.
>>>
>>> ...
>>> @@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>>         if (transaction->state != REF_TRANSACTION_OPEN)
>>>                 die("BUG: update called for transaction that is not open");
>>>
>>> -       if (have_old && !old_sha1)
>>> -               die("BUG: have_old is true but old_sha1 is NULL");
>>> -
>> 
>> In the old world, old_sha1 here used to be one of
>>  (1) NULL, (2) null_sha1[], or (3) a real object name.
>> What is the rule in the new world?
> ...
>     ... If old_sha1 is NULL, then the previous
>     value is not checked.

OK.  That makes it perfectly clear that removing these lines is the
right thing to do.

Thanks.

Re: [PATCH v2 03/12] struct ref_update: move "have_old" into "flags"

[Michael Haggerty]

On 02/12/2015 08:15 PM, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
>> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> -       int flags; /* REF_NODEREF? */
>>> -       int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>>> +       /*
>>> +        * One or more of REF_HAVE_OLD, REF_NODEREF,
>>> +        * REF_DELETING, and REF_IS_PRUNING:
>>> +        */
>>> +       int flags;
>>
>> Nit:
>> I'd find it more readable if it would be:
>>     /*
>>      * One or more of
>>      * REF_HAVE_OLD,
>>      * REF_NODEREF,
>>      * REF_DELETING,
>>      * REF_IS_PRUNING:
>>      * whose definition is found at the top of this file.
>>      */
> 
> I wouldn't do either, though, as you would have to keep repeating
> yourself here and over there.  Wouldn't it be sufficient to:
> 
>  - Have a header that says "these are bits meant to go to struct
>    ref_update.flags" at the beginning of series of #define's;
> 
>  - Say "ref_update.flags bits are defined above" here.  The phrasing
>    can be "One or more of REF_HAVE_OLD, etc. defined above." as long
>    as it is clear that this is not meant to be an exhausitive list.

This would be an easy change but for the fact that REF_NODEREF is
defined in refs.h whereas the other constants are internal to refs.c. If
it's OK with you guys, I'd rather leave it the way it is for now and
come back to it later.

For example I want to rename the constants to REF_NODEREF ->
REF_NO_DEREF and REF_ISPRUNING -> REF_IS_PRUNING [1], but am leaving
that for when the refs code is not in so much flux. I can reorganize the
constants and docs then.

Michael

[1] As I type this I realize that the comment misspells the name of
REF_ISPRUNING. I'll fix that, too.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 06/12] commit: add tests of commit races

[Michael Haggerty]

On 02/12/2015 07:13 PM, Stefan Beller wrote:
> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Committing involves the following steps:
>> [...]
>> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
>> new file mode 100755
>> index 0000000..08e6a6c
>> --- /dev/null
>> +++ b/t/t7516-commit-races.sh
>> @@ -0,0 +1,33 @@
>> +#!/bin/sh
>> +
>> +test_description='git commit races'
>> +. ./test-lib.sh
>> +
>> +test_tick
> 
> So I am wondering why we need to have a test_tick here.
> In case we need to pass simulation time after loading the
> test-lib, we should rather have it inside the test-lib.

You're right; I don't think it's necessary. I will remove it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 06/12] commit: add tests of commit races

[Michael Haggerty]

On 02/12/2015 08:36 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
>> new file mode 100755
>> index 0000000..08e6a6c
>> --- /dev/null
>> +++ b/t/t7516-commit-races.sh
>> @@ -0,0 +1,33 @@
>> +#!/bin/sh
>> +
>> +test_description='git commit races'
>> +. ./test-lib.sh
>> +
>> +test_tick
>> +
>> +test_expect_success 'set up editor' '
>> +	write_script editor <<-\EOF
>> +	git commit --allow-empty -m hare
>> +	echo tortoise >"$1"
>> +	EOF
>> +'
>> +
>> +test_expect_failure 'race to create orphan commit' '
>> +	test_must_fail env EDITOR=./editor git commit --allow-empty &&
>> +	git show -s --pretty=format:%s >subject &&
>> +	grep -q hare subject &&
> 
> Why "grep -q" in the test?  Normal invocation of the tests will hide
> the output anyway, no?
> 
> Wouldn't letting "sh tDDDD-name-of-test.sh -v" show the output
> better for those who are hunting for breakages to see at which step
> of the &&-chain things break?

Good point. I will remove the "-q" from the two grep invocations in this
file.

>> +	test -z "$(git show -s --pretty=format:%P)"
>> +'
> 
>> +test_expect_success 'race to create non-orphan commit' '
>> +	git checkout --orphan branch &&
>> +	git commit --allow-empty -m base &&
>> +	git rev-parse HEAD >base &&
>> +	test_must_fail env EDITOR=./editor git commit --allow-empty &&
>> +	git show -s --pretty=format:%s >subject &&
>> +	grep -q hare subject &&
> 
> Can we use a token different from hare and tortoise here?  If the
> previous one worked correctly, the main "commit" process would have
> failed to add 'tortoise' on top of 'hare' that raced from sideways
> (which is simulated by making 'hare' from the editor), so the tip of
> the history would be 'hare' when this test starts.  Expecting 'hare'
> here makes it unclear if you are expecting _both_ of the competing
> processes to fail (i.e. the main 'commit' fails to add 'tortoise'
> and the racing 'commit' fails to do 'hare'), leaving the 'hare' the
> previous test left at the tip of the history, or if you are expecting
> that the competing one that tries to create the second 'hare' on top
> of the existing 'hare' to win.

Yes, you're right. I will change the second test to use different tokens.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 03/12] struct ref_update: move "have_old" into "flags"

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> For example I want to rename the constants to REF_NODEREF ->
> REF_NO_DEREF and REF_ISPRUNING -> REF_IS_PRUNING [1], but am leaving
> that for when the refs code is not in so much flux. I can reorganize the
> constants and docs then.

These renames are very sensible.  Thanks for your attention to the
details.



>
> Michael
>
> [1] As I type this I realize that the comment misspells the name of
> REF_ISPRUNING. I'll fix that, too.