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

Commit Message

Taylor Blau April 5, 2019, 3:37 a.m. UTC

From: Jeff King <[e-mail]>

Commit 7c0fe330d5 (rev-list: handle missing tree objects properly,
2018-10-05) taught the traversal machinery used by git-rev-list to
ignore missing trees, so that rev-list could handle them itself.

However, it does so only by checking via oid_object_info_extended() that
the object exists at all. This can miss several classes of errors that
were previously detected by rev-list:

  - type mismatches (e.g., we expected a tree but got a blob)

  - failure to read the object data (e.g., due to bitrot on disk)

This is especially important because we use "rev-list --objects" as our
connectivity check to admit new objects to the repository, and it will
now miss these cases (though the bitrot one is less important here,
because we'd typically have just hashed and stored the object).

There are a few options to fix this:

 1. we could check these properties in rev-list when we do the existence
    check. This is probably too expensive in practice (perhaps even for
    a type check, but definitely for checking the whole content again,
    which implies loading each object into memory twice).

 2. teach the traversal machinery to differentiate between a missing
    object, and one that could not be loaded as expected. This probably
    wouldn't be too hard to detect type mismatches, but detecting bitrot
    versus a truly missing object would require deep changes to the
    object-loading code.

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

Note that we can't get rid of the object-existence check in
finish_object(), because this also handles blobs (which are not
otherwise checked at all by the traversal code).

Signed-off-by: Jeff King <[e-mail]>
---
 builtin/rev-list.c                     | 4 +++-
 t/t6102-rev-list-unexpected-objects.sh | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

Comments

Jeff King April 5, 2019, 6:41 p.m. UTC | #1

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

Taylor Blau April 6, 2019, 5:36 a.m. UTC | #2

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

Jeff King April 7, 2019, 1:41 p.m. UTC | #3

On Fri, Apr 05, 2019 at 10:36:48PM -0700, Taylor Blau wrote:

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

I'm not sure what "it" is here. My earlier message was admittedly
rambling, but I think I'm arguing that it's OK to continue to include
this patch that you already have, and punt further changes to make
"rev-list --objects" detect blob problems down the road. I.e., leave the
two expect_failures in place that your v1 series ends with.

-Peff

Taylor Blau April 9, 2019, 2:11 a.m. UTC | #4

Hi Peff,

On Sun, Apr 07, 2019 at 09:41:13AM -0400, Jeff King wrote:
> On Fri, Apr 05, 2019 at 10:36:48PM -0700, Taylor Blau wrote:
>
> > > > 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.
>
> I'm not sure what "it" is here.

Yes... as I read this email again after the weekend had passed, I found
myself a little confused, too.

> My earlier message was admittedly rambling, but I think I'm arguing
> that it's OK to continue to include this patch that you already have,
> and punt further changes to make "rev-list --objects" detect blob
> problems down the road. I.e., leave the two expect_failures in place
> that your v1 series ends with.

I believe that that was the "it" that I was talking about it. To be
explicit, I think I was suggesting that we should not change this patch
much or add more to the series, and rather address the blob checking in
a new series after this one.

> -Peff

Thanks,
Taylor

Patch

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 425a5774db..9f31837d30 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -379,7 +379,6 @@  int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	repo_init_revisions(the_repository, &revs, prefix);
 	revs.abbrev = DEFAULT_ABBREV;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
-	revs.do_not_die_on_missing_tree = 1;
 
 	/*
 	 * Scan the argument list before invoking setup_revisions(), so that we
@@ -409,6 +408,9 @@  int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (arg_missing_action)
+		revs.do_not_die_on_missing_tree = 1;
+
 	argc = setup_revisions(argc, argv, &revs, &s_r_opt);
 
 	memset(&info, 0, sizeof(info));
diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
index 30976385a8..c8d4b31f8f 100755
--- a/t/t6102-rev-list-unexpected-objects.sh
+++ b/t/t6102-rev-list-unexpected-objects.sh
@@ -29,7 +29,7 @@  test_expect_success 'setup unexpected non-tree entry' '
 	broken_tree="$(git hash-object -w --literally -t tree broken-tree)"
 '
 
-test_expect_failure 'traverse unexpected non-tree entry (lone)' '
+test_expect_success 'traverse unexpected non-tree entry (lone)' '
 	test_must_fail git rev-list --objects $broken_tree
 '
 
@@ -64,7 +64,7 @@  test_expect_success 'setup unexpected non-tree root' '
 		broken-commit)"
 '
 
-test_expect_failure 'traverse unexpected non-tree root (lone)' '
+test_expect_success 'traverse unexpected non-tree root (lone)' '
 	test_must_fail git rev-list --objects $broken_commit
 '