[PATCH 0/3] Fix how for-each-ref handles broken loose references

[PATCH 0/3] Fix how for-each-ref handles broken loose references

[Michael Haggerty]

Currently, "git for-each-ref" fails to handle REF_ISBROKEN references
correctly, treating them instead as valid references that point at
NULL_SHA1. Sometimes this makes for-each-ref emit the wrong error
message; sometimes it even appears to complete successfully.

Instead, emit warnings for broken references but otherwise ignore
them. Also, add special-case code to defend against an actual
NULL_SHA1 in a loose reference file being treated as valid. (We've
seen that problem on our servers before, probably caused by the Ruby
Git client that we used to use.)

This patch series was prepared against "maint" as it might be suitable
for that branch, but it also applies cleanly against "master".

This patch series is also available from my GitHub account [1], as
branch for-each-ref-errors.

[1] https://github.com/mhagger/git

Michael Haggerty (3):
  t6301: new tests of for-each-ref error handling
  for-each-ref: report broken references correctly
  read_loose_refs(): treat NULL_SHA1 loose references as broken

 builtin/for-each-ref.c         |  5 +++++
 refs.c                         |  7 +++++++
 t/t6301-for-each-ref-errors.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100755 t/t6301-for-each-ref-errors.sh

-- 
2.1.4

[PATCH 1/3] t6301: new tests of for-each-ref error handling

[Michael Haggerty]

Add tests that for-each-ref correctly reports broken loose reference
files and references that point at missing objects. In fact, two of
these tests fail, because (1) NULL_SHA1 is not recognized as an
invalid reference value, and (2) for-each-ref doesn't respect
REF_ISBROKEN. Fixes to come.

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

Notes (discussion):
    Note that a reference that points at NULL_SHA1 is reported as "broken"
    rather than "missing". This is because NULL_SHA1 is manifestly bogus,
    whereas we have no systematic basis for rejecting any other
    40-character hexadecimal string without doing an object lookup.

 t/t6301-for-each-ref-errors.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100755 t/t6301-for-each-ref-errors.sh

diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
new file mode 100755
index 0000000..dc68947
--- /dev/null
+++ b/t/t6301-for-each-ref-errors.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='for-each-ref errors for broken refs'
+
+. ./test-lib.sh
+
+ZEROS=0000000000000000000000000000000000000000
+MISSING=abababababababababababababababababababab
+
+test_expect_success setup '
+	git commit --allow-empty -m "Initial" &&
+	git tag testtag &&
+	git for-each-ref >full-list
+'
+
+test_expect_failure 'Broken refs are reported correctly' '
+	r=refs/heads/bogus &&
+	: >.git/$r &&
+	test_when_finished "rm -f .git/$r" &&
+	echo "warning: ignoring broken ref $r" >broken-err &&
+	git for-each-ref >out 2>err &&
+	test_cmp full-list out &&
+	test_cmp broken-err err
+'
+
+test_expect_failure 'NULL_SHA1 refs are reported correctly' '
+	r=refs/heads/zeros &&
+	echo $ZEROS >.git/$r &&
+	test_when_finished "rm -f .git/$r" &&
+	echo "warning: ignoring broken ref $r" >zeros-err &&
+	git for-each-ref >out 2>err &&
+	test_cmp full-list out &&
+	test_cmp zeros-err err
+'
+
+test_expect_success 'Missing objects are reported correctly' '
+	r=refs/heads/missing &&
+	echo $MISSING >.git/$r &&
+	test_when_finished "rm -f .git/$r" &&
+	echo "fatal: missing object $MISSING for $r" >missing-err &&
+	test_must_fail git for-each-ref 2>err &&
+	test_cmp missing-err err
+'
+
+test_done
-- 
2.1.4

Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling

[Jeff King]

On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:

> Add tests that for-each-ref correctly reports broken loose reference
> files and references that point at missing objects. In fact, two of
> these tests fail, because (1) NULL_SHA1 is not recognized as an
> invalid reference value, and (2) for-each-ref doesn't respect
> REF_ISBROKEN. Fixes to come.

This whole series looks straightforward and correct to me. Thanks for a
pleasant read. I have two minor comments on the tests:

> --- /dev/null
> +++ b/t/t6301-for-each-ref-errors.sh
> @@ -0,0 +1,45 @@
> +#!/bin/sh
> +
> +test_description='for-each-ref errors for broken refs'
> +
> +. ./test-lib.sh
> +
> +ZEROS=0000000000000000000000000000000000000000
> +MISSING=abababababababababababababababababababab

