patch, 3 partst6301: new tests of for-each-ref error handling

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

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

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

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

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.

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

Jeff King <peff@peff.net> writes:

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

Sound like a good thing to check, too.

Thanks.

On 06/01/2015 06:08 PM, Jeff King wrote:

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

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

Thanks for the review! Michael

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(-)

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(-)