v2, 3 partsFix how for-each-ref handles broken loose references

This is a reroll of [1]. Thanks to Peff and Junio for their comments about v1.

Changes since v1:

* Use $_z40 to initialize $ZEROS in test.

* Test that "git for-each-ref --format="%(objectname) %(refname)"
  *doesn't* notice references that point at missing objects.

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

[1] http://thread.gmane.org/gmane.comp.version-control.git/270429 [2] 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 | 56 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 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.

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

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 | 56 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100755 t/t6301-for-each-ref-errors.sh

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

On Tue, Jun 2, 2015 at 8:57 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

I don't quite agree with the reasoning here. Just because it's cheap doesn't mean it's right. ;) But I fully agree with (b) so this still makes sense.

Why do we do the extra check for !(flag & REF_ISBROKEN) here?

... *by our definition*, because we believe it helps detecting errors/mistakes in the future.

*/

While this code looks consistent to the rest around, at closer inspection this feels a bit redundant to me. If is_null_sha1(sha1) is true, then hashclr(sha1); doesn't change the state. Or did I miss a subtle side effect?

On 06/02/2015 07:28 PM, Stefan Beller wrote:

Its cheapness improves the cost/benefit ratio of adding this check.

That was an attempt to avoid calling is_null_sha1() unnecessarily. I think I can make this go away and make the code clearer in general by restructuring the logic a little bit. I will do that in the next round.

It's not even by our definition. It is just astronomically more likely that NULL_SHA1 got set there due to an error than that it is the SHA-1 of legitimate content. In fact it is so unlikely that we use NULL_SHA1 throughout our code to indicate "invalid SHA-1", ignoring the theoretical possibility that it could appear some day as a real SHA-1.

I'll try to explain this point better in the comment.

You're right, there is no reason to call hashclr() here.

Thanks for your review!

Michael

On Tue, Jun 02, 2015 at 11:10:22PM +0200, Michael Haggerty wrote:

If you get rid of the useless hashclr(), then this just becomes:

  if (!(flag & REF_ISBROKEN) && is_null_sha1(sha1))
	flag |= REF_ISBROKEN;

The reason for the initial check seems pretty obvious then (but it would also be OK without it; is_null_sha1 is not that expensive).

-Peff

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

Do you need to clear after checking with is_null_sha1()?