[PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction

[PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction

[Michael Haggerty]

This is v2 of this patch series.  See also [1] for more context.

Thanks to Brad, Junio, and Johan for their feedback on v1 [2].  I
think I have addressed all of your points.

Changes relative to v1:

* Rename the functions associated with ref_transactions to be more
  reminiscent of database transactions:

  * create_ref_transaction() -> ref_transaction_begin()
  * free_ref_transaction() -> ref_transaction_rollback()
  * queue_update_ref() -> ref_transaction_update()
  * queue_create_ref() -> ref_transaction_create()
  * queue_delete_ref() -> ref_transaction_delete()
  * commit_ref_transaction() -> ref_transaction_commit()

* Change ref_transaction_commit() to also free the transaction, so the
  user doesn't have to think about memory resources at all.

* Fix backwards compatibility of "git update-ref --stdin -z"'s
  handling of the "create" command: allow <newvalue> to be the empty
  string, treating it the same zeros.  But deprecate this usage.

* Rebased to current master (there were no conflicts).

[1] http://article.gmane.org/gmane.comp.version-control.git/243726
[2] http://thread.gmane.org/gmane.comp.version-control.git/243731

Michael Haggerty (27):
  t1400: Fix name and expected result of one test
  t1400: Provide more usual input to the command
  parse_arg(): Really test that argument is properly terminated
  t1400: Add some more tests involving quoted arguments
  refs.h: Rename the action_on_err constants
  update_refs(): Fix constness
  update-ref --stdin: Read the whole input at once
  parse_cmd_verify(): Copy old_sha1 instead of evaluating <oldvalue>
    twice
  update-ref.c: Extract a new function, parse_refname()
  update-ref --stdin: Improve error messages for invalid values
  update-ref --stdin: Make error messages more consistent
  update-ref --stdin: Simplify error messages for missing oldvalues
  t1400: Test that stdin -z update treats empty <newvalue> as zeros
  update-ref.c: Extract a new function, parse_next_sha1()
  update-ref --stdin -z: Deprecate interpreting the empty string as
    zeros
  t1400: Test one mistake at a time
  update-ref --stdin: Improve the error message for unexpected EOF
  update-ref --stdin: Harmonize error messages
  refs: Add a concept of a reference transaction
  update-ref --stdin: Reimplement using reference transactions
  refs: Remove API function update_refs()
  struct ref_update: Rename field "ref_name" to "refname"
  struct ref_update: Store refname as a FLEX_ARRAY.
  ref_transaction_commit(): Introduce temporary variables
  struct ref_update: Add a lock member
  struct ref_update: Add type field
  ref_transaction_commit(): Work with transaction->updates in place

 Documentation/git-update-ref.txt       |  18 +-
 builtin/checkout.c                     |   2 +-
 builtin/clone.c                        |   9 +-
 builtin/merge.c                        |   6 +-
 builtin/notes.c                        |   6 +-
 builtin/reset.c                        |   6 +-
 builtin/update-ref.c                   | 425 ++++++++++++++++++++-------------
 contrib/examples/builtin-fetch--tool.c |   3 +-
 notes-cache.c                          |   2 +-
 notes-utils.c                          |   3 +-
 refs.c                                 | 192 +++++++++++----
 refs.h                                 |  94 ++++++--
 t/t1400-update-ref.sh                  | 100 +++++---
 13 files changed, 582 insertions(+), 284 deletions(-)

-- 
1.9.0

[PATCH v2 01/27] t1400: Fix name and expected result of one test

[Michael Haggerty]

The test

    stdin -z create ref fails with zero new value

actually passes an empty new value, not a zero new value.  So rename
the test s/zero/empty/, and change the expected error from

    fatal: create $c given zero new value

to

    fatal: create $c missing <newvalue>

Of course, this makes the test fail now, so mark it
test_expect_failure.  The failure will be fixed later in this patch
series.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1400-update-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6ffd82f..fa927d2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
-test_expect_success 'stdin -z create ref fails with zero new value' '
+test_expect_failure 'stdin -z create ref fails with empty new value' '
 	printf $F "create $c" "" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: create $c given zero new value" err &&
+	grep "fatal: create $c missing <newvalue>" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
-- 
1.9.0

[PATCH v2 02/27] t1400: Provide more usual input to the command

[Michael Haggerty]

The old version was passing (among other things)

    update SP refs/heads/c NUL NUL 0{40} NUL

to "git update-ref -z --stdin" to test whether the old-value check for
c is working.  But the <newvalue> is empty, which is a bit off the
beaten track.

So, to be sure that we are testing what we want to test, provide an
actual <newvalue> on the "update" line.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1400-update-ref.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fa927d2..29391c6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with identity updates' '
 
 test_expect_success 'stdin -z update refs fails with wrong old value' '
 	git update-ref $c $m &&
-	printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "" "$Z" >stdin &&
+	printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m" "$Z" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
 	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
-- 
1.9.0

[PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated

[Michael Haggerty]

Test that the argument is properly terminated by either whitespace or
a NUL character, even if it is quoted, to be consistent with the
non-quoted case.  Adjust the tests to expect the new error message.
Add a docstring to the function, incorporating the comments that were
formerly within the function plus some added information.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  | 20 +++++++++++++++-----
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1292cfe..02b5f95 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
 	update->have_old = *oldvalue || line_termination;
 }
 
+/*
+ * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
+ * and append the result to arg.  Return a pointer to the terminator.
+ * Die if there is an error in how the argument is C-quoted.  This
+ * function is only used if not -z.
+ */
 static const char *parse_arg(const char *next, struct strbuf *arg)
 {
-	/* Parse SP-terminated, possibly C-quoted argument */
-	if (*next != '"')
+	if (*next == '"') {
+		const char *orig = next;
+
+		if (unquote_c_style(arg, next, &next))
+			die("badly quoted argument: %s", orig);
+		if (*next && !isspace(*next))
+			die("unexpected character after quoted argument: %s", orig);
+	} else {
 		while (*next && !isspace(*next))
 			strbuf_addch(arg, *next++);
-	else if (unquote_c_style(arg, next, &next))
-		die("badly quoted argument: %s", next);
+	}
 
-	/* Return position after the argument */
 	return next;
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 29391c6..774f8c5 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
 	grep "fatal: badly quoted argument: \\\"master" err
 '
 
-test_expect_success 'stdin fails on arguments not separated by space' '
+test_expect_success 'stdin fails on junk after quoted argument' '
 	echo "create \"$a\"master" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: expected SP but got: master" err
+	grep "fatal: unexpected character after quoted argument: \\\"$a\\\"master" err
 '
 
 test_expect_success 'stdin fails create with no ref' '
-- 
1.9.0

[PATCH v2 04/27] t1400: Add some more tests involving quoted arguments

[Michael Haggerty]

Previously there were no good tests of C-quoted arguments.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1400-update-ref.sh | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 774f8c5..00862bc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -350,12 +350,18 @@ test_expect_success 'stdin fails on unknown command' '
 	grep "fatal: unknown command: unknown $a" err
 '
 
-test_expect_success 'stdin fails on badly quoted input' '
+test_expect_success 'stdin fails on unbalanced quotes' '
 	echo "create $a \"master" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
 	grep "fatal: badly quoted argument: \\\"master" err
 '
 
+test_expect_success 'stdin fails on invalid escape' '
+	echo "create $a \"ma\zter\"" >stdin &&
+	test_must_fail git update-ref --stdin <stdin 2>err &&
+	grep "fatal: badly quoted argument: \\\"ma\\\\zter\\\"" err
+'
+
 test_expect_success 'stdin fails on junk after quoted argument' '
 	echo "create \"$a\"master" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
@@ -458,6 +464,24 @@ test_expect_success 'stdin create ref works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stdin succeeds with quoted argument' '
+	git update-ref -d $a &&
+	echo "create $a \"$m\"" >stdin &&
+	git update-ref --stdin <stdin &&
+	git rev-parse $m >expect &&
+	git rev-parse $a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'stdin succeeds with escaped character' '
+	git update-ref -d $a &&
+	echo "create $a \"ma\\163ter\"" >stdin &&
+	git update-ref --stdin <stdin &&
+	git rev-parse $m >expect &&
+	git rev-parse $a >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'stdin update ref creates with zero old value' '
 	echo "update $b $m $Z" >stdin &&
 	git update-ref --stdin <stdin &&
-- 
1.9.0

[PATCH v2 05/27] refs.h: Rename the action_on_err constants

[Michael Haggerty]

Given that these constants are only being used when updating
references, it is inappropriate to give them such generic names as
"DIE_ON_ERR".  So prefix their names with "UPDATE_REFS_".

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/checkout.c                     |  2 +-
 builtin/clone.c                        |  9 +++++----
 builtin/merge.c                        |  6 +++---
 builtin/notes.c                        |  6 +++---
 builtin/reset.c                        |  6 ++++--
 builtin/update-ref.c                   |  5 +++--
 contrib/examples/builtin-fetch--tool.c |  3 ++-
 notes-cache.c                          |  2 +-
 notes-utils.c                          |  3 ++-
 refs.c                                 | 18 +++++++++---------
 refs.h                                 |  9 +++++++--
 11 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..f79b222 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -624,7 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 		/* Nothing to do. */
 	} else if (opts->force_detach || !new->path) {	/* No longer on any branch. */
 		update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
-			   REF_NODEREF, DIE_ON_ERR);
+			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old->path && advice_detached_head)
 				detach_advice(new->name);
diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..af3b86f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -521,7 +521,7 @@ static void write_followtags(const struct ref *refs, const char *msg)
 		if (!has_sha1_file(ref->old_sha1))
 			continue;
 		update_ref(msg, ref->name, ref->old_sha1,
-			   NULL, 0, DIE_ON_ERR);
+			   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 	}
 }
 
@@ -589,14 +589,15 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		create_symref("HEAD", our->name, NULL);
 		if (!option_bare) {
 			const char *head = skip_prefix(our->name, "refs/heads/");
-			update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR);
+			update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
+				   UPDATE_REFS_DIE_ON_ERR);
 			install_branch_config(0, head, option_origin, our->name);
 		}
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(our->old_sha1);
 		/* --branch specifies a non-branch (i.e. tags), detach HEAD */
 		update_ref(msg, "HEAD", c->object.sha1,
-			   NULL, REF_NODEREF, DIE_ON_ERR);
+			   NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 	} else if (remote) {
 		/*
 		 * We know remote HEAD points to a non-branch, or
@@ -604,7 +605,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		 * Detach HEAD in all these cases.
 		 */
 		update_ref(msg, "HEAD", remote->old_sha1,
-			   NULL, REF_NODEREF, DIE_ON_ERR);
+			   NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 	}
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..a4c3b17 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -398,7 +398,7 @@ static void finish(struct commit *head_commit,
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
 			update_ref(reflog_message.buf, "HEAD",
 				new_head, head, 0,
-				DIE_ON_ERR);
+				UPDATE_REFS_DIE_ON_ERR);
 			/*
 			 * We ignore errors in 'gc --auto', since the
 			 * user should see them.
@@ -1222,7 +1222,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			die(_("%s - not something we can merge"), argv[0]);
 		read_empty(remote_head->object.sha1, 0);
 		update_ref("initial pull", "HEAD", remote_head->object.sha1,
-			   NULL, 0, DIE_ON_ERR);
+			   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 		goto done;
 	} else {
 		struct strbuf merge_names = STRBUF_INIT;
@@ -1339,7 +1339,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
-		   NULL, 0, DIE_ON_ERR);
+		   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
 	if (remoteheads && !common)
 		; /* No common ancestors found. We need a real merge. */
diff --git a/builtin/notes.c b/builtin/notes.c
index bb89930..66147b6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -717,7 +717,7 @@ static int merge_commit(struct notes_merge_options *o)
 	strbuf_insert(&msg, 0, "notes: ", 7);
 	update_ref(msg.buf, o->local_ref, sha1,
 		   is_null_sha1(parent_sha1) ? NULL : parent_sha1,
-		   0, DIE_ON_ERR);
+		   0, UPDATE_REFS_DIE_ON_ERR);
 
 	free_notes(t);
 	strbuf_release(&msg);
@@ -812,11 +812,11 @@ static int merge(int argc, const char **argv, const char *prefix)
 	if (result >= 0) /* Merge resulted (trivially) in result_sha1 */
 		/* Update default notes ref with new commit */
 		update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
-			   0, DIE_ON_ERR);
+			   0, UPDATE_REFS_DIE_ON_ERR);
 	else { /* Merge has unresolved conflicts */
 		/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
 		update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
-			   0, DIE_ON_ERR);
+			   0, UPDATE_REFS_DIE_ON_ERR);
 		/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
 		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
 			die("Failed to store link to current notes ref (%s)",
diff --git a/builtin/reset.c b/builtin/reset.c
index f4e0875..f368266 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -252,11 +252,13 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
 	if (!get_sha1("HEAD", sha1_orig)) {
 		orig = sha1_orig;
 		set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
-		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
+			   UPDATE_REFS_MSG_ON_ERR);
 	} else if (old_orig)
 		delete_ref("ORIG_HEAD", old_orig, 0);
 	set_reflog_message(&msg, "updating HEAD", rev);
-	update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+	update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0,
+				       UPDATE_REFS_MSG_ON_ERR);
 	strbuf_release(&msg);
 	return update_ref_status;
 }
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 02b5f95..f6345e5 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -282,7 +282,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		return update_refs(msg, updates, updates_count, DIE_ON_ERR);
+		return update_refs(msg, updates, updates_count,
+				   UPDATE_REFS_DIE_ON_ERR);
 	}
 
 	if (end_null)
@@ -314,5 +315,5 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  flags, DIE_ON_ERR);
+				  flags, UPDATE_REFS_DIE_ON_ERR);
 }
diff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c
index 8bc8c75..ee19166 100644
--- a/contrib/examples/builtin-fetch--tool.c
+++ b/contrib/examples/builtin-fetch--tool.c
@@ -31,7 +31,8 @@ static int update_ref_env(const char *action,
 		rla = "(reflog update)";
 	if (snprintf(msg, sizeof(msg), "%s: %s", rla, action) >= sizeof(msg))
 		warning("reflog message too long: %.*s...", 50, msg);
-	return update_ref(msg, refname, sha1, oldval, 0, QUIET_ON_ERR);
+	return update_ref(msg, refname, sha1, oldval, 0,
+			  UPDATE_REFS_QUIET_ON_ERR);
 }
 
 static int update_local_ref(const char *name,
diff --git a/notes-cache.c b/notes-cache.c
index eabe4a0..97dfd63 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -62,7 +62,7 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)
 		return -1;
 	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, QUIET_ON_ERR) < 0)
+		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index 4aa7023..a0b1d7b 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -48,7 +48,8 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, &buf, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);
+	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
+		   UPDATE_REFS_DIE_ON_ERR);
 
 	strbuf_release(&buf);
 }
diff --git a/refs.c b/refs.c
index 28d5eca..196984e 100644
--- a/refs.c
+++ b/refs.c
@@ -3243,9 +3243,9 @@ static struct ref_lock *update_ref_lock(const char *refname,
 	if (!lock) {
 		const char *str = "Cannot lock the ref '%s'.";
 		switch (onerr) {
-		case MSG_ON_ERR: error(str, refname); break;
-		case DIE_ON_ERR: die(str, refname); break;
-		case QUIET_ON_ERR: break;
+		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 lock;
@@ -3258,9 +3258,9 @@ static int update_ref_write(const char *action, const char *refname,
 	if (write_ref_sha1(lock, sha1, action) < 0) {
 		const char *str = "Cannot update the ref '%s'.";
 		switch (onerr) {
-		case MSG_ON_ERR: error(str, refname); break;
-		case DIE_ON_ERR: die(str, refname); break;
-		case QUIET_ON_ERR: break;
+		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;
 	}
@@ -3294,11 +3294,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 			const char *str =
 				"Multiple updates for ref '%s' not allowed.";
 			switch (onerr) {
-			case MSG_ON_ERR:
+			case UPDATE_REFS_MSG_ON_ERR:
 				error(str, updates[i]->ref_name); break;
-			case DIE_ON_ERR:
+			case UPDATE_REFS_DIE_ON_ERR:
 				die(str, updates[i]->ref_name); break;
-			case QUIET_ON_ERR:
+			case UPDATE_REFS_QUIET_ON_ERR:
 				break;
 			}
 			return 1;
diff --git a/refs.h b/refs.h
index 87a1a79..a713b34 100644
--- a/refs.h
+++ b/refs.h
@@ -214,8 +214,13 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg
  */
 extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);
 
-/** lock a ref and then write its file */
-enum action_on_err { MSG_ON_ERR, DIE_ON_ERR, QUIET_ON_ERR };
+enum action_on_err {
+	UPDATE_REFS_MSG_ON_ERR,
+	UPDATE_REFS_DIE_ON_ERR,
+	UPDATE_REFS_QUIET_ON_ERR
+};
+
+/** 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);
-- 
1.9.0

[PATCH v2 06/27] update_refs(): Fix constness

[Michael Haggerty]

Since full const correctness is beyond the ability of C's type system,
just put the const where it doesn't do any harm.  A (struct ref_update
**) can be passed to a (struct ref_update * const *) argument, but not
to a (const struct ref_update **) argument.

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

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index f6345e5..a8a68e8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {
 
 static int updates_alloc;
 static int updates_count;
-static const struct ref_update **updates;
+static struct ref_update **updates;
 
 static char line_termination = '\n';
 static int update_flags;
diff --git a/refs.c b/refs.c
index 196984e..1305eb1 100644
--- a/refs.c
+++ b/refs.c
@@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 	return 0;
 }
 
-int update_refs(const char *action, const struct ref_update **updates_orig,
+int update_refs(const char *action, struct ref_update * const *updates_orig,
 		int n, enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
diff --git a/refs.h b/refs.h
index a713b34..08e60ac 100644
--- a/refs.h
+++ b/refs.h
@@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,
 /**
  * Lock all refs and then perform all modifications.
  */
-int update_refs(const char *action, const struct ref_update **updates,
+int update_refs(const char *action, struct ref_update * const *updates,
 		int n, enum action_on_err onerr);
 
 extern int parse_hide_refs_config(const char *var, const char *value, const char *);
-- 
1.9.0

[PATCH v2 07/27] update-ref --stdin: Read the whole input at once

[Michael Haggerty]

Read the whole input into a strbuf at once, and then parse it from
there.  This might also be a tad faster, but that is not the point.
The point is to decouple the parsing code from the input source (the
old parsing code had to read new data even in the middle of commands).
Add docstrings for the parsing functions.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c | 170 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 108 insertions(+), 62 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a8a68e8..5f197fe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -85,44 +85,70 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
 	return next;
 }
 
-static const char *parse_first_arg(const char *next, struct strbuf *arg)
+/*
+ * Parse the argument immediately after "command SP".  If not -z, then
+ * handle C-quoting.  Write the argument to arg.  Set *next to point
+ * at the character that terminates the argument.  Die if C-quoting is
+ * malformed.
+ */
+static void parse_first_arg(struct strbuf *input, const char **next,
+			    struct strbuf *arg)
 {
-	/* Parse argument immediately after "command SP" */
 	strbuf_reset(arg);
 	if (line_termination) {
 		/* Without -z, use the next argument */
-		next = parse_arg(next, arg);
+		*next = parse_arg(*next, arg);
 	} else {
-		/* With -z, use rest of first NUL-terminated line */
-		strbuf_addstr(arg, next);
-		next = next + arg->len;
+		/* With -z, use everything up to the next NUL */
+		strbuf_addstr(arg, *next);
+		*next += arg->len;
 	}
-	return next;
 }
 
-static const char *parse_next_arg(const char *next, struct strbuf *arg)
+/*
+ * Parse a SP/NUL separator followed by the next SP- or NUL-terminated
+ * argument, if any.  If there is an argument, write it to arg, set
+ * *next to point at the character terminating the argument, and
+ * return 0.  If there is no argument at all (not even the empty
+ * string), return a non-zero result and leave *next unchanged.
+ */
+static int parse_next_arg(struct strbuf *input, const char **next,
+			  struct strbuf *arg)
 {
-	/* Parse next SP-terminated or NUL-terminated argument, if any */
 	strbuf_reset(arg);
 	if (line_termination) {
 		/* Without -z, consume SP and use next argument */
-		if (!*next)
-			return NULL;
-		if (*next != ' ')
-			die("expected SP but got: %s", next);
-		next = parse_arg(next + 1, arg);
+		if (!**next || **next == line_termination)
+			return -1;
+		if (**next != ' ')
+			die("expected SP but got: %s", *next);
+		(*next)++;
+		*next = parse_arg(*next, arg);
 	} else {
 		/* With -z, read the next NUL-terminated line */
-		if (*next)
-			die("expected NUL but got: %s", next);
-		if (strbuf_getline(arg, stdin, '\0') == EOF)
-			return NULL;
-		next = arg->buf + arg->len;
+		if (**next)
+			die("expected NUL but got: %s", *next);
+		(*next)++;
+		if (*next == input->buf + input->len)
+			return -1;
+		strbuf_addstr(arg, *next);
+		*next += arg->len;
 	}
-	return next;
+	return 0;
 }
 
-static void parse_cmd_update(const char *next)
+
+/*
+ * The following five parse_cmd_*() functions parse the corresponding
+ * command.  In each case, next points at the character following the
+ * command name and the following space.  They each return a pointer
+ * to the character terminating the command, and die with an
+ * explanatory message if there are any parsing problems.  All of
+ * these functions handle either text or binary format input,
+ * depending on how line_termination is set.
+ */
+
+static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
 	struct strbuf ref = STRBUF_INIT;
 	struct strbuf newvalue = STRBUF_INIT;
@@ -131,26 +157,28 @@ static void parse_cmd_update(const char *next)
 
 	update = update_alloc();
 
-	if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])
+	parse_first_arg(input, &next, &ref);
+	if (ref.buf[0])
 		update_store_ref_name(update, ref.buf);
 	else
 		die("update line missing <ref>");
 
-	if ((next = parse_next_arg(next, &newvalue)) != NULL)
+	if (!parse_next_arg(input, &next, &newvalue))
 		update_store_new_sha1(update, newvalue.buf);
 	else
 		die("update %s missing <newvalue>", ref.buf);
 
-	if ((next = parse_next_arg(next, &oldvalue)) != NULL)
+	if (!parse_next_arg(input, &next, &oldvalue)) {
 		update_store_old_sha1(update, oldvalue.buf);
-	else if(!line_termination)
+		if (*next != line_termination)
+			die("update %s has extra input: %s", ref.buf, next);
+	} else if (!line_termination)
 		die("update %s missing [<oldvalue>] NUL", ref.buf);
 
-	if (next && *next)
-		die("update %s has extra input: %s", ref.buf, next);
+	return next;
 }
 
-static void parse_cmd_create(const char *next)
+static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
 	struct strbuf ref = STRBUF_INIT;
 	struct strbuf newvalue = STRBUF_INIT;
@@ -158,23 +186,27 @@ static void parse_cmd_create(const char *next)
 
 	update = update_alloc();
 
-	if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])
+	parse_first_arg(input, &next, &ref);
+	if (ref.buf[0])
 		update_store_ref_name(update, ref.buf);
 	else
 		die("create line missing <ref>");
 
-	if ((next = parse_next_arg(next, &newvalue)) != NULL)
+	if (!parse_next_arg(input, &next, &newvalue))
 		update_store_new_sha1(update, newvalue.buf);
 	else
 		die("create %s missing <newvalue>", ref.buf);
+
 	if (is_null_sha1(update->new_sha1))
 		die("create %s given zero new value", ref.buf);
 
-	if (next && *next)
+	if (*next != line_termination)
 		die("create %s has extra input: %s", ref.buf, next);
+
+	return next;
 }
 
-static void parse_cmd_delete(const char *next)
+static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 {
 	struct strbuf ref = STRBUF_INIT;
 	struct strbuf oldvalue = STRBUF_INIT;
@@ -182,23 +214,26 @@ static void parse_cmd_delete(const char *next)
 
 	update = update_alloc();
 
-	if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])
+	parse_first_arg(input, &next, &ref);
+	if (ref.buf[0])
 		update_store_ref_name(update, ref.buf);
 	else
 		die("delete line missing <ref>");
 
-	if ((next = parse_next_arg(next, &oldvalue)) != NULL)
+	if (!parse_next_arg(input, &next, &oldvalue)) {
 		update_store_old_sha1(update, oldvalue.buf);
-	else if(!line_termination)
+		if (update->have_old && is_null_sha1(update->old_sha1))
+			die("delete %s given zero old value", ref.buf);
+	} else if (!line_termination)
 		die("delete %s missing [<oldvalue>] NUL", ref.buf);
-	if (update->have_old && is_null_sha1(update->old_sha1))
-		die("delete %s given zero old value", ref.buf);
 
-	if (next && *next)
+	if (*next != line_termination)
 		die("delete %s has extra input: %s", ref.buf, next);
+
+	return next;
 }
 
-static void parse_cmd_verify(const char *next)
+static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 {
 	struct strbuf ref = STRBUF_INIT;
 	struct strbuf value = STRBUF_INIT;
@@ -206,53 +241,64 @@ static void parse_cmd_verify(const char *next)
 
 	update = update_alloc();
 
-	if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])
+	parse_first_arg(input, &next, &ref);
+	if (ref.buf[0])
 		update_store_ref_name(update, ref.buf);
 	else
 		die("verify line missing <ref>");
 
-	if ((next = parse_next_arg(next, &value)) != NULL) {
+	if (!parse_next_arg(input, &next, &value)) {
 		update_store_old_sha1(update, value.buf);
 		update_store_new_sha1(update, value.buf);
-	} else if(!line_termination)
+	} else if (!line_termination)
 		die("verify %s missing [<oldvalue>] NUL", ref.buf);
 
-	if (next && *next)
+	if (*next != line_termination)
 		die("verify %s has extra input: %s", ref.buf, next);
+
+	return next;
 }
 
-static void parse_cmd_option(const char *next)
+static const char *parse_cmd_option(struct strbuf *input, const char *next)
 {
-	if (!strcmp(next, "no-deref"))
+	if (!strncmp(next, "no-deref", 8) && next[8] == line_termination)
 		update_flags |= REF_NODEREF;
 	else
 		die("option unknown: %s", next);
+	return next + 8;
 }
 
 static void update_refs_stdin(void)
 {
-	struct strbuf cmd = STRBUF_INIT;
+	struct strbuf input = STRBUF_INIT;
+	const char *next;
 
+	if (strbuf_read(&input, 0, 1000) < 0)
+		die_errno("could not read from stdin");
+	next = input.buf;
 	/* Read each line dispatch its command */
-	while (strbuf_getline(&cmd, stdin, line_termination) != EOF)
-		if (!cmd.buf[0])
+	while (next < input.buf + input.len) {
+		if (*next == line_termination)
 			die("empty command in input");
-		else if (isspace(*cmd.buf))
-			die("whitespace before command: %s", cmd.buf);
-		else if (starts_with(cmd.buf, "update "))
-			parse_cmd_update(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "create "))
-			parse_cmd_create(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "delete "))
-			parse_cmd_delete(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "verify "))
-			parse_cmd_verify(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "option "))
-			parse_cmd_option(cmd.buf + 7);
+		else if (isspace(*next))
+			die("whitespace before command: %s", next);
+		else if (starts_with(next, "update "))
+			next = parse_cmd_update(&input, next + 7);
+		else if (starts_with(next, "create "))
+			next = parse_cmd_create(&input, next + 7);
+		else if (starts_with(next, "delete "))
+			next = parse_cmd_delete(&input, next + 7);
+		else if (starts_with(next, "verify "))
+			next = parse_cmd_verify(&input, next + 7);
+		else if (starts_with(next, "option "))
+			next = parse_cmd_option(&input, next + 7);
 		else
-			die("unknown command: %s", cmd.buf);
+			die("unknown command: %s", next);
+
+		next++;
+	}
 
-	strbuf_release(&cmd);
+	strbuf_release(&input);
 }
 
 int cmd_update_ref(int argc, const char **argv, const char *prefix)
-- 
1.9.0

[PATCH v2 08/27] parse_cmd_verify(): Copy old_sha1 instead of evaluating <oldvalue> twice

[Michael Haggerty]

Aside from avoiding a tiny bit of work, this makes it transparently
obvious that old_sha1 and new_sha1 are identical.  It is arguably a
bit silly to have to set new_sha1 in order to verify old_sha1, but
that is a problem for another day.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5f197fe..51adf2d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -249,7 +249,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 
 	if (!parse_next_arg(input, &next, &value)) {
 		update_store_old_sha1(update, value.buf);
-		update_store_new_sha1(update, value.buf);
+		hashcpy(update->new_sha1, update->old_sha1);
 	} else if (!line_termination)
 		die("verify %s missing [<oldvalue>] NUL", ref.buf);
 
-- 
1.9.0

[PATCH v2 09/27] update-ref.c: Extract a new function, parse_refname()

[Michael Haggerty]

There is no reason to obscure the fact that parse_first_arg() always
parses refnames.  Form the new function by combining parse_first_arg()
and update_store_ref_name().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c | 90 ++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 49 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 51adf2d..0dc2061 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,14 +35,6 @@ static struct ref_update *update_alloc(void)
 	return update;
 }
 
-static void update_store_ref_name(struct ref_update *update,
-				  const char *ref_name)
-{
-	if (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL))
-		die("invalid ref format: %s", ref_name);
-	update->ref_name = xstrdup(ref_name);
-}
-
 static void update_store_new_sha1(struct ref_update *update,
 				  const char *newvalue)
 {
@@ -86,23 +78,35 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
 }
 
 /*
- * Parse the argument immediately after "command SP".  If not -z, then
- * handle C-quoting.  Write the argument to arg.  Set *next to point
- * at the character that terminates the argument.  Die if C-quoting is
- * malformed.
+ * Parse the reference name immediately after "command SP".  If not
+ * -z, then handle C-quoting.  Return a pointer to a newly allocated
+ * string containing the name of the reference, or NULL if there was
+ * an error.  Update *next to point at the character that terminates
+ * the argument.  Die if C-quoting is malformed or the reference name
+ * is invalid.
  */
-static void parse_first_arg(struct strbuf *input, const char **next,
-			    struct strbuf *arg)
+static char *parse_refname(struct strbuf *input, const char **next)
 {
-	strbuf_reset(arg);
+	struct strbuf ref = STRBUF_INIT;
+
 	if (line_termination) {
 		/* Without -z, use the next argument */
-		*next = parse_arg(*next, arg);
+		*next = parse_arg(*next, &ref);
 	} else {
 		/* With -z, use everything up to the next NUL */
-		strbuf_addstr(arg, *next);
-		*next += arg->len;
+		strbuf_addstr(&ref, *next);
+		*next += ref.len;
+	}
+
+	if (!ref.len) {
+		strbuf_release(&ref);
+		return NULL;
 	}
+
+	if (check_refname_format(ref.buf, REFNAME_ALLOW_ONELEVEL))
+		die("invalid ref format: %s", ref.buf);
+
+	return strbuf_detach(&ref, NULL);
 }
 
 /*
@@ -150,111 +154,99 @@ static int parse_next_arg(struct strbuf *input, const char **next,
 
 static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
-	struct strbuf ref = STRBUF_INIT;
 	struct strbuf newvalue = STRBUF_INIT;
 	struct strbuf oldvalue = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
 
-	parse_first_arg(input, &next, &ref);
-	if (ref.buf[0])
-		update_store_ref_name(update, ref.buf);
-	else
+	update->ref_name = parse_refname(input, &next);
+	if (!update->ref_name)
 		die("update line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &newvalue))
 		update_store_new_sha1(update, newvalue.buf);
 	else
-		die("update %s missing <newvalue>", ref.buf);
+		die("update %s missing <newvalue>", update->ref_name);
 
 	if (!parse_next_arg(input, &next, &oldvalue)) {
 		update_store_old_sha1(update, oldvalue.buf);
 		if (*next != line_termination)
-			die("update %s has extra input: %s", ref.buf, next);
+			die("update %s has extra input: %s", update->ref_name, next);
 	} else if (!line_termination)
-		die("update %s missing [<oldvalue>] NUL", ref.buf);
+		die("update %s missing [<oldvalue>] NUL", update->ref_name);
 
 	return next;
 }
 
 static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
-	struct strbuf ref = STRBUF_INIT;
 	struct strbuf newvalue = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
 
-	parse_first_arg(input, &next, &ref);
-	if (ref.buf[0])
-		update_store_ref_name(update, ref.buf);
-	else
+	update->ref_name = parse_refname(input, &next);
+	if (!update->ref_name)
 		die("create line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &newvalue))
 		update_store_new_sha1(update, newvalue.buf);
 	else
-		die("create %s missing <newvalue>", ref.buf);
+		die("create %s missing <newvalue>", update->ref_name);
 
 	if (is_null_sha1(update->new_sha1))
-		die("create %s given zero new value", ref.buf);
+		die("create %s given zero new value", update->ref_name);
 
 	if (*next != line_termination)
-		die("create %s has extra input: %s", ref.buf, next);
+		die("create %s has extra input: %s", update->ref_name, next);
 
 	return next;
 }
 
 static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 {
-	struct strbuf ref = STRBUF_INIT;
 	struct strbuf oldvalue = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
 
-	parse_first_arg(input, &next, &ref);
-	if (ref.buf[0])
-		update_store_ref_name(update, ref.buf);
-	else
+	update->ref_name = parse_refname(input, &next);
+	if (!update->ref_name)
 		die("delete line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &oldvalue)) {
 		update_store_old_sha1(update, oldvalue.buf);
 		if (update->have_old && is_null_sha1(update->old_sha1))
-			die("delete %s given zero old value", ref.buf);
+			die("delete %s given zero old value", update->ref_name);
 	} else if (!line_termination)
-		die("delete %s missing [<oldvalue>] NUL", ref.buf);
+		die("delete %s missing [<oldvalue>] NUL", update->ref_name);
 
 	if (*next != line_termination)
-		die("delete %s has extra input: %s", ref.buf, next);
+		die("delete %s has extra input: %s", update->ref_name, next);
 
 	return next;
 }
 
 static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 {
-	struct strbuf ref = STRBUF_INIT;
 	struct strbuf value = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
 
-	parse_first_arg(input, &next, &ref);
-	if (ref.buf[0])
-		update_store_ref_name(update, ref.buf);
-	else
+	update->ref_name = parse_refname(input, &next);
+	if (!update->ref_name)
 		die("verify line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &value)) {
 		update_store_old_sha1(update, value.buf);
 		hashcpy(update->new_sha1, update->old_sha1);
 	} else if (!line_termination)
-		die("verify %s missing [<oldvalue>] NUL", ref.buf);
+		die("verify %s missing [<oldvalue>] NUL", update->ref_name);
 
 	if (*next != line_termination)
-		die("verify %s has extra input: %s", ref.buf, next);
+		die("verify %s has extra input: %s", update->ref_name, next);
 
 	return next;
 }
-- 
1.9.0

[PATCH v2 10/27] update-ref --stdin: Improve error messages for invalid values

[Michael Haggerty]

If an invalid value is passed to "update-ref --stdin" as <oldvalue> or
<newvalue>, include the command and the name of the reference at the
beginning of the error message.  Update the tests accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  | 24 +++++++++++++-----------
 t/t1400-update-ref.sh |  8 ++++----
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0dc2061..13a884a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,20 +35,22 @@ static struct ref_update *update_alloc(void)
 	return update;
 }
 
-static void update_store_new_sha1(struct ref_update *update,
+static void update_store_new_sha1(const char *command,
+				  struct ref_update *update,
 				  const char *newvalue)
 {
 	if (*newvalue && get_sha1(newvalue, update->new_sha1))
-		die("invalid new value for ref %s: %s",
-		    update->ref_name, newvalue);
+		die("%s %s: invalid new value: %s",
+		    command, update->ref_name, newvalue);
 }
 
-static void update_store_old_sha1(struct ref_update *update,
+static void update_store_old_sha1(const char *command,
+				  struct ref_update *update,
 				  const char *oldvalue)
 {
 	if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
-		die("invalid old value for ref %s: %s",
-		    update->ref_name, oldvalue);
+		die("%s %s: invalid old value: %s",
+		    command, update->ref_name, oldvalue);
 
 	/* We have an old value if non-empty, or if empty without -z */
 	update->have_old = *oldvalue || line_termination;
@@ -165,12 +167,12 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 		die("update line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &newvalue))
-		update_store_new_sha1(update, newvalue.buf);
+		update_store_new_sha1("update", update, newvalue.buf);
 	else
 		die("update %s missing <newvalue>", update->ref_name);
 
 	if (!parse_next_arg(input, &next, &oldvalue)) {
-		update_store_old_sha1(update, oldvalue.buf);
+		update_store_old_sha1("update", update, oldvalue.buf);
 		if (*next != line_termination)
 			die("update %s has extra input: %s", update->ref_name, next);
 	} else if (!line_termination)
@@ -191,7 +193,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 		die("create line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &newvalue))
-		update_store_new_sha1(update, newvalue.buf);
+		update_store_new_sha1("create", update, newvalue.buf);
 	else
 		die("create %s missing <newvalue>", update->ref_name);
 
@@ -216,7 +218,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 		die("delete line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &oldvalue)) {
-		update_store_old_sha1(update, oldvalue.buf);
+		update_store_old_sha1("delete", update, oldvalue.buf);
 		if (update->have_old && is_null_sha1(update->old_sha1))
 			die("delete %s given zero old value", update->ref_name);
 	} else if (!line_termination)
@@ -240,7 +242,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 		die("verify line missing <ref>");
 
 	if (!parse_next_arg(input, &next, &value)) {
-		update_store_old_sha1(update, value.buf);
+		update_store_old_sha1("verify", update, value.buf);
 		hashcpy(update->new_sha1, update->old_sha1);
 	} else if (!line_termination)
 		die("verify %s missing [<oldvalue>] NUL", update->ref_name);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 00862bc..f6c6e96 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -518,14 +518,14 @@ test_expect_success 'stdin update ref fails with wrong old value' '
 test_expect_success 'stdin update ref fails with bad old value' '
 	echo "update $c $m does-not-exist" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: invalid old value for ref $c: does-not-exist" err &&
+	grep "fatal: update $c: invalid old value: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin create ref fails with bad new value' '
 	echo "create $c does-not-exist" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: invalid new value for ref $c: does-not-exist" err &&
+	grep "fatal: create $c: invalid new value: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong old value' '
 test_expect_success 'stdin -z update ref fails with bad old value' '
 	printf $F "update $c" "$m" "does-not-exist" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: invalid old value for ref $c: does-not-exist" err &&
+	grep "fatal: update $c: invalid old value: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin -z create ref fails with bad new value' '
 	printf $F "create $c" "does-not-exist" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: invalid new value for ref $c: does-not-exist" err &&
+	grep "fatal: create $c: invalid new value: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
-- 
1.9.0

[PATCH v2 11/27] update-ref --stdin: Make error messages more consistent

[Michael Haggerty]

The old error messages emitted for invalid input sometimes said
"<oldvalue>"/"<newvalue>" and sometimes said "old value"/"new value".
Convert them all to the former.  Update the tests accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  |  8 ++++----
 t/t1400-update-ref.sh | 14 +++++++-------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 13a884a..e4c0854 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -40,7 +40,7 @@ static void update_store_new_sha1(const char *command,
 				  const char *newvalue)
 {
 	if (*newvalue && get_sha1(newvalue, update->new_sha1))
-		die("%s %s: invalid new value: %s",
+		die("%s %s: invalid <newvalue>: %s",
 		    command, update->ref_name, newvalue);
 }
 
@@ -49,7 +49,7 @@ static void update_store_old_sha1(const char *command,
 				  const char *oldvalue)
 {
 	if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
-		die("%s %s: invalid old value: %s",
+		die("%s %s: invalid <oldvalue>: %s",
 		    command, update->ref_name, oldvalue);
 
 	/* We have an old value if non-empty, or if empty without -z */
@@ -198,7 +198,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 		die("create %s missing <newvalue>", update->ref_name);
 
 	if (is_null_sha1(update->new_sha1))
-		die("create %s given zero new value", update->ref_name);
+		die("create %s given zero <newvalue>", update->ref_name);
 
 	if (*next != line_termination)
 		die("create %s has extra input: %s", update->ref_name, next);
@@ -220,7 +220,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 	if (!parse_next_arg(input, &next, &oldvalue)) {
 		update_store_old_sha1("delete", update, oldvalue.buf);
 		if (update->have_old && is_null_sha1(update->old_sha1))
-			die("delete %s given zero old value", update->ref_name);
+			die("delete %s given zero <oldvalue>", update->ref_name);
 	} else if (!line_termination)
 		die("delete %s missing [<oldvalue>] NUL", update->ref_name);
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f6c6e96..ef61fe3 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -518,21 +518,21 @@ test_expect_success 'stdin update ref fails with wrong old value' '
 test_expect_success 'stdin update ref fails with bad old value' '
 	echo "update $c $m does-not-exist" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: update $c: invalid old value: does-not-exist" err &&
+	grep "fatal: update $c: invalid <oldvalue>: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin create ref fails with bad new value' '
 	echo "create $c does-not-exist" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: create $c: invalid new value: does-not-exist" err &&
+	grep "fatal: create $c: invalid <newvalue>: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin create ref fails with zero new value' '
 	echo "create $c " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: create $c given zero new value" err &&
+	grep "fatal: create $c given zero <newvalue>" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' '
 test_expect_success 'stdin delete ref fails with zero old value' '
 	echo "delete $a " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: delete $a given zero old value" err &&
+	grep "fatal: delete $a given zero <oldvalue>" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong old value' '
 test_expect_success 'stdin -z update ref fails with bad old value' '
 	printf $F "update $c" "$m" "does-not-exist" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: update $c: invalid old value: does-not-exist" err &&
+	grep "fatal: update $c: invalid <oldvalue>: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
 test_expect_success 'stdin -z create ref fails with bad new value' '
 	printf $F "create $c" "does-not-exist" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: create $c: invalid new value: does-not-exist" err &&
+	grep "fatal: create $c: invalid <newvalue>: does-not-exist" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -878,7 +878,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' '
 test_expect_success 'stdin -z delete ref fails with zero old value' '
 	printf $F "delete $a" "$Z" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: delete $a given zero old value" err &&
+	grep "fatal: delete $a given zero <oldvalue>" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
-- 
1.9.0

[PATCH v2 12/27] update-ref --stdin: Simplify error messages for missing oldvalues

[Michael Haggerty]

Instead of, for example,

    fatal: update refs/heads/master missing [<oldvalue>] NUL

emit

    fatal: update refs/heads/master missing <oldvalue>

Update the tests accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  | 6 +++---
 t/t1400-update-ref.sh | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index e4c0854..a9eb5fe 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -176,7 +176,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 		if (*next != line_termination)
 			die("update %s has extra input: %s", update->ref_name, next);
 	} else if (!line_termination)
-		die("update %s missing [<oldvalue>] NUL", update->ref_name);
+		die("update %s missing <oldvalue>", update->ref_name);
 
 	return next;
 }
@@ -222,7 +222,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 		if (update->have_old && is_null_sha1(update->old_sha1))
 			die("delete %s given zero <oldvalue>", update->ref_name);
 	} else if (!line_termination)
-		die("delete %s missing [<oldvalue>] NUL", update->ref_name);
+		die("delete %s missing <oldvalue>", update->ref_name);
 
 	if (*next != line_termination)
 		die("delete %s has extra input: %s", update->ref_name, next);
@@ -245,7 +245,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 		update_store_old_sha1("verify", update, value.buf);
 		hashcpy(update->new_sha1, update->old_sha1);
 	} else if (!line_termination)
-		die("verify %s missing [<oldvalue>] NUL", update->ref_name);
+		die("verify %s missing <oldvalue>", update->ref_name);
 
 	if (*next != line_termination)
 		die("verify %s has extra input: %s", update->ref_name, next);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ef61fe3..a2015d0 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -739,7 +739,7 @@ test_expect_success 'stdin -z fails update with no new value' '
 test_expect_success 'stdin -z fails update with no old value' '
 	printf $F "update $a" "$m" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: update $a missing \\[<oldvalue>\\] NUL" err
+	grep "fatal: update $a missing <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails update with too many arguments' '
@@ -763,7 +763,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' '
 test_expect_success 'stdin -z fails delete with no old value' '
 	printf $F "delete $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: delete $a missing \\[<oldvalue>\\] NUL" err
+	grep "fatal: delete $a missing <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails delete with too many arguments' '
@@ -781,7 +781,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' '
 test_expect_success 'stdin -z fails verify with no old value' '
 	printf $F "verify $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: verify $a missing \\[<oldvalue>\\] NUL" err
+	grep "fatal: verify $a missing <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails option with unknown name' '
-- 
1.9.0

[PATCH v2 13/27] t1400: Test that stdin -z update treats empty <newvalue> as zeros

[Michael Haggerty]

This is the (slightly inconsistent) status quo; make sure it doesn't
change by accident.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1400-update-ref.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a2015d0..208f56e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref name' '
 	grep "fatal: invalid ref format: ~a" err
 '
 
+test_expect_success 'stdin -z treats empty new value as zeros' '
+	git update-ref $a $m &&
+	printf $F "update $a" "" "" >stdin &&
+	git update-ref -z --stdin <stdin &&
+	test_must_fail git rev-parse --verify -q $a
+'
+
 test_expect_success 'stdin -z fails update with no new value' '
 	printf $F "update $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-- 
1.9.0

[PATCH v2 14/27] update-ref.c: Extract a new function, parse_next_sha1()

[Michael Haggerty]

Replace three functions, update_store_new_sha1(),
update_store_old_sha1(), and parse_next_arg(), with a single function,
parse_next_sha1().  The new function takes care of a whole argument,
including checking whether it is there, converting it to an SHA-1, and
emitting errors on EOF or for invalid values.  The return value
indicates whether the argument was present or absent, which requires
a bit of intelligence because absent values are represented
differently depending on whether "-z" was used.

The new interface means that the calling functions, parse_cmd_*(),
don't have to interpret the result differently based on the
line_termination mode that is in effect.  It also means that
parse_cmd_create() can distinguish unambiguously between an empty new
value and a zeros new value, which fixes a failure in t1400.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  | 160 +++++++++++++++++++++++++++++++-------------------
 t/t1400-update-ref.sh |   2 +-
 2 files changed, 99 insertions(+), 63 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a9eb5fe..6462b2f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -35,27 +35,6 @@ static struct ref_update *update_alloc(void)
 	return update;
 }
 
-static void update_store_new_sha1(const char *command,
-				  struct ref_update *update,
-				  const char *newvalue)
-{
-	if (*newvalue && get_sha1(newvalue, update->new_sha1))
-		die("%s %s: invalid <newvalue>: %s",
-		    command, update->ref_name, newvalue);
-}
-
-static void update_store_old_sha1(const char *command,
-				  struct ref_update *update,
-				  const char *oldvalue)
-{
-	if (*oldvalue && get_sha1(oldvalue, update->old_sha1))
-		die("%s %s: invalid <oldvalue>: %s",
-		    command, update->ref_name, oldvalue);
-
-	/* We have an old value if non-empty, or if empty without -z */
-	update->have_old = *oldvalue || line_termination;
-}
-
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
  * and append the result to arg.  Return a pointer to the terminator.
@@ -112,35 +91,94 @@ static char *parse_refname(struct strbuf *input, const char **next)
 }
 
 /*
- * Parse a SP/NUL separator followed by the next SP- or NUL-terminated
- * argument, if any.  If there is an argument, write it to arg, set
- * *next to point at the character terminating the argument, and
+ * The value being parsed is <oldvalue> (as opposed to <newvalue>; the
+ * difference affects which error messages are generated):
+ */
+#define PARSE_SHA1_OLD 0x01
+
+/*
+ * For backwards compatibility, accept an empty string for create's
+ * <newvalue> in binary mode to be equivalent to specifying zeros.
+ */
+#define PARSE_SHA1_ALLOW_EMPTY 0x02
+
+/*
+ * Parse an argument separator followed by the next argument, if any.
+ * If there is an argument, convert it to a SHA-1, write it to sha1,
+ * set *next to point at the character terminating the argument, and
  * return 0.  If there is no argument at all (not even the empty
- * string), return a non-zero result and leave *next unchanged.
+ * string), return 1 and leave *next unchanged.  If the value is
+ * provided but cannot be converted to a SHA-1, die.  flags can
+ * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
  */
-static int parse_next_arg(struct strbuf *input, const char **next,
-			  struct strbuf *arg)
+static int parse_next_sha1(struct strbuf *input, const char **next,
+			   unsigned char *sha1,
+			   const char *command, const char *refname,
+			   int flags)
 {
-	strbuf_reset(arg);
+	struct strbuf arg = STRBUF_INIT;
+	int ret = 0;
+
+	if (*next == input->buf + input->len)
+		goto eof;
+
 	if (line_termination) {
 		/* Without -z, consume SP and use next argument */
 		if (!**next || **next == line_termination)
-			return -1;
+			return 1;
 		if (**next != ' ')
-			die("expected SP but got: %s", *next);
+			die("%s %s: expected SP but got: %s",
+			    command, refname, *next);
 		(*next)++;
-		*next = parse_arg(*next, arg);
+		*next = parse_arg(*next, &arg);
+		if (arg.len) {
+			if (get_sha1(arg.buf, sha1))
+				goto invalid;
+		} else {
+			/* Without -z, an empty value means all zeros: */
+			hashclr(sha1);
+		}
 	} else {
 		/* With -z, read the next NUL-terminated line */
 		if (**next)
-			die("expected NUL but got: %s", *next);
+			die("%s %s: expected NUL but got: %s",
+			    command, refname, *next);
 		(*next)++;
 		if (*next == input->buf + input->len)
-			return -1;
-		strbuf_addstr(arg, *next);
-		*next += arg->len;
+			goto eof;
+		strbuf_addstr(&arg, *next);
+		*next += arg.len;
+
+		if (arg.len) {
+			if (get_sha1(arg.buf, sha1))
+				goto invalid;
+		} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {
+			/* With -z, treat an empty value as all zeros: */
+			hashclr(sha1);
+		} else {
+			/*
+			 * With -z, an empty non-required value means
+			 * unspecified:
+			 */
+			ret = 1;
+		}
 	}
-	return 0;
+
+	strbuf_release(&arg);
+
+	return ret;
+
+ invalid:
+	die(flags & PARSE_SHA1_OLD ?
+	    "%s %s: invalid <oldvalue>: %s" :
+	    "%s %s: invalid <newvalue>: %s",
+	    command, refname, arg.buf);
+
+ eof:
+	die(flags & PARSE_SHA1_OLD ?
+	    "%s %s missing <oldvalue>" :
+	    "%s %s missing <newvalue>",
+	    command, refname);
 }
 
 
@@ -156,8 +194,6 @@ static int parse_next_arg(struct strbuf *input, const char **next,
 
 static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
-	struct strbuf newvalue = STRBUF_INIT;
-	struct strbuf oldvalue = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
@@ -166,24 +202,23 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 	if (!update->ref_name)
 		die("update line missing <ref>");
 
-	if (!parse_next_arg(input, &next, &newvalue))
-		update_store_new_sha1("update", update, newvalue.buf);
-	else
+	if (parse_next_sha1(input, &next, update->new_sha1,
+			    "update", update->ref_name,
+			    PARSE_SHA1_ALLOW_EMPTY))
 		die("update %s missing <newvalue>", update->ref_name);
 
-	if (!parse_next_arg(input, &next, &oldvalue)) {
-		update_store_old_sha1("update", update, oldvalue.buf);
-		if (*next != line_termination)
-			die("update %s has extra input: %s", update->ref_name, next);
-	} else if (!line_termination)
-		die("update %s missing <oldvalue>", update->ref_name);
+	update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
+					    "update", update->ref_name,
+					    PARSE_SHA1_OLD);
+
+	if (*next != line_termination)
+		die("update %s has extra input: %s", update->ref_name, next);
 
 	return next;
 }
 
 static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
-	struct strbuf newvalue = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
@@ -192,9 +227,8 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 	if (!update->ref_name)
 		die("create line missing <ref>");
 
-	if (!parse_next_arg(input, &next, &newvalue))
-		update_store_new_sha1("create", update, newvalue.buf);
-	else
+	if (parse_next_sha1(input, &next, update->new_sha1,
+			    "create", update->ref_name, 0))
 		die("create %s missing <newvalue>", update->ref_name);
 
 	if (is_null_sha1(update->new_sha1))
@@ -208,7 +242,6 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 
 static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 {
-	struct strbuf oldvalue = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
@@ -217,12 +250,14 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 	if (!update->ref_name)
 		die("delete line missing <ref>");
 
-	if (!parse_next_arg(input, &next, &oldvalue)) {
-		update_store_old_sha1("delete", update, oldvalue.buf);
-		if (update->have_old && is_null_sha1(update->old_sha1))
+	if (parse_next_sha1(input, &next, update->old_sha1,
+			    "delete", update->ref_name, PARSE_SHA1_OLD)) {
+		update->have_old = 0;
+	} else {
+		if (is_null_sha1(update->old_sha1))
 			die("delete %s given zero <oldvalue>", update->ref_name);
-	} else if (!line_termination)
-		die("delete %s missing <oldvalue>", update->ref_name);
+		update->have_old = 1;
+	}
 
 	if (*next != line_termination)
 		die("delete %s has extra input: %s", update->ref_name, next);
@@ -232,7 +267,6 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 
 static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 {
-	struct strbuf value = STRBUF_INIT;
 	struct ref_update *update;
 
 	update = update_alloc();
@@ -241,11 +275,13 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 	if (!update->ref_name)
 		die("verify line missing <ref>");
 
-	if (!parse_next_arg(input, &next, &value)) {
-		update_store_old_sha1("verify", update, value.buf);
+	if (parse_next_sha1(input, &next, update->old_sha1,
+			    "verify", update->ref_name, PARSE_SHA1_OLD)) {
+		update->have_old = 0;
+	} else {
 		hashcpy(update->new_sha1, update->old_sha1);
-	} else if (!line_termination)
-		die("verify %s missing <oldvalue>", update->ref_name);
+		update->have_old = 1;
+	}
 
 	if (*next != line_termination)
 		die("verify %s has extra input: %s", update->ref_name, next);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 208f56e..15f5bfd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -858,7 +858,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
-test_expect_failure 'stdin -z create ref fails with empty new value' '
+test_expect_success 'stdin -z create ref fails with empty new value' '
 	printf $F "create $c" "" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
 	grep "fatal: create $c missing <newvalue>" err &&
-- 
1.9.0

[PATCH v2 15/27] update-ref --stdin -z: Deprecate interpreting the empty string as zeros

[Michael Haggerty]

In the original version of this command, for the single case of the
"update" command's <newvalue>, the empty string was interpreted as
being equivalent to 40 "0"s.  This shorthand is unnecessary (binary
input will usually be generated programmatically anyway), and it
complicates the parser and the documentation.

So gently deprecate this usage: remove its description from the
documentation and emit a warning if it is found.  But for reasons of
backwards compatibility, continue to accept it.

Helped-by: Brad King <brad.king@kitware.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-update-ref.txt | 18 ++++++++++++------
 builtin/update-ref.c             |  2 ++
 t/t1400-update-ref.sh            |  5 +++--
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 0a0a551..c8f5ae5 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -68,7 +68,12 @@ performs all modifications together.  Specify commands of the form:
 	option SP <opt> LF
 
 Quote fields containing whitespace as if they were strings in C source
-code.  Alternatively, use `-z` to specify commands without quoting:
+code; i.e., surrounded by double-quotes and with backslash escapes.
+Use 40 "0" characters or the empty string to specify a zero value.  To
+specify a missing value, omit the value and its preceding SP entirely.
+
+Alternatively, use `-z` to specify in NUL-terminated format, without
+quoting:
 
 	update SP <ref> NUL <newvalue> NUL [<oldvalue>] NUL
 	create SP <ref> NUL <newvalue> NUL
@@ -76,8 +81,12 @@ code.  Alternatively, use `-z` to specify commands without quoting:
 	verify SP <ref> NUL [<oldvalue>] NUL
 	option SP <opt> NUL
 
-Lines of any other format or a repeated <ref> produce an error.
-Command meanings are:
+In this format, use 40 "0" to specify a zero value, and use the empty
+string to specify a missing value.
+
+In either format, values can be specified in any form that Git
+recognizes as an object name.  Commands in any other format or a
+repeated <ref> produce an error.  Command meanings are:
 
 update::
 	Set <ref> to <newvalue> after verifying <oldvalue>, if given.
@@ -102,9 +111,6 @@ option::
 	The only valid option is `no-deref` to avoid dereferencing
 	a symbolic ref.
 
-Use 40 "0" or the empty string to specify a zero value, except that
-with `-z` an empty <oldvalue> is considered missing.
-
 If all <ref>s can be locked with matching <oldvalue>s
 simultaneously, all modifications are performed.  Otherwise, no
 modifications are performed.  Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6462b2f..eef7537 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
 				goto invalid;
 		} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {
 			/* With -z, treat an empty value as all zeros: */
+			warning("%s %s: missing <newvalue>, treating as zero",
+				command, refname);
 			hashclr(sha1);
 		} else {
 			/*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 15f5bfd..2d61cce 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref name' '
 	grep "fatal: invalid ref format: ~a" err
 '
 
-test_expect_success 'stdin -z treats empty new value as zeros' '
+test_expect_success 'stdin -z emits warning with empty new value' '
 	git update-ref $a $m &&
 	printf $F "update $a" "" "" >stdin &&
-	git update-ref -z --stdin <stdin &&
+	git update-ref -z --stdin <stdin 2>err &&
+	grep "warning: update $a: missing <newvalue>, treating as zero" err &&
 	test_must_fail git rev-parse --verify -q $a
 '
 
-- 
1.9.0

[PATCH v2 16/27] t1400: Test one mistake at a time

[Michael Haggerty]

This case wants to test passing a bad refname to the "update" command.
But it also passes too few arguments to "update", which muddles the
situation: which error should be diagnosed?  So split this test into
two:

* One that passes too few arguments to update

* One that passes all three arguments to "update", but with a bad
  refname.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

t1400: Add a test of "update" with too few arguments

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1400-update-ref.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 2d61cce..6b21e45 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' '
 	grep "fatal: update line missing <ref>" err
 '
 
+test_expect_success 'stdin -z fails update with too few args' '
+	printf $F "update $a" "$m" >stdin &&
+	test_must_fail git update-ref -z --stdin <stdin 2>err &&
+	grep "fatal: update $a missing <oldvalue>" err
+'
+
 test_expect_success 'stdin -z fails update with bad ref name' '
-	printf $F "update ~a" "$m" >stdin &&
+	printf $F "update ~a" "$m" "" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
 	grep "fatal: invalid ref format: ~a" err
 '
-- 
1.9.0

[PATCH v2 17/27] update-ref --stdin: Improve the error message for unexpected EOF

[Michael Haggerty]

Distinguish this error from the error that an argument is missing for
another reason.  Update the tests accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  |  4 ++--
 t/t1400-update-ref.sh | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index eef7537..b49a5b0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -178,8 +178,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
 
  eof:
 	die(flags & PARSE_SHA1_OLD ?
-	    "%s %s missing <oldvalue>" :
-	    "%s %s missing <newvalue>",
+	    "%s %s: unexpected end of input when reading <oldvalue>" :
+	    "%s %s: unexpected end of input when reading <newvalue>",
 	    command, refname);
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6b21e45..1db0689 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -709,7 +709,7 @@ test_expect_success 'stdin -z fails create with bad ref name' '
 test_expect_success 'stdin -z fails create with no new value' '
 	printf $F "create $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: create $a missing <newvalue>" err
+	grep "fatal: create $a: unexpected end of input when reading <newvalue>" err
 '
 
 test_expect_success 'stdin -z fails create with too many arguments' '
@@ -727,7 +727,7 @@ test_expect_success 'stdin -z fails update with no ref' '
 test_expect_success 'stdin -z fails update with too few args' '
 	printf $F "update $a" "$m" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: update $a missing <oldvalue>" err
+	grep "fatal: update $a: unexpected end of input when reading <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails update with bad ref name' '
@@ -747,13 +747,13 @@ test_expect_success 'stdin -z emits warning with empty new value' '
 test_expect_success 'stdin -z fails update with no new value' '
 	printf $F "update $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: update $a missing <newvalue>" err
+	grep "fatal: update $a: unexpected end of input when reading <newvalue>" err
 '
 
 test_expect_success 'stdin -z fails update with no old value' '
 	printf $F "update $a" "$m" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: update $a missing <oldvalue>" err
+	grep "fatal: update $a: unexpected end of input when reading <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails update with too many arguments' '
@@ -777,7 +777,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' '
 test_expect_success 'stdin -z fails delete with no old value' '
 	printf $F "delete $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: delete $a missing <oldvalue>" err
+	grep "fatal: delete $a: unexpected end of input when reading <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails delete with too many arguments' '
@@ -795,7 +795,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' '
 test_expect_success 'stdin -z fails verify with no old value' '
 	printf $F "verify $a" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: verify $a missing <oldvalue>" err
+	grep "fatal: verify $a: unexpected end of input when reading <oldvalue>" err
 '
 
 test_expect_success 'stdin -z fails option with unknown name' '
-- 
1.9.0

[PATCH v2 18/27] update-ref --stdin: Harmonize error messages

[Michael Haggerty]

Make (most of) the error messages for invalid input have the same
format [1]:

    $COMMAND [SP $REFNAME]: $MESSAGE

Update the tests accordingly.

[1] A few error messages are left with their old form, because
    $COMMAND and $REFNAME aren't passed all the way down the call
    stack.  Maybe those sites should be changed some day, too.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  | 24 ++++++++++++------------
 t/t1400-update-ref.sh | 32 ++++++++++++++++----------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index b49a5b0..bbc04b2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 
 	update->ref_name = parse_refname(input, &next);
 	if (!update->ref_name)
-		die("update line missing <ref>");
+		die("update: missing <ref>");
 
 	if (parse_next_sha1(input, &next, update->new_sha1,
 			    "update", update->ref_name,
 			    PARSE_SHA1_ALLOW_EMPTY))
-		die("update %s missing <newvalue>", update->ref_name);
+		die("update %s: missing <newvalue>", update->ref_name);
 
 	update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
 					    "update", update->ref_name,
 					    PARSE_SHA1_OLD);
 
 	if (*next != line_termination)
-		die("update %s has extra input: %s", update->ref_name, next);
+		die("update %s: extra input: %s", update->ref_name, next);
 
 	return next;
 }
@@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 
 	update->ref_name = parse_refname(input, &next);
 	if (!update->ref_name)
-		die("create line missing <ref>");
+		die("create: missing <ref>");
 
 	if (parse_next_sha1(input, &next, update->new_sha1,
 			    "create", update->ref_name, 0))
-		die("create %s missing <newvalue>", update->ref_name);
+		die("create %s: missing <newvalue>", update->ref_name);
 
 	if (is_null_sha1(update->new_sha1))
-		die("create %s given zero <newvalue>", update->ref_name);
+		die("create %s: zero <newvalue>", update->ref_name);
 
 	if (*next != line_termination)
-		die("create %s has extra input: %s", update->ref_name, next);
+		die("create %s: extra input: %s", update->ref_name, next);
 
 	return next;
 }
@@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 
 	update->ref_name = parse_refname(input, &next);
 	if (!update->ref_name)
-		die("delete line missing <ref>");
+		die("delete: missing <ref>");
 
 	if (parse_next_sha1(input, &next, update->old_sha1,
 			    "delete", update->ref_name, PARSE_SHA1_OLD)) {
 		update->have_old = 0;
 	} else {
 		if (is_null_sha1(update->old_sha1))
-			die("delete %s given zero <oldvalue>", update->ref_name);
+			die("delete %s: zero <oldvalue>", update->ref_name);
 		update->have_old = 1;
 	}
 
 	if (*next != line_termination)
-		die("delete %s has extra input: %s", update->ref_name, next);
+		die("delete %s: extra input: %s", update->ref_name, next);
 
 	return next;
 }
@@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 
 	update->ref_name = parse_refname(input, &next);
 	if (!update->ref_name)
-		die("verify line missing <ref>");
+		die("verify: missing <ref>");
 
 	if (parse_next_sha1(input, &next, update->old_sha1,
 			    "verify", update->ref_name, PARSE_SHA1_OLD)) {
@@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 	}
 
 	if (*next != line_termination)
-		die("verify %s has extra input: %s", update->ref_name, next);
+		die("verify %s: extra input: %s", update->ref_name, next);
 
 	return next;
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1db0689..48ccc4d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted argument' '
 test_expect_success 'stdin fails create with no ref' '
 	echo "create " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: create line missing <ref>" err
+	grep "fatal: create: missing <ref>" err
 '
 
 test_expect_success 'stdin fails create with bad ref name' '
@@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref name' '
 test_expect_success 'stdin fails create with no new value' '
 	echo "create $a" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: create $a missing <newvalue>" err
+	grep "fatal: create $a: missing <newvalue>" err
 '
 
 test_expect_success 'stdin fails create with too many arguments' '
 	echo "create $a $m $m" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: create $a has extra input:  $m" err
+	grep "fatal: create $a: extra input:  $m" err
 '
 
 test_expect_success 'stdin fails update with no ref' '
 	echo "update " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: update line missing <ref>" err
+	grep "fatal: update: missing <ref>" err
 '
 
 test_expect_success 'stdin fails update with bad ref name' '
@@ -407,19 +407,19 @@ test_expect_success 'stdin fails update with bad ref name' '
 test_expect_success 'stdin fails update with no new value' '
 	echo "update $a" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: update $a missing <newvalue>" err
+	grep "fatal: update $a: missing <newvalue>" err
 '
 
 test_expect_success 'stdin fails update with too many arguments' '
 	echo "update $a $m $m $m" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: update $a has extra input:  $m" err
+	grep "fatal: update $a: extra input:  $m" err
 '
 
 test_expect_success 'stdin fails delete with no ref' '
 	echo "delete " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: delete line missing <ref>" err
+	grep "fatal: delete: missing <ref>" err
 '
 
 test_expect_success 'stdin fails delete with bad ref name' '
@@ -431,13 +431,13 @@ test_expect_success 'stdin fails delete with bad ref name' '
 test_expect_success 'stdin fails delete with too many arguments' '
 	echo "delete $a $m $m" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: delete $a has extra input:  $m" err
+	grep "fatal: delete $a: extra input:  $m" err
 '
 
 test_expect_success 'stdin fails verify with too many arguments' '
 	echo "verify $a $m $m" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: verify $a has extra input:  $m" err
+	grep "fatal: verify $a: extra input:  $m" err
 '
 
 test_expect_success 'stdin fails option with unknown name' '
@@ -532,7 +532,7 @@ test_expect_success 'stdin create ref fails with bad new value' '
 test_expect_success 'stdin create ref fails with zero new value' '
 	echo "create $c " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: create $c given zero <newvalue>" err &&
+	grep "fatal: create $c: zero <newvalue>" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' '
 test_expect_success 'stdin delete ref fails with zero old value' '
 	echo "delete $a " >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: delete $a given zero <oldvalue>" err &&
+	grep "fatal: delete $a: zero <oldvalue>" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -697,7 +697,7 @@ test_expect_success 'stdin -z fails on unknown command' '
 test_expect_success 'stdin -z fails create with no ref' '
 	printf $F "create " >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: create line missing <ref>" err
+	grep "fatal: create: missing <ref>" err
 '
 
 test_expect_success 'stdin -z fails create with bad ref name' '
@@ -721,7 +721,7 @@ test_expect_success 'stdin -z fails create with too many arguments' '
 test_expect_success 'stdin -z fails update with no ref' '
 	printf $F "update " >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: update line missing <ref>" err
+	grep "fatal: update: missing <ref>" err
 '
 
 test_expect_success 'stdin -z fails update with too few args' '
@@ -765,7 +765,7 @@ test_expect_success 'stdin -z fails update with too many arguments' '
 test_expect_success 'stdin -z fails delete with no ref' '
 	printf $F "delete " >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: delete line missing <ref>" err
+	grep "fatal: delete: missing <ref>" err
 '
 
 test_expect_success 'stdin -z fails delete with bad ref name' '
@@ -868,7 +868,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' '
 test_expect_success 'stdin -z create ref fails with empty new value' '
 	printf $F "create $c" "" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: create $c missing <newvalue>" err &&
+	grep "fatal: create $c: missing <newvalue>" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -892,7 +892,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' '
 test_expect_success 'stdin -z delete ref fails with zero old value' '
 	printf $F "delete $a" "$Z" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: delete $a given zero <oldvalue>" err &&
+	grep "fatal: delete $a: zero <oldvalue>" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
-- 
1.9.0

[PATCH v2 19/27] refs: Add a concept of a reference transaction

[Michael Haggerty]

Build out the API for dealing with a bunch of reference checks and
changes within a transaction.  Define an opaque ref_transaction type
that is managed entirely within refs.c.  Introduce functions for
beginning a transaction, adding updates to a transaction, and
committing/rolling back a transaction.

This API will soon replace update_refs().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+)

diff --git a/refs.c b/refs.c
index 1305eb1..e788c27 100644
--- a/refs.c
+++ b/refs.c
@@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const char *refname,
 	return 0;
 }
 
+/*
+ * Data structure for holding a reference transaction, which can
+ * consist of checks and updates to multiple references, carried out
+ * as atomically as possible.  This structure is opaque to callers.
+ */
+struct ref_transaction {
+	struct ref_update **updates;
+	size_t alloc;
+	size_t nr;
+};
+
+struct ref_transaction *ref_transaction_begin(void)
+{
+	return xcalloc(1, sizeof(struct ref_transaction));
+}
+
+static void ref_transaction_free(struct ref_transaction *transaction)
+{
+	int i;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = transaction->updates[i];
+
+		free((char *)update->ref_name);
+		free(update);
+	}
+
+	free(transaction->updates);
+	free(transaction);
+}
+
+void ref_transaction_rollback(struct ref_transaction *transaction)
+{
+	ref_transaction_free(transaction);
+}
+
+static struct ref_update *add_update(struct ref_transaction *transaction,
+				     const char *refname)
+{
+	struct ref_update *update = xcalloc(1, sizeof(*update));
+
+	update->ref_name = xstrdup(refname);
+	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
+	transaction->updates[transaction->nr++] = update;
+	return update;
+}
+
+void ref_transaction_update(struct ref_transaction *transaction,
+			    const char *refname,
+			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    int flags, int have_old)
+{
+	struct ref_update *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);
+}
+
+void ref_transaction_create(struct ref_transaction *transaction,
+			    const char *refname,
+			    unsigned char *new_sha1,
+			    int flags)
+{
+	struct ref_update *update = add_update(transaction, refname);
+
+	hashcpy(update->new_sha1, new_sha1);
+	hashclr(update->old_sha1);
+	update->flags = flags;
+	update->have_old = 1;
+}
+
+void ref_transaction_delete(struct ref_transaction *transaction,
+			    const char *refname,
+			    unsigned char *old_sha1,
+			    int flags, int have_old)
+{
+	struct ref_update *update = add_update(transaction, refname);
+
+	update->flags = flags;
+	update->have_old = have_old;
+	if (have_old)
+		hashcpy(update->old_sha1, old_sha1);
+}
+
 int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
 	       int flags, enum action_on_err onerr)
