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

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

[Michael Haggerty]

This reroll addresses all of the comments about v1 [1] and v2 [2].
Thanks to Stefan, Junio, and Peff for their comments about v2.

Changes since v2:

* Simplify logic flow in read_loose_refs().
* Remove unnecessary call to hashclr() in read_loose_refs().
* Improve a comment and commit message.

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

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

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

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

-- 
2.1.4

[PATCH v3 1/4] 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.

Note that when for-each-ref is run with a --format option that doesn't
require the object to be looked up, then we should still notice if a
loose reference file is corrupt or contains NULL_SHA1, but we don't
notice if it points at a missing object because we don't do an object
lookup. This is OK.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t6301-for-each-ref-errors.sh | 56 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 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..cf25244
--- /dev/null
+++ b/t/t6301-for-each-ref-errors.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+
+test_description='for-each-ref errors for broken refs'
+
+. ./test-lib.sh
+
+ZEROS=$_z40
+MISSING=abababababababababababababababababababab
+
+test_expect_success setup '
+	git commit --allow-empty -m "Initial" &&
+	git tag testtag &&
+	git for-each-ref >full-list &&
+	git for-each-ref --format="%(objectname) %(refname)" >brief-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 &&
+	git for-each-ref --format="%(objectname) %(refname)" >brief-out 2>brief-err &&
+	test_cmp brief-list brief-out &&
+	test_cmp zeros-err brief-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 &&
+	(
+		cat brief-list &&
+		echo "$MISSING $r"
+	) | sort -k 2 >missing-brief-expected &&
+	git for-each-ref --format="%(objectname) %(refname)" >brief-out 2>brief-err &&
+	test_cmp missing-brief-expected brief-out &&
+	test_must_be_empty brief-err
+'
+
+test_done
-- 
2.1.4

[PATCH v3 2/4] 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 cf25244..72d2397 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -14,7 +14,7 @@ test_expect_success setup '
 	git for-each-ref --format="%(objectname) %(refname)" >brief-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 v3 3/4] read_loose_refs(): simplify function logic

[Michael Haggerty]

Make it clearer that there are two possible ways to read the
reference, but that we handle read errors uniformly regardless of
which way it was read.

This refactoring also makes the following change easier to implement.

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