The test suite provides $_z40, so you can skip $ZEROS. I don't think
it's a big deal, though, and it may be nicer to have it explicitly next
to $MISSING here.

> +test_expect_success 'Missing objects are reported correctly' '
> +	r=refs/heads/missing &&
> +	echo $MISSING >.git/$r &&
> +	test_when_finished "rm -f .git/$r" &&
> +	echo "fatal: missing object $MISSING for $r" >missing-err &&
> +	test_must_fail git for-each-ref 2>err &&
> +	test_cmp missing-err err
> +'

Due to b7dd2d2 (that you mentioned in the message for patch 2), we only
sometimes notice the missing objects. Is it worth testing that:

  git for-each-ref --format='%(refname)'

does _not_ barf here?

-Peff

Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:
>
>> Add tests that for-each-ref correctly reports broken loose reference
>> files and references that point at missing objects. In fact, two of
>> these tests fail, because (1) NULL_SHA1 is not recognized as an
>> invalid reference value, and (2) for-each-ref doesn't respect
>> REF_ISBROKEN. Fixes to come.
>
> This whole series looks straightforward and correct to me. Thanks for a
> pleasant read. I have two minor comments on the tests:
>
>> --- /dev/null
>> +++ b/t/t6301-for-each-ref-errors.sh
>> @@ -0,0 +1,45 @@
>> +#!/bin/sh
>> +
>> +test_description='for-each-ref errors for broken refs'
>> +
>> +. ./test-lib.sh
>> +
>> +ZEROS=0000000000000000000000000000000000000000
>> +MISSING=abababababababababababababababababababab
>
> The test suite provides $_z40, so you can skip $ZEROS. I don't think
> it's a big deal, though, and it may be nicer to have it explicitly next
> to $MISSING here.

Yeah, ZEROS=$_z40 next to MISSING=abababa... may not be too bad.

>> +test_expect_success 'Missing objects are reported correctly' '
>> +	r=refs/heads/missing &&
>> +	echo $MISSING >.git/$r &&
>> +	test_when_finished "rm -f .git/$r" &&
>> +	echo "fatal: missing object $MISSING for $r" >missing-err &&
>> +	test_must_fail git for-each-ref 2>err &&
>> +	test_cmp missing-err err
>> +'
>
> Due to b7dd2d2 (that you mentioned in the message for patch 2), we only
> sometimes notice the missing objects. Is it worth testing that:
>
>   git for-each-ref --format='%(refname)'
>
> does _not_ barf here?

Sound like a good thing to check, too.

Thanks.

Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling

[Michael Haggerty]

On 06/01/2015 06:08 PM, Jeff King wrote:
> On Mon, Jun 01, 2015 at 05:53:49PM +0200, Michael Haggerty wrote:
> 
>> Add tests that for-each-ref correctly reports broken loose reference
>> files and references that point at missing objects. In fact, two of
>> these tests fail, because (1) NULL_SHA1 is not recognized as an
>> invalid reference value, and (2) for-each-ref doesn't respect
>> REF_ISBROKEN. Fixes to come.
> 
> This whole series looks straightforward and correct to me. Thanks for a
> pleasant read. I have two minor comments on the tests:
> 
>> --- /dev/null
>> +++ b/t/t6301-for-each-ref-errors.sh
>> @@ -0,0 +1,45 @@
>> +#!/bin/sh
>> +
>> +test_description='for-each-ref errors for broken refs'
>> +
>> +. ./test-lib.sh
>> +
>> +ZEROS=0000000000000000000000000000000000000000
>> +MISSING=abababababababababababababababababababab
> 
> The test suite provides $_z40, so you can skip $ZEROS. I don't think
> it's a big deal, though, and it may be nicer to have it explicitly next
> to $MISSING here.

Dang, I knew about that variable but just forgot it. I'll make this change.

>> +test_expect_success 'Missing objects are reported correctly' '
>> +	r=refs/heads/missing &&
>> +	echo $MISSING >.git/$r &&
>> +	test_when_finished "rm -f .git/$r" &&
>> +	echo "fatal: missing object $MISSING for $r" >missing-err &&
>> +	test_must_fail git for-each-ref 2>err &&
>> +	test_cmp missing-err err
>> +'
> 
> Due to b7dd2d2 (that you mentioned in the message for patch 2), we only
> sometimes notice the missing objects. Is it worth testing that:
> 
>   git for-each-ref --format='%(refname)'
> 
> does _not_ barf here?