@@ -3378,6 +3465,15 @@ cleanup:
 	return ret;
 }
 
+int ref_transaction_commit(struct ref_transaction *transaction,
+			   const char *msg, enum action_on_err onerr)
+{
+	int ret = update_refs(msg, transaction->updates, transaction->nr,
+			      onerr);
+	ref_transaction_free(transaction);
+	return ret;
+}
+
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
 	int i;
diff --git a/refs.h b/refs.h
index 08e60ac..476a923 100644
--- a/refs.h
+++ b/refs.h
@@ -24,6 +24,8 @@ struct ref_update {
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 };
 
+struct ref_transaction;
+
 /*
  * Bit values set in the flags argument passed to each_ref_fn():
  */
@@ -220,6 +222,69 @@ enum action_on_err {
 	UPDATE_REFS_QUIET_ON_ERR
 };
 
+/*
+ * Begin a reference transaction.  The reference transaction must
+ * eventually be commited using ref_transaction_commit() or rolled
+ * back using ref_transaction_rollback().
+ */
+struct ref_transaction *ref_transaction_begin(void);
+
+/*
+ * Roll back a ref_transaction and free all associated data.
+ */
+void ref_transaction_rollback(struct ref_transaction *transaction);
+
+
+/*
+ * 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, so the caller retains ownership of the parameter.  flags
+ * can be REF_NODEREF; it is passed to update_ref_lock().
+ */
+
+
+/*
+ * Add a reference update to transaction.  new_sha1 is the value that
+ * the reference should have after the update, or zeros 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
+ * it must not have existed beforehand.
+ */
+void ref_transaction_update(struct ref_transaction *transaction,
+			    const char *refname,
+			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    int flags, int have_old);
+
+/*
+ * Add a reference creation to transaction.  new_sha1 is the value
+ * that the reference should have after the update, or zeros if it
+ * should be deleted.  It is verified that the reference does not
+ * exist already.
+ */
+void ref_transaction_create(struct ref_transaction *transaction,
+			    const char *refname,
+			    unsigned char *new_sha1,
+			    int flags);
+
+/*
+ * Add a reference deletion to transaction.  If have_old is true, then
+ * old_sha1 holds the value that the reference should have had before
+ * the update.
+ */
+void ref_transaction_delete(struct ref_transaction *transaction,
+			    const char *refname,
+			    unsigned char *old_sha1,
+			    int flags, int have_old);
+
+/*
+ * Commit all of the changes that have been queued in transaction, as
+ * atomically as possible.  Return a nonzero value if there is a
+ * problem.  The ref_transaction is freed by this function.
+ */
+int ref_transaction_commit(struct ref_transaction *transaction,
+			   const char *msg, enum action_on_err onerr);
+
 /** 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,
-- 
1.9.0

[PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

[Michael Haggerty]

This change is mostly clerical: the parse_cmd_*() functions need to
use local variables rather than a struct ref_update to collect the
arguments needed for each update, and then call ref_transaction_*() to
queue the change rather than building up the list of changes at the
caller side.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c | 142 +++++++++++++++++++++++++++------------------------
 1 file changed, 75 insertions(+), 67 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index bbc04b2..2c8678b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
 	NULL
 };
 
-static int updates_alloc;
-static int updates_count;
-static struct ref_update **updates;
+static struct ref_transaction *transaction;
 
 static char line_termination = '\n';
 static int update_flags;
 
-static struct ref_update *update_alloc(void)
-{
-	struct ref_update *update;
-
-	/* Allocate and zero-init a struct ref_update */
-	update = xcalloc(1, sizeof(*update));
-	ALLOC_GROW(updates, updates_count + 1, updates_alloc);
-	updates[updates_count++] = update;
-
-	/* Store and reset accumulated options */
-	update->flags = update_flags;
-	update_flags = 0;
-
-	return update;
-}
-
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
  * and append the result to arg.  Return a pointer to the terminator.
@@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
 
 static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
-	struct ref_update *update;
-
-	update = update_alloc();
+	char *refname;
+	unsigned char new_sha1[20];
+	unsigned char old_sha1[20];
+	int have_old;
 
-	update->ref_name = parse_refname(input, &next);
-	if (!update->ref_name)
+	refname = parse_refname(input, &next);
+	if (!refname)
 		die("update: missing <ref>");
 
-	if (parse_next_sha1(input, &next, update->new_sha1,
-			    "update", update->ref_name,
+	if (parse_next_sha1(input, &next, new_sha1, "update", refname,
 			    PARSE_SHA1_ALLOW_EMPTY))
-		die("update %s: missing <newvalue>", update->ref_name);
+		die("update %s: missing <newvalue>", refname);
 
-	update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
-					    "update", update->ref_name,
-					    PARSE_SHA1_OLD);
+	have_old = !parse_next_sha1(input, &next, old_sha1, "update", refname,
+				    PARSE_SHA1_OLD);
 
 	if (*next != line_termination)
-		die("update %s: extra input: %s", update->ref_name, next);
+		die("update %s: extra input: %s", refname, next);
+
+	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+			       update_flags, have_old);
+
+	update_flags = 0;
+	free(refname);
 
 	return next;
 }
 
 static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
-	struct ref_update *update;
-
-	update = update_alloc();
+	char *refname;
+	unsigned char new_sha1[20];
 
-	update->ref_name = parse_refname(input, &next);
-	if (!update->ref_name)
+	refname = parse_refname(input, &next);
+	if (!refname)
 		die("create: missing <ref>");
 
-	if (parse_next_sha1(input, &next, update->new_sha1,
-			    "create", update->ref_name, 0))
-		die("create %s: missing <newvalue>", update->ref_name);
+	if (parse_next_sha1(input, &next, new_sha1, "create", refname, 0))
+		die("create %s: missing <newvalue>", refname);
 
-	if (is_null_sha1(update->new_sha1))
-		die("create %s: zero <newvalue>", update->ref_name);
+	if (is_null_sha1(new_sha1))
+		die("create %s: zero <newvalue>", refname);
 
 	if (*next != line_termination)
-		die("create %s: extra input: %s", update->ref_name, next);
+		die("create %s: extra input: %s", refname, next);
+
+	ref_transaction_create(transaction, refname, new_sha1, update_flags);
+
+	update_flags = 0;
+	free(refname);
 
 	return next;
 }
 
 static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 {
-	struct ref_update *update;
+	char *refname;
+	unsigned char old_sha1[20];
+	int have_old;
 
-	update = update_alloc();
-
-	update->ref_name = parse_refname(input, &next);
-	if (!update->ref_name)
+	refname = parse_refname(input, &next);
+	if (!refname)
 		die("delete: missing <ref>");
 
-	if (parse_next_sha1(input, &next, update->old_sha1,
-			    "delete", update->ref_name, PARSE_SHA1_OLD)) {
-		update->have_old = 0;
+	if (parse_next_sha1(input, &next, old_sha1, "delete", refname,
+			    PARSE_SHA1_OLD)) {
+		have_old = 0;
 	} else {
-		if (is_null_sha1(update->old_sha1))
-			die("delete %s: zero <oldvalue>", update->ref_name);
-		update->have_old = 1;
+		if (is_null_sha1(old_sha1))
+			die("delete %s: zero <oldvalue>", refname);
+		have_old = 1;
 	}
 
 	if (*next != line_termination)
-		die("delete %s: extra input: %s", update->ref_name, next);
+		die("delete %s: extra input: %s", refname, next);
+
+	ref_transaction_delete(transaction, refname, old_sha1,
+			       update_flags, have_old);
+
+	update_flags = 0;
+	free(refname);
 
 	return next;
 }
 
 static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 {
-	struct ref_update *update;
-
-	update = update_alloc();
+	char *refname;
+	unsigned char new_sha1[20];
+	unsigned char old_sha1[20];
+	int have_old;
 
-	update->ref_name = parse_refname(input, &next);
-	if (!update->ref_name)
+	refname = parse_refname(input, &next);
+	if (!refname)
 		die("verify: missing <ref>");
 
-	if (parse_next_sha1(input, &next, update->old_sha1,
-			    "verify", update->ref_name, PARSE_SHA1_OLD)) {
-		update->have_old = 0;
+	if (parse_next_sha1(input, &next, old_sha1, "verify", refname,
+			    PARSE_SHA1_OLD)) {
+		hashclr(new_sha1);
+		have_old = 0;
 	} else {
-		hashcpy(update->new_sha1, update->old_sha1);
-		update->have_old = 1;
+		hashcpy(new_sha1, old_sha1);
+		have_old = 1;
 	}
 
 	if (*next != line_termination)
-		die("verify %s: extra input: %s", update->ref_name, next);
+		die("verify %s: extra input: %s", refname, next);
+
+	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+			       update_flags, have_old);
+
+	update_flags = 0;
+	free(refname);
 
 	return next;
 }
@@ -355,13 +359,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("Refusing to perform update with empty message.");
 
 	if (read_stdin) {
+		int ret;
+		transaction = ref_transaction_begin();
+
 		if (delete || no_deref || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		return update_refs(msg, updates, updates_count,
-				   UPDATE_REFS_DIE_ON_ERR);
+		ret = ref_transaction_commit(transaction, msg,
+					     UPDATE_REFS_DIE_ON_ERR);
+		return ret;
 	}
 
 	if (end_null)
-- 
1.9.0

[PATCH v2 21/27] refs: Remove API function update_refs()

[Michael Haggerty]

It has been superseded by reference transactions.  This also means
that struct ref_update can become private.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 33 ++++++++++++++++++++-------------
 refs.h | 20 --------------------
 2 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/refs.c b/refs.c
index e788c27..dfff117 100644
--- a/refs.c
+++ b/refs.c
@@ -3267,6 +3267,20 @@ static int update_ref_write(const char *action, const char *refname,
 	return 0;
 }
 
+/**
+ * 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.
+ */
+struct ref_update {
+	const char *ref_name;
+	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 */
+};
+
 /*
  * Data structure for holding a reference transaction, which can
  * consist of checks and updates to multiple references, carried out
@@ -3393,16 +3407,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 	return 0;
 }
 
-int update_refs(const char *action, struct ref_update * const *updates_orig,
-		int n, enum action_on_err onerr)
+int ref_transaction_commit(struct ref_transaction *transaction,
+			   const char *msg, enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
 	struct ref_update **updates;
 	int *types;
 	struct ref_lock **locks;
 	const char **delnames;
+	int n = transaction->nr;
 
-	if (!updates_orig || !n)
+	if (!n)
 		return 0;
 
 	/* Allocate work space */
@@ -3412,7 +3427,7 @@ int update_refs(const char *action, struct ref_update * const *updates_orig,
 	delnames = xmalloc(sizeof(*delnames) * n);
 
 	/* Copy, sort, and reject duplicate refs */
-	memcpy(updates, updates_orig, sizeof(*updates) * n);
+	memcpy(updates, transaction->updates, sizeof(*updates) * n);
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	ret = ref_update_reject_duplicates(updates, n, onerr);
 	if (ret)
@@ -3434,7 +3449,7 @@ int update_refs(const char *action, struct ref_update * const *updates_orig,
 	/* Perform updates first so live commits remain referenced */
 	for (i = 0; i < n; i++)
 		if (!is_null_sha1(updates[i]->new_sha1)) {
-			ret = update_ref_write(action,
+			ret = update_ref_write(msg,
 					       updates[i]->ref_name,
 					       updates[i]->new_sha1,
 					       locks[i], onerr);
@@ -3462,14 +3477,6 @@ cleanup:
 	free(types);
 	free(locks);
 	free(delnames);
-	return ret;
-}
-
-int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, enum action_on_err onerr)
-{
-	int ret = update_refs(msg, transaction->updates, transaction->nr,
-			      onerr);
 	ref_transaction_free(transaction);
 	return ret;
 }
diff --git a/refs.h b/refs.h
index 476a923..99c194b 100644
--- a/refs.h
+++ b/refs.h
@@ -10,20 +10,6 @@ struct ref_lock {
 	int force_write;
 };
 
-/**
- * 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.
- */
-struct ref_update {
-	const char *ref_name;
-	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 */
-};
-
 struct ref_transaction;
 
 /*
@@ -290,12 +276,6 @@ 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 all refs and then perform all modifications.
- */
-int update_refs(const char *action, struct ref_update * const *updates,
-		int n, 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 *);
 
-- 
1.9.0

[PATCH v2 22/27] struct ref_update: Rename field "ref_name" to "refname"

[Michael Haggerty]

This is consistent with the usual nomenclature.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 18 +++++++++---------
 refs.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index dfff117..d72d0ab 100644
--- a/refs.c
+++ b/refs.c
@@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const char *refname,
  * value or to zero to ensure the ref does not exist before update.
  */
 struct ref_update {
-	const char *ref_name;
+	const char *refname;
 	unsigned char new_sha1[20];
 	unsigned char old_sha1[20];
 	int flags; /* REF_NODEREF? */
@@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction *transaction)
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 
-		free((char *)update->ref_name);
+		free((char *)update->refname);
 		free(update);
 	}
 
@@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 {
 	struct ref_update *update = xcalloc(1, sizeof(*update));
 
-	update->ref_name = xstrdup(refname);
+	update->refname = xstrdup(refname);
 	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
 	transaction->updates[transaction->nr++] = update;
 	return update;
@@ -3383,7 +3383,7 @@ static int ref_update_compare(const void *r1, const void *r2)
 {
 	const struct ref_update * const *u1 = r1;
 	const struct ref_update * const *u2 = r2;
-	return strcmp((*u1)->ref_name, (*u2)->ref_name);
+	return strcmp((*u1)->refname, (*u2)->refname);
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
@@ -3391,14 +3391,14 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 {
 	int i;
 	for (i = 1; i < n; i++)
-		if (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) {
+		if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
 			const char *str =
 				"Multiple updates for ref '%s' not allowed.";
 			switch (onerr) {
 			case UPDATE_REFS_MSG_ON_ERR:
-				error(str, updates[i]->ref_name); break;
+				error(str, updates[i]->refname); break;
 			case UPDATE_REFS_DIE_ON_ERR:
-				die(str, updates[i]->ref_name); break;
+				die(str, updates[i]->refname); break;
 			case UPDATE_REFS_QUIET_ON_ERR:
 				break;
 			}
@@ -3435,7 +3435,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/* Acquire all locks while verifying old values */
 	for (i = 0; i < n; i++) {
-		locks[i] = update_ref_lock(updates[i]->ref_name,
+		locks[i] = update_ref_lock(updates[i]->refname,
 					   (updates[i]->have_old ?
 					    updates[i]->old_sha1 : NULL),
 					   updates[i]->flags,
@@ -3450,7 +3450,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++)
 		if (!is_null_sha1(updates[i]->new_sha1)) {
 			ret = update_ref_write(msg,
-					       updates[i]->ref_name,
+					       updates[i]->refname,
 					       updates[i]->new_sha1,
 					       locks[i], onerr);
 			locks[i] = NULL; /* freed by update_ref_write */
diff --git a/refs.h b/refs.h
index 99c194b..30ee721 100644
--- a/refs.h
+++ b/refs.h
@@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *lock);
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
 /** Setup reflog before using. **/
-int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
+int log_ref_setup(const char *refname, char *logfile, int bufsize);
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
-- 
1.9.0

[PATCH v2 23/27] struct ref_update: Store refname as a FLEX_ARRAY.

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index d72d0ab..2b80f6d 100644
--- a/refs.c
+++ b/refs.c
@@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const char *refname,
  * value or to zero to ensure the ref does not exist before update.
  */
 struct ref_update {
-	const char *refname;
 	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 */
+	const char refname[FLEX_ARRAY];
 };
 
 /*
@@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction *transaction)
 {
 	int i;
 
-	for (i = 0; i < transaction->nr; i++) {
-		struct ref_update *update = transaction->updates[i];
-
-		free((char *)update->refname);
-		free(update);
-	}
+	for (i = 0; i < transaction->nr; i++)
+		free(transaction->updates[i]);
 
 	free(transaction->updates);
 	free(transaction);
@@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction *transaction)
 static struct ref_update *add_update(struct ref_transaction *transaction,
 				     const char *refname)
 {
-	struct ref_update *update = xcalloc(1, sizeof(*update));
+	size_t len = strlen(refname);
+	struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
 
-	update->refname = xstrdup(refname);
+	strcpy((char *)update->refname, refname);
 	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
 	transaction->updates[transaction->nr++] = update;
 	return update;
-- 
1.9.0

[PATCH v2 24/27] ref_transaction_commit(): Introduce temporary variables

[Michael Haggerty]

Use temporary variables in the for-loop blocks to simplify expressions
in the rest of the loop.

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

diff --git a/refs.c b/refs.c
index 2b80f6d..d51566c 100644
--- a/refs.c
+++ b/refs.c
@@ -3432,10 +3432,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/* Acquire all locks while verifying old values */
 	for (i = 0; i < n; i++) {
-		locks[i] = update_ref_lock(updates[i]->refname,
-					   (updates[i]->have_old ?
-					    updates[i]->old_sha1 : NULL),
-					   updates[i]->flags,
+		struct ref_update *update = updates[i];
+
+		locks[i] = update_ref_lock(update->refname,
+					   (update->have_old ?
+					    update->old_sha1 : NULL),
+					   update->flags,
 					   &types[i], onerr);
 		if (!locks[i]) {
 			ret = 1;
@@ -3444,16 +3446,19 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	}
 
 	/* Perform updates first so live commits remain referenced */
-	for (i = 0; i < n; i++)
-		if (!is_null_sha1(updates[i]->new_sha1)) {
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (!is_null_sha1(update->new_sha1)) {
 			ret = update_ref_write(msg,
-					       updates[i]->refname,
-					       updates[i]->new_sha1,
+					       update->refname,
+					       update->new_sha1,
 					       locks[i], onerr);
 			locks[i] = NULL; /* freed by update_ref_write */
 			if (ret)
 				goto cleanup;
 		}
+	}
 
 	/* Perform deletes now that updates are safely completed */
 	for (i = 0; i < n; i++)
-- 
1.9.0

[PATCH v2 25/27] struct ref_update: Add a lock member

[Michael Haggerty]

Now that we manage ref_update objects internally, we can use them to
hold some of the scratch space we need when actually carrying out the
updates.  Store the (struct ref_lock *) there.

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

diff --git a/refs.c b/refs.c
index d51566c..d1edd57 100644
--- a/refs.c
+++ b/refs.c
@@ -3278,6 +3278,7 @@ struct ref_update {
 	unsigned char old_sha1[20];
 	int flags; /* REF_NODEREF? */
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+	struct ref_lock *lock;
 	const char refname[FLEX_ARRAY];
 };
 
@@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int ret = 0, delnum = 0, i;
 	struct ref_update **updates;
 	int *types;
-	struct ref_lock **locks;
 	const char **delnames;
 	int n = transaction->nr;
 
@@ -3420,7 +3420,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	/* Allocate work space */
 	updates = xmalloc(sizeof(*updates) * n);
 	types = xmalloc(sizeof(*types) * n);
-	locks = xcalloc(n, sizeof(*locks));
 	delnames = xmalloc(sizeof(*delnames) * n);
 
 	/* Copy, sort, and reject duplicate refs */
@@ -3434,12 +3433,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		locks[i] = update_ref_lock(update->refname,
-					   (update->have_old ?
-					    update->old_sha1 : NULL),
-					   update->flags,
-					   &types[i], onerr);
-		if (!locks[i]) {
+		update->lock = update_ref_lock(update->refname,
+					       (update->have_old ?
+						update->old_sha1 : NULL),
+					       update->flags,
+					       &types[i], onerr);
+		if (!update->lock) {
 			ret = 1;
 			goto cleanup;
 		}
@@ -3453,19 +3452,23 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
-					       locks[i], onerr);
-			locks[i] = NULL; /* freed by update_ref_write */
+					       update->lock, onerr);
+			update->lock = NULL; /* freed by update_ref_write */
 			if (ret)
 				goto cleanup;
 		}
 	}
 
 	/* Perform deletes now that updates are safely completed */
-	for (i = 0; i < n; i++)
-		if (locks[i]) {
-			delnames[delnum++] = locks[i]->ref_name;
-			ret |= delete_ref_loose(locks[i], types[i]);
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->lock) {
+			delnames[delnum++] = update->lock->ref_name;
+			ret |= delete_ref_loose(update->lock, types[i]);
 		}
+	}
+
 	ret |= repack_without_refs(delnames, delnum);
 	for (i = 0; i < delnum; i++)
 		unlink_or_warn(git_path("logs/%s", delnames[i]));
@@ -3473,11 +3476,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 cleanup:
 	for (i = 0; i < n; i++)
-		if (locks[i])
-			unlock_ref(locks[i]);
+		if (updates[i]->lock)
+			unlock_ref(updates[i]->lock);
 	free(updates);
 	free(types);
-	free(locks);
 	free(delnames);
 	ref_transaction_free(transaction);
 	return ret;
-- 
1.9.0

[PATCH v2 26/27] struct ref_update: Add type field

[Michael Haggerty]

This is temporary space for ref_transaction_commit().

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

diff --git a/refs.c b/refs.c
index d1edd57..07f900a 100644
--- a/refs.c
+++ b/refs.c
@@ -3279,6 +3279,7 @@ struct ref_update {
 	int flags; /* REF_NODEREF? */
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 	struct ref_lock *lock;
+	int type;
 	const char refname[FLEX_ARRAY];
 };
 
@@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 {
 	int ret = 0, delnum = 0, i;
 	struct ref_update **updates;
-	int *types;
 	const char **delnames;
 	int n = transaction->nr;
 
@@ -3419,7 +3419,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/* Allocate work space */
 	updates = xmalloc(sizeof(*updates) * n);
-	types = xmalloc(sizeof(*types) * n);
 	delnames = xmalloc(sizeof(*delnames) * n);
 
 	/* Copy, sort, and reject duplicate refs */
@@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       (update->have_old ?
 						update->old_sha1 : NULL),
 					       update->flags,
-					       &types[i], onerr);
+					       &update->type, onerr);
 		if (!update->lock) {
 			ret = 1;
 			goto cleanup;
@@ -3465,7 +3464,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (update->lock) {
 			delnames[delnum++] = update->lock->ref_name;
-			ret |= delete_ref_loose(update->lock, types[i]);
+			ret |= delete_ref_loose(update->lock, update->type);
 		}
 	}
 
@@ -3479,7 +3478,6 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	free(updates);
-	free(types);
 	free(delnames);
 	ref_transaction_free(transaction);
 	return ret;
-- 
1.9.0

[PATCH v2 27/27] ref_transaction_commit(): Work with transaction->updates in place

[Michael Haggerty]

Now that we free the transaction when we are done, there is no need to
make a copy of transaction->updates before working with it.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 07f900a..aaf75f6 100644
--- a/refs.c
+++ b/refs.c
@@ -3410,19 +3410,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			   const char *msg, enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
-	struct ref_update **updates;
 	const char **delnames;
 	int n = transaction->nr;
+	struct ref_update **updates = transaction->updates;
 
 	if (!n)
 		return 0;
 
 	/* Allocate work space */
-	updates = xmalloc(sizeof(*updates) * n);
 	delnames = xmalloc(sizeof(*delnames) * n);
 
 	/* Copy, sort, and reject duplicate refs */
-	memcpy(updates, transaction->updates, sizeof(*updates) * n);
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	ret = ref_update_reject_duplicates(updates, n, onerr);
 	if (ret)
@@ -3477,7 +3475,6 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(updates);
 	free(delnames);
 	ref_transaction_free(transaction);
 	return ret;
-- 
1.9.0

Re: [PATCH v2 14/27] update-ref.c: Extract a new function, parse_next_sha1()

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
> +/*
> + * For backwards compatibility, accept an empty string for create's
> + * <newvalue> in binary mode to be equivalent to specifying zeros.
> + */
> +#define PARSE_SHA1_ALLOW_EMPTY 0x02

The comment should say "update's", not "create's".

-Brad

Re: [PATCH v2 16/27] t1400: Test one mistake at a time

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> t1400: Add a test of "update" with too few arguments
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

This looks like a stray squash message.

-Brad

Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
> +void ref_transaction_update(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *new_sha1, unsigned char *old_sha1,
> +			    int flags, int have_old);
[snip]
> +void ref_transaction_create(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *new_sha1,
> +			    int flags);
[snip]
> +void ref_transaction_delete(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *old_sha1,
> +			    int flags, int have_old);

Perhaps we also need:

void ref_transaction_verify(struct ref_transaction *transaction,
			    const char *refname,
			    unsigned char *old_sha1,
			    int flags, int have_old);

as equivalent to the "verify" command in "update-ref --stdin"?

-Brad

Re: [PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction

[Brad King]

On 03/24/2014 01:56 PM, Michael Haggerty wrote:
> Changes relative to v1:
> 
> * Rename the functions associated with ref_transactions to be more
>   reminiscent of database transactions:
> 
>   * create_ref_transaction() -> ref_transaction_begin()
>   * free_ref_transaction() -> ref_transaction_rollback()
>   * queue_update_ref() -> ref_transaction_update()
>   * queue_create_ref() -> ref_transaction_create()
>   * queue_delete_ref() -> ref_transaction_delete()
>   * commit_ref_transaction() -> ref_transaction_commit()

Those new names look better.

> * Fix backwards compatibility of "git update-ref --stdin -z"'s
>   handling of the "create" command: allow <newvalue> to be the empty
>   string, treating it the same zeros.  But deprecate this usage.

The changes related to that look good.  The new documentation is
much clearer than my old wording.

Series v2 looks good to me except for my responses to individual
commits.

Thanks,
-Brad

Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction

[Michael Haggerty]

On 03/26/2014 07:39 PM, Brad King wrote:
> On 03/24/2014 01:56 PM, Michael Haggerty wrote:
>> +void ref_transaction_update(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *new_sha1, unsigned char *old_sha1,
>> +			    int flags, int have_old);
> [snip]
>> +void ref_transaction_create(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *new_sha1,
>> +			    int flags);
> [snip]
>> +void ref_transaction_delete(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *old_sha1,
>> +			    int flags, int have_old);
> 
> Perhaps we also need:
> 
> void ref_transaction_verify(struct ref_transaction *transaction,
> 			    const char *refname,
> 			    unsigned char *old_sha1,
> 			    int flags, int have_old);
> 
> as equivalent to the "verify" command in "update-ref --stdin"?

Yes.  That's already on my todo list for a future batch of patches.  But
first I was going to beef up the ref_update structure to handle verify
actions directly rather than as updates with oldvalue==newvalue,
probably by turning has_old into a flag with HAS_OLD and HAS_NEW bits.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction

[Michael Haggerty]

On 03/26/2014 07:39 PM, Brad King wrote:
> On 03/24/2014 01:56 PM, Michael Haggerty wrote:
>> Changes relative to v1:
>>
>> * Rename the functions associated with ref_transactions to be more
>>   reminiscent of database transactions:
>>
>>   * create_ref_transaction() -> ref_transaction_begin()
>>   * free_ref_transaction() -> ref_transaction_rollback()
>>   * queue_update_ref() -> ref_transaction_update()
>>   * queue_create_ref() -> ref_transaction_create()
>>   * queue_delete_ref() -> ref_transaction_delete()
>>   * commit_ref_transaction() -> ref_transaction_commit()
> 
> Those new names look better.
> 
>> * Fix backwards compatibility of "git update-ref --stdin -z"'s
>>   handling of the "create" command: allow <newvalue> to be the empty
>>   string, treating it the same zeros.  But deprecate this usage.
> 
> The changes related to that look good.  The new documentation is
> much clearer than my old wording.
> 
> Series v2 looks good to me except for my responses to individual
> commits.

Thanks a lot for the review.  Your other two comments are correct, of
course, and I will fix them if there needs to be a re-roll.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 02/27] t1400: Provide more usual input to the command

[Junio C Hamano]

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

> Subject: Re: [PATCH v2 02/27] t1400: Provide more usual input to the command

This applies to the patches throughout the series, but during the
microproject reviews, Eric pointed out that we seem to start the
summary after area: on the subject with lowercase and omit the full
stop at the end, so I'll try to tweak the subjects while queueing to
read patches in the series.

> The old version was passing (among other things)
>
>     update SP refs/heads/c NUL NUL 0{40} NUL
>
> to "git update-ref -z --stdin" to test whether the old-value check for
> c is working.  But the <newvalue> is empty, which is a bit off the
> beaten track.
>
> So, to be sure that we are testing what we want to test, provide an
> actual <newvalue> on the "update" line.

Interesting.  So the test used to expect failure, but we couldn't
tell if that was due to giving a "Please update to this value" which
is malformed, or "I am giving 0{40} as the old value, telling you
that you have to make sure the ref does not exist" which does not
hold because we already have that ref?

That would mean that the test may not have been testing the right
thing.  A good change.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t1400-update-ref.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index fa927d2..29391c6 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with identity updates' '
>  
>  test_expect_success 'stdin -z update refs fails with wrong old value' '
>  	git update-ref $c $m &&
> -	printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "" "$Z" >stdin &&
> +	printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m" "$Z" >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
>  	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
>  	git rev-parse $m >expect &&

Re: [PATCH v2 01/27] t1400: Fix name and expected result of one test

[Junio C Hamano]

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

> The test
>
>     stdin -z create ref fails with zero new value
>
> actually passes an empty new value, not a zero new value.  So rename
> the test s/zero/empty/, and change the expected error from
>
>     fatal: create $c given zero new value
>
> to
>
>     fatal: create $c missing <newvalue>

I have a feeling that "zero new value" might have been done by a
non-native (like me) to say "no new value"; "missing newvalue"
sounds like a good phrasing to use.

> Of course, this makes the test fail now, so mark it
> test_expect_failure.  The failure will be fixed later in this patch
> series.

That sounds somewhat strange.  Why not just give a single-liner to
update-ref.c instead?

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t1400-update-ref.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 6ffd82f..fa927d2 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' '
>  	test_must_fail git rev-parse --verify -q $c
>  '
>  
> -test_expect_success 'stdin -z create ref fails with zero new value' '
> +test_expect_failure 'stdin -z create ref fails with empty new value' '
>  	printf $F "create $c" "" >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> -	grep "fatal: create $c given zero new value" err &&
> +	grep "fatal: create $c missing <newvalue>" err &&
>  	test_must_fail git rev-parse --verify -q $c
>  '

Re: [PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated

[Junio C Hamano]

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

> Test that the argument is properly terminated by either whitespace or
> a NUL character, even if it is quoted, to be consistent with the
> non-quoted case.  Adjust the tests to expect the new error message.
> Add a docstring to the function, incorporating the comments that were
> formerly within the function plus some added information.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/update-ref.c  | 20 +++++++++++++++-----
>  t/t1400-update-ref.sh |  4 ++--
>  2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 1292cfe..02b5f95 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
>  	update->have_old = *oldvalue || line_termination;
>  }
>  
> +/*
> + * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
> + * and append the result to arg.  Return a pointer to the terminator.
> + * Die if there is an error in how the argument is C-quoted.  This
> + * function is only used if not -z.
> + */
>  static const char *parse_arg(const char *next, struct strbuf *arg)
>  {
> -	/* Parse SP-terminated, possibly C-quoted argument */
> -	if (*next != '"')
> +	if (*next == '"') {
> +		const char *orig = next;
> +
> +		if (unquote_c_style(arg, next, &next))
> +			die("badly quoted argument: %s", orig);
> +		if (*next && !isspace(*next))
> +			die("unexpected character after quoted argument: %s", orig);
> +	} else {
>  		while (*next && !isspace(*next))
>  			strbuf_addch(arg, *next++);
> -	else if (unquote_c_style(arg, next, &next))
> -		die("badly quoted argument: %s", next);
> +	}
>  
> -	/* Return position after the argument */
>  	return next;
>  }
>  
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 29391c6..774f8c5 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
>  	grep "fatal: badly quoted argument: \\\"master" err
>  '
>  
> -test_expect_success 'stdin fails on arguments not separated by space' '
> +test_expect_success 'stdin fails on junk after quoted argument' '
>  	echo "create \"$a\"master" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: expected SP but got: master" err
> +	grep "fatal: unexpected character after quoted argument: \\\"$a\\\"master" err
>  '

Interesting.

I would have expected that "We used to check only one of the two
codepaths and the other was loose, fix it" to be accompanied by "So
here is an _addition_ to the test suite to validate the other case
that used to be loose, now tightened", not "We update one existing
case".  The test before and after the patch is about a c-quoted
string, so I am not sure if we are still testing the right thing.

The code in update-ref.c after the patch does look reasonable,
though.

Re: [PATCH v2 06/27] update_refs(): Fix constness

[Junio C Hamano]

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

> Since full const correctness is beyond the ability of C's type system,
> just put the const where it doesn't do any harm.  A (struct ref_update
> **) can be passed to a (struct ref_update * const *) argument, but not
> to a (const struct ref_update **) argument.

Sounds good, but next time please try not to break lines inside a
single typename, which is somewhat unreadable ;-)

I'd suggest rewording "s/Fix/tighten/".  Because a patch that
changes constness can loosen constness to make things more correct,
"git shortlog" output that says if it is tightening or loosening
would be more informative than the one that says that it is "fixing".

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/update-ref.c | 2 +-
>  refs.c               | 2 +-
>  refs.h               | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index f6345e5..a8a68e8 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {
>  
>  static int updates_alloc;
>  static int updates_count;
> -static const struct ref_update **updates;
> +static struct ref_update **updates;
>  
>  static char line_termination = '\n';
>  static int update_flags;
> diff --git a/refs.c b/refs.c
> index 196984e..1305eb1 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>  	return 0;
>  }
>  
> -int update_refs(const char *action, const struct ref_update **updates_orig,
> +int update_refs(const char *action, struct ref_update * const *updates_orig,
>  		int n, enum action_on_err onerr)
>  {
>  	int ret = 0, delnum = 0, i;
> diff --git a/refs.h b/refs.h
> index a713b34..08e60ac 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,
>  /**
>   * Lock all refs and then perform all modifications.
>   */
> -int update_refs(const char *action, const struct ref_update **updates,
> +int update_refs(const char *action, struct ref_update * const *updates,
>  		int n, enum action_on_err onerr);
>  
>  extern int parse_hide_refs_config(const char *var, const char *value, const char *);

Re: [PATCH v2 13/27] t1400: Test that stdin -z update treats empty <newvalue> as zeros

[Junio C Hamano]

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

> This is the (slightly inconsistent) status quo; make sure it doesn't
> change by accident.

Interesting.  So "oldvalue" being empty is "we do not care what it
is" (as opposed to "we know it must not exist yet" aka 0{40}), and
"newvalue" being empty is the same as "delete it" aka 0{40}.

That is unfortunate, but I agree it is a good idea to add a test for
it, so that we will notice when we decide to fix it.

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t1400-update-ref.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index a2015d0..208f56e 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref name' '
>  	grep "fatal: invalid ref format: ~a" err
>  '
>  
> +test_expect_success 'stdin -z treats empty new value as zeros' '
> +	git update-ref $a $m &&
> +	printf $F "update $a" "" "" >stdin &&
> +	git update-ref -z --stdin <stdin &&
> +	test_must_fail git rev-parse --verify -q $a
> +'
> +
>  test_expect_success 'stdin -z fails update with no new value' '
>  	printf $F "update $a" >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&

Re: [PATCH v2 15/27] update-ref --stdin -z: Deprecate interpreting the empty string as zeros

[Junio C Hamano]

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

> In the original version of this command, for the single case of the
> "update" command's <newvalue>, the empty string was interpreted as
> being equivalent to 40 "0"s.  This shorthand is unnecessary (binary
> input will usually be generated programmatically anyway), and it
> complicates the parser and the documentation.

Nice.

>
> So gently deprecate this usage: remove its description from the
> documentation and emit a warning if it is found.  But for reasons of
> backwards compatibility, continue to accept it.
>
> Helped-by: Brad King <brad.king@kitware.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  Documentation/git-update-ref.txt | 18 ++++++++++++------
>  builtin/update-ref.c             |  2 ++
>  t/t1400-update-ref.sh            |  5 +++--
>  3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
> index 0a0a551..c8f5ae5 100644
> --- a/Documentation/git-update-ref.txt
> +++ b/Documentation/git-update-ref.txt
> @@ -68,7 +68,12 @@ performs all modifications together.  Specify commands of the form:
>  	option SP <opt> LF
>  
>  Quote fields containing whitespace as if they were strings in C source
> -code.  Alternatively, use `-z` to specify commands without quoting:
> +code; i.e., surrounded by double-quotes and with backslash escapes.
> +Use 40 "0" characters or the empty string to specify a zero value.  To
> +specify a missing value, omit the value and its preceding SP entirely.
> +
> +Alternatively, use `-z` to specify in NUL-terminated format, without
> +quoting:
>  
>  	update SP <ref> NUL <newvalue> NUL [<oldvalue>] NUL
>  	create SP <ref> NUL <newvalue> NUL
> @@ -76,8 +81,12 @@ code.  Alternatively, use `-z` to specify commands without quoting:
>  	verify SP <ref> NUL [<oldvalue>] NUL
>  	option SP <opt> NUL
>  
> -Lines of any other format or a repeated <ref> produce an error.
> -Command meanings are:
> +In this format, use 40 "0" to specify a zero value, and use the empty
> +string to specify a missing value.
> +
> +In either format, values can be specified in any form that Git
> +recognizes as an object name.  Commands in any other format or a
> +repeated <ref> produce an error.  Command meanings are:
>  
>  update::
>  	Set <ref> to <newvalue> after verifying <oldvalue>, if given.
> @@ -102,9 +111,6 @@ option::
>  	The only valid option is `no-deref` to avoid dereferencing
>  	a symbolic ref.
>  
> -Use 40 "0" or the empty string to specify a zero value, except that
> -with `-z` an empty <oldvalue> is considered missing.
> -
>  If all <ref>s can be locked with matching <oldvalue>s
>  simultaneously, all modifications are performed.  Otherwise, no
>  modifications are performed.  Note that while each individual
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 6462b2f..eef7537 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
>  				goto invalid;
>  		} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {
>  			/* With -z, treat an empty value as all zeros: */
> +			warning("%s %s: missing <newvalue>, treating as zero",
> +				command, refname);
>  			hashclr(sha1);
>  		} else {
>  			/*
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 15f5bfd..2d61cce 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref name' '
>  	grep "fatal: invalid ref format: ~a" err
>  '
>  
> -test_expect_success 'stdin -z treats empty new value as zeros' '
> +test_expect_success 'stdin -z emits warning with empty new value' '
>  	git update-ref $a $m &&
>  	printf $F "update $a" "" "" >stdin &&
> -	git update-ref -z --stdin <stdin &&
> +	git update-ref -z --stdin <stdin 2>err &&
> +	grep "warning: update $a: missing <newvalue>, treating as zero" err &&
>  	test_must_fail git rev-parse --verify -q $a
>  '

Re: [PATCH v2 01/27] t1400: Fix name and expected result of one test

[Michael Haggerty]

On 03/31/2014 11:30 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The test
>>
>>     stdin -z create ref fails with zero new value
>>
>> actually passes an empty new value, not a zero new value.  So rename
>> the test s/zero/empty/, and change the expected error from
>>
>>     fatal: create $c given zero new value
>>
>> to
>>
>>     fatal: create $c missing <newvalue>
> 
> I have a feeling that "zero new value" might have been done by a
> non-native (like me) to say "no new value"; "missing newvalue"
> sounds like a good phrasing to use.
> 
>> Of course, this makes the test fail now, so mark it
>> test_expect_failure.  The failure will be fixed later in this patch
>> series.
> 
> That sounds somewhat strange.  Why not just give a single-liner to
> update-ref.c instead?

This is because there really is a difference between the two errors, and
"git update-ref" tries to emit distinct error messages for them:

* "zero new value" means that the new value was 0{40}
* "missing <newvalue>" means that the new value was absent

The problem is that it is not distinguishing between these two cases
correctly, and fixing *that* is more than a one-liner.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 16/27] t1400: Test one mistake at a time

[Junio C Hamano]

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

> This case wants to test passing a bad refname to the "update" command.
> But it also passes too few arguments to "update", which muddles the
> situation: which error should be diagnosed?  So split this test into
> two:
>
> * One that passes too few arguments to update
>
> * One that passes all three arguments to "update", but with a bad
>   refname.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>
> t1400: Add a test of "update" with too few arguments
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

What's happening here?

> ---
>  t/t1400-update-ref.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 2d61cce..6b21e45 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' '
>  	grep "fatal: update line missing <ref>" err
>  '
>  
> +test_expect_success 'stdin -z fails update with too few args' '
> +	printf $F "update $a" "$m" >stdin &&
> +	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> +	grep "fatal: update $a missing <oldvalue>" err
> +'
> +
>  test_expect_success 'stdin -z fails update with bad ref name' '
> -	printf $F "update ~a" "$m" >stdin &&
> +	printf $F "update ~a" "$m" "" >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
>  	grep "fatal: invalid ref format: ~a" err
>  '

Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages

[Junio C Hamano]

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

> Make (most of) the error messages for invalid input have the same
> format [1]:
>
>     $COMMAND [SP $REFNAME]: $MESSAGE
>
> Update the tests accordingly.
>
> [1] A few error messages are left with their old form, because
>     $COMMAND and $REFNAME aren't passed all the way down the call
>     stack.  Maybe those sites should be changed some day, too.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Up to this point, modulo nits that have been pointed out separately,
the series looked reasonably well done.

Thanks.

>  builtin/update-ref.c  | 24 ++++++++++++------------
>  t/t1400-update-ref.sh | 32 ++++++++++++++++----------------
>  2 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index b49a5b0..bbc04b2 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
>  
>  	update->ref_name = parse_refname(input, &next);
>  	if (!update->ref_name)
> -		die("update line missing <ref>");
> +		die("update: missing <ref>");
>  
>  	if (parse_next_sha1(input, &next, update->new_sha1,
>  			    "update", update->ref_name,
>  			    PARSE_SHA1_ALLOW_EMPTY))
> -		die("update %s missing <newvalue>", update->ref_name);
> +		die("update %s: missing <newvalue>", update->ref_name);
>  
>  	update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
>  					    "update", update->ref_name,
>  					    PARSE_SHA1_OLD);
>  
>  	if (*next != line_termination)
> -		die("update %s has extra input: %s", update->ref_name, next);
> +		die("update %s: extra input: %s", update->ref_name, next);
>  
>  	return next;
>  }
> @@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
>  
>  	update->ref_name = parse_refname(input, &next);
>  	if (!update->ref_name)
> -		die("create line missing <ref>");
> +		die("create: missing <ref>");
>  
>  	if (parse_next_sha1(input, &next, update->new_sha1,
>  			    "create", update->ref_name, 0))
> -		die("create %s missing <newvalue>", update->ref_name);
> +		die("create %s: missing <newvalue>", update->ref_name);
>  
>  	if (is_null_sha1(update->new_sha1))
> -		die("create %s given zero <newvalue>", update->ref_name);
> +		die("create %s: zero <newvalue>", update->ref_name);
>  
>  	if (*next != line_termination)
> -		die("create %s has extra input: %s", update->ref_name, next);
> +		die("create %s: extra input: %s", update->ref_name, next);
>  
>  	return next;
>  }
> @@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
>  
>  	update->ref_name = parse_refname(input, &next);
>  	if (!update->ref_name)
> -		die("delete line missing <ref>");
> +		die("delete: missing <ref>");
>  
>  	if (parse_next_sha1(input, &next, update->old_sha1,
>  			    "delete", update->ref_name, PARSE_SHA1_OLD)) {
>  		update->have_old = 0;
>  	} else {
>  		if (is_null_sha1(update->old_sha1))
> -			die("delete %s given zero <oldvalue>", update->ref_name);
> +			die("delete %s: zero <oldvalue>", update->ref_name);
>  		update->have_old = 1;
>  	}
>  
>  	if (*next != line_termination)
> -		die("delete %s has extra input: %s", update->ref_name, next);
> +		die("delete %s: extra input: %s", update->ref_name, next);
>  
>  	return next;
>  }
> @@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>  
>  	update->ref_name = parse_refname(input, &next);
>  	if (!update->ref_name)
> -		die("verify line missing <ref>");
> +		die("verify: missing <ref>");
>  
>  	if (parse_next_sha1(input, &next, update->old_sha1,
>  			    "verify", update->ref_name, PARSE_SHA1_OLD)) {
> @@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>  	}
>  
>  	if (*next != line_termination)
> -		die("verify %s has extra input: %s", update->ref_name, next);
> +		die("verify %s: extra input: %s", update->ref_name, next);
>  
>  	return next;
>  }
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 1db0689..48ccc4d 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted argument' '
>  test_expect_success 'stdin fails create with no ref' '
>  	echo "create " >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: create line missing <ref>" err
> +	grep "fatal: create: missing <ref>" err
>  '
>  
>  test_expect_success 'stdin fails create with bad ref name' '
> @@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref name' '
>  test_expect_success 'stdin fails create with no new value' '
>  	echo "create $a" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: create $a missing <newvalue>" err
> +	grep "fatal: create $a: missing <newvalue>" err
>  '
>  
>  test_expect_success 'stdin fails create with too many arguments' '
>  	echo "create $a $m $m" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: create $a has extra input:  $m" err
> +	grep "fatal: create $a: extra input:  $m" err
>  '
>  
>  test_expect_success 'stdin fails update with no ref' '
>  	echo "update " >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: update line missing <ref>" err
> +	grep "fatal: update: missing <ref>" err
>  '
>  
>  test_expect_success 'stdin fails update with bad ref name' '
> @@ -407,19 +407,19 @@ test_expect_success 'stdin fails update with bad ref name' '
>  test_expect_success 'stdin fails update with no new value' '
>  	echo "update $a" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: update $a missing <newvalue>" err
> +	grep "fatal: update $a: missing <newvalue>" err
>  '
>  
>  test_expect_success 'stdin fails update with too many arguments' '
>  	echo "update $a $m $m $m" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: update $a has extra input:  $m" err
> +	grep "fatal: update $a: extra input:  $m" err
>  '
>  
>  test_expect_success 'stdin fails delete with no ref' '
>  	echo "delete " >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: delete line missing <ref>" err
> +	grep "fatal: delete: missing <ref>" err
>  '
>  
>  test_expect_success 'stdin fails delete with bad ref name' '
> @@ -431,13 +431,13 @@ test_expect_success 'stdin fails delete with bad ref name' '
>  test_expect_success 'stdin fails delete with too many arguments' '
>  	echo "delete $a $m $m" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: delete $a has extra input:  $m" err
> +	grep "fatal: delete $a: extra input:  $m" err
>  '
>  
>  test_expect_success 'stdin fails verify with too many arguments' '
>  	echo "verify $a $m $m" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: verify $a has extra input:  $m" err
> +	grep "fatal: verify $a: extra input:  $m" err
>  '
>  
>  test_expect_success 'stdin fails option with unknown name' '
> @@ -532,7 +532,7 @@ test_expect_success 'stdin create ref fails with bad new value' '
>  test_expect_success 'stdin create ref fails with zero new value' '
>  	echo "create $c " >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: create $c given zero <newvalue>" err &&
> +	grep "fatal: create $c: zero <newvalue>" err &&
>  	test_must_fail git rev-parse --verify -q $c
>  '
>  
> @@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' '
>  test_expect_success 'stdin delete ref fails with zero old value' '
>  	echo "delete $a " >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: delete $a given zero <oldvalue>" err &&
> +	grep "fatal: delete $a: zero <oldvalue>" err &&
>  	git rev-parse $m >expect &&
>  	git rev-parse $a >actual &&
>  	test_cmp expect actual
> @@ -697,7 +697,7 @@ test_expect_success 'stdin -z fails on unknown command' '
>  test_expect_success 'stdin -z fails create with no ref' '
>  	printf $F "create " >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> -	grep "fatal: create line missing <ref>" err
> +	grep "fatal: create: missing <ref>" err
>  '
>  
>  test_expect_success 'stdin -z fails create with bad ref name' '
> @@ -721,7 +721,7 @@ test_expect_success 'stdin -z fails create with too many arguments' '
>  test_expect_success 'stdin -z fails update with no ref' '
>  	printf $F "update " >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> -	grep "fatal: update line missing <ref>" err
> +	grep "fatal: update: missing <ref>" err
>  '
>  
>  test_expect_success 'stdin -z fails update with too few args' '
> @@ -765,7 +765,7 @@ test_expect_success 'stdin -z fails update with too many arguments' '
>  test_expect_success 'stdin -z fails delete with no ref' '
>  	printf $F "delete " >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> -	grep "fatal: delete line missing <ref>" err
> +	grep "fatal: delete: missing <ref>" err
>  '
>  
>  test_expect_success 'stdin -z fails delete with bad ref name' '
> @@ -868,7 +868,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' '
>  test_expect_success 'stdin -z create ref fails with empty new value' '
>  	printf $F "create $c" "" >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> -	grep "fatal: create $c missing <newvalue>" err &&
> +	grep "fatal: create $c: missing <newvalue>" err &&
>  	test_must_fail git rev-parse --verify -q $c
>  '
>  
> @@ -892,7 +892,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' '
>  test_expect_success 'stdin -z delete ref fails with zero old value' '
>  	printf $F "delete $a" "$Z" >stdin &&
>  	test_must_fail git update-ref -z --stdin <stdin 2>err &&
> -	grep "fatal: delete $a given zero <oldvalue>" err &&
> +	grep "fatal: delete $a: zero <oldvalue>" err &&
>  	git rev-parse $m >expect &&
>  	git rev-parse $a >actual &&
>  	test_cmp expect actual

Re: [PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated

[Michael Haggerty]

On 03/31/2014 11:36 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Test that the argument is properly terminated by either whitespace or
>> a NUL character, even if it is quoted, to be consistent with the
>> non-quoted case.  Adjust the tests to expect the new error message.
>> Add a docstring to the function, incorporating the comments that were
>> formerly within the function plus some added information.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  builtin/update-ref.c  | 20 +++++++++++++++-----
>>  t/t1400-update-ref.sh |  4 ++--
>>  2 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index 1292cfe..02b5f95 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
>>  	update->have_old = *oldvalue || line_termination;
>>  }
>>  
>> +/*
>> + * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
>> + * and append the result to arg.  Return a pointer to the terminator.
>> + * Die if there is an error in how the argument is C-quoted.  This
>> + * function is only used if not -z.
>> + */
>>  static const char *parse_arg(const char *next, struct strbuf *arg)
>>  {
>> -	/* Parse SP-terminated, possibly C-quoted argument */
>> -	if (*next != '"')
>> +	if (*next == '"') {
>> +		const char *orig = next;
>> +
>> +		if (unquote_c_style(arg, next, &next))
>> +			die("badly quoted argument: %s", orig);
>> +		if (*next && !isspace(*next))
>> +			die("unexpected character after quoted argument: %s", orig);
>> +	} else {
>>  		while (*next && !isspace(*next))
>>  			strbuf_addch(arg, *next++);
>> -	else if (unquote_c_style(arg, next, &next))
>> -		die("badly quoted argument: %s", next);
>> +	}
>>  
>> -	/* Return position after the argument */
>>  	return next;
>>  }
>>  
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index 29391c6..774f8c5 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
>>  	grep "fatal: badly quoted argument: \\\"master" err
>>  '
>>  
>> -test_expect_success 'stdin fails on arguments not separated by space' '
>> +test_expect_success 'stdin fails on junk after quoted argument' '
>>  	echo "create \"$a\"master" >stdin &&
>>  	test_must_fail git update-ref --stdin <stdin 2>err &&
>> -	grep "fatal: expected SP but got: master" err
>> +	grep "fatal: unexpected character after quoted argument: \\\"$a\\\"master" err
>>  '
> 
> Interesting.
> 
> I would have expected that "We used to check only one of the two
> codepaths and the other was loose, fix it" to be accompanied by "So
> here is an _addition_ to the test suite to validate the other case
> that used to be loose, now tightened", not "We update one existing
> case".  The test before and after the patch is about a c-quoted
> string, so I am not sure if we are still testing the right thing.
> 
> The code in update-ref.c after the patch does look reasonable,
> though.

The old parse_arg(), when fed an argument

    "refs/heads/a"master

parsed 'refs/heads/a' off of the front of the argument and considered
itself successful.  It was only when parse_next_arg() tried to parse the
*next* argument that a problem was noticed.  But in fact, the definition
of the input format requires arguments to be terminated by SP or NUL, so
*this* argument is already erroneous and parse_arg() should diagnose the
problem.

The point of this patch is to move the error detection for C-quoted
arguments that have trailing junk to the parse_arg() call for the broken
argument and to make the error message more descriptive of the situation.

There is no corresponding error case for non-C-quoted arguments, because
the end of the argument is *by definition* a space or NUL, so there is
no way to insert other junk between the "end" of the argument and the
argument terminator.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 06/27] update_refs(): Fix constness

[Michael Haggerty]

On 03/31/2014 11:40 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Since full const correctness is beyond the ability of C's type system,
>> just put the const where it doesn't do any harm.  A (struct ref_update
>> **) can be passed to a (struct ref_update * const *) argument, but not
>> to a (const struct ref_update **) argument.
> 
> Sounds good, but next time please try not to break lines inside a
> single typename, which is somewhat unreadable ;-)
> 
> I'd suggest rewording "s/Fix/tighten/".  Because a patch that
> changes constness can loosen constness to make things more correct,
> "git shortlog" output that says if it is tightening or loosening
> would be more informative than the one that says that it is "fixing".

It is not a strict tightening, because I add a "const" in one place but
remove it from another:

    const struct ref_update **

becomes

    struct ref_update * const *

in the update_refs() signature.  In fact, the old declaration was too
strict for some changes later in the patch series, which is why I needed
to loosen (one aspect) of it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 13/27] t1400: Test that stdin -z update treats empty <newvalue> as zeros

[Michael Haggerty]

On 03/31/2014 11:48 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This is the (slightly inconsistent) status quo; make sure it doesn't
>> change by accident.
> 
> Interesting.  So "oldvalue" being empty is "we do not care what it
> is" (as opposed to "we know it must not exist yet" aka 0{40}), and
> "newvalue" being empty is the same as "delete it" aka 0{40}.
> 
> That is unfortunate, but I agree it is a good idea to add a test for
> it, so that we will notice when we decide to fix it.

Correct.  This was discussed at some more length here [1].  In v1 of
this patch series I incorrectly changed this behavior, thinking it to
have been an accident.

Michael

[1]
http://thread.gmane.org/gmane.comp.version-control.git/243731/focus=243773

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 16/27] t1400: Test one mistake at a time

[Michael Haggerty]

On 03/31/2014 11:50 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This case wants to test passing a bad refname to the "update" command.
>> But it also passes too few arguments to "update", which muddles the
>> situation: which error should be diagnosed?  So split this test into
>> two:
>>
>> * One that passes too few arguments to update
>>
>> * One that passes all three arguments to "update", but with a bad
>>   refname.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>>
>> t1400: Add a test of "update" with too few arguments
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> What's happening here?

That was a squashing accident, also pointed out by Brad [1].  The last
three lines should be deleted.  This and an error in a comment in patch
14/27 that was also pointed out by Brad are both fixed in my GitHub repo
[2].  I haven't sent the fixed version to the list, though; let me know
if I should.

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/245205
[2] Branch "ref-transactions" at https://github.com/mhagger/git

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages

[Michael Haggerty]

On 03/31/2014 11:51 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Make (most of) the error messages for invalid input have the same
>> format [1]:
>>
>>     $COMMAND [SP $REFNAME]: $MESSAGE
>>
>> Update the tests accordingly.
>>
>> [1] A few error messages are left with their old form, because
>>     $COMMAND and $REFNAME aren't passed all the way down the call
>>     stack.  Maybe those sites should be changed some day, too.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> Up to this point, modulo nits that have been pointed out separately,
> the series looked reasonably well done.

Thanks for the feedback!  Would you like me to expand the commit
messages to answer the questions that you asked about the previous
patches?  And if so, do you want a v3 sent to the list already or should
I wait for you to review patches 19-27 first?

Cheers,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 06/27] update_refs(): Fix constness

[Junio C Hamano]

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

> On 03/31/2014 11:40 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>> Since full const correctness is beyond the ability of C's type system,
>>> just put the const where it doesn't do any harm.  A (struct ref_update
>>> **) can be passed to a (struct ref_update * const *) argument, but not
>>> to a (const struct ref_update **) argument.
>> 
>> Sounds good, but next time please try not to break lines inside a
>> single typename, which is somewhat unreadable ;-)
>> 
>> I'd suggest rewording "s/Fix/tighten/".  Because a patch that
>> changes constness can loosen constness to make things more correct,
>> "git shortlog" output that says if it is tightening or loosening
>> would be more informative than the one that says that it is "fixing".
>
> It is not a strict tightening, because I add a "const" in one place but
> remove it from another:
>
>     const struct ref_update **
>
> becomes
>
>     struct ref_update * const *
>
> in the update_refs() signature.  In fact, the old declaration was too
> strict for some changes later in the patch series, which is why I needed
> to loosen (one aspect) of it.

