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

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

[Michael Haggerty]

We currently don't handle D/F conflicts very well.

A "D/F conflict" is what we call a conflict between references like
"refs/foo" and "refs/foo/bar". Such references cannot coexist because,
when stored as loose references, "refs/foo" would have to be both a
directory and a file.

The problems fixed by this series:

* We had no tests of D/F conflict handling within transactions.

* D/F conflicts between references being created in a single
  transaction used to be detected too late, possibly after part of the
  transaction had already been committed.

* D/F errors against loose references were typically reported as
  locking errors, which was not very illuminating. Also, D/F errors
  were typically reported in two separate error messages, like

      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'.

  which was confusing.

There is probably still room for improving the error messages.

This patch series applies on top of
mh/ref-lock-avoid-running-out-of-fds. I did it that way because I
expected significant conflicts between the series, and the older
series is simple/mature enough that I expect it to be merged early in
the post-2.4 cycle. But in retrospect it turns out that there are only
minor conflicts between the two series. So if you would like me to
rebase this series to another starting point, please let me know.

This series is also available from my GitHub account,

    https://github.com/mhagger/git branch check-df-conflicts-earlier

Michael Haggerty (18):
  t1404: new tests of D/F conflicts within ref transactions
  is_refname_available(): explain the reason for an early exit
  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 processed 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 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                             | 280 +++++++++++++++++++++++--------------
 t/t1400-update-ref.sh              |  14 +-
 t/t1404-update-ref-df-conflicts.sh | 107 ++++++++++++++
 3 files changed, 286 insertions(+), 115 deletions(-)
 create mode 100755 t/t1404-update-ref-df-conflicts.sh

-- 
2.1.4

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

[Michael Haggerty]

Add some tests of D/F conflicts (as in, the type of directory vs. file
conflict that exists between references "refs/foo" and "refs/foo/bar")
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..2fc2ac6
--- /dev/null
+++ b/t/t1404-update-ref-df-conflicts.sh
@@ -0,0 +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

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

[Eric Sunshine]

On Fri, May 1, 2015 at 8:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Add some tests of D/F conflicts (as in, the type of directory vs. file
> conflict that exists between references "refs/foo" and "refs/foo/bar")
> in the context of reference transactions.
> [...]
> 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..2fc2ac6
> --- /dev/null
> +++ b/t/t1404-update-ref-df-conflicts.sh
> @@ -0,0 +1,107 @@
> +#!/bin/sh
> +
> +test_description='Test git update-ref with D/F conflicts'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +
> +       git commit --allow-empty -m Initial

Broken &&-chain.

> +       C=$(git rev-parse HEAD)
> +
> +'

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

[Michael Haggerty]

On 05/05/2015 07:12 AM, Eric Sunshine wrote:
> On Fri, May 1, 2015 at 8:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Add some tests of D/F conflicts (as in, the type of directory vs. file
>> conflict that exists between references "refs/foo" and "refs/foo/bar")
>> in the context of reference transactions.
>> [...]
>> 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..2fc2ac6
>> --- /dev/null
>> +++ b/t/t1404-update-ref-df-conflicts.sh
>> @@ -0,0 +1,107 @@
>> +#!/bin/sh
>> +
>> +test_description='Test git update-ref with D/F conflicts'
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup' '
>> +
>> +       git commit --allow-empty -m Initial
> 
> Broken &&-chain.

Thanks, Eric. I'll fix it in v2.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 02/18] is_refname_available(): explain the reason for an early exit

[Michael Haggerty]

The reason why we can exit early if we find a reference in skip whose
name is a prefix of refname is a bit subtle, so explain it in a
comment.

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

