Re: [PATCH v4 4/6] rev-list: handle missing tree objects properly

>
> > Previously, we assumed only blob objects could be missing. This patch
> > makes rev-list handle missing trees like missing blobs. A missing tree
> > will cause an error if --missing indicates an error should be caused,
> > and the hash is printed even if the tree is missing.
>
> The last sentence is difficult to understand - probably better to say
> that all --missing= arguments and --exclude-promisor-objects work for
> missing trees like they currently do for blobs (and do not fixate on
> just --missing=error). And also demonstrate this in tests, like in
> t6612.

>
> > In list-objects.c we no longer print a message to stderr if a tree
> > object is missing (quiet_on_missing is always true). I couldn't find
> > any place where this would matter, or where the caller of
> > traverse_commit_list would need to be fixed to show the error. However,
> > in the future it would be trivial to make the caller show the message if
> > we needed to.
> >
> > This is not tested very thoroughly, since we cannot create promisor
> > objects in tests without using an actual partial clone. t0410 has a
> > promise_and_delete utility function, but the is_promisor_object function
> > does not return 1 for objects deleted in this way. More tests will will
> > come in a patch that implements a filter that can be used with git
> > clone.
>
> These two paragraphs are no longer applicable, I think.

>
> > @@ -373,6 +375,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >       init_revisions(&revs, prefix);
> >       revs.abbrev = DEFAULT_ABBREV;
> >       revs.commit_format = CMIT_FMT_UNSPECIFIED;
> > +     revs.do_not_die_on_missing_tree = 1;
>
> Is this correct? I would have expected this to be set only if --missing
> was set.

>
> > -     process_tree_contents(ctx, tree, base);
> > +     /*
> > +      * NEEDSWORK: we should not have to process this tree's contents if the
> > +      * filter wants to exclude all its contents AND the filter doesn't need
> > +      * to collect the omitted OIDs. We should add a LOFR_SKIP_TREE bit which
> > +      * allows skipping all children.
> > +      */
> > +     if (parsed)
> > +             process_tree_contents(ctx, tree, base);
>
> I agree with Jeff Hostetler in [1] that a LOFR_SKIP_TREE bit is
> desirable, but I don't think that this patch is the right place to
> introduce this NEEDSWORK. For me, this patch is about skipping iterating
> over the contents of a tree because the tree does not exist; this
> NEEDSWORK is about skipping iterating over the contents of a tree
> because we don't want its contents, and it is quite confusing to
> conflate the two.

>
> [1] https://public-inbox.org/git/d751d56b-84bb-a03d-5f2a-7dbaf8d947cc@jeffhostetler.com/
>
> > @@ -124,6 +124,7 @@ struct rev_info {
> >                       first_parent_only:1,
> >                       line_level_traverse:1,
> >                       tree_blobs_in_commit_order:1,
> > +                     do_not_die_on_missing_tree:1,
>
> I know that the other flags don't have documentation, but I think it's
> worth documenting this one because it is rather complicated. I have
> provided a sample one in my earlier review - feel free to use that or
> come up with your own.

>
> > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> > index 4984ca583..74e3c5767 100755
> > --- a/t/t0410-partial-clone.sh
> > +++ b/t/t0410-partial-clone.sh
> > @@ -186,6 +186,72 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
> >       ! grep $FOO out
> >  '
> >
> > +test_expect_success 'show missing tree objects with --missing=print' '
> > +     rm -rf repo &&
> > +     test_create_repo repo &&
> > +     test_commit -C repo foo &&
> > +     test_commit -C repo bar &&
> > +     test_commit -C repo baz &&
> > +
> > +     TREE=$(git -C repo rev-parse bar^{tree}) &&
> > +
> > +     promise_and_delete $TREE &&
> > +
> > +     git -C repo config core.repositoryformatversion 1 &&
> > +     git -C repo config extensions.partialclone "arbitrary string" &&
> > +     git -C repo rev-list --quiet --missing=print --objects HEAD >missing_objs 2>rev_list_err &&
> > +     echo "?$TREE" >expected &&
> > +     test_cmp expected missing_objs &&
> > +
> > +     # do not complain when a missing tree cannot be parsed
> > +     ! grep -q "Could not read " rev_list_err
> > +'
>
> I think that the --exclude-promisor-tests can go in t0410 as you have
> done, but the --missing tests (except for --missing=allow-promisor)
> should go in t6112. (And like the existing --missing tests, they should
> be done without setting extensions.partialclone.)