diff --git a/refs.c b/refs.c
index 47e4e53..6736424 100644
--- a/refs.c
+++ b/refs.c
@@ -1308,19 +1308,24 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 					 create_dir_entry(refs, refname.buf,
 							  refname.len, 1));
 		} else {
+			int read_ok;
+
 			if (*refs->name) {
 				hashclr(sha1);
 				flag = 0;
-				if (resolve_gitlink_ref(refs->name, refname.buf, sha1) < 0) {
-					hashclr(sha1);
-					flag |= REF_ISBROKEN;
-				}
-			} else if (read_ref_full(refname.buf,
-						 RESOLVE_REF_READING,
-						 sha1, &flag)) {
+				read_ok = !resolve_gitlink_ref(refs->name,
+							       refname.buf, sha1);
+			} else {
+				read_ok = !read_ref_full(refname.buf,
+							 RESOLVE_REF_READING,
+							 sha1, &flag);
+			}
+
+			if (!read_ok) {
 				hashclr(sha1);
 				flag |= REF_ISBROKEN;
 			}
+
 			if (check_refname_format(refname.buf,
 						 REFNAME_ALLOW_ONELEVEL)) {
 				hashclr(sha1);
-- 
2.1.4

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

[Michael Haggerty]

NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code
(and the code of other git implementations), so it is vastly more
likely that a reference was set to this value due to a software bug
than that NULL_SHA1 is the legitimate SHA-1 of an actual object.
Therefore, if a loose reference has the value NULL_SHA1, consider it
to be broken.

Amusingly, each of the other 2^160-1 possible SHA-1 values is exactly
as unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The
difference is that most of those other values are also very unlikely
to be written to a loose reference file by accident, whereas
accidentally writing NULL_SHA1 to a loose reference file would be an
easy mistake to make.

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

diff --git a/refs.c b/refs.c
index 6736424..83af13d 100644
--- a/refs.c
+++ b/refs.c
@@ -1324,6 +1324,16 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 			if (!read_ok) {
 				hashclr(sha1);
 				flag |= REF_ISBROKEN;
+			} else if (is_null_sha1(sha1)) {
+				/*
+				 * It is so astronomically unlikely
+				 * that NULL_SHA1 is the SHA-1 of an
+				 * actual object that we consider its
+				 * appearance in a loose reference
+				 * file to be repo corruption
+				 * (probably due to a software bug).
+				 */
+				flag |= REF_ISBROKEN;
 			}
 
 			if (check_refname_format(refname.buf,
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 72d2397..cdb67a0 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -24,7 +24,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

Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Jeff King]

On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:

> NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code

s/of/an/ ?

> (and the code of other git implementations), so it is vastly more
> likely that a reference was set to this value due to a software bug
> than that NULL_SHA1 is the legitimate SHA-1 of an actual object.
> Therefore, if a loose reference has the value NULL_SHA1, consider it
> to be broken.
> 
> Amusingly, each of the other 2^160-1 possible SHA-1 values is exactly
> as unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The
> difference is that most of those other values are also very unlikely
> to be written to a loose reference file by accident, whereas
> accidentally writing NULL_SHA1 to a loose reference file would be an
> easy mistake to make.

FWIW, I think this justification (and the comment below) reads better
than what you had before.

> diff --git a/refs.c b/refs.c
> index 6736424..83af13d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1324,6 +1324,16 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
>  			if (!read_ok) {
>  				hashclr(sha1);
>  				flag |= REF_ISBROKEN;
> +			} else if (is_null_sha1(sha1)) {
> +				/*
> +				 * It is so astronomically unlikely
> +				 * that NULL_SHA1 is the SHA-1 of an
> +				 * actual object that we consider its
> +				 * appearance in a loose reference
> +				 * file to be repo corruption
> +				 * (probably due to a software bug).
> +				 */
> +				flag |= REF_ISBROKEN;

Nice. After reading the other thread, I did not think we needed to
bother with more refactoring here, but I agree this end result flows
nicely.

-Peff

Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:
>
>> NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code
>
> s/of/an/ ?

Also possibly s/invalid SHA-1/invalid ref/?

>> (and the code of other git implementations), so it is vastly more
>> likely that a reference was set to this value due to a software bug
>> than that NULL_SHA1 is the legitimate SHA-1 of an actual object.
>> Therefore, if a loose reference has the value NULL_SHA1, consider it
>> to be broken.
>> 
>> Amusingly, each of the other 2^160-1 possible SHA-1 values is exactly
>> as unlikely as NULL_SHA1 to be the SHA-1 of an actual object. The
>> difference is that most of those other values are also very unlikely
>> to be written to a loose reference file by accident, whereas
>> accidentally writing NULL_SHA1 to a loose reference file would be an
>> easy mistake to make.
>
> FWIW, I think this justification (and the comment below) reads better
> than what you had before.

I agree, and further I think the second paragraph is redundant and
unnecessary.  If you update "... likely that a reference was set to
this value" to clarify that the "reference" it talks about is an
on-disk entity, not the in-core representation (which can
legitimately have NULL_SHA1 to signal "invalid ref", it would be
sufficient.  I.e.

	... so it is vastly more likely that an on-disk reference
	was set to this value due to a bug ...

Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Jeff King]

On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:
> >
> >> NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code
> >
> > s/of/an/ ?
> 
> Also possibly s/invalid SHA-1/invalid ref/?

I thought it was trying to express that we use the null sha1 as a
sentinel value throughout the code, no matter if the value came from a
ref or otherwise.

-Peff

Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:
>> >
>> >> NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code
>> >
>> > s/of/an/ ?
>> 
>> Also possibly s/invalid SHA-1/invalid ref/?
>
> I thought it was trying to express that we use the null sha1 as a
> sentinel value throughout the code, no matter if the value came from a
> ref or otherwise.

Yeah, an invalid object name, not limited to refs, is correct.

Thanks.

Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Michael Haggerty]

On 06/03/2015 11:20 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:
>>
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> On Wed, Jun 03, 2015 at 03:51:59PM +0200, Michael Haggerty wrote:
>>>>
>>>>> NULL_SHA1 is used to indicate of "invalid SHA-1" throughout our code
>>>>
>>>> s/of/an/ ?
>>>
>>> Also possibly s/invalid SHA-1/invalid ref/?
>>
>> I thought it was trying to express that we use the null sha1 as a
>> sentinel value throughout the code, no matter if the value came from a
>> ref or otherwise.
> 
> Yeah, an invalid object name, not limited to refs, is correct.
> 
> Thanks.

Thanks for all the comments. Taking them into consideration, I suggest
changing the last commit message to

--8<-----------------------------------------------------------------
read_loose_refs(): treat NULL_SHA1 loose references as broken

NULL_SHA1 is used to indicate an "invalid object name" throughout our
code (and the code of other git implementations), so it is vastly more
likely that an on-disk reference was set to this value due to a
software bug than that NULL_SHA1 is the legitimate SHA-1 of an actual
object. Therefore, if a loose reference has the value NULL_SHA1,
consider it to be broken.

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

Since the only comments were about this one commit message, I won't send
an updated patch series to the mailing list unless you request it. You
can also get the patches from my GitHub account [1], branch
"for-each-ref-errors".

Michael

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

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v3 4/4] read_loose_refs(): treat NULL_SHA1 loose references as broken

[Junio C Hamano]

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

> Thanks for all the comments. Taking them into consideration, I suggest
> changing the last commit message to
> ...
> Since the only comments were about this one commit message,...

Yeah, this round looked good otherwise.  Will amend in-place.

Thanks.