* [PATCH 1/3] t6301: new tests of for-each-ref error handling
2015-06-01 15:53 [PATCH 0/3] Fix how for-each-ref handles broken loose references Michael Haggerty
@ 2015-06-01 15:53 ` Michael Haggerty
2015-06-01 16:08 ` Jeff King
2015-06-01 15:53 ` [PATCH 2/3] for-each-ref: report broken references correctly Michael Haggerty
2015-06-01 15:53 ` [PATCH 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken Michael Haggerty
2 siblings, 1 reply; 7+ messages in thread
From: Michael Haggerty @ 2015-06-01 15:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Anders Kaseorg, Stefan Beller, Jeff King, git, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling
2015-06-01 15:53 ` [PATCH 1/3] t6301: new tests of for-each-ref error handling Michael Haggerty
@ 2015-06-01 16:08 ` Jeff King
2015-06-01 17:33 ` Junio C Hamano
2015-06-02 15:46 ` Michael Haggerty
0 siblings, 2 replies; 7+ messages in thread
From: Jeff King @ 2015-06-01 16:08 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, Anders Kaseorg, Stefan Beller, git
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling
2015-06-01 16:08 ` Jeff King
@ 2015-06-01 17:33 ` Junio C Hamano
2015-06-02 15:46 ` Michael Haggerty
1 sibling, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2015-06-01 17:33 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, Anders Kaseorg, Stefan Beller, git
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] t6301: new tests of for-each-ref error handling
2015-06-01 16:08 ` Jeff King
2015-06-01 17:33 ` Junio C Hamano
@ 2015-06-02 15:46 ` Michael Haggerty
1 sibling, 0 replies; 7+ messages in thread
From: Michael Haggerty @ 2015-06-02 15:46 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Anders Kaseorg, Stefan Beller, git
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] for-each-ref: report broken references correctly
2015-06-01 15:53 [PATCH 0/3] Fix how for-each-ref handles broken loose references Michael Haggerty
2015-06-01 15:53 ` [PATCH 1/3] t6301: new tests of for-each-ref error handling Michael Haggerty
@ 2015-06-01 15:53 ` Michael Haggerty
2015-06-01 15:53 ` [PATCH 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken Michael Haggerty
2 siblings, 0 replies; 7+ messages in thread
From: Michael Haggerty @ 2015-06-01 15:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Anders Kaseorg, Stefan Beller, Jeff King, git, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/3] read_loose_refs(): treat NULL_SHA1 loose references as broken
2015-06-01 15:53 [PATCH 0/3] Fix how for-each-ref handles broken loose references Michael Haggerty
2015-06-01 15:53 ` [PATCH 1/3] t6301: new tests of for-each-ref error handling Michael Haggerty
2015-06-01 15:53 ` [PATCH 2/3] for-each-ref: report broken references correctly Michael Haggerty
@ 2015-06-01 15:53 ` Michael Haggerty
2 siblings, 0 replies; 7+ messages in thread
From: Michael Haggerty @ 2015-06-01 15:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Anders Kaseorg, Stefan Beller, Jeff King, git, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread