[PATCH v2 00/18] Improve handling of D/F conflicts

[PATCH v2 00/18] Improve handling of D/F conflicts

[Michael Haggerty]

This is a re-roll of [1]. This patch series improves the handling of
conflicts between references like "refs/foo" and "refs/foo/bar" that
are not allowed to co-exist. It also changes some functions in this
area to record their error messages in a "struct strbuf *err" argument
rather than sometimes emitting errors directly to stderr.

Changes relative to v1:

* Rebase to master rather than depending on
  mh/ref-lock-avoid-running-out-of-fds, as did v1. There were no
  significant dependencies between the two patch series, and now that
  mh/ref-lock-avoid-running-out-of-fds (now renamed to
  mh/write-refs-sooner.*) has been backported to 2.2, the dependency
  makes even less sense.

* Fix leak of "reason" string in ref_transaction_commit().

* Fix a broken &&-chain in t1404 and fix the function definition as
  per Junio's "SQUASH???" commit.

Thanks to Stefan, Junio, and Eric for their comments regarding v1.

As usual, this patch series is also available from my GitHub
repository [2], as branch "check-df-conflicts-earlier".

Michael

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

Michael Haggerty (18):
  t1404: new tests of ref D/F conflicts within transactions
  is_refname_available(): revamp the comments
  is_refname_available(): avoid shadowing "dir" variable
  is_refname_available(): convert local variable "dirname" to strbuf
  entry_matches(): inline function
  report_refname_conflict(): inline function
  struct nonmatching_ref_data: store a refname instead of a ref_entry
  is_refname_available(): use dirname in first loop
  ref_transaction_commit(): use a string_list for detecting duplicates
  refs: check for D/F conflicts among refs created in a transaction
  verify_refname_available(): rename function
  verify_refname_available(): report errors via a "struct strbuf *err"
  lock_ref_sha1_basic(): report errors via a "struct strbuf *err"
  lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts
  rename_ref(): integrate lock_ref_sha1_basic() errors into ours
  ref_transaction_commit(): provide better error messages
  ref_transaction_commit(): delete extra "the" from error message
  reflog_expire(): integrate lock_ref_sha1_basic() errors into ours

 refs.c                             | 309 ++++++++++++++++++++++++-------------
 t/t1400-update-ref.sh              |  14 +-
 t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++
 3 files changed, 312 insertions(+), 118 deletions(-)
 create mode 100755 t/t1404-update-ref-df-conflicts.sh

-- 
2.1.4

[PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Michael Haggerty]

Add some tests of reference D/F conflicts (by which I mean the fact
that references like "refs/foo" and "refs/foo/bar" are not allowed to
coexist) in the context of reference transactions.

The test of creating two conflicting references in the same
transaction fails, leaving the transaction half-completed. This will
be fixed later in this patch series.

Please note that the error messages emitted in the case of conflicts
are not very user-friendly. In particular, when the conflicts involve
loose references, then the errors are reported as

    error: there are still refs under 'refs/foo'
    fatal: Cannot lock the ref 'refs/foo'.

or

    error: unable to resolve reference refs/foo/bar: Not a directory
    fatal: Cannot lock the ref 'refs/foo/bar'.

This is because lock_ref_sha1_basic() fails while trying to lock the
new reference, before it even gets to the is_refname_available()
check. This situation will also be improved later in this patch
series.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100755 t/t1404-update-ref-df-conflicts.sh

diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
new file mode 100755
index 0000000..ed1b93e
--- /dev/null
+++ b/t/t1404-update-ref-df-conflicts.sh
@@ -0,1 +1,107 @@
+#!/bin/sh
+
+test_description='Test git update-ref with D/F conflicts'
+. ./test-lib.sh
+
+test_update_rejected () {
+	prefix="$1" &&
+	before="$2" &&
+	pack="$3" &&
+	create="$4" &&
+	error="$5" &&
+	printf "create $prefix/%s $C\n" $before |
+	git update-ref --stdin &&
+	git for-each-ref $prefix >unchanged &&
+	if $pack
+	then
+		git pack-refs --all
+	fi &&
+	printf "create $prefix/%s $C\n" $create >input &&
+	test_must_fail git update-ref --stdin <input 2>output.err &&
+	grep -F "$error" output.err &&
+	git for-each-ref $prefix >actual &&
+	test_cmp unchanged actual
+}
+
+Q="'"
+
+test_expect_success 'setup' '
+
+	git commit --allow-empty -m Initial &&
+	C=$(git rev-parse HEAD)
+
+'
+
+test_expect_success 'existing loose ref is a simple prefix of new' '
+
+	prefix=refs/1l &&
+	test_update_rejected $prefix "a c e" false "b c/x d" \
+		"unable to resolve reference $prefix/c/x: Not a directory"
+
+'
+
+test_expect_success 'existing packed ref is a simple prefix of new' '
+
+	prefix=refs/1p &&
+	test_update_rejected $prefix "a c e" true "b c/x d" \
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
+
+'
+
+test_expect_success 'existing loose ref is a deeper prefix of new' '
+
+	prefix=refs/2l &&
+	test_update_rejected $prefix "a c e" false "b c/x/y d" \
+		"unable to resolve reference $prefix/c/x/y: Not a directory"
+
+'
+
+test_expect_success 'existing packed ref is a deeper prefix of new' '
+
+	prefix=refs/2p &&
+	test_update_rejected $prefix "a c e" true "b c/x/y d" \
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
+
+'
+
+test_expect_success 'new ref is a simple prefix of existing loose' '
+
+	prefix=refs/3l &&
+	test_update_rejected $prefix "a c/x e" false "b c d" \
+		"there are still refs under $Q$prefix/c$Q"
+
+'
+
+test_expect_success 'new ref is a simple prefix of existing packed' '
+
+	prefix=refs/3p &&
+	test_update_rejected $prefix "a c/x e" true "b c d" \
+		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
+
+'
+
+test_expect_success 'new ref is a deeper prefix of existing loose' '
+
+	prefix=refs/4l &&
+	test_update_rejected $prefix "a c/x/y e" false "b c d" \
+		"there are still refs under $Q$prefix/c$Q"
+
+'
+
+test_expect_success 'new ref is a deeper prefix of existing packed' '
+
+	prefix=refs/4p &&
+	test_update_rejected $prefix "a c/x/y e" true "b c d" \
+		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
+
+'
+
+test_expect_failure 'one new ref is a simple prefix of another' '
+
+	prefix=refs/5 &&
+	test_update_rejected $prefix "a e" false "b c c/x d" \
+		"cannot process $Q$prefix/c$Q and $Q$prefix/c/x$Q at the same time"
+
+'
+
+test_done
-- 
2.1.4

[PATCH v2 02/18] is_refname_available(): revamp the comments

[Michael Haggerty]

Change the comments to a running example of running the function with
refname set to "refs/foo/bar". Add some more explanation of the logic.

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

diff --git a/refs.c b/refs.c
index 47e4e53..776bbce 100644
--- a/refs.c
+++ b/refs.c
@@ -876,9 +876,9 @@ static void report_refname_conflict(struct ref_entry *entry,
  * operation).
  *
  * Two reference names conflict if one of them exactly matches the
- * leading components of the other; e.g., "foo/bar" conflicts with
- * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
- * "foo/barbados".
+ * leading components of the other; e.g., "refs/foo/bar" conflicts
+ * with both "refs/foo" and with "refs/foo/bar/baz" but not with
+ * "refs/foo/bar" or "refs/foo/barbados".
  *
  * skip must be sorted.
  */
