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

[Matthew DeVore <matvore@google.com>]

On Tue, Aug 14, 2018 at 11:06 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > 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.
Fixed the commit message. And for the tests, in t0410 I changed the
--missing=allow-any to --missing-allow-promisor, and in t6112 I added
--missing=allow-any and --missing=print test cases.

>
> > 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.
Sorry about that. Removed.

>
> > @@ -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.
If --missing is not set, then we want to fetch missing objects
automatically, and then die if we fail to do that, which is what
happens for blobs. So we don't want to die in list-objects.c. If we
fail to fetch, then we will die on line 213 in rev-list.c.

>
> > -     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.
I've removed this.

>
> [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.
Added your wording to revision.h without major change.

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

>
> As for --missing=allow-promisor, I don't see them being tested anywhere
> :-( so feel free to make a suggestion. I would put them in t6112 for
> easy comparison with the other --missing tests.
Kept my allow-promisor test in t0410 since it requires partial clone
to be turned on in the config, and because it is pretty similar to
--exclude-promisor-objects.