From 49e818762aaf08791504d4de2076027e7d275844 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:04 +0200
Subject: is_refname_available(): revamp the comments

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 47e4e5380a..776bbcebd6 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;
 }
 
-- 
cgit v1.2.3


From 9ef6eaa287b12ff9469c7775507f107b86f9a8c8 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:05 +0200
Subject: is_refname_available(): avoid shadowing "dir" variable

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 776bbcebd6..9d87e84f42 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))
-- 
cgit v1.2.3


From 6075f3076e8a7fea0a132d459d1e21e771cdd880 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:06 +0200
Subject: is_refname_available(): convert local variable "dirname" to strbuf

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 9d87e84f42..faabd685c4 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) {
 		/*
-- 
cgit v1.2.3


From 8bfac19ab4844a63e2f660fcaf4fec4fbe08e47f Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:07 +0200
Subject: entry_matches(): inline function

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index faabd685c4..6bdd34fc72 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
-- 
cgit v1.2.3


From 385e8af5a2d1056b75428eea810bce65d2c83363 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:08 +0200
Subject: report_refname_conflict(): inline function

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 6bdd34fc72..74225943b6 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;
 	}
 
-- 
cgit v1.2.3


From 521331cc9f71515e214d0821a35cafd5c03eaad3 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:09 +0200
Subject: struct nonmatching_ref_data: store a refname instead of a ref_entry

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 74225943b6..effd91a2ed 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;
 	}
 
-- 
cgit v1.2.3


From 61da59699263afcbf8f1e3c66763237fe35ba670 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:10 +0200
Subject: is_refname_available(): use dirname in first loop

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index effd91a2ed..8316bb102a 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 {
-- 
cgit v1.2.3


From 07f9c881d6acef35b7dc8e5c783e63b1cac71084 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:11 +0200
Subject: ref_transaction_commit(): use a string_list for detecting duplicates

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 8316bb102a..6bb65abb31 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;
 }
 
-- 
cgit v1.2.3


From e911104c84f2efd64aded6831bd7e6625c8a0fab Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:12 +0200
Subject: refs: check for D/F conflicts among refs created in a transaction

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 +++++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 94 insertions(+), 62 deletions(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 6bb65abb31..0fa70fb5fb 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)) {
-- 
cgit v1.2.3


From 5baf37d383ff80e913da1cc99a325a9baadc844c Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:13 +0200
Subject: verify_refname_available(): rename function

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 0fa70fb5fb..703e93dec9 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;
 }
-- 
cgit v1.2.3


From 1146f17e2ce6054b502a46bafc102312c60a8380 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:14 +0200
Subject: verify_refname_available(): report errors via a "struct strbuf *err"

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 703e93dec9..43e7bdd896 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;
 }
 
-- 
cgit v1.2.3


From 4a32b2e08be8daf965949956e8ea16718797a031 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:15 +0200
Subject: lock_ref_sha1_basic(): report errors via a "struct strbuf *err"

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 43e7bdd896..ff9b9cf92a 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;
-- 
cgit v1.2.3


From 5b2d8d6f2184381b76c13504a2f5ec8a62cd584e Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:16 +0200
Subject: lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts

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 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index ff9b9cf92a..ce993bd125 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;
 	}
 	/*
-- 
cgit v1.2.3


From abeef9c85657fddf98f01c8479f1437719e95864 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:17 +0200
Subject: rename_ref(): integrate lock_ref_sha1_basic() errors into ours

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index ce993bd125..87c1ad161d 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;
 	}
 
-- 
cgit v1.2.3


From cbaabcbc6fa9568269d6baeccb0a621cd0413c6c Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:18 +0200
Subject: ref_transaction_commit(): provide better error messages

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index 87c1ad161d..ecaf80499e 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;
 		}
 	}
-- 
cgit v1.2.3


From 3553944aa8ca05fef7c24c5aa98c7de4bb3167d5 Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:19 +0200
Subject: ref_transaction_commit(): delete extra "the" from error message

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 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index ecaf80499e..bc4b1ab8b7 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;
-- 
cgit v1.2.3


From c628edfddbf37eefab810a3107a2c32b45abaedc Mon Sep 17 00:00:00 2001
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 11 May 2015 17:25:20 +0200
Subject: reflog_expire(): integrate lock_ref_sha1_basic() errors into ours

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(-)

(limited to 'refs.c')

diff --git a/refs.c b/refs.c
index bc4b1ab8b7..97043fd2ef 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);
-- 
cgit v1.2.3