Re: [PATCH] fsck: return non-zero status on missing ref tips

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 15, 2014, 14:57 UTC
Message-ID: <5416FE5D.5050102@alum.mit.edu>
In-Reply-To: <20140912042939.GA5968@peff.net>

On 09/12/2014 06:29 AM, Jeff King wrote:

Show 30 quoted lines

> [+cc mhagger for packed-refs wisdom]
> 
> On Thu, Sep 11, 2014 at 11:38:30PM -0400, Jeff King wrote:
> 
>> Fsck tries hard to detect missing objects, and will complain
>> (and exit non-zero) about any inter-object links that are
>> missing. However, it will not exit non-zero for any missing
>> ref tips, meaning that a severely broken repository may
>> still pass "git fsck && echo ok".
>>
>> The problem is that we use for_each_ref to iterate over the
>> ref tips, which hides broken tips. It does at least print an
>> error from the refs.c code, but fsck does not ever see the
>> ref and cannot note the problem in its exit code. We can solve
>> this by using for_each_rawref and noting the error ourselves.
> 
> There's a possibly related problem with packed-refs that I noticed while
> looking at this.
> 
> When we call pack-refs, it will refuse to pack any broken loose refs,
> and leave them loose. Which is sane. But when we delete a ref, we need
> to rewrite the packed-refs file, and we omit any broken packed refs. We
> wouldn't have written a broken entry, but we may get broken later (i.e.,
> the tip object may go missing after the packed-refs file is written).
> 
> If we only have a packed copy of "refs/heads/master" and it is broken,
> then deleting any _other_ unrelated ref will cause refs/heads/master to
> be dropped from the packed-refs file entirely. We get an error message,
> but that's easy to miss, and the pointer to master's sha1 is lost
> forever.

I was confused for a while by your observation, because the curate function has

	if (read_ref_full(entry->name, sha1, 0, &flags))
		/* We should at least have found the packed ref. */
		die("Internal error");

, which looks like more than "emit an error message and continue". But in fact the flow never gets this far, because iterating without DO_FOR_EACH_INCLUDE_BROKEN doesn't just skip references for which REF_ISBROKEN is set, but also (do to a test in do_one_ref()) references for which ref_resolves_to_object() fails. The ultimate source of my confusion is that the word BROKEN has two different meanings in the two constants' names.

Show 19 quoted lines

> [...]
> I am tempted to say that we do not need to do curate_each_ref_fn at all.
> Any entry with a broken sha1 is either:
> 
>   1. A truly broken ref, in which case we should make sure to keep it
>      (i.e., it is not cruft at all).
> 
>   2. A crufty entry that has been replaced by a loose reference that has
>      not yet been packed. Such a crufty entry may point to broken
>      objects, and that is OK.
> 
> In case 2, we _could_ delete the cruft. But I do not think we need to.
> The loose ref will take precedence to anybody who actually does a ref
> lookup, so the cruft is not hurting anybody.
> 
> Dropping curate_packed_ref_fn (as below) fixes the test above. And
> miraculously does not even seem to conflict with ref patches in pu. :)
> 
> Am I missing any case that it is actually helping?

Something inside me screams out in horror that we would pass up an opportunity to delete unneeded cruft from the packed-refs file. But I can't think of a rational reason to disagree with you, so as far as I'm concerned your suggestion seems OK.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu