> 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@xxxxxxxxxxxxxxxxx/ > @@ -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.) 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.