Interesting.  Then that _is_ a fix.  Thanks for explaining it to
me.  As always, I would prefer it be explained to the proposed
commit log, not to me over an e-mail ;-)

Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages

[Michael Haggerty]

On 04/01/2014 12:37 AM, Michael Haggerty wrote:
> On 03/31/2014 11:51 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> Make (most of) the error messages for invalid input have the same
>>> format [1]:
>>>
>>>     $COMMAND [SP $REFNAME]: $MESSAGE
>>>
>>> Update the tests accordingly.
>>>
>>> [1] A few error messages are left with their old form, because
>>>     $COMMAND and $REFNAME aren't passed all the way down the call
>>>     stack.  Maybe those sites should be changed some day, too.
>>>
>>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>>> ---
>>
>> Up to this point, modulo nits that have been pointed out separately,
>> the series looked reasonably well done.
> 
> Thanks for the feedback!  Would you like me to expand the commit
> messages to answer the questions that you asked about the previous
> patches?  And if so, do you want a v3 sent to the list already or should
> I wait for you to review patches 19-27 first?

Junio, I incorporated your feedback (which so far has only affected
commit messages).  I also rebased the patch series to the current
master.  I pushed the result to GitHub [1].  I'll refrain from spamming
the list with v3 yet.

Michael

[1] Branch "ref-transactions" at https://github.com/mhagger/git

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 24/27] ref_transaction_commit(): Introduce temporary variables

[Junio C Hamano]

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

> Use temporary variables in the for-loop blocks to simplify expressions
> in the rest of the loop.

Shouldn't the summary of the change "simplify expressions"?  Use of
temporary variables is a means to the end.  If you have enough room
to say "achieve X by doing Y", please do so; otherwise "achieve X"
is more important part than "do Y".

Other than that, this looks good.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 2b80f6d..d51566c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3432,10 +3432,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  	/* Acquire all locks while verifying old values */
>  	for (i = 0; i < n; i++) {
> -		locks[i] = update_ref_lock(updates[i]->refname,
> -					   (updates[i]->have_old ?
> -					    updates[i]->old_sha1 : NULL),
> -					   updates[i]->flags,
> +		struct ref_update *update = updates[i];
> +
> +		locks[i] = update_ref_lock(update->refname,
> +					   (update->have_old ?
> +					    update->old_sha1 : NULL),
> +					   update->flags,
>  					   &types[i], onerr);
>  		if (!locks[i]) {
>  			ret = 1;
> @@ -3444,16 +3446,19 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  	}
>  
>  	/* Perform updates first so live commits remain referenced */
> -	for (i = 0; i < n; i++)
> -		if (!is_null_sha1(updates[i]->new_sha1)) {
> +	for (i = 0; i < n; i++) {
> +		struct ref_update *update = updates[i];
> +
> +		if (!is_null_sha1(update->new_sha1)) {
>  			ret = update_ref_write(msg,
> -					       updates[i]->refname,
> -					       updates[i]->new_sha1,
> +					       update->refname,
> +					       update->new_sha1,
>  					       locks[i], onerr);
>  			locks[i] = NULL; /* freed by update_ref_write */
>  			if (ret)
>  				goto cleanup;
>  		}
> +	}
>  
>  	/* Perform deletes now that updates are safely completed */
>  	for (i = 0; i < n; i++)

Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction

[Junio C Hamano]

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

> Build out the API for dealing with a bunch of reference checks and
> changes within a transaction.  Define an opaque ref_transaction type
> that is managed entirely within refs.c.  Introduce functions for
> beginning a transaction, adding updates to a transaction, and
> committing/rolling back a transaction.
>
> This API will soon replace update_refs().
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 161 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index 1305eb1..e788c27 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const char *refname,
>  	return 0;
>  }
>  
> +/*
> + * Data structure for holding a reference transaction, which can
> + * consist of checks and updates to multiple references, carried out
> + * as atomically as possible.  This structure is opaque to callers.
> + */
> +struct ref_transaction {
> +	struct ref_update **updates;

Don't we try to name an array update[] (not plural updates[]) so
that we can say update[7] to mean the seventh update?

> +	size_t alloc;
> +	size_t nr;
> +};
> +
> +struct ref_transaction *ref_transaction_begin(void)
> +{
> +	return xcalloc(1, sizeof(struct ref_transaction));
> +}
> +
> +static void ref_transaction_free(struct ref_transaction *transaction)
> +{
> +	int i;
> +
> +	for (i = 0; i < transaction->nr; i++) {
> +		struct ref_update *update = transaction->updates[i];
> +
> +		free((char *)update->ref_name);
> +		free(update);
> +	}
> +
> +	free(transaction->updates);
> +	free(transaction);
> +}

OK.

> +void ref_transaction_rollback(struct ref_transaction *transaction)
> +{
> +	ref_transaction_free(transaction);
> +}

OK.

> +static struct ref_update *add_update(struct ref_transaction *transaction,
> +				     const char *refname)
> +{
> +	struct ref_update *update = xcalloc(1, sizeof(*update));
> +
> +	update->ref_name = xstrdup(refname);
> +	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
> +	transaction->updates[transaction->nr++] = update;
> +	return update;
> +}
> +
> +void ref_transaction_update(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *new_sha1, unsigned char *old_sha1,
> +			    int flags, int have_old)
> +{
> +	struct ref_update *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);
> +}
> +
> +void ref_transaction_create(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *new_sha1,
> +			    int flags)
> +{
> +	struct ref_update *update = add_update(transaction, refname);
> +
> +	hashcpy(update->new_sha1, new_sha1);
> +	hashclr(update->old_sha1);
> +	update->flags = flags;
> +	update->have_old = 1;
> +}
> +
> +void ref_transaction_delete(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *old_sha1,
> +			    int flags, int have_old)
> +{
> +	struct ref_update *update = add_update(transaction, refname);
> +
> +	update->flags = flags;
> +	update->have_old = have_old;
> +	if (have_old)
> +		hashcpy(update->old_sha1, old_sha1);
> +}
> +

I can see that the chosen set of primitives update/create/delete
mirrors what update-ref allows us to do, but given the explanation
of "update" in refs.h, wouldn't it make more sense to implement the
others in terms of it?

>  int update_ref(const char *action, const char *refname,
>  	       const unsigned char *sha1, const unsigned char *oldval,
>  	       int flags, enum action_on_err onerr)
> @@ -3378,6 +3465,15 @@ cleanup:
>  	return ret;
>  }
>  
> +int ref_transaction_commit(struct ref_transaction *transaction,
> +			   const char *msg, enum action_on_err onerr)
> +{
> +	int ret = update_refs(msg, transaction->updates, transaction->nr,
> +			      onerr);
> +	ref_transaction_free(transaction);
> +	return ret;
> +}

OK.

