threads / patch / 39515

v3, 4 partst6301: new tests of for-each-ref error handling

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

## tl;dr

11 messages between Jun 3, 2015 and Jun 8, 2015. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Michael Haggerty· Jun 3, 2015, 13:51 UTC · lore

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

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
Michael Haggerty· Jun 3, 2015, 13:51 UTC · re: Michael Haggerty · lore

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
Show changes to t/t6301-for-each-ref-errors.sh +56 −0
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
Michael Haggerty· Jun 3, 2015, 13:51 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to 2 files +6 −1

builtin/for-each-ref.c, t/t6301-for-each-ref-errors.sh

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
Michael Haggerty· Jun 3, 2015, 13:51 UTC · re: Michael Haggerty · lore

[PATCH v3 3/4] read_loose_refs(): simplify function logic

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(-)
Show changes to refs.c +12 −7
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
Michael Haggerty· Jun 3, 2015, 13:51 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to 2 files +11 −1

refs.c, t/t6301-for-each-ref-errors.sh

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
Jeff King· Jun 3, 2015, 14:08 UTC · re: Michael Haggerty · lore

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

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/ ?
Show 12 quoted lines
> (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.

Show 18 quoted lines
> 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
Junio C Hamano· Jun 3, 2015, 18:51 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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/?
Show 15 quoted lines
>> (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 ...
Jeff King· Jun 3, 2015, 20:15 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 03, 2015 at 11:51:43AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Junio C Hamano· Jun 3, 2015, 21:20 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> 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.
Michael Haggerty· Jun 8, 2015, 09:26 UTC · re: Junio C Hamano · lore

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

On 06/03/2015 11:20 PM, Junio C Hamano wrote:
Show 21 quoted lines
> 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
Junio C Hamano· Jun 8, 2015, 17:37 UTC · re: Michael Haggerty · lore

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

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.

← back to recent threads