It makes sense. I will add it, with --format='%(objectname) %(refname)'
for added fun.

Thanks for the review!
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 2/3] for-each-ref: report broken references correctly

[Michael Haggerty]

If there is a loose reference file with invalid contents, "git
for-each-ref" incorrectly reports the problem as being a missing
object with name NULL_SHA1:

    $ echo '12345678' >.git/refs/heads/nonsense
    $ git for-each-ref
    fatal: missing object 0000000000000000000000000000000000000000 for refs/heads/nonsense

With an explicit "--format" string, it can even report that the
reference validly points at NULL_SHA1:

    $ git for-each-ref --format='%(objectname) %(refname)'
    0000000000000000000000000000000000000000 refs/heads/nonsense
    $ echo $?
    0

This has been broken since

    b7dd2d2 for-each-ref: Do not lookup objects when they will not be used (2009-05-27)

, which changed for-each-ref from using for_each_ref() to using
git_for_each_rawref() in order to avoid looking up the referred-to
objects unnecessarily. (When "git for-each-ref" is given a "--format"
string that doesn't include information about the pointed-to object,
it does not look up the object at all, which makes it considerably
faster. Iterating with DO_FOR_EACH_INCLUDE_BROKEN is essential to this
optimization because otherwise for_each_ref() would itself need to
check whether the object exists as part of its brokenness test.)

But for_each_rawref() includes broken references in the iteration, and
"git for-each-ref" doesn't itself reject references with REF_ISBROKEN.
The result is that broken references are processed *as if* they had
the value NULL_SHA1, which is the value stored in entries for broken
references.

Change "git for-each-ref" to emit warnings for references that are
REF_ISBROKEN but to otherwise skip them.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/for-each-ref.c         | 5 +++++
 t/t6301-for-each-ref-errors.sh | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 83f9cf9..13d2172 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -851,6 +851,11 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 		  return 0;
 	}
 
+	if (flag & REF_ISBROKEN) {
+		  warning("ignoring broken ref %s", refname);
+		  return 0;
+	}
+
 	if (*cb->grab_pattern) {
 		const char **pattern;
 		int namelen = strlen(refname);
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index dc68947..b9af9a9 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -13,7 +13,7 @@ test_expect_success setup '
 	git for-each-ref >full-list
 '
 
-test_expect_failure 'Broken refs are reported correctly' '
+test_expect_success 'Broken refs are reported correctly' '
 	r=refs/heads/bogus &&
 	: >.git/$r &&
 	test_when_finished "rm -f .git/$r" &&
-- 
2.1.4

[PATCH 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Michael Haggerty]

NULL_SHA1 is never a valid value for a reference. If a loose reference
has that value, mark it as broken.

Why check NULL_SHA1 and not the nearly 2^160 other SHA-1s that are
also invalid in a given repository? Because (a) it is cheap to test
for NULL_SHA1, and (b) NULL_SHA1 is often used as a "SHA-1 is invalid"
value inside of Git client source code (not only ours!), and
accidentally writing it to a loose reference file would be an easy
mistake to make.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                         | 7 +++++++
 t/t6301-for-each-ref-errors.sh | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 47e4e53..c28fde1 100644
--- a/refs.c
+++ b/refs.c
@@ -1321,6 +1321,13 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 				hashclr(sha1);
 				flag |= REF_ISBROKEN;
 			}
+
+			if (!(flag & REF_ISBROKEN) && is_null_sha1(sha1)) {
+				/* NULL_SHA1 is never a valid reference value. */
+				hashclr(sha1);
+				flag |= REF_ISBROKEN;
+			}
+
 			if (check_refname_format(refname.buf,
 						 REFNAME_ALLOW_ONELEVEL)) {
 				hashclr(sha1);
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index b9af9a9..f737cce 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -23,7 +23,7 @@ test_expect_success 'Broken refs are reported correctly' '
 	test_cmp broken-err err
 '
 
-test_expect_failure 'NULL_SHA1 refs are reported correctly' '
+test_expect_success 'NULL_SHA1 refs are reported correctly' '
 	r=refs/heads/zeros &&
 	echo $ZEROS >.git/$r &&
 	test_when_finished "rm -f .git/$r" &&
-- 
2.1.4