@@ -891,19 +891,39 @@ static int is_refname_available(const char *refname,
 	int pos;
 	char *dirname;
 
+	/*
+	 * For the sake of comments in this function, suppose that
+	 * refname is "refs/foo/bar".
+	 */
+
 	for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
 		/*
-		 * We are still at a leading dir of the refname; we are
-		 * looking for a conflict with a leaf entry.
-		 *
-		 * If we find one, we still must make sure it is
-		 * not in "skip".
+		 * We are still at a leading dir of the refname (e.g.,
+		 * "refs/foo"; if there is a reference with that name,
+		 * it is a conflict, *unless* it is in skip.
 		 */
 		pos = search_ref_dir(dir, refname, slash - refname);
 		if (pos >= 0) {
+			/*
+			 * We found a reference whose name is a proper
+			 * prefix of refname; e.g., "refs/foo".
+			 */
 			struct ref_entry *entry = dir->entries[pos];
-			if (entry_matches(entry, skip))
+			if (entry_matches(entry, skip)) {
+				/*
+				 * The reference we just found, e.g.,
+				 * "refs/foo", is also in skip, so it
+				 * is not considered a conflict.
+				 * Moreover, the fact that "refs/foo"
+				 * exists means that there cannot be
+				 * any references anywhere under the
+				 * "refs/foo/" namespace (because they
+				 * would have conflicted with
+				 * "refs/foo"). So we can stop looking
+				 * now and return true.
+				 */
 				return 1;
+			}
 			report_refname_conflict(entry, refname);
 			return 0;
 		}
@@ -911,19 +931,29 @@ static int is_refname_available(const char *refname,
 
 		/*
 		 * Otherwise, we can try to continue our search with
-		 * the next component; if we come up empty, we know
-		 * there is nothing under this whole prefix.
+		 * the next component. So try to look up the
+		 * directory, e.g., "refs/foo/".
 		 */
 		pos = search_ref_dir(dir, refname, slash + 1 - refname);
-		if (pos < 0)
+		if (pos < 0) {
+			/*
+			 * There was no directory "refs/foo/", so
+			 * there is nothing under this whole prefix,
+			 * and we are OK.
+			 */
 			return 1;
+		}
 
 		dir = get_ref_dir(dir->entries[pos]);
 	}
 
 	/*
-	 * We are at the leaf of our refname; we want to
-	 * make sure there are no directories which match it.
+	 * We are at the leaf of our refname (e.g., "refs/foo/bar").
+	 * There is no point in searching for a reference with that
+	 * name, because a refname isn't considered to conflict with
+	 * itself. But we still need to check for references whose
+	 * names are in the "refs/foo/bar/" namespace, because they
+	 * *do* conflict.
 	 */
 	len = strlen(refname);
 	dirname = xmallocz(len + 1);
@@ -933,9 +963,9 @@ static int is_refname_available(const char *refname,
 
 	if (pos >= 0) {
 		/*
-		 * We found a directory named "refname". It is a
-		 * problem iff it contains any ref that is not
-		 * in "skip".
+		 * We found a directory named "$refname/" (e.g.,
+		 * "refs/foo/bar/"). It is a problem iff it contains
+		 * any ref that is not in "skip".
 		 */
 		struct ref_entry *entry = dir->entries[pos];
 		struct ref_dir *dir = get_ref_dir(entry);
@@ -950,11 +980,6 @@ static int is_refname_available(const char *refname,
 		return 0;
 	}
 
-	/*
-	 * There is no point in searching for another leaf
-	 * node which matches it; such an entry would be the
-	 * ref we are looking for, not a conflict.
-	 */
 	return 1;
 }
 
-- 
2.1.4

[PATCH v2 03/18] is_refname_available(): avoid shadowing "dir" variable

[Michael Haggerty]

The function had a "dir" parameter that was shadowed by a local "dir"
variable within a code block. Use the former in place of the latter.
(This is consistent with "dir"'s use elsewhere in the function.)

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

diff --git a/refs.c b/refs.c
index 776bbce..9d87e84 100644
--- a/refs.c
+++ b/refs.c
@@ -967,10 +967,10 @@ static int is_refname_available(const char *refname,
 		 * "refs/foo/bar/"). It is a problem iff it contains
 		 * any ref that is not in "skip".
 		 */
-		struct ref_entry *entry = dir->entries[pos];
-		struct ref_dir *dir = get_ref_dir(entry);
 		struct nonmatching_ref_data data;
+		struct ref_entry *entry = dir->entries[pos];
 
+		dir = get_ref_dir(entry);
 		data.skip = skip;
 		sort_ref_dir(dir);
 		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))
-- 
2.1.4

[PATCH v2 04/18] is_refname_available(): convert local variable "dirname" to strbuf

[Michael Haggerty]

This change wouldn't be worth it by itself, but in a moment we will
use the strbuf for more string juggling.

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

diff --git a/refs.c b/refs.c
index 9d87e84..faabd68 100644
--- a/refs.c
+++ b/refs.c
@@ -887,9 +887,8 @@ static int is_refname_available(const char *refname,
 				struct ref_dir *dir)
 {
 	const char *slash;
-	size_t len;
 	int pos;
-	char *dirname;
+	struct strbuf dirname = STRBUF_INIT;
 
 	/*
 	 * For the sake of comments in this function, suppose that
@@ -955,11 +954,10 @@ static int is_refname_available(const char *refname,
 	 * names are in the "refs/foo/bar/" namespace, because they
 	 * *do* conflict.
 	 */
-	len = strlen(refname);
-	dirname = xmallocz(len + 1);
-	sprintf(dirname, "%s/", refname);
-	pos = search_ref_dir(dir, dirname, len + 1);
-	free(dirname);
+	strbuf_addstr(&dirname, refname);
+	strbuf_addch(&dirname, '/');
+	pos = search_ref_dir(dir, dirname.buf, dirname.len);
+	strbuf_release(&dirname);
 
 	if (pos >= 0) {
 		/*
-- 
2.1.4

[PATCH v2 05/18] entry_matches(): inline function

[Michael Haggerty]

It wasn't pulling its weight. And in a moment we will need similar
tests that take a refname rather than a ref_entry as parameter, which
would have made entry_matches() even less useful.

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

diff --git a/refs.c b/refs.c
index faabd68..6bdd34f 100644
--- a/refs.c
+++ b/refs.c
@@ -841,11 +841,6 @@ static void prime_ref_dir(struct ref_dir *dir)
 	}
 }
 
-static int entry_matches(struct ref_entry *entry, const struct string_list *list)
-{
-	return list && string_list_has_string(list, entry->name);
-}
-
 struct nonmatching_ref_data {
 	const struct string_list *skip;
 	struct ref_entry *found;
@@ -855,7 +850,7 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 {
 	struct nonmatching_ref_data *data = vdata;
 
-	if (entry_matches(entry, data->skip))
+	if (data->skip && string_list_has_string(data->skip, entry->name))
 		return 0;
 
 	data->found = entry;
@@ -908,7 +903,7 @@ static int is_refname_available(const char *refname,
 			 * prefix of refname; e.g., "refs/foo".
 			 */
 			struct ref_entry *entry = dir->entries[pos];
-			if (entry_matches(entry, skip)) {
+			if (skip && string_list_has_string(skip, entry->name)) {
 				/*
 				 * The reference we just found, e.g.,
 				 * "refs/foo", is also in skip, so it
-- 
2.1.4

[PATCH v2 06/18] report_refname_conflict(): inline function

[Michael Haggerty]

It wasn't pulling its weight. And we are about to need code similar to
this where no ref_entry is available and with more diverse error
messages. Rather than try to generalize the function, just inline it.

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

diff --git a/refs.c b/refs.c
index 6bdd34f..7422594 100644
--- a/refs.c
+++ b/refs.c
@@ -857,12 +857,6 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 	return 1;
 }
 
-static void report_refname_conflict(struct ref_entry *entry,
-				    const char *refname)
-{
-	error("'%s' exists; cannot create '%s'", entry->name, refname);
-}
-
 /*
  * Return true iff a reference named refname could be created without
  * conflicting with the name of an existing reference in dir.  If
@@ -918,7 +912,7 @@ static int is_refname_available(const char *refname,
 				 */
 				return 1;
 			}
-			report_refname_conflict(entry, refname);
+			error("'%s' exists; cannot create '%s'", entry->name, refname);
 			return 0;
 		}
 
@@ -969,7 +963,7 @@ static int is_refname_available(const char *refname,
 		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))
 			return 1;
 
-		report_refname_conflict(data.found, refname);
+		error("'%s' exists; cannot create '%s'", data.found->name, refname);
 		return 0;
 	}
 
-- 
2.1.4

[PATCH v2 07/18] struct nonmatching_ref_data: store a refname instead of a ref_entry

[Michael Haggerty]

Now that we don't need a ref_entry to pass to
report_refname_conflict(), it is sufficient to store the refname of
the conflicting reference.

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

diff --git a/refs.c b/refs.c
index 7422594..effd91a 100644
--- a/refs.c
+++ b/refs.c
@@ -843,7 +843,7 @@ static void prime_ref_dir(struct ref_dir *dir)
 
 struct nonmatching_ref_data {
 	const struct string_list *skip;
-	struct ref_entry *found;
+	const char *conflicting_refname;
 };
 
 static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
@@ -853,7 +853,7 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 	if (data->skip && string_list_has_string(data->skip, entry->name))
 		return 0;
 
-	data->found = entry;
+	data->conflicting_refname = entry->name;
 	return 1;
 }
 
@@ -963,7 +963,8 @@ static int is_refname_available(const char *refname,
 		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))
 			return 1;
 
-		error("'%s' exists; cannot create '%s'", data.found->name, refname);
+		error("'%s' exists; cannot create '%s'",
+		      data.conflicting_refname, refname);
 		return 0;
 	}
 
-- 
2.1.4

[PATCH v2 08/18] is_refname_available(): use dirname in first loop

[Michael Haggerty]

In the first loop (over prefixes of refname), use dirname to keep
track of the current prefix. This is not an improvement in itself, but
in a moment we will start using dirname for a role where a
NUL-terminated string is needed.

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

