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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Apr 05, 2019 at 02:41:11PM -0400, Jeff King wrote:
> 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):

I agree that it's worth doing. One question that I have is _when_ you
feel it's good to do. I'm happy to write it and include the change in
v2, but if others would be happy not to grow the series too much between
re-rolls, I'd be just as pleased to send it in a new series after this
one.

>  - 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
Thanks,
Taylor