>  char *shorten_unambiguous_ref(const char *refname, int strict)
>  {
>  	int i;
> diff --git a/refs.h b/refs.h
> index 08e60ac..476a923 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -24,6 +24,8 @@ struct ref_update {
>  	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>  };
>  
> +struct ref_transaction;
> +
>  /*
>   * Bit values set in the flags argument passed to each_ref_fn():
>   */
> @@ -220,6 +222,69 @@ enum action_on_err {
>  	UPDATE_REFS_QUIET_ON_ERR
>  };
>  
> +/*
> + * Begin a reference transaction.  The reference transaction must
> + * eventually be commited using ref_transaction_commit() or rolled
> + * back using ref_transaction_rollback().
> + */
> +struct ref_transaction *ref_transaction_begin(void);
> +
> +/*
> + * Roll back a ref_transaction and free all associated data.
> + */
> +void ref_transaction_rollback(struct ref_transaction *transaction);
> +
> +
> +/*
> + * 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, so the caller retains ownership of the parameter.  flags

Good to see the ownership rules described.

> + * can be REF_NODEREF; it is passed to update_ref_lock().
> + */
> +
> +
> +/*
> + * Add a reference update to transaction.  new_sha1 is the value that
> + * the reference should have after the update, or zeros 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
> + * it must not have existed beforehand.
> + */
> +void ref_transaction_update(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *new_sha1, unsigned char *old_sha1,
> +			    int flags, int have_old);
> +
> +/*
> + * Add a reference creation to transaction.  new_sha1 is the value
> + * that the reference should have after the update, or zeros if it
> + * should be deleted.  It is verified that the reference does not
> + * exist already.
> + */

Sounds a bit crazy that you can ask "create", which verifies the
absense of the thing, to delete a thing.

> +void ref_transaction_create(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *new_sha1,
> +			    int flags);
> +
> +/*
> + * Add a reference deletion to transaction.  If have_old is true, then
> + * old_sha1 holds the value that the reference should have had before
> + * the update.
> + */
> +void ref_transaction_delete(struct ref_transaction *transaction,
> +			    const char *refname,
> +			    unsigned char *old_sha1,
> +			    int flags, int have_old);
> +
> +/*
> + * Commit all of the changes that have been queued in transaction, as
> + * atomically as possible.  Return a nonzero value if there is a
> + * problem.  The ref_transaction is freed by this function.
> + */
> +int ref_transaction_commit(struct ref_transaction *transaction,
> +			   const char *msg, enum action_on_err onerr);
> +
>  /** 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,

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

[Junio C Hamano]

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

> This change is mostly clerical: the parse_cmd_*() functions need to
> use local variables rather than a struct ref_update to collect the
> arguments needed for each update, and then call ref_transaction_*() to
> queue the change rather than building up the list of changes at the
> caller side.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

With the implementation of ref_transaction at this point in the
series it does not matter, but the updated code in this patch means
that it is perfectly acceptable to do this sequence:

    ref_transaction_begin();
    ref_transaction_update();
    ...
    ref_transaction_update();
    die();

without ever calling ref_transaction_rollback() API function.
Depending on the future backends, we may want to ensure rollback is
called, no?  And if that is the case, we would want to prepare
callers of the API with some at-exit facility to call rollback, no?

Other than that, the code looks straight-forward.

Thanks.

>  builtin/update-ref.c | 142 +++++++++++++++++++++++++++------------------------
>  1 file changed, 75 insertions(+), 67 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index bbc04b2..2c8678b 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
>  	NULL
>  };
>  
> -static int updates_alloc;
> -static int updates_count;
> -static struct ref_update **updates;
> +static struct ref_transaction *transaction;
>  
>  static char line_termination = '\n';
>  static int update_flags;
>  
> -static struct ref_update *update_alloc(void)
> -{
> -	struct ref_update *update;
> -
> -	/* Allocate and zero-init a struct ref_update */
> -	update = xcalloc(1, sizeof(*update));
> -	ALLOC_GROW(updates, updates_count + 1, updates_alloc);
> -	updates[updates_count++] = update;
> -
> -	/* Store and reset accumulated options */
> -	update->flags = update_flags;
> -	update_flags = 0;
> -
> -	return update;
> -}
> -
>  /*
>   * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
>   * and append the result to arg.  Return a pointer to the terminator.
> @@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
>  
>  static const char *parse_cmd_update(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> -
> -	update = update_alloc();
> +	char *refname;
> +	unsigned char new_sha1[20];
> +	unsigned char old_sha1[20];
> +	int have_old;
>  
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("update: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->new_sha1,
> -			    "update", update->ref_name,
> +	if (parse_next_sha1(input, &next, new_sha1, "update", refname,
>  			    PARSE_SHA1_ALLOW_EMPTY))
> -		die("update %s: missing <newvalue>", update->ref_name);
> +		die("update %s: missing <newvalue>", refname);
>  
> -	update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
> -					    "update", update->ref_name,
> -					    PARSE_SHA1_OLD);
> +	have_old = !parse_next_sha1(input, &next, old_sha1, "update", refname,
> +				    PARSE_SHA1_OLD);
>  
>  	if (*next != line_termination)
> -		die("update %s: extra input: %s", update->ref_name, next);
> +		die("update %s: extra input: %s", refname, next);
> +
> +	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +			       update_flags, have_old);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
>  
>  static const char *parse_cmd_create(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> -
> -	update = update_alloc();
> +	char *refname;
> +	unsigned char new_sha1[20];
>  
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("create: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->new_sha1,
> -			    "create", update->ref_name, 0))
> -		die("create %s: missing <newvalue>", update->ref_name);
> +	if (parse_next_sha1(input, &next, new_sha1, "create", refname, 0))
> +		die("create %s: missing <newvalue>", refname);
>  
> -	if (is_null_sha1(update->new_sha1))
> -		die("create %s: zero <newvalue>", update->ref_name);
> +	if (is_null_sha1(new_sha1))
> +		die("create %s: zero <newvalue>", refname);
>  
>  	if (*next != line_termination)
> -		die("create %s: extra input: %s", update->ref_name, next);
> +		die("create %s: extra input: %s", refname, next);
> +
> +	ref_transaction_create(transaction, refname, new_sha1, update_flags);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
>  
>  static const char *parse_cmd_delete(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> +	char *refname;
> +	unsigned char old_sha1[20];
> +	int have_old;
>  
> -	update = update_alloc();
> -
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("delete: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->old_sha1,
> -			    "delete", update->ref_name, PARSE_SHA1_OLD)) {
> -		update->have_old = 0;
> +	if (parse_next_sha1(input, &next, old_sha1, "delete", refname,
> +			    PARSE_SHA1_OLD)) {
> +		have_old = 0;
>  	} else {
> -		if (is_null_sha1(update->old_sha1))
> -			die("delete %s: zero <oldvalue>", update->ref_name);
> -		update->have_old = 1;
> +		if (is_null_sha1(old_sha1))
> +			die("delete %s: zero <oldvalue>", refname);
> +		have_old = 1;
>  	}
>  
>  	if (*next != line_termination)
> -		die("delete %s: extra input: %s", update->ref_name, next);
> +		die("delete %s: extra input: %s", refname, next);
> +
> +	ref_transaction_delete(transaction, refname, old_sha1,
> +			       update_flags, have_old);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
>  
>  static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> -
> -	update = update_alloc();
> +	char *refname;
> +	unsigned char new_sha1[20];
> +	unsigned char old_sha1[20];
> +	int have_old;
>  
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("verify: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->old_sha1,
> -			    "verify", update->ref_name, PARSE_SHA1_OLD)) {
> -		update->have_old = 0;
> +	if (parse_next_sha1(input, &next, old_sha1, "verify", refname,
> +			    PARSE_SHA1_OLD)) {
> +		hashclr(new_sha1);
> +		have_old = 0;
>  	} else {
> -		hashcpy(update->new_sha1, update->old_sha1);
> -		update->have_old = 1;
> +		hashcpy(new_sha1, old_sha1);
> +		have_old = 1;
>  	}
>  
>  	if (*next != line_termination)
> -		die("verify %s: extra input: %s", update->ref_name, next);
> +		die("verify %s: extra input: %s", refname, next);
> +
> +	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +			       update_flags, have_old);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
> @@ -355,13 +359,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>  		die("Refusing to perform update with empty message.");
>  
>  	if (read_stdin) {
> +		int ret;
> +		transaction = ref_transaction_begin();
> +
>  		if (delete || no_deref || argc > 0)
>  			usage_with_options(git_update_ref_usage, options);
>  		if (end_null)
>  			line_termination = '\0';
>  		update_refs_stdin();
> -		return update_refs(msg, updates, updates_count,
> -				   UPDATE_REFS_DIE_ON_ERR);
> +		ret = ref_transaction_commit(transaction, msg,
> +					     UPDATE_REFS_DIE_ON_ERR);
> +		return ret;
>  	}
>  
>  	if (end_null)

Re: [PATCH v2 21/27] refs: Remove API function update_refs()

[Junio C Hamano]

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

> It has been superseded by reference transactions.  This also means
> that struct ref_update can become private.

Good.

Re: [PATCH v2 22/27] struct ref_update: Rename field "ref_name" to "refname"

[Junio C Hamano]

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

> This is consistent with the usual nomenclature.

I am of two minds.

Looking for "\(\.\|->\)ref_name" used to ignore refname fields of
other structures and let us focus on the ref_update structure.  Yes,
there is the ref_lock structure that shares ref_name to contaminate
such a grep output already, but this change makes the output even
more noisy, as you have to now look for "\(\.\|->\)refname" which
would give you more hits from other unrelated structures.

On the other hand, I do not like to name this to "update_refname" or
some nonsense like that, of course. A reference name field in a
"ref_update" structure shouldn't have to say that it is about
updating in its name; it should be known from the name of the
structure it appears in.

So I dunno.

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 18 +++++++++---------
>  refs.h |  2 +-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index dfff117..d72d0ab 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const char *refname,
>   * value or to zero to ensure the ref does not exist before update.
>   */
>  struct ref_update {
> -	const char *ref_name;
> +	const char *refname;
>  	unsigned char new_sha1[20];
>  	unsigned char old_sha1[20];
>  	int flags; /* REF_NODEREF? */
> @@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction *transaction)
>  	for (i = 0; i < transaction->nr; i++) {
>  		struct ref_update *update = transaction->updates[i];
>  
> -		free((char *)update->ref_name);
> +		free((char *)update->refname);
>  		free(update);
>  	}
>  
> @@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
>  {
>  	struct ref_update *update = xcalloc(1, sizeof(*update));
>  
> -	update->ref_name = xstrdup(refname);
> +	update->refname = xstrdup(refname);
>  	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
>  	transaction->updates[transaction->nr++] = update;
>  	return update;
> @@ -3383,7 +3383,7 @@ static int ref_update_compare(const void *r1, const void *r2)
>  {
>  	const struct ref_update * const *u1 = r1;
>  	const struct ref_update * const *u2 = r2;
> -	return strcmp((*u1)->ref_name, (*u2)->ref_name);
> +	return strcmp((*u1)->refname, (*u2)->refname);
>  }
>  
>  static int ref_update_reject_duplicates(struct ref_update **updates, int n,
> @@ -3391,14 +3391,14 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>  {
>  	int i;
>  	for (i = 1; i < n; i++)
> -		if (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) {
> +		if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
>  			const char *str =
>  				"Multiple updates for ref '%s' not allowed.";
>  			switch (onerr) {
>  			case UPDATE_REFS_MSG_ON_ERR:
> -				error(str, updates[i]->ref_name); break;
> +				error(str, updates[i]->refname); break;
>  			case UPDATE_REFS_DIE_ON_ERR:
> -				die(str, updates[i]->ref_name); break;
> +				die(str, updates[i]->refname); break;
>  			case UPDATE_REFS_QUIET_ON_ERR:
>  				break;
>  			}
> @@ -3435,7 +3435,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  	/* Acquire all locks while verifying old values */
>  	for (i = 0; i < n; i++) {
> -		locks[i] = update_ref_lock(updates[i]->ref_name,
> +		locks[i] = update_ref_lock(updates[i]->refname,
>  					   (updates[i]->have_old ?
>  					    updates[i]->old_sha1 : NULL),
>  					   updates[i]->flags,
> @@ -3450,7 +3450,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  	for (i = 0; i < n; i++)
>  		if (!is_null_sha1(updates[i]->new_sha1)) {
>  			ret = update_ref_write(msg,
> -					       updates[i]->ref_name,
> +					       updates[i]->refname,
>  					       updates[i]->new_sha1,
>  					       locks[i], onerr);
>  			locks[i] = NULL; /* freed by update_ref_write */
> diff --git a/refs.h b/refs.h
> index 99c194b..30ee721 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *lock);
>  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
>  
>  /** Setup reflog before using. **/
> -int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
> +int log_ref_setup(const char *refname, char *logfile, int bufsize);
>  
>  /** Reads log for the value of ref during at_time. **/
>  extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,

Re: [PATCH v2 23/27] struct ref_update: Store refname as a FLEX_ARRAY.

[Junio C Hamano]

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

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d72d0ab..2b80f6d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const char *refname,
>   * value or to zero to ensure the ref does not exist before update.
>   */
>  struct ref_update {
> -	const char *refname;
>  	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 */
> +	const char refname[FLEX_ARRAY];
>  };

Yeah, as we no longer borrow a pointer but make our own copy since
the earlier patch in the series, this perfectly makes sense.

>  
>  /*
> @@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction *transaction)
>  {
>  	int i;
>  
> -	for (i = 0; i < transaction->nr; i++) {
> -		struct ref_update *update = transaction->updates[i];
> -
> -		free((char *)update->refname);
> -		free(update);
> -	}
> +	for (i = 0; i < transaction->nr; i++)
> +		free(transaction->updates[i]);
>  
>  	free(transaction->updates);
>  	free(transaction);
> @@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction *transaction)
>  static struct ref_update *add_update(struct ref_transaction *transaction,
>  				     const char *refname)
>  {
> -	struct ref_update *update = xcalloc(1, sizeof(*update));
> +	size_t len = strlen(refname);
> +	struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
>  
> -	update->refname = xstrdup(refname);
> +	strcpy((char *)update->refname, refname);
>  	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
>  	transaction->updates[transaction->nr++] = update;
>  	return update;

Re: [PATCH v2 26/27] struct ref_update: Add type field

[Junio C Hamano]

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

> This is temporary space for ref_transaction_commit().
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

I was about to complain to "*Add* type" that does not say what it is
used for at all, with "Please do not add something for unknown purpose
only to utilise it in a later patch".

But that was before I noticed that these are already used and
realized that the change is about "moving what is recorded in the
type array, which is used to receive the existing reftype discovered
by calling resolve_ref_unsafe() in ref_transaction_commit() and not
used anywhere else, to a field of individual ref_update structure".

So it was somewhat of a "Huh?", but perhaps it is OK.

I wonder if ref-transaction-commit can shrink its parameter list by
accepting a single pointer to one ref_update?

>  refs.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d1edd57..07f900a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3279,6 +3279,7 @@ struct ref_update {
>  	int flags; /* REF_NODEREF? */
>  	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>  	struct ref_lock *lock;
> +	int type;
>  	const char refname[FLEX_ARRAY];
>  };
>  
> @@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  {
>  	int ret = 0, delnum = 0, i;
>  	struct ref_update **updates;
> -	int *types;
>  	const char **delnames;
>  	int n = transaction->nr;
>  
> @@ -3419,7 +3419,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  	/* Allocate work space */
>  	updates = xmalloc(sizeof(*updates) * n);
> -	types = xmalloc(sizeof(*types) * n);
>  	delnames = xmalloc(sizeof(*delnames) * n);
>  
>  	/* Copy, sort, and reject duplicate refs */
> @@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  					       (update->have_old ?
>  						update->old_sha1 : NULL),
>  					       update->flags,
> -					       &types[i], onerr);
> +					       &update->type, onerr);
>  		if (!update->lock) {
>  			ret = 1;
>  			goto cleanup;
> @@ -3465,7 +3464,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  		if (update->lock) {
>  			delnames[delnum++] = update->lock->ref_name;
> -			ret |= delete_ref_loose(update->lock, types[i]);
> +			ret |= delete_ref_loose(update->lock, update->type);
>  		}
>  	}
>  
> @@ -3479,7 +3478,6 @@ cleanup:
>  		if (updates[i]->lock)
>  			unlock_ref(updates[i]->lock);
>  	free(updates);
> -	free(types);
>  	free(delnames);
>  	ref_transaction_free(transaction);
>  	return ret;

Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction

[Michael Haggerty]

On 04/01/2014 09:39 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Build out the API for dealing with a bunch of reference checks and
>> changes within a transaction.  Define an opaque ref_transaction type
>> that is managed entirely within refs.c.  Introduce functions for
>> beginning a transaction, adding updates to a transaction, and
>> committing/rolling back a transaction.
>>
>> This API will soon replace update_refs().
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 161 insertions(+)
>>
>> diff --git a/refs.c b/refs.c
>> index 1305eb1..e788c27 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const char *refname,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Data structure for holding a reference transaction, which can
>> + * consist of checks and updates to multiple references, carried out
>> + * as atomically as possible.  This structure is opaque to callers.
>> + */
>> +struct ref_transaction {
>> +	struct ref_update **updates;
> 
> Don't we try to name an array update[] (not plural updates[]) so
> that we can say update[7] to mean the seventh update?

I know that some people prefer to name their arrays using singular, but
I wasn't aware that this is a project rule.  If it is, I think it is a
bad rule.  If followed, it would basically rule out the use of plural
nouns as identifiers, and why would we want to deprive ourselves of the
ability to use the singular/plural distinction to help clarify our code?

I like to use plural names to make it clear that an identifier refers to
a collection of objects.  This leaves the singular noun available to
refer to single objects taken out of the aggregate like

    for (i = 0; i < nr; i++) {
            struct ref_update *update = updates[i];
            /* ... work with update... */
    }

The singular/plural distinction can also be used to give a good hint
about a pointer: does it point at a single object, or does it point at
the start of an array, linked list, etc?  This convention is especially
useful in C, given that C declarations mostly don't distinguish between
pointers and arrays.

In SQL, the "name aggregates using singular noun" convention makes
sense.  SQL table names appear directly in expressions, and there is no
need to "assign" a record to an iteration variable.  I suspect that this
convention, sensible in SQL, has been carried over to traditional
programming languages where it is not (IMHO) sensible.

But...you're the maintainer.  If you would like to make this a
CodingGuidelines-level policy, I will of course conform to it.

>> +	size_t alloc;
>> +	size_t nr;
>> +};
>> +
>> +struct ref_transaction *ref_transaction_begin(void)
>> +{
>> +	return xcalloc(1, sizeof(struct ref_transaction));
>> +}
>> +
>> +static void ref_transaction_free(struct ref_transaction *transaction)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < transaction->nr; i++) {
>> +		struct ref_update *update = transaction->updates[i];
>> +
>> +		free((char *)update->ref_name);
>> +		free(update);
>> +	}
>> +
>> +	free(transaction->updates);
>> +	free(transaction);
>> +}
> 
> OK.
> 
>> +void ref_transaction_rollback(struct ref_transaction *transaction)
>> +{
>> +	ref_transaction_free(transaction);
>> +}
> 
> OK.
> 
>> +static struct ref_update *add_update(struct ref_transaction *transaction,
>> +				     const char *refname)
>> +{
>> +	struct ref_update *update = xcalloc(1, sizeof(*update));
>> +
>> +	update->ref_name = xstrdup(refname);
>> +	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
>> +	transaction->updates[transaction->nr++] = update;
>> +	return update;
>> +}
>> +
>> +void ref_transaction_update(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *new_sha1, unsigned char *old_sha1,
>> +			    int flags, int have_old)
>> +{
>> +	struct ref_update *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);
>> +}
>> +
>> +void ref_transaction_create(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *new_sha1,
>> +			    int flags)
>> +{
>> +	struct ref_update *update = add_update(transaction, refname);
>> +
>> +	hashcpy(update->new_sha1, new_sha1);
>> +	hashclr(update->old_sha1);
>> +	update->flags = flags;
>> +	update->have_old = 1;
>> +}
>> +
>> +void ref_transaction_delete(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *old_sha1,
>> +			    int flags, int have_old)
>> +{
>> +	struct ref_update *update = add_update(transaction, refname);
>> +
>> +	update->flags = flags;
>> +	update->have_old = have_old;
>> +	if (have_old)
>> +		hashcpy(update->old_sha1, old_sha1);
>> +}
>> +
> 
> I can see that the chosen set of primitives update/create/delete
> mirrors what update-ref allows us to do, but given the explanation
> of "update" in refs.h, wouldn't it make more sense to implement the
> others in terms of it?

I didn't want to put a lot of thought and refactoring into the
implementation yet, because I still have plans to change ref_update but
I haven't yet had time to decide how.  The two possibilities that I am
considering are:

1. Change ref_update to have not just have_old but also a have_new
boolean attribute (perhaps inserting both into the flags field).  This
would allow a neater coding of all of the elementary operations,
including "verify", which currently is implemented as "update
$EXPECTED_VALUE to $EXPECTED_VALUE", which is a bit silly.

2. Simplify each ref_update object to do only one operation: either
verify an old value, or set a new value.  It would then only need one
SHA-1 field.  A new flag bit would tell whether the old or new value is
being operated on, and no "have_old" attribute would be needed at all.
This approach would require a general "update <newvalue> <oldvalue>" to
be decomposed into two ref_update records, but many other operations
would only require one.  If I go this route, then it would make more
sense to implement ref_transaction_update() and the others in terms of a
"verify" and/or a "set" operation.

So this patch series has mostly focused on putting a new API between
users and refs.c and I'd rather put off this decision.

>>  int update_ref(const char *action, const char *refname,
>>  	       const unsigned char *sha1, const unsigned char *oldval,
>>  	       int flags, enum action_on_err onerr)
>> @@ -3378,6 +3465,15 @@ cleanup:
>>  	return ret;
>>  }
>>  
>> +int ref_transaction_commit(struct ref_transaction *transaction,
>> +			   const char *msg, enum action_on_err onerr)
>> +{
>> +	int ret = update_refs(msg, transaction->updates, transaction->nr,
>> +			      onerr);
>> +	ref_transaction_free(transaction);
>> +	return ret;
>> +}
> 
> OK.
> 
>>  char *shorten_unambiguous_ref(const char *refname, int strict)
>>  {
>>  	int i;
>> diff --git a/refs.h b/refs.h
>> index 08e60ac..476a923 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -24,6 +24,8 @@ struct ref_update {
>>  	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>>  };
>>  
>> +struct ref_transaction;
>> +
>>  /*
>>   * Bit values set in the flags argument passed to each_ref_fn():
>>   */
>> @@ -220,6 +222,69 @@ enum action_on_err {
>>  	UPDATE_REFS_QUIET_ON_ERR
>>  };
>>  
>> +/*
>> + * Begin a reference transaction.  The reference transaction must
>> + * eventually be commited using ref_transaction_commit() or rolled
>> + * back using ref_transaction_rollback().
>> + */
>> +struct ref_transaction *ref_transaction_begin(void);
>> +
>> +/*
>> + * Roll back a ref_transaction and free all associated data.
>> + */
>> +void ref_transaction_rollback(struct ref_transaction *transaction);
>> +
>> +
>> +/*
>> + * 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, so the caller retains ownership of the parameter.  flags
> 
> Good to see the ownership rules described.
> 
>> + * can be REF_NODEREF; it is passed to update_ref_lock().
>> + */
>> +
>> +
>> +/*
>> + * Add a reference update to transaction.  new_sha1 is the value that
>> + * the reference should have after the update, or zeros 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
>> + * it must not have existed beforehand.
>> + */
>> +void ref_transaction_update(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *new_sha1, unsigned char *old_sha1,
>> +			    int flags, int have_old);
>> +
>> +/*
>> + * Add a reference creation to transaction.  new_sha1 is the value
>> + * that the reference should have after the update, or zeros if it
>> + * should be deleted.  It is verified that the reference does not
>> + * exist already.
>> + */
> 
> Sounds a bit crazy that you can ask "create", which verifies the
> absense of the thing, to delete a thing.

Yes, you are right.  I can't remember why I defined it this way.  Maybe
I just wanted to avoid the extra call to is_null_sha1() as a consistency
check?  In any case, I'll change "create" to require a nonzero new
value.  I think an assert() will be adequate, since it would be a
programming error to call it with a zero hash.

By analogy, ref_transaction_delete() should insist that if have_old is
set, then old_sha1 is not zeros.  I will make that change as well.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

[Michael Haggerty]

On 04/01/2014 09:46 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This change is mostly clerical: the parse_cmd_*() functions need to
>> use local variables rather than a struct ref_update to collect the
>> arguments needed for each update, and then call ref_transaction_*() to
>> queue the change rather than building up the list of changes at the
>> caller side.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> With the implementation of ref_transaction at this point in the
> series it does not matter, but the updated code in this patch means
> that it is perfectly acceptable to do this sequence:
> 
>     ref_transaction_begin();
>     ref_transaction_update();
>     ...
>     ref_transaction_update();
>     die();
> 
> without ever calling ref_transaction_rollback() API function.
> Depending on the future backends, we may want to ensure rollback is
> called, no?  And if that is the case, we would want to prepare
> callers of the API with some at-exit facility to call rollback, no?

I assumed that rolling back a non-consummated transaction in the case of
early program death should be the responsibility of the library, not of
the caller.  If I'm correct, the caller(s) won't have to be modified
when the atexit facility is added, so I don't see a reason to add it
before it is needed by a concrete backend.

But you suggest that the caller should be involved.  Do you have an idea
for something that a caller might want to do besides roll back the
transaction?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 22/27] struct ref_update: Rename field "ref_name" to "refname"

[Michael Haggerty]

On 04/01/2014 09:53 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This is consistent with the usual nomenclature.
> 
> I am of two minds.
> 
> Looking for "\(\.\|->\)ref_name" used to ignore refname fields of
> other structures and let us focus on the ref_update structure.  Yes,
> there is the ref_lock structure that shares ref_name to contaminate
> such a grep output already, but this change makes the output even
> more noisy, as you have to now look for "\(\.\|->\)refname" which
> would give you more hits from other unrelated structures.
> 
> On the other hand, I do not like to name this to "update_refname" or
> some nonsense like that, of course. A reference name field in a
> "ref_update" structure shouldn't have to say that it is about
> updating in its name; it should be known from the name of the
> structure it appears in.
> 
> So I dunno.

I prefer naming consistency but whatever.

When I want to find all users of a common identifier, I usually rename
the identifier at its declaration (e.g., to "refnameXXX") and see where
gcc flags errors.  Or if I'm doing a lot of this sort of thing, I might
even fire up Eclipse, which does a pretty good job of finding instances
of a particular identifier throughout the code.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 26/27] struct ref_update: Add type field

[Michael Haggerty]

On 04/01/2014 10:03 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This is temporary space for ref_transaction_commit().
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> I was about to complain to "*Add* type" that does not say what it is
> used for at all, with "Please do not add something for unknown purpose
> only to utilise it in a later patch".
> 
> But that was before I noticed that these are already used and
> realized that the change is about "moving what is recorded in the
> type array, which is used to receive the existing reftype discovered
> by calling resolve_ref_unsafe() in ref_transaction_commit() and not
> used anywhere else, to a field of individual ref_update structure".
> 
> So it was somewhat of a "Huh?", but perhaps it is OK.

I will expand the comment in v3.

> I wonder if ref-transaction-commit can shrink its parameter list by
> accepting a single pointer to one ref_update?

I don't understand this last point.  ref_transaction_commit() has the
following signature:

int ref_transaction_commit(struct ref_transaction *transaction,
			   const char *msg, enum action_on_err onerr)

What change are you proposing?

By the way, longer-term, I wonder if msg and maybe action_on_err should
be set for each ref_update, rather than for a whole transaction.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages

[Junio C Hamano]

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

> Junio, I incorporated your feedback (which so far has only affected
> commit messages).  I also rebased the patch series to the current
> master.  I pushed the result to GitHub [1].  I'll refrain from spamming
> the list with v3 yet.

Thanks; let us know when you are ready ;-) I finished reading the
remainder of the v2, and I think I sent out what I found worth
commenting on (either positive or negative).

I think the next thing to convert to the transaction API would be
the "ok we know the set of updates from the pusher; let's update all
of them" in receive-pack?  In a sense that is of a lot more
real-world impact than the update-ref plumbing.  

           Side note: honestly speaking, I was dissapointed to see
           that the ref updates by the receive-pack process was not
           included in the series when I saw the cover letter that
           said this was a series about transactional updates to
           refs.  Anyway...

There are a few things that need to be thought through.

Making the update in receive-pack all-or-none is a behaviour change,
even though it may be a good one.  We may want to allow the user a
way to ask for the traditional "reject only the ones that cannot be
updated".  It probably goes like this:

 - On the wire, a new "ref-update-aon" capability is
   advertised from receive-pack to send-pack and can be requested in
   the opposite direction.

 - On the "git push" side, a new "--all-or-none" option, and
   optionally a new "push.allOrNone" configuration, is used to
   request the "ref-update-aon" capability over the wire.

 - On the receive-pack side, a new "receive.allOrNone" configuration 
   can be used to always update refs in all-or-none fashion, no
   matter what the pusher says.

 - The receive-pack uses the ref transaction to update the refs in
   all-or-none fashion if it has receive.allOrNone, or both sides
   agree to use ref-update-aon in the capability exchange.  If not,
   it updates the refs in some-may-succeed-some-may-fail fashion,
   one by one.

Or something like that.

Re: [PATCH v2 26/27] struct ref_update: Add type field

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I wonder if ref-transaction-commit can shrink its parameter list by
> accepting a single pointer to one ref_update?

Disregard this one.  I was fooled into thinking that the function is
called with parameters such as update->old_sha1, update_flags,
update->type when looking at the hunk starting at l.3437; the called
function there is not ref-transaction-commit.

Sorry, and thanks.

>> @@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>  					       (update->have_old ?
>>  						update->old_sha1 : NULL),
>>  					       update->flags,
>> -					       &types[i], onerr);
>> +					       &update->type, onerr);
>>  		if (!update->lock) {
>>  			ret = 1;
>>  			goto cleanup;

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

[Junio C Hamano]

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

> I assumed that rolling back a non-consummated transaction in the case of
> early program death should be the responsibility of the library, not of
> the caller.  If I'm correct, the caller(s) won't have to be modified
> when the atexit facility is added, so I don't see a reason to add it
> before it is needed by a concrete backend.
>
> But you suggest that the caller should be involved.

I didn't say "should".  If the library can automatically rollback
without being called upon die() anywhere in the system, that is
better.  The suggestion was because I didn't think you were shooting
for such a completeness in the library part, and a possible way out
is for the caller to help.

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

[Michael Haggerty]

On 04/03/2014 05:57 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I assumed that rolling back a non-consummated transaction in the case of
>> early program death should be the responsibility of the library, not of
>> the caller.  If I'm correct, the caller(s) won't have to be modified
>> when the atexit facility is added, so I don't see a reason to add it
>> before it is needed by a concrete backend.
>>
>> But you suggest that the caller should be involved.
> 
> I didn't say "should".  If the library can automatically rollback
> without being called upon die() anywhere in the system, that is
> better.  The suggestion was because I didn't think you were shooting
> for such a completeness in the library part, and a possible way out
> is for the caller to help.

I was assuming that any ref backends that required rollback-on-fail
would register an atexit handler and a signal handler, similar to how
lock_file rollbacks are done.

I admit that I haven't thought through all the details; for example, are
there restrictions on the things that a signal handler is allowed to do
that would preclude its being able to rollback the types of transactions
that back ends might want to implement?  (Though if so, what hope do we
have that the caller can do better?)

So, if somebody can think of a reason that we would need to involve the
caller in cleanup, please speak up.  Otherwise I think it would be less
error-prone to leave this responsibility with the individual back ends.
 (And if something unexpected comes up, we can make this change later.)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/