diff --git a/refs.c b/refs.c
index effd91a..8316bb1 100644
--- a/refs.c
+++ b/refs.c
@@ -878,26 +878,30 @@ static int is_refname_available(const char *refname,
 	const char *slash;
 	int pos;
 	struct strbuf dirname = STRBUF_INIT;
+	int ret = 0;
 
 	/*
 	 * For the sake of comments in this function, suppose that
 	 * refname is "refs/foo/bar".
 	 */
 
+	strbuf_grow(&dirname, strlen(refname) + 1);
 	for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
+		/* Expand dirname to the new prefix, not including the trailing slash: */
+		strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);
+
 		/*
 		 * We are still at a leading dir of the refname (e.g.,
 		 * "refs/foo"; if there is a reference with that name,
 		 * it is a conflict, *unless* it is in skip.
 		 */
-		pos = search_ref_dir(dir, refname, slash - refname);
+		pos = search_ref_dir(dir, dirname.buf, dirname.len);
 		if (pos >= 0) {
 			/*
 			 * We found a reference whose name is a proper
 			 * prefix of refname; e.g., "refs/foo".
 			 */
-			struct ref_entry *entry = dir->entries[pos];
-			if (skip && string_list_has_string(skip, entry->name)) {
+			if (skip && string_list_has_string(skip, dirname.buf)) {
 				/*
 				 * The reference we just found, e.g.,
 				 * "refs/foo", is also in skip, so it
@@ -910,10 +914,11 @@ static int is_refname_available(const char *refname,
 				 * "refs/foo"). So we can stop looking
 				 * now and return true.
 				 */
-				return 1;
+				ret = 1;
+				goto cleanup;
 			}
-			error("'%s' exists; cannot create '%s'", entry->name, refname);
-			return 0;
+			error("'%s' exists; cannot create '%s'", dirname.buf, refname);
+			goto cleanup;
 		}
 
 
@@ -922,14 +927,16 @@ static int is_refname_available(const char *refname,
 		 * the next component. So try to look up the
 		 * directory, e.g., "refs/foo/".
 		 */
-		pos = search_ref_dir(dir, refname, slash + 1 - refname);
+		strbuf_addch(&dirname, '/');
+		pos = search_ref_dir(dir, dirname.buf, dirname.len);
 		if (pos < 0) {
 			/*
 			 * There was no directory "refs/foo/", so
 			 * there is nothing under this whole prefix,
 			 * and we are OK.
 			 */
-			return 1;
+			ret = 1;
+			goto cleanup;
 		}
 
 		dir = get_ref_dir(dir->entries[pos]);
@@ -943,10 +950,9 @@ static int is_refname_available(const char *refname,
 	 * names are in the "refs/foo/bar/" namespace, because they
 	 * *do* conflict.
 	 */
-	strbuf_addstr(&dirname, refname);
+	strbuf_addstr(&dirname, refname + dirname.len);
 	strbuf_addch(&dirname, '/');
 	pos = search_ref_dir(dir, dirname.buf, dirname.len);
-	strbuf_release(&dirname);
 
 	if (pos >= 0) {
 		/*
@@ -960,15 +966,21 @@ static int is_refname_available(const char *refname,
 		dir = get_ref_dir(entry);
 		data.skip = skip;
 		sort_ref_dir(dir);
-		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))
-			return 1;
+		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
+			ret = 1;
+			goto cleanup;
+		}
 
 		error("'%s' exists; cannot create '%s'",
 		      data.conflicting_refname, refname);
-		return 0;
+		goto cleanup;
 	}
 
-	return 1;
+	ret = 1;
+
+cleanup:
+	strbuf_release(&dirname);
+	return ret;
 }
 
 struct packed_ref_cache {
-- 
2.1.4

[PATCH v2 09/18] ref_transaction_commit(): use a string_list for detecting duplicates

[Michael Haggerty]

Detect duplicates by storing the reference names in a string_list and
sorting that, instead of sorting the ref_updates directly.

* In a moment the string_list will be used for another purpose, too.

* This removes the need for the custom comparison function
  ref_update_compare().

* This means that we can carry out the updates in the order that the
  user specified them instead of reordering them. This might be handy
  someday if, we want to permit multiple updates to a single reference
  as long as they are compatible with each other.

Note: we can't use string_list_remove_duplicates() to check for
duplicates, because we need to know the name of the reference that
appeared multiple times, to be used in the error message.

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

diff --git a/refs.c b/refs.c
index 8316bb1..6bb65ab 100644
--- a/refs.c
+++ b/refs.c
@@ -3720,25 +3720,18 @@ int update_ref(const char *msg, const char *refname,
 	return 0;
 }
 
-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)->refname, (*u2)->refname);
-}
-
-static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+static int ref_update_reject_duplicates(struct string_list *refnames,
 					struct strbuf *err)
 {
-	int i;
+	int i, n = refnames->nr;
 
 	assert(err);
 
 	for (i = 1; i < n; i++)
-		if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
+		if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
 			strbuf_addf(err,
 				    "Multiple updates for ref '%s' not allowed.",
-				    updates[i]->refname);
+				    refnames->items[i].string);
 			return 1;
 		}
 	return 0;
@@ -3752,6 +3745,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	struct ref_update **updates = transaction->updates;
 	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
 	struct string_list_item *ref_to_delete;
+	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 
 	assert(err);
 
@@ -3763,9 +3757,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Copy, sort, and reject duplicate refs */
-	qsort(updates, n, sizeof(*updates), ref_update_compare);
-	if (ref_update_reject_duplicates(updates, n, err)) {
+	/* Fail if a refname appears more than once in the transaction: */
+	for (i = 0; i < n; i++)
+		string_list_append(&affected_refnames, updates[i]->refname);
+	string_list_sort(&affected_refnames);
+	if (ref_update_reject_duplicates(&affected_refnames, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
@@ -3857,6 +3853,7 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	string_list_clear(&refs_to_delete, 0);
+	string_list_clear(&affected_refnames, 0);
 	return ret;
 }
 
-- 
2.1.4

[PATCH v2 10/18] refs: check for D/F conflicts among refs created in a transaction

[Michael Haggerty]

If two references that D/F conflict (e.g., "refs/foo" and
"refs/foo/bar") are created in a single transaction, the old code
discovered the problem only after the "commit" phase of
ref_transaction_commit() had already begun. This could leave some
references updated and others not, which violates the promise of
atomicity.

Instead, check for such conflicts during the "locking" phase:

* Teach is_refname_available() to take an "extras" parameter that can
  contain extra reference names with which the specified refname must
  not conflict.

* Change lock_ref_sha1_basic() to take an "extras" parameter, which it
  passes through to is_refname_available().

* Change ref_transaction_commit() to pass "affected_refnames" to
  lock_ref_sha1_basic() as its "extras" argument.

This change fixes a test case in t1404.

This code is a bit stricter than it needs to be. We could conceivably
allow reference "refs/foo/bar" to be created in the same transaction
as "refs/foo" is deleted (or vice versa). But that would be
complicated to implement, because it is not possible to lock
"refs/foo/bar" while "refs/foo" exists as a loose reference, but on
the other hand we don't want to delete some references before adding
others (because that could leave a gap during which required objects
are unreachable). There is also a complication that reflog files'
paths can conflict.

Any less-strict implementation would probably require tricks like the
packing of all references before the start of the real transaction, or
the use of temporary intermediate reference names.

So for now let's accept too-strict checks. Some reference update
transactions will be rejected unnecessarily, but they will be rejected
in their entirety rather than leaving the repository in an
intermediate state, as would happen now.

Please note that there is still one kind of D/F conflict that is *not*
handled correctly. If two processes are running at the same time, and
one tries to create "refs/foo" at the same time that the other tries
to create "refs/foo/bar", then they can race with each other. Both
processes can obtain their respective locks ("refs/foo.lock" and
"refs/foo/bar.lock"), proceed to the "commit" phase of
ref_transaction_commit(), and then the slower process will discover
that it cannot rename its lockfile into place (after possibly having
committed changes to other references). There appears to be no way to
fix this race without changing the locking policy, which in turn would
require a change to *all* Git clients.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                             | 156 ++++++++++++++++++++++---------------
 t/t1404-update-ref-df-conflicts.sh |   2 +-
 2 files changed, 95 insertions(+), 63 deletions(-)

diff --git a/refs.c b/refs.c
index 6bb65ab..0fa70fb 100644
--- a/refs.c
+++ b/refs.c
@@ -859,19 +859,22 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 
 /*
  * Return true iff a reference named refname could be created without
- * conflicting with the name of an existing reference in dir.  If
- * skip is non-NULL, ignore potential conflicts with refs in skip
- * (e.g., because they are scheduled for deletion in the same
- * operation).
+ * conflicting with the name of an existing reference in dir. If
+ * extras is non-NULL, it is a list of additional refnames with which
+ * refname is not allowed to conflict. If skip is non-NULL, ignore
+ * potential conflicts with refs in skip (e.g., because they are
+ * scheduled for deletion in the same operation). Behavior is
+ * undefined if the same name is listed in both extras and skip.
  *
  * Two reference names conflict if one of them exactly matches the
  * leading components of the other; e.g., "refs/foo/bar" conflicts
  * with both "refs/foo" and with "refs/foo/bar/baz" but not with
  * "refs/foo/bar" or "refs/foo/barbados".
  *
- * skip must be sorted.
+ * extras and skip must be sorted.
  */
 static int is_refname_available(const char *refname,
+				const struct string_list *extras,
 				const struct string_list *skip,
 				struct ref_dir *dir)
 {
@@ -895,51 +898,53 @@ static int is_refname_available(const char *refname,
 		 * "refs/foo"; if there is a reference with that name,
 		 * it is a conflict, *unless* it is in skip.
 		 */
-		pos = search_ref_dir(dir, dirname.buf, dirname.len);
-		if (pos >= 0) {
-			/*
-			 * We found a reference whose name is a proper
-			 * prefix of refname; e.g., "refs/foo".
-			 */
-			if (skip && string_list_has_string(skip, dirname.buf)) {
+		if (dir) {
+			pos = search_ref_dir(dir, dirname.buf, dirname.len);
+			if (pos >= 0 &&
+			    (!skip || !string_list_has_string(skip, dirname.buf))) {
 				/*
-				 * The reference we just found, e.g.,
-				 * "refs/foo", is also in skip, so it
-				 * is not considered a conflict.
-				 * Moreover, the fact that "refs/foo"
-				 * exists means that there cannot be
-				 * any references anywhere under the
-				 * "refs/foo/" namespace (because they
-				 * would have conflicted with
-				 * "refs/foo"). So we can stop looking
-				 * now and return true.
+				 * We found a reference whose name is
+				 * a proper prefix of refname; e.g.,
+				 * "refs/foo", and is not in skip.
 				 */
-				ret = 1;
+				error("'%s' exists; cannot create '%s'",
+				      dirname.buf, refname);
 				goto cleanup;
 			}
-			error("'%s' exists; cannot create '%s'", dirname.buf, refname);
-			goto cleanup;
 		}
 
+		if (extras && string_list_has_string(extras, dirname.buf) &&
+		    (!skip || !string_list_has_string(skip, dirname.buf))) {
+			error("cannot process '%s' and '%s' at the same time",
+			      refname, dirname.buf);
+			goto cleanup;
+		}
 
 		/*
 		 * Otherwise, we can try to continue our search with
 		 * the next component. So try to look up the
-		 * directory, e.g., "refs/foo/".
+		 * directory, e.g., "refs/foo/". If we come up empty,
+		 * we know there is nothing under this whole prefix,
+		 * but even in that case we still have to continue the
+		 * search for conflicts with extras.
 		 */
 		strbuf_addch(&dirname, '/');
-		pos = search_ref_dir(dir, dirname.buf, dirname.len);
-		if (pos < 0) {
-			/*
-			 * There was no directory "refs/foo/", so
-			 * there is nothing under this whole prefix,
-			 * and we are OK.
-			 */
-			ret = 1;
-			goto cleanup;
+		if (dir) {
+			pos = search_ref_dir(dir, dirname.buf, dirname.len);
+			if (pos < 0) {
+				/*
+				 * There was no directory "refs/foo/",
+				 * so there is nothing under this
+				 * whole prefix. So there is no need
+				 * to continue looking for conflicting
+				 * references. But we need to continue
+				 * looking for conflicting extras.
+				 */
+				dir = NULL;
+			} else {
+				dir = get_ref_dir(dir->entries[pos]);
+			}
 		}
-
-		dir = get_ref_dir(dir->entries[pos]);
 	}
 
 	/*
@@ -952,30 +957,56 @@ static int is_refname_available(const char *refname,
 	 */
 	strbuf_addstr(&dirname, refname + dirname.len);
 	strbuf_addch(&dirname, '/');
-	pos = search_ref_dir(dir, dirname.buf, dirname.len);
 
-	if (pos >= 0) {
+	if (dir) {
+		pos = search_ref_dir(dir, dirname.buf, dirname.len);
+
+		if (pos >= 0) {
+			/*
+			 * We found a directory named "$refname/"
+			 * (e.g., "refs/foo/bar/"). It is a problem
+			 * iff it contains any ref that is not in
+			 * "skip".
+			 */
+			struct nonmatching_ref_data data;
+
+			data.skip = skip;
+			data.conflicting_refname = NULL;
+			dir = get_ref_dir(dir->entries[pos]);
+			sort_ref_dir(dir);
+			if (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
+				error("'%s' exists; cannot create '%s'",
+				      data.conflicting_refname, refname);
+				goto cleanup;
+			}
+		}
+	}
+
+	if (extras) {
 		/*
-		 * We found a directory named "$refname/" (e.g.,
-		 * "refs/foo/bar/"). It is a problem iff it contains
-		 * any ref that is not in "skip".
+		 * Check for entries in extras that start with
+		 * "$refname/". We do that by looking for the place
+		 * where "$refname/" would be inserted in extras. If
+		 * there is an entry at that position that starts with
+		 * "$refname/" and is not in skip, then we have a
+		 * conflict.
 		 */
-		struct nonmatching_ref_data data;
-		struct ref_entry *entry = dir->entries[pos];
-
-		dir = get_ref_dir(entry);
-		data.skip = skip;
-		sort_ref_dir(dir);
-		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
-			ret = 1;
-			goto cleanup;
-		}
+		for (pos = string_list_find_insert_index(extras, dirname.buf, 0);
+		     pos < extras->nr; pos++) {
+			const char *extra_refname = extras->items[pos].string;
 
-		error("'%s' exists; cannot create '%s'",
-		      data.conflicting_refname, refname);
-		goto cleanup;
+			if (!starts_with(extra_refname, dirname.buf))
+				break;
+
+			if (!skip || !string_list_has_string(skip, extra_refname)) {
+				error("cannot process '%s' and '%s' at the same time",
+				      refname, extra_refname);
+				goto cleanup;
+			}
+		}
 	}
 
+	/* No conflicts were found */
 	ret = 1;
 
 cleanup:
@@ -2296,6 +2327,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
  */
 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    const unsigned char *old_sha1,
+					    const struct string_list *extras,
 					    const struct string_list *skip,
 					    unsigned int flags, int *type_p)
 {
@@ -2351,7 +2383,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * our refname.
 	 */
 	if (is_null_sha1(lock->old_sha1) &&
-	     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
+	    !is_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2792,8 +2824,8 @@ static int rename_ref_available(const char *oldname, const char *newname)
 	int ret;
 
 	string_list_insert(&skip, oldname);
-	ret = is_refname_available(newname, &skip, get_packed_refs(&ref_cache))
-	    && is_refname_available(newname, &skip, get_loose_refs(&ref_cache));
+	ret = is_refname_available(newname, NULL, &skip, get_packed_refs(&ref_cache))
+		&& is_refname_available(newname, NULL, &skip, get_loose_refs(&ref_cache));
 	string_list_clear(&skip, 0);
 	return ret;
 }
@@ -2851,7 +2883,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL);
 	if (!lock) {
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
@@ -2865,7 +2897,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 0;
 
  rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL);
 	if (!lock) {
 		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
@@ -3777,7 +3809,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				update->refname,
 				((update->flags & REF_HAVE_OLD) ?
 				 update->old_sha1 : NULL),
-				NULL,
+				&affected_refnames, NULL,
 				flags,
 				&update->type);
 		if (!update->lock) {
@@ -4054,7 +4086,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
+	lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type);
 	if (!lock)
 		return error("cannot lock ref '%s'", refname);
 	if (!reflog_exists(refname)) {
diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
index ed1b93e..b0bb7d4 100755
--- a/t/t1404-update-ref-df-conflicts.sh
+++ b/t/t1404-update-ref-df-conflicts.sh
@@ -96,7 +96,7 @@ test_expect_success 'new ref is a deeper prefix of existing packed' '
 
 '
 
-test_expect_failure 'one new ref is a simple prefix of another' '
+test_expect_success 'one new ref is a simple prefix of another' '
 
 	prefix=refs/5 &&
 	test_update_rejected $prefix "a e" false "b c c/x d" \
-- 
2.1.4

[PATCH v2 11/18] verify_refname_available(): rename function

[Michael Haggerty]

Rename is_refname_available() to verify_refname_available() and change
its return value from 1 for success to 0 for success, to be consistent
with our error-handling convention. In a moment it will also get a
"struct strbuf *err" parameter.

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

diff --git a/refs.c b/refs.c
index 0fa70fb..703e93d 100644
--- a/refs.c
+++ b/refs.c
@@ -263,7 +263,7 @@ struct ref_dir {
  * presence of an empty subdirectory does not block the creation of a
  * similarly-named reference.  (The fact that reference names with the
  * same leading components can conflict *with each other* is a
- * separate issue that is regulated by is_refname_available().)
+ * separate issue that is regulated by verify_refname_available().)
  *
  * Please note that the name field contains the fully-qualified
  * reference (or subdirectory) name.  Space could be saved by only
@@ -858,13 +858,14 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 }
 
 /*
- * Return true iff a reference named refname could be created without
- * conflicting with the name of an existing reference in dir. If
- * extras is non-NULL, it is a list of additional refnames with which
- * refname is not allowed to conflict. If skip is non-NULL, ignore
- * potential conflicts with refs in skip (e.g., because they are
- * scheduled for deletion in the same operation). Behavior is
- * undefined if the same name is listed in both extras and skip.
+ * Return 0 if a reference named refname could be created without
+ * conflicting with the name of an existing reference in dir.
+ * Otherwise, return a negative value. If extras is non-NULL, it is a
+ * list of additional refnames with which refname is not allowed to
+ * conflict. If skip is non-NULL, ignore potential conflicts with refs
+ * in skip (e.g., because they are scheduled for deletion in the same
+ * operation). Behavior is undefined if the same name is listed in
+ * both extras and skip.
  *
  * Two reference names conflict if one of them exactly matches the
  * leading components of the other; e.g., "refs/foo/bar" conflicts
@@ -873,15 +874,15 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
  *
  * extras and skip must be sorted.
  */
-static int is_refname_available(const char *refname,
-				const struct string_list *extras,
-				const struct string_list *skip,
-				struct ref_dir *dir)
+static int verify_refname_available(const char *refname,
+				    const struct string_list *extras,
+				    const struct string_list *skip,
+				    struct ref_dir *dir)
 {
 	const char *slash;
 	int pos;
 	struct strbuf dirname = STRBUF_INIT;
-	int ret = 0;
+	int ret = -1;
 
 	/*
 	 * For the sake of comments in this function, suppose that
@@ -1007,7 +1008,7 @@ static int is_refname_available(const char *refname,
 	}
 
 	/* No conflicts were found */
-	ret = 1;
+	ret = 0;
 
 cleanup:
 	strbuf_release(&dirname);
@@ -2383,7 +2384,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * our refname.
 	 */
 	if (is_null_sha1(lock->old_sha1) &&
-	    !is_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {
+	    verify_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2824,8 +2825,10 @@ static int rename_ref_available(const char *oldname, const char *newname)
 	int ret;
 
 	string_list_insert(&skip, oldname);
-	ret = is_refname_available(newname, NULL, &skip, get_packed_refs(&ref_cache))
-		&& is_refname_available(newname, NULL, &skip, get_loose_refs(&ref_cache));
+	ret = !verify_refname_available(newname, NULL, &skip,
+					get_packed_refs(&ref_cache))
+		&& !verify_refname_available(newname, NULL, &skip,
+					     get_loose_refs(&ref_cache));
 	string_list_clear(&skip, 0);
 	return ret;
 }
-- 
2.1.4

[PATCH v2 12/18] verify_refname_available(): report errors via a "struct strbuf *err"

[Michael Haggerty]

It shouldn't be spewing errors directly to stderr.

For now, change its callers to spew the errors to stderr.

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

diff --git a/refs.c b/refs.c
index 703e93d..43e7bdd 100644
--- a/refs.c
+++ b/refs.c
@@ -860,12 +860,12 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 /*
  * Return 0 if a reference named refname could be created without
  * conflicting with the name of an existing reference in dir.
- * Otherwise, return a negative value. If extras is non-NULL, it is a
- * list of additional refnames with which refname is not allowed to
- * conflict. If skip is non-NULL, ignore potential conflicts with refs
- * in skip (e.g., because they are scheduled for deletion in the same
- * operation). Behavior is undefined if the same name is listed in
- * both extras and skip.
+ * Otherwise, return a negative value and write an explanation to err.
+ * If extras is non-NULL, it is a list of additional refnames with
+ * which refname is not allowed to conflict. If skip is non-NULL,
+ * ignore potential conflicts with refs in skip (e.g., because they
+ * are scheduled for deletion in the same operation). Behavior is
+ * undefined if the same name is listed in both extras and skip.
  *
  * Two reference names conflict if one of them exactly matches the
  * leading components of the other; e.g., "refs/foo/bar" conflicts
@@ -877,7 +877,8 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
 static int verify_refname_available(const char *refname,
 				    const struct string_list *extras,
 				    const struct string_list *skip,
-				    struct ref_dir *dir)
+				    struct ref_dir *dir,
+				    struct strbuf *err)
 {
 	const char *slash;
 	int pos;
@@ -889,6 +890,8 @@ static int verify_refname_available(const char *refname,
 	 * refname is "refs/foo/bar".
 	 */
 
+	assert(err);
+
 	strbuf_grow(&dirname, strlen(refname) + 1);
 	for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
 		/* Expand dirname to the new prefix, not including the trailing slash: */
@@ -908,16 +911,16 @@ static int verify_refname_available(const char *refname,
 				 * a proper prefix of refname; e.g.,
 				 * "refs/foo", and is not in skip.
 				 */
-				error("'%s' exists; cannot create '%s'",
-				      dirname.buf, refname);
+				strbuf_addf(err, "'%s' exists; cannot create '%s'",
+					    dirname.buf, refname);
 				goto cleanup;
 			}
 		}
 
 		if (extras && string_list_has_string(extras, dirname.buf) &&
 		    (!skip || !string_list_has_string(skip, dirname.buf))) {
-			error("cannot process '%s' and '%s' at the same time",
-			      refname, dirname.buf);
+			strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
+				    refname, dirname.buf);
 			goto cleanup;
 		}
 
@@ -976,8 +979,8 @@ static int verify_refname_available(const char *refname,
 			dir = get_ref_dir(dir->entries[pos]);
 			sort_ref_dir(dir);
 			if (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
-				error("'%s' exists; cannot create '%s'",
-				      data.conflicting_refname, refname);
+				strbuf_addf(err, "'%s' exists; cannot create '%s'",
+					    data.conflicting_refname, refname);
 				goto cleanup;
 			}
 		}
@@ -1000,8 +1003,8 @@ static int verify_refname_available(const char *refname,
 				break;
 
 			if (!skip || !string_list_has_string(skip, extra_refname)) {
-				error("cannot process '%s' and '%s' at the same time",
-				      refname, extra_refname);
+				strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
+					    refname, extra_refname);
 				goto cleanup;
 			}
 		}
@@ -2340,6 +2343,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;
 	int attempts_remaining = 3;
+	struct strbuf err = STRBUF_INIT;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
@@ -2384,7 +2388,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * our refname.
 	 */
 	if (is_null_sha1(lock->old_sha1) &&
-	    verify_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {
+	    verify_refname_available(refname, extras, skip,
+				     get_packed_refs(&ref_cache), &err)) {
+		error("%s", err.buf);
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2425,10 +2431,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 */
 			goto retry;
 		else {
-			struct strbuf err = STRBUF_INIT;
 			unable_to_lock_message(ref_file, errno, &err);
 			error("%s", err.buf);
-			strbuf_release(&err);
 			goto error_return;
 		}
 	}
@@ -2436,6 +2440,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
  error_return:
 	unlock_ref(lock);
+	strbuf_release(&err);
 	errno = last_errno;
 	return NULL;
 }
@@ -2822,14 +2827,19 @@ static int rename_tmp_log(const char *newrefname)
 static int rename_ref_available(const char *oldname, const char *newname)
 {
 	struct string_list skip = STRING_LIST_INIT_NODUP;
+	struct strbuf err = STRBUF_INIT;
 	int ret;
 
 	string_list_insert(&skip, oldname);
 	ret = !verify_refname_available(newname, NULL, &skip,
-					get_packed_refs(&ref_cache))
+					get_packed_refs(&ref_cache), &err)
 		&& !verify_refname_available(newname, NULL, &skip,
-					     get_loose_refs(&ref_cache));
+					     get_loose_refs(&ref_cache), &err);
+	if (!ret)
+		error("%s", err.buf);
+
 	string_list_clear(&skip, 0);
+	strbuf_release(&err);
 	return ret;
 }
 
-- 
2.1.4

[PATCH v2 13/18] lock_ref_sha1_basic(): report errors via a "struct strbuf *err"

[Michael Haggerty]

For now, change the callers to spew the error to stderr like before.
But soon we will change them to incorporate the reason for the failure
into their own error messages.

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

diff --git a/refs.c b/refs.c
index 43e7bdd..ff9b9cf 100644
--- a/refs.c
+++ b/refs.c
@@ -2333,7 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    const unsigned char *old_sha1,
 					    const struct string_list *extras,
 					    const struct string_list *skip,
-					    unsigned int flags, int *type_p)
+					    unsigned int flags, int *type_p,
+					    struct strbuf *err)
 {
 	char *ref_file;
 	const char *orig_refname = refname;
@@ -2343,7 +2344,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;
 	int attempts_remaining = 3;
-	struct strbuf err = STRBUF_INIT;
+
+	assert(err);
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
@@ -2367,7 +2369,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		ref_file = git_path("%s", orig_refname);
 		if (remove_empty_directories(ref_file)) {
 			last_errno = errno;
-			error("there are still refs under '%s'", orig_refname);
+			strbuf_addf(err, "there are still refs under '%s'",
+				    orig_refname);
 			goto error_return;
 		}
 		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
@@ -2377,8 +2380,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	    *type_p = type;
 	if (!refname) {
 		last_errno = errno;
-		error("unable to resolve reference %s: %s",
-			orig_refname, strerror(errno));
+		strbuf_addf(err, "unable to resolve reference %s: %s",
+			    orig_refname, strerror(errno));
 		goto error_return;
 	}
 	/*
@@ -2389,8 +2392,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 */
 	if (is_null_sha1(lock->old_sha1) &&
 	    verify_refname_available(refname, extras, skip,
-				     get_packed_refs(&ref_cache), &err)) {
-		error("%s", err.buf);
+				     get_packed_refs(&ref_cache), err)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2416,7 +2418,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		/* fall through */
 	default:
 		last_errno = errno;
-		error("unable to create directory for %s", ref_file);
+		strbuf_addf(err, "unable to create directory for %s", ref_file);
 		goto error_return;
 	}
 
@@ -2431,8 +2433,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 */
 			goto retry;
 		else {
-			unable_to_lock_message(ref_file, errno, &err);
-			error("%s", err.buf);
+			unable_to_lock_message(ref_file, errno, err);
 			goto error_return;
 		}
 	}
@@ -2440,7 +2441,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
  error_return:
 	unlock_ref(lock);
-	strbuf_release(&err);
 	errno = last_errno;
 	return NULL;
 }
@@ -2854,6 +2854,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	const char *symref = NULL;
+	struct strbuf err = STRBUF_INIT;
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
@@ -2896,8 +2897,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);
 	if (!lock) {
+		error("%s", err.buf);
+		strbuf_release(&err);
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
 	}
@@ -2910,8 +2913,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 0;
 
  rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);
 	if (!lock) {
+		error("%s", err.buf);
+		strbuf_release(&err);
 		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
 	}
@@ -3824,11 +3829,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				 update->old_sha1 : NULL),
 				&affected_refnames, NULL,
 				flags,
-				&update->type);
+				&update->type,
+				err);
 		if (!update->lock) {
 			ret = (errno == ENOTDIR)
 				? TRANSACTION_NAME_CONFLICT
 				: TRANSACTION_GENERIC_ERROR;
+			error("%s", err->buf);
+			strbuf_reset(err);
 			strbuf_addf(err, "Cannot lock the ref '%s'.",
 				    update->refname);
 			goto cleanup;
@@ -4088,6 +4096,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	char *log_file;
 	int status = 0;
 	int type;
+	struct strbuf err = STRBUF_INIT;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
@@ -4099,9 +4108,12 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type);
-	if (!lock)
+	lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err);
+	if (!lock) {
+		error("%s", err.buf);
+		strbuf_release(&err);
 		return error("cannot lock ref '%s'", refname);
+	}
 	if (!reflog_exists(refname)) {
 		unlock_ref(lock);
 		return 0;
-- 
2.1.4

[PATCH v2 14/18] lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts

[Michael Haggerty]

If there is a failure to lock a reference that is likely caused by a
D/F conflict (e.g., trying to lock "refs/foo/bar" when reference
"refs/foo" already exists), invoke verify_refname_available() to try
to generate a more helpful error message.

That function might not detect an error. For example, some
non-reference file might be blocking the deletion of an
otherwise-empty directory tree, or there might be a race with another
process that just deleted the offending reference. In such cases,
generate the strerror-based error message like before.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                             | 16 ++++++++++++----
 t/t1404-update-ref-df-conflicts.sh |  8 ++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index ff9b9cf..ce993bd 100644
--- a/refs.c
+++ b/refs.c
@@ -2369,8 +2369,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		ref_file = git_path("%s", orig_refname);
 		if (remove_empty_directories(ref_file)) {
 			last_errno = errno;
-			strbuf_addf(err, "there are still refs under '%s'",
-				    orig_refname);
+
+			if (!verify_refname_available(orig_refname, extras, skip,
+						      get_loose_refs(&ref_cache), err))
+				strbuf_addf(err, "there are still refs under '%s'",
+					    orig_refname);
+
 			goto error_return;
 		}
 		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
@@ -2380,8 +2384,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	    *type_p = type;
 	if (!refname) {
 		last_errno = errno;
-		strbuf_addf(err, "unable to resolve reference %s: %s",
-			    orig_refname, strerror(errno));
+		if (last_errno != ENOTDIR ||
+		    !verify_refname_available(orig_refname, extras, skip,
+					      get_loose_refs(&ref_cache), err))
+			strbuf_addf(err, "unable to resolve reference %s: %s",
+				    orig_refname, strerror(last_errno));
+
 		goto error_return;
 	}
 	/*
diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
index b0bb7d4..348a07a 100755
--- a/t/t1404-update-ref-df-conflicts.sh
+++ b/t/t1404-update-ref-df-conflicts.sh
@@ -36,7 +36,7 @@ test_expect_success 'existing loose ref is a simple prefix of new' '
 
 	prefix=refs/1l &&
 	test_update_rejected $prefix "a c e" false "b c/x d" \
-		"unable to resolve reference $prefix/c/x: Not a directory"
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q"
 
 '
 
@@ -52,7 +52,7 @@ test_expect_success 'existing loose ref is a deeper prefix of new' '
 
 	prefix=refs/2l &&
 	test_update_rejected $prefix "a c e" false "b c/x/y d" \
-		"unable to resolve reference $prefix/c/x/y: Not a directory"
+		"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q"
 
 '
 
@@ -68,7 +68,7 @@ test_expect_success 'new ref is a simple prefix of existing loose' '
 
 	prefix=refs/3l &&
 	test_update_rejected $prefix "a c/x e" false "b c d" \
-		"there are still refs under $Q$prefix/c$Q"
+		"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q"
 
 '
 
@@ -84,7 +84,7 @@ test_expect_success 'new ref is a deeper prefix of existing loose' '
 
 	prefix=refs/4l &&
 	test_update_rejected $prefix "a c/x/y e" false "b c d" \
-		"there are still refs under $Q$prefix/c$Q"
+		"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q"
 
 '
 
-- 
2.1.4

[PATCH v2 15/18] rename_ref(): integrate lock_ref_sha1_basic() errors into ours

[Michael Haggerty]

Now that lock_ref_sha1_basic() gives us back its error messages via a
strbuf, incorporate its error message into our error message rather
than emitting two separate error messages.

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

diff --git a/refs.c b/refs.c
index ce993bd..87c1ad1 100644
--- a/refs.c
+++ b/refs.c
@@ -2907,9 +2907,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);
 	if (!lock) {
-		error("%s", err.buf);
+		error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 		strbuf_release(&err);
-		error("unable to lock %s for update", newrefname);
 		goto rollback;
 	}
 	hashcpy(lock->old_sha1, orig_sha1);
@@ -2923,9 +2922,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  rollback:
 	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);
 	if (!lock) {
-		error("%s", err.buf);
+		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
-		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
 	}
 
-- 
2.1.4

[PATCH v2 16/18] ref_transaction_commit(): provide better error messages

[Michael Haggerty]

Now that lock_ref_sha1_basic() gives us back its error messages via a
strbuf, incorporate its error message into our error message rather
than emitting one error messages to stderr immediately and returning a
second to our caller.

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

diff --git a/refs.c b/refs.c
index 87c1ad1..ecaf804 100644
--- a/refs.c
+++ b/refs.c
@@ -3838,13 +3838,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				&update->type,
 				err);
 		if (!update->lock) {
+			char *reason;
+
 			ret = (errno == ENOTDIR)
 				? TRANSACTION_NAME_CONFLICT
 				: TRANSACTION_GENERIC_ERROR;
-			error("%s", err->buf);
-			strbuf_reset(err);
-			strbuf_addf(err, "Cannot lock the ref '%s'.",
-				    update->refname);
+			reason = strbuf_detach(err, NULL);
+			strbuf_addf(err, "Cannot lock the ref '%s': %s",
+				    update->refname, reason);
+			free(reason);
 			goto cleanup;
 		}
 	}
-- 
2.1.4

[PATCH v2 17/18] ref_transaction_commit(): delete extra "the" from error message

[Michael Haggerty]

While we are in the area, let's remove a superfluous definite article
from the error message that is emitted when the reference cannot be
locked. This improves how it reads and makes it a bit shorter.

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

diff --git a/refs.c b/refs.c
index ecaf804..bc4b1ab 100644
--- a/refs.c
+++ b/refs.c
@@ -3844,7 +3844,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				? TRANSACTION_NAME_CONFLICT
 				: TRANSACTION_GENERIC_ERROR;
 			reason = strbuf_detach(err, NULL);
-			strbuf_addf(err, "Cannot lock the ref '%s': %s",
+			strbuf_addf(err, "Cannot lock ref '%s': %s",
 				    update->refname, reason);
 			free(reason);
 			goto cleanup;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6805b9e..86fa468 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -519,7 +519,7 @@ test_expect_success 'stdin create ref works with path with space to blob' '
 test_expect_success 'stdin update ref fails with wrong old value' '
 	echo "update $c $m $m~1" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$c'"'"'" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -555,7 +555,7 @@ test_expect_success 'stdin update ref works with right old value' '
 test_expect_success 'stdin delete ref fails with wrong old value' '
 	echo "delete $a $m~1" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$a'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$a'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -688,7 +688,7 @@ test_expect_success 'stdin update refs fails with wrong old value' '
 	update $c  ''
 	EOF
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual &&
@@ -883,7 +883,7 @@ test_expect_success 'stdin -z create ref works with path with space to blob' '
 test_expect_success 'stdin -z update ref fails with wrong old value' '
 	printf $F "update $c" "$m" "$m~1" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$c'"'"'" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -899,7 +899,7 @@ test_expect_success 'stdin -z create ref fails when ref exists' '
 	git rev-parse "$c" >expect &&
 	printf $F "create $c" "$m~1" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$c'"'"'" err &&
 	git rev-parse "$c" >actual &&
 	test_cmp expect actual
 '
@@ -930,7 +930,7 @@ test_expect_success 'stdin -z update ref works with right old value' '
 test_expect_success 'stdin -z delete ref fails with wrong old value' '
 	printf $F "delete $a" "$m~1" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$a'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$a'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -1045,7 +1045,7 @@ 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" "$m" "$Z" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: Cannot lock ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual &&
-- 
2.1.4

[PATCH v2 18/18] reflog_expire(): integrate lock_ref_sha1_basic() errors into ours

[Michael Haggerty]

Now that lock_ref_sha1_basic() gives us back its error messages via a
strbuf, incorporate its error message into our error message rather
than emitting two separate error messages.

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

diff --git a/refs.c b/refs.c
index bc4b1ab..97043fd 100644
--- a/refs.c
+++ b/refs.c
@@ -4118,9 +4118,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	 */
 	lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err);
 	if (!lock) {
-		error("%s", err.buf);
+		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
-		return error("cannot lock ref '%s'", refname);
+		return -1;
 	}
 	if (!reflog_exists(refname)) {
 		unlock_ref(lock);
-- 
2.1.4

Re: [PATCH v2 00/18] Improve handling of D/F conflicts

[Junio C Hamano]

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

> * Rebase to master rather than depending on
>   mh/ref-lock-avoid-running-out-of-fds, as did v1. There were no
>   significant dependencies between the two patch series, and now that
>   mh/ref-lock-avoid-running-out-of-fds (now renamed to
>   mh/write-refs-sooner.*) has been backported to 2.2, the dependency
>   makes even less sense.

Thanks, that makes sense.

Re: [PATCH v2 00/18] Improve handling of D/F conflicts

[Stefan Beller]

On Mon, May 11, 2015 at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> * Rebase to master rather than depending on
>>   mh/ref-lock-avoid-running-out-of-fds, as did v1. There were no
>>   significant dependencies between the two patch series, and now that
>>   mh/ref-lock-avoid-running-out-of-fds (now renamed to
>>   mh/write-refs-sooner.*) has been backported to 2.2, the dependency
>>   makes even less sense.
>
> Thanks, that makes sense.
>

The series is Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks,
Stefan

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Junio C Hamano]

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

>  t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
>  create mode 100755 t/t1404-update-ref-df-conflicts.sh
>
> diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
> new file mode 100755
> index 0000000..ed1b93e
> --- /dev/null
> +++ b/t/t1404-update-ref-df-conflicts.sh
> @@ -0,1 +1,107 @@

Where does this "-0,1" come from???

After fixing it up and then running "format-patch" (or "show"), I
would get

    diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
    new file mode 100755
    index 0000000..2541a23
    --- /dev/null
    +++ b/t/t1404-update-ref-df-conflicts.sh
    @@ -0,0 +1,107 @@
    ...

which is more in line with what I would expect.

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Junio C Hamano]

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

> +test_update_rejected () {
> +	prefix="$1" &&
> +	before="$2" &&
> +	pack="$3" &&
> +	create="$4" &&
> +	error="$5" &&
> +	printf "create $prefix/%s $C\n" $before |
> +	git update-ref --stdin &&
> +	git for-each-ref $prefix >unchanged &&
> +	if $pack
> +	then
> +		git pack-refs --all
> +	fi &&
> +	printf "create $prefix/%s $C\n" $create >input &&
> +	test_must_fail git update-ref --stdin <input 2>output.err &&
> +	grep -F "$error" output.err &&

I am not 100% confident that "grep -F" is the right thing to use
here.  I checked all the error message these tests are expecting and
none seems to go through _(), but is it fundamental that these
errors should not be translated?

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Michael Haggerty]

On 05/11/2015 09:37 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> +test_update_rejected () {
>> +	prefix="$1" &&
>> +	before="$2" &&
>> +	pack="$3" &&
>> +	create="$4" &&
>> +	error="$5" &&
>> +	printf "create $prefix/%s $C\n" $before |
>> +	git update-ref --stdin &&
>> +	git for-each-ref $prefix >unchanged &&
>> +	if $pack
>> +	then
>> +		git pack-refs --all
>> +	fi &&
>> +	printf "create $prefix/%s $C\n" $create >input &&
>> +	test_must_fail git update-ref --stdin <input 2>output.err &&
>> +	grep -F "$error" output.err &&
> 
> I am not 100% confident that "grep -F" is the right thing to use
> here.  I checked all the error message these tests are expecting and
> none seems to go through _(), but is it fundamental that these
> errors should not be translated?

Aren't tests run in the "C" locale so that tests don't have to worry
about i18n?

Regarding "grep -F", what would be a better alternative, given that I
want to test that the conflict is being reported correctly?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Michael Haggerty]

On 05/11/2015 08:52 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>  t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 107 insertions(+)
>>  create mode 100755 t/t1404-update-ref-df-conflicts.sh
>>
>> diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
>> new file mode 100755
>> index 0000000..ed1b93e
>> --- /dev/null
>> +++ b/t/t1404-update-ref-df-conflicts.sh
>> @@ -0,1 +1,107 @@
> 
> Where does this "-0,1" come from???

Good question. I had forgotten to insert the space in

        test_update_rejected () {
                            ^ space was missing when I ran format-patch

so I edited the email file in emacs. Emacs detected that the file
contained a patch, so when I edited the hunk, it tried to adjust the
hunk line numbers to keep them in agreement with the patch. Obviously,
it screwed up. So the incorrect line numbers were caused by a bug in
emacs diff-mode.

Sorry for the confusion.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Junio C Hamano]

On Tue, May 12, 2015 at 1:32 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/11/2015 09:37 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> ...
>>> +    test_must_fail git update-ref --stdin <input 2>output.err &&
>>> +    grep -F "$error" output.err &&
>>
>> I am not 100% confident that "grep -F" is the right thing to use
>> here.  I checked all the error message these tests are expecting and
>> none seems to go through _(), but is it fundamental that these
>> errors should not be translated?
>
> Aren't tests run in the "C" locale so that tests don't have to worry
> about i18n?

But there is a i18n markings test, for which test-i18ngrep was invented for.

Re: [PATCH v2 02/18] is_refname_available(): revamp the comments

[Junio C Hamano]

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

> Change the comments to a running example of running the function with
> refname set to "refs/foo/bar". Add some more explanation of the logic.

I initially had a hard time understanding what these two "running"s
were before reading the changes.  They all look good.

Thanks.


> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 69 +++++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 22 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 47e4e53..776bbce 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -876,9 +876,9 @@ static void report_refname_conflict(struct ref_entry *entry,
>   * operation).
>   *
>   * Two reference names conflict if one of them exactly matches the
> - * leading components of the other; e.g., "foo/bar" conflicts with
> - * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
> - * "foo/barbados".
> + * leading components of the other; e.g., "refs/foo/bar" conflicts
> + * with both "refs/foo" and with "refs/foo/bar/baz" but not with
> + * "refs/foo/bar" or "refs/foo/barbados".
>   *
>   * skip must be sorted.
>   */
> @@ -891,19 +891,39 @@ static int is_refname_available(const char *refname,
>  	int pos;
>  	char *dirname;
>  
> +	/*
> +	 * For the sake of comments in this function, suppose that
> +	 * refname is "refs/foo/bar".
> +	 */
> +
>  	for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
>  		/*
> -		 * We are still at a leading dir of the refname; we are
> -		 * looking for a conflict with a leaf entry.
> -		 *
> -		 * If we find one, we still must make sure it is
> -		 * not in "skip".
> +		 * We are still at a leading dir of the refname (e.g.,
> +		 * "refs/foo"; if there is a reference with that name,
> +		 * it is a conflict, *unless* it is in skip.
>  		 */
>  		pos = search_ref_dir(dir, refname, slash - refname);
>  		if (pos >= 0) {
> +			/*
> +			 * We found a reference whose name is a proper
> +			 * prefix of refname; e.g., "refs/foo".
> +			 */
>  			struct ref_entry *entry = dir->entries[pos];
> -			if (entry_matches(entry, skip))
> +			if (entry_matches(entry, skip)) {
> +				/*
> +				 * The reference we just found, e.g.,
> +				 * "refs/foo", is also in skip, so it
> +				 * is not considered a conflict.
> +				 * Moreover, the fact that "refs/foo"
> +				 * exists means that there cannot be
> +				 * any references anywhere under the
> +				 * "refs/foo/" namespace (because they
> +				 * would have conflicted with
> +				 * "refs/foo"). So we can stop looking
> +				 * now and return true.
> +				 */
>  				return 1;
> +			}
>  			report_refname_conflict(entry, refname);
>  			return 0;
>  		}
> @@ -911,19 +931,29 @@ static int is_refname_available(const char *refname,
>  
>  		/*
>  		 * Otherwise, we can try to continue our search with
> -		 * the next component; if we come up empty, we know
> -		 * there is nothing under this whole prefix.
> +		 * the next component. So try to look up the
> +		 * directory, e.g., "refs/foo/".
>  		 */
>  		pos = search_ref_dir(dir, refname, slash + 1 - refname);
> -		if (pos < 0)
> +		if (pos < 0) {
> +			/*
> +			 * There was no directory "refs/foo/", so
> +			 * there is nothing under this whole prefix,
> +			 * and we are OK.
> +			 */
>  			return 1;
> +		}
>  
>  		dir = get_ref_dir(dir->entries[pos]);
>  	}
>  
>  	/*
> -	 * We are at the leaf of our refname; we want to
> -	 * make sure there are no directories which match it.
> +	 * We are at the leaf of our refname (e.g., "refs/foo/bar").
> +	 * There is no point in searching for a reference with that
> +	 * name, because a refname isn't considered to conflict with
> +	 * itself. But we still need to check for references whose
> +	 * names are in the "refs/foo/bar/" namespace, because they
> +	 * *do* conflict.
>  	 */
>  	len = strlen(refname);
>  	dirname = xmallocz(len + 1);
> @@ -933,9 +963,9 @@ static int is_refname_available(const char *refname,
>  
>  	if (pos >= 0) {
>  		/*
> -		 * We found a directory named "refname". It is a
> -		 * problem iff it contains any ref that is not
> -		 * in "skip".
> +		 * We found a directory named "$refname/" (e.g.,
> +		 * "refs/foo/bar/"). It is a problem iff it contains
> +		 * any ref that is not in "skip".
>  		 */
>  		struct ref_entry *entry = dir->entries[pos];
>  		struct ref_dir *dir = get_ref_dir(entry);
> @@ -950,11 +980,6 @@ static int is_refname_available(const char *refname,
>  		return 0;
>  	}
>  
> -	/*
> -	 * There is no point in searching for another leaf
> -	 * node which matches it; such an entry would be the
> -	 * ref we are looking for, not a conflict.
> -	 */
>  	return 1;
>  }

Re: [PATCH v2 03/18] is_refname_available(): avoid shadowing "dir" variable

[Junio C Hamano]

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

> The function had a "dir" parameter that was shadowed by a local "dir"
> variable within a code block. Use the former in place of the latter.
> (This is consistent with "dir"'s use elsewhere in the function.)
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 776bbce..9d87e84 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -967,10 +967,10 @@ static int is_refname_available(const char *refname,
>  		 * "refs/foo/bar/"). It is a problem iff it contains
>  		 * any ref that is not in "skip".
>  		 */
> -		struct ref_entry *entry = dir->entries[pos];
> -		struct ref_dir *dir = get_ref_dir(entry);
>  		struct nonmatching_ref_data data;

Wow, the original is a tricky code, but that helps us be confident
that this change is not breaking anything by "unmasking" the value
of incoming "dir" ;-)

> +		struct ref_entry *entry = dir->entries[pos];
>  
> +		dir = get_ref_dir(entry);
>  		data.skip = skip;
>  		sort_ref_dir(dir);
>  		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))

Re: [PATCH v2 06/18] report_refname_conflict(): inline function

[Junio C Hamano]

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

> It wasn't pulling its weight. And we are about to need code similar to
> this where no ref_entry is available and with more diverse error
> messages. Rather than try to generalize the function, just inline it.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

I am not sure "not pulling its weight" in this and also 05/18 is a
fair judgement.  A short helper function sometimes helps readability
by giving an extra abstraction.

But I agree 100% with these two steps that they weren't making them
easier to read.  A raw call to error() shows clearly that we are
reporting an error and more importantly by losing an intermediary
we do not have to wonder at the call site if there is anything else
going on in that "report" function (where there isn't).

Thanks.


>  refs.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 6bdd34f..7422594 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -857,12 +857,6 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
>  	return 1;
>  }
>  
> -static void report_refname_conflict(struct ref_entry *entry,
> -				    const char *refname)
> -{
> -	error("'%s' exists; cannot create '%s'", entry->name, refname);
> -}
> -
>  /*
>   * Return true iff a reference named refname could be created without
>   * conflicting with the name of an existing reference in dir.  If
> @@ -918,7 +912,7 @@ static int is_refname_available(const char *refname,
>  				 */
>  				return 1;
>  			}
> -			report_refname_conflict(entry, refname);
> +			error("'%s' exists; cannot create '%s'", entry->name, refname);
>  			return 0;
>  		}
>  
> @@ -969,7 +963,7 @@ static int is_refname_available(const char *refname,
>  		if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))
>  			return 1;
>  
> -		report_refname_conflict(data.found, refname);
> +		error("'%s' exists; cannot create '%s'", data.found->name, refname);
>  		return 0;
>  	}

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Michael Haggerty]

On 05/12/2015 05:45 PM, Junio C Hamano wrote:
> On Tue, May 12, 2015 at 1:32 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 05/11/2015 09:37 PM, Junio C Hamano wrote:
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>> ...
>>>> +    test_must_fail git update-ref --stdin <input 2>output.err &&
>>>> +    grep -F "$error" output.err &&
>>>
>>> I am not 100% confident that "grep -F" is the right thing to use
>>> here.  I checked all the error message these tests are expecting and
>>> none seems to go through _(), but is it fundamental that these
>>> errors should not be translated?
>>
>> Aren't tests run in the "C" locale so that tests don't have to worry
>> about i18n?
> 
> But there is a i18n markings test, for which test-i18ngrep was invented for.

Thanks for the info. I wasn't aware of that facility.

So if I understand correctly, s/grep/test_i18ngrep/ will address your
concern? That's fine with me.

Meanwhile, I realized that verify_lock() also writes errors directly to
stderr rather than storing them to a strbuf. I have a few patches that
fix this [1], which would thematically fit well on the end of this patch
series. But maybe this patch series is too big already?

In any case I'll be away for a 4-day weekend so it will have to wait
until Monday.

Michael

[1] They are available as branch "verify-lock-strbuf-err" on

    https://github.com/mhagger/git

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Junio C Hamano]

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

>> But there is a i18n markings test, for which test-i18ngrep was invented for.
>
> Thanks for the info. I wasn't aware of that facility.
>
> So if I understand correctly, s/grep/test_i18ngrep/ will address your
> concern? That's fine with me.

Thinking about it again, should these messages ever be translated,
or are they plumbing messages that should never get translated?

If the latter, then 'grep' is the right thing to do; in fact, it
would be a bug in the test if we used test_i18ngrep.

	Side note: besides, I think gettext-poison tests have bitrot
	and are no longer very useful (if they ever were, that is).

> In any case I'll be away for a 4-day weekend so it will have to wait
> until Monday.

Enjoy your long weekend ;-)

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Michael Haggerty]

On 05/14/2015 07:00 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> But there is a i18n markings test, for which test-i18ngrep was invented for.
>>
>> Thanks for the info. I wasn't aware of that facility.
>>
>> So if I understand correctly, s/grep/test_i18ngrep/ will address your
>> concern? That's fine with me.
> 
> Thinking about it again, should these messages ever be translated,
> or are they plumbing messages that should never get translated?
> 
> If the latter, then 'grep' is the right thing to do; in fact, it
> would be a bug in the test if we used test_i18ngrep.
> 
> 	Side note: besides, I think gettext-poison tests have bitrot
> 	and are no longer very useful (if they ever were, that is).

I don't really know how to decide which messages should be translatable
and which not.

Obviously 'update-ref' is a plumbing command, but these same error
messages can be emitted by any reference transaction, such as when a
push fails, in which case they are visible to the user.

Some other errors that can result from ref transaction failures are
tested in t1400 with 'grep'.

I think using 'grep' is OK for now, and if they are internationalized in
the future the breakage will be pretty obvious and straightforward to fix.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions

[Junio C Hamano]

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

> I think using 'grep' is OK for now, and if they are internationalized in
> the future the breakage will be pretty obvious and straightforward to fix.

Yeah, I think so, too.

Thanks.