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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 14, 2018, 18:06 UTC
Message-ID: <20180814180626.31788-1-jonathantanmy@google.com>
In-Reply-To: <e01e202cb0445b72dc52a1c8763dbd9c737fb86e.1534267611.git.matvore@google.com>

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

Show 13 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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

Show 28 quoted lines

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