Re: [PATCH 6/7] rev-list: let traversal die when --missing is not in use

From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH 6/7] rev-list: let traversal die when --missing is not in use
Date: Fri, 5 Apr 2019 14:41:11 -0400	[thread overview]
Message-ID: <20190405184111.GE2284@sigill.intra.peff.net> (raw)
In-Reply-To: <a3a80b4b2a988eb65d85a5acd54c584d047073c7.1554435033.git.me@ttaylorr.com>

On Thu, Apr 04, 2019 at 08:37:54PM -0700, Taylor Blau wrote:

>  3. have the traversal machinery communicate the failure to the caller,
>     so that it can decide how to proceed without re-evaluting the object
>     itself.
> 
> Of those, I think (3) is probably the best path forward. However, this
> patch does none of them. In the name of expediently fixing the
> regression to a normal "rev-list --objects" that we use for connectivity
> checks, this simply restores the pre-7c0fe330d5 behavior of having the
> traversal die as soon as it fails to load a tree (when --missing is set
> to MA_ERROR, which is the default).

I think this is worth doing, as it restores the earlier behavior. But a
few general thoughts (which I've shared already with you, but for the
benefit of the list):

 - actually doing the "communicate failure to the caller" would probably
   not be too bad as a single-bit PARSE_FAILED flag in obj->flags. But
   it does require the caller understanding which objects the traversal
   would try to parse (i.e., rev-list would have to understand that it
   is on its own to check blobs, even if they don't have a PARSE_FAILED
   flag).

 - speaking of blobs, this series does not help rev-list find a
   mis-typed or bit-rotted blob at all, because it never opens the
   blobs. Does that mean my expectations for rev-list are simply too
   high, and that we should be expecting fsck-like checks to catch
   these? I dunno.

   It would not be too expensive to convert the existing "do we have the
   blob" check in rev-list to "do we have it, and is its type correct?".
   But obviously finding bitrot would be super-expensive. Which leads me
   to...

 - there actually _is_ a --verify-objects option, which would check even
   blobs for bitrot. It was added long ago in 5a48d24012 (rev-list
   --verify-object, 2011-09-01) for use with check_connected(). But it
   was deemed too slow for normal use, and ripped out in d21c463d55
   (fetch/receive: remove over-pessimistic connectivity check,
   2012-03-15).

That last one implies that we're OK relying on the incoming index-pack
to catch these cases (which is going to do a sha1 over each object).

It does seem like we should bother to notice failures when it's _free_
to do so, which is the case with these tree-loading failures. Which is
basically what this patch is doing.

-Peff