diff --git a/refs.c b/refs.c
index 2bdd93c..ab438a5 100644
--- a/refs.c
+++ b/refs.c
@@ -907,8 +907,20 @@ static int is_refname_available(const char *refname,
 		pos = search_ref_dir(dir, refname, slash - refname);
 		if (pos >= 0) {
 			struct ref_entry *entry = dir->entries[pos];
-			if (entry_matches(entry, skip))
+			if (entry_matches(entry, skip)) {
+				/*
+				 * The fact that entry is a ref whose
+				 * name is a prefix of refname means
+				 * that there cannot be any other ref
+				 * whose name starts with that prefix
+				 * (because it would have been a D/F
+				 * conflict with entry). So, since we
+				 * don't care about entry (because it
+				 * is in skip), we can stop looking
+				 * now and return true.
+				 */
 				return 1;
+			}
 			report_refname_conflict(entry, refname);
 			return 0;
 		}
-- 
2.1.4

Re: [PATCH 02/18] is_refname_available(): explain the reason for an early exit

[Stefan Beller]

On Fri, May 1, 2015 at 5:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> The reason why we can exit early if we find a reference in skip whose
> name is a prefix of refname is a bit subtle, so explain it in a
> comment.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 2bdd93c..ab438a5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -907,8 +907,20 @@ static int is_refname_available(const char *refname,
>                 pos = search_ref_dir(dir, refname, slash - refname);
>                 if (pos >= 0) {
>                         struct ref_entry *entry = dir->entries[pos];
> -                       if (entry_matches(entry, skip))
> +                       if (entry_matches(entry, skip)) {
> +                               /*
> +                                * The fact that entry is a ref whose
> +                                * name is a prefix of refname means
> +                                * that there cannot be any other ref
> +                                * whose name starts with that prefix
> +                                * (because it would have been a D/F
> +                                * conflict with entry). So, since we
> +                                * don't care about entry (because it
> +                                * is in skip), we can stop looking
> +                                * now and return true.

At first I thought this is not true, what about:
refs/heads/foo
refs/heads/foobar
They go well together and one is a prefix of the other.
What is crucial is the existence of a '/' just between the
prefix and the ending part, so
refs/heads/foo/bar doesn't fly here.

The assumption may be the case if the prefix itself always
ends with a /, which is probably the case here?
I don't know if that is worth noting as well.

> +                                */
>                                 return 1;
> +                       }
>                         report_refname_conflict(entry, refname);
>                         return 0;
>                 }
> --
> 2.1.4
>

Re: [PATCH 02/18] is_refname_available(): explain the reason for an early exit

[Michael Haggerty]

On 05/01/2015 07:21 PM, Stefan Beller wrote:
> On Fri, May 1, 2015 at 5:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> The reason why we can exit early if we find a reference in skip whose
>> name is a prefix of refname is a bit subtle, so explain it in a
>> comment.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 2bdd93c..ab438a5 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -907,8 +907,20 @@ static int is_refname_available(const char *refname,
>>                 pos = search_ref_dir(dir, refname, slash - refname);
>>                 if (pos >= 0) {
>>                         struct ref_entry *entry = dir->entries[pos];
>> -                       if (entry_matches(entry, skip))
>> +                       if (entry_matches(entry, skip)) {
>> +                               /*
>> +                                * The fact that entry is a ref whose
>> +                                * name is a prefix of refname means
>> +                                * that there cannot be any other ref
>> +                                * whose name starts with that prefix
>> +                                * (because it would have been a D/F
>> +                                * conflict with entry). So, since we
>> +                                * don't care about entry (because it
>> +                                * is in skip), we can stop looking
>> +                                * now and return true.
> 
> At first I thought this is not true, what about:
> refs/heads/foo
> refs/heads/foobar
> They go well together and one is a prefix of the other.
> What is crucial is the existence of a '/' just between the
> prefix and the ending part, so
> refs/heads/foo/bar doesn't fly here.
> 
> The assumption may be the case if the prefix itself always
> ends with a /, which is probably the case here?
> I don't know if that is worth noting as well.

Yes, this is all rather subtle. Here we have found a reference whose
name is *exactly* a proper prefix of refname; for example, refname is
"refs/foo/bar" and we just found "refs/foo". But "refs/foo" is in skip,
so it is not a conflict. Moreover, its existence means that there cannot
be any other references in the "refs/foo/*" namespace (e.g.,
"refs/foo/bar/banana") because such a reference would have conflicted
with "refs/foo".

If it's any consolation, this logic will be simplified a bit later in
the patch series. Nevertheless, I will try to explain this better in v2.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 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 ab438a5..09d753f 100644
--- a/refs.c
+++ b/refs.c
@@ -954,10 +954,10 @@ static int is_refname_available(const char *refname,
 		 * 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 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 09d753f..2ef1c5e 100644
--- a/refs.c
+++ b/refs.c
@@ -892,9 +892,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 (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
 		/*
@@ -942,11 +941,10 @@ static int is_refname_available(const char *refname,
 	 * We are at the leaf of our refname; we want to
 	 * make sure there are no directories which match it.
 	 */
-	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 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 2ef1c5e..e9c9146 100644
--- a/refs.c
+++ b/refs.c
@@ -846,11 +846,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;
@@ -860,7 +855,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;
@@ -906,7 +901,7 @@ static int is_refname_available(const char *refname,
 		pos = search_ref_dir(dir, refname, slash - refname);
 		if (pos >= 0) {
 			struct ref_entry *entry = dir->entries[pos];
-			if (entry_matches(entry, skip)) {
+			if (skip && string_list_has_string(skip, entry->name)) {
 				/*
 				 * The fact that entry is a ref whose
 				 * name is a prefix of refname means
-- 
2.1.4

[PATCH 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 e9c9146..f1da87b 100644
--- a/refs.c
+++ b/refs.c
@@ -862,12 +862,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
@@ -915,7 +909,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;
 		}
 
@@ -956,7 +950,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 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 f1da87b..0ec3f0a 100644
--- a/refs.c
+++ b/refs.c
@@ -848,7 +848,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)
@@ -858,7 +858,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;
 }
 
@@ -950,7 +950,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 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 | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/refs.c b/refs.c
index 0ec3f0a..8e929c7 100644
--- a/refs.c
+++ b/refs.c
@@ -883,8 +883,13 @@ static int is_refname_available(const char *refname,
 	const char *slash;
 	int pos;
 	struct strbuf dirname = STRBUF_INIT;
+	int ret = 0;
 
+	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; we are
 		 * looking for a conflict with a leaf entry.
@@ -892,10 +897,9 @@ static int is_refname_available(const char *refname,
 		 * If we find one, we still must make sure it is
 		 * not in "skip".
 		 */
-		pos = search_ref_dir(dir, refname, slash - refname);
+		pos = search_ref_dir(dir, dirname.buf, dirname.len);
 		if (pos >= 0) {
-			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 fact that entry is a ref whose
 				 * name is a prefix of refname means
@@ -907,10 +911,11 @@ static int is_refname_available(const char *refname,
 				 * is in skip), 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;
 		}
 
 
@@ -919,9 +924,12 @@ static int is_refname_available(const char *refname,
 		 * the next component; if we come up empty, we know
 		 * there is nothing under this whole prefix.
 		 */
-		pos = search_ref_dir(dir, refname, slash + 1 - refname);
-		if (pos < 0)
-			return 1;
+		strbuf_addch(&dirname, '/');
+		pos = search_ref_dir(dir, dirname.buf, dirname.len);
+		if (pos < 0) {
+			ret = 1;
+			goto cleanup;
+		}
 
 		dir = get_ref_dir(dir->entries[pos]);
 	}
@@ -930,10 +938,9 @@ static int is_refname_available(const char *refname,
 	 * We are at the leaf of our refname; we want to
 	 * make sure there are no directories which match it.
 	 */
-	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) {
 		/*
@@ -947,12 +954,14 @@ 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;
 	}
 
 	/*
@@ -960,7 +969,11 @@ static int is_refname_available(const char *refname,
 	 * node which matches it; such an entry would be the
 	 * ref we are looking for, not a conflict.
 	 */
-	return 1;
+	ret = 1;
+
+cleanup:
+	strbuf_release(&dirname);
+	return ret;
 }
 
 struct packed_ref_cache {
-- 
2.1.4

[PATCH 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 8e929c7..fca3ef1 100644
--- a/refs.c
+++ b/refs.c
@@ -3718,25 +3718,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;
@@ -3750,6 +3743,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);
 
@@ -3761,9 +3755,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;
 	}
@@ -3879,6 +3875,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 10/18] refs: check for D/F conflicts among refs processed 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, 90 insertions(+), 68 deletions(-)

diff --git a/refs.c b/refs.c
index fca3ef1..81fbb33 100644
--- a/refs.c
+++ b/refs.c
@@ -864,19 +864,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., "foo/bar" conflicts with
  * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
  * "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)
 {
@@ -891,84 +894,101 @@ static int is_refname_available(const char *refname,
 		strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);
 
 		/*
-		 * We are still at a leading dir of the refname; we are
-		 * looking for a conflict with a leaf entry.
+		 * We are still at a leading dir of refname; look for
+		 * a conflict with a leaf entry.
 		 *
 		 * If we find one, we still must make sure it is
 		 * not in "skip".
 		 */
-		pos = search_ref_dir(dir, dirname.buf, dirname.len);
-		if (pos >= 0) {
-			if (skip && string_list_has_string(skip, dirname.buf)) {
-				/*
-				 * The fact that entry is a ref whose
-				 * name is a prefix of refname means
-				 * that there cannot be any other ref
-				 * whose name starts with that prefix
-				 * (because it would have been a D/F
-				 * conflict with entry). So, since we
-				 * don't care about entry (because it
-				 * is in skip), we can stop looking
-				 * now and return true.
-				 */
-				ret = 1;
+		if (dir) {
+			pos = search_ref_dir(dir, dirname.buf, dirname.len);
+			if (pos >= 0 &&
+			    (!skip || !string_list_has_string(skip, dirname.buf))) {
+				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; if we come up empty, we know
-		 * there is nothing under this whole prefix.
+		 * the next component. 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) {
-			ret = 1;
-			goto cleanup;
+		if (dir) {
+			pos = search_ref_dir(dir, dirname.buf, dirname.len);
+			dir = (pos < 0) ? NULL : get_ref_dir(dir->entries[pos]);
 		}
-
-		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; we want to make sure
+	 * that there are no directories that match it. (There is no
+	 * point in searching for another leaf node that matches it;
+	 * such an entry would be the ref we are looking for, which is
+	 * not a conflict.)
 	 */
 	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". It is
+			 * a problem if 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". 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;
+			}
+		}
 	}
 
-	/*
-	 * 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.
-	 */
+	/* No conflicts were found */
 	ret = 1;
 
 cleanup:
@@ -2289,6 +2309,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)
 {
@@ -2343,7 +2364,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;
 	}
@@ -2783,8 +2804,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;
 }
@@ -2844,7 +2865,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;
@@ -2859,7 +2880,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;
@@ -3766,9 +3787,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/*
 	 * Acquire all locks, verify old values if provided, check
-	 * that new values are valid, and write new values to the
-	 * lockfiles, ready to be activated. Only keep one lockfile
-	 * open at a time to avoid running out of file descriptors.
+	 * that new values are valid, check for D/F conflicts, and
+	 * write new values to the lockfiles, ready to be activated.
+	 * Only keep one lockfile open at a time to avoid running out
+	 * of file descriptors.
 	 */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
@@ -3779,7 +3801,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				update->refname,
 				((update->flags & REF_HAVE_OLD) ?
 				 update->old_sha1 : NULL),
-				NULL,
+				&affected_refnames, NULL,
 				update->flags,
 				&update->type);
 		if (!update->lock) {
@@ -4076,7 +4098,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 2fc2ac6..136fb48 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 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 81fbb33..cd8aeed 100644
--- a/refs.c
+++ b/refs.c
@@ -268,7 +268,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
@@ -863,13 +863,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., "foo/bar" conflicts with
@@ -878,15 +879,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;
 
 	strbuf_grow(&dirname, strlen(refname) + 1);
 	for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
@@ -989,7 +990,7 @@ static int is_refname_available(const char *refname,
 	}
 
 	/* No conflicts were found */
-	ret = 1;
+	ret = 0;
 
 cleanup:
 	strbuf_release(&dirname);
@@ -2364,7 +2365,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;
 	}
@@ -2804,8 +2805,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 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 cd8aeed..730e248 100644
--- a/refs.c
+++ b/refs.c
@@ -865,12 +865,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., "foo/bar" conflicts with
@@ -882,13 +882,16 @@ 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;
 	struct strbuf dirname = STRBUF_INIT;
 	int ret = -1;
 
+	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: */
@@ -905,16 +908,16 @@ static int verify_refname_available(const char *refname,
 			pos = search_ref_dir(dir, dirname.buf, dirname.len);
 			if (pos >= 0 &&
 			    (!skip || !string_list_has_string(skip, dirname.buf))) {
-				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;
 		}
 
@@ -958,8 +961,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;
 			}
 		}
@@ -982,8 +985,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;
 			}
 		}
@@ -2322,6 +2325,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));
 
@@ -2365,7 +2369,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;
 	}
@@ -2405,10 +2411,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;
 		}
 	}
@@ -2416,6 +2420,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;
 }
@@ -2802,14 +2807,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 13/18] lock_ref_sha1_basic(): report errors via a "struct strbuf *err"

[Michael Haggerty]

For now, change the callers spew the error to stderr like before. But
eventually, they should 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 730e248..1b466bc 100644
--- a/refs.c
+++ b/refs.c
@@ -2315,7 +2315,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;
@@ -2325,7 +2326,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));
 
@@ -2348,7 +2350,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,
@@ -2358,8 +2361,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;
 	}
 	/*
@@ -2370,8 +2373,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;
 	}
@@ -2397,7 +2399,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;
 	}
 
@@ -2411,8 +2413,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;
 		}
 	}
@@ -2420,7 +2421,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;
 }
@@ -2836,6 +2836,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);
@@ -2878,8 +2879,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;
 	}
@@ -2893,8 +2896,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;
 	}
@@ -3816,11 +3821,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				 update->old_sha1 : NULL),
 				&affected_refnames, NULL,
 				update->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;
@@ -4100,6 +4108,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;
@@ -4111,9 +4120,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 14/18] lock_ref_sha1_basic(): improve diagnostics for D/F conflicts

[Michael Haggerty]

If there is a failure to lock that is likely caused by a D/F conflict,
invoke verify_refname_available() to try to generate a more helpful
error message. If that function doesn't detect an error (which could
happen if, for example, some non-reference file is blocking the
deletion of an otherwise-empty directory tree or if there was a race
with another process that just deleted the offending reference), then
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 1b466bc..cdbd31a 100644
--- a/refs.c
+++ b/refs.c
@@ -2350,8 +2350,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,
@@ -2361,8 +2365,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 136fb48..11b6d5b 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 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 cdbd31a..6077e83 100644
--- a/refs.c
+++ b/refs.c
@@ -2889,9 +2889,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);
@@ -2906,9 +2905,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 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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 6077e83..2d0d8bd 100644
--- a/refs.c
+++ b/refs.c
@@ -3830,13 +3830,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				&update->type,
 				err);
 		if (!update->lock) {
+			const 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);
 			goto cleanup;
 		}
 		if ((update->flags & REF_HAVE_NEW) && !(update->flags & REF_DELETING)) {
-- 
2.1.4

[PATCH 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 2d0d8bd..f64f6ae 100644
--- a/refs.c
+++ b/refs.c
@@ -3836,7 +3836,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);
 			goto cleanup;
 		}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 636d3a1..ba89f4c 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 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 f64f6ae..0bff903 100644
--- a/refs.c
+++ b/refs.c
@@ -4129,9 +4129,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 00/18] Improve handling of D/F conflicts

[Junio C Hamano]

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

> We currently don't handle D/F conflicts very well.
>
> A "D/F conflict" is what we call a conflict between references like
> "refs/foo" and "refs/foo/bar".

Could you phrase that slightly differently, so that readers can tell
between the usual D/F conflict that is between directory and file in
the tracked contents (i.e. conflict between the branches being
merged, killing of a directory necessary when checking out a file,
etc.) and this thing?  They are similar in nature, but "D/F
conflict" has been used to refer to the clashes that happen to the
user contents, not refnames.  Starting a paragraph with "... don't
handle D/F conflicts in refnames very well" and then using "D/F
conflict" as a short-hand for "D/F conflict in refnames" throughout
the rest of the message is perfectly fine, as long as the message
never talks about the D/F conflict in the traditional sense.

> * D/F conflicts between references being created in a single
>   transaction used to be detected too late, possibly after part of the
>   transaction had already been committed.
>
> * D/F errors against loose references were typically reported as

Be consistent by s/errors/conflicts/ perhaps?

> This patch series applies on top of
> mh/ref-lock-avoid-running-out-of-fds. I did it that way because I
> expected significant conflicts between the series, and the older
> series is simple/mature enough that I expect it to be merged early in
> the post-2.4 cycle. But in retrospect it turns out that there are only
> minor conflicts between the two series. So if you would like me to
> rebase this series to another starting point, please let me know.

Thanks for being considerate about back-porting necessity.

As I have written, I actually would prefer to see that the other
topic, ref-lock-avoid-running-out-of-fds, to be made applicable to
older maintenance tracks.  So...

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

[Michael Haggerty]

On 05/03/2015 04:09 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> We currently don't handle D/F conflicts very well.
>>
>> A "D/F conflict" is what we call a conflict between references like
>> "refs/foo" and "refs/foo/bar".
> 
> Could you phrase that slightly differently, so that readers can tell
> between the usual D/F conflict that is between directory and file in
> the tracked contents (i.e. conflict between the branches being
> merged, killing of a directory necessary when checking out a file,
> etc.) and this thing?  They are similar in nature, but "D/F
> conflict" has been used to refer to the clashes that happen to the
> user contents, not refnames.  Starting a paragraph with "... don't
> handle D/F conflicts in refnames very well" and then using "D/F
> conflict" as a short-hand for "D/F conflict in refnames" throughout
> the rest of the message is perfectly fine, as long as the message
> never talks about the D/F conflict in the traditional sense.

Good point. I will clarify this.

>> * D/F conflicts between references being created in a single
>>   transaction used to be detected too late, possibly after part of the
>>   transaction had already been committed.
>>
>> * D/F errors against loose references were typically reported as
> 
> Be consistent by s/errors/conflicts/ perhaps?

Yes, thanks.

>> This patch series applies on top of
>> mh/ref-lock-avoid-running-out-of-fds. I did it that way because I
>> expected significant conflicts between the series, and the older
>> series is simple/mature enough that I expect it to be merged early in
>> the post-2.4 cycle. But in retrospect it turns out that there are only
>> minor conflicts between the two series. So if you would like me to
>> rebase this series to another starting point, please let me know.
> 
> Thanks for being considerate about back-porting necessity.
> 
> As I have written, I actually would prefer to see that the other
> topic, ref-lock-avoid-running-out-of-fds, to be made applicable to
> older maintenance tracks.  So...

OK, I will rebase this branch onto "master", and simultaneously check
how much work it would be to implement a version of
ref-lock-avoid-running-out-of-fds on top of 2.3 and/or 2.2.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu