Re: [PATCH v2 3/5] rev-list: handle missing tree objects properly

[Jonathan Tan <jonathantanmy@google.com>]

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

Indeed, and I'm not sure why the message was there in the first place -
if parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the "die" call, so any message printed by parse_tree_gently() seems
superfluous.

It might be better to add an additional commit that removes the "gently"
condition (in other words, always parsing gently), with a message
explaining the above. Also, in that commit, I prefer not to add the
"/*quiet_on_missing*/" explanation (we don't seem to do that in Git
code); I also know that the ">= 0" is a holdover from the existing "< 0"
code, but we don't need to do that either.

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

is_promisor_object() should. If you still have the code you used to
verify that, can you share it? In particular, pay attention to the path
of the repo - promise_and_delete is hardcoded to use one particular
path.

Whether you test in this patch or in the last patch, make sure that the
following are tested:
 git rev-list --missing=error, allow-any, allow-promisor, print
 git rev-list --exclude-promisor-objects

Also, test when a tree pointed to by a commit is missing, and when a
tree pointed to by a tree is missing.

> @@ -152,20 +151,21 @@ static void process_tree(struct traversal_context *ctx,
>  		die("bad tree object");
>  	if (obj->flags & (UNINTERESTING | SEEN))
>  		return;
> -	if (parse_tree_gently(tree, gently) < 0) {
> +	parsed = parse_tree_gently(tree, /*quiet_on_missing=*/1) >= 0;
> +	if (!parsed) {
>  		if (revs->ignore_missing_links)
>  			return;
>  
> +		if (!is_promisor_object(&obj->oid))
> +			die("bad tree object %s", oid_to_hex(&obj->oid));
> +
>  		/*
>  		 * Pre-filter known-missing tree objects when explicitly
>  		 * requested.  This may cause the actual filter to report
>  		 * an incomplete list of missing objects.
>  		 */
> -		if (revs->exclude_promisor_objects &&
> -		    is_promisor_object(&obj->oid))
> +		if (revs->exclude_promisor_objects)
>  			return;
> -
> -		die("bad tree object %s", oid_to_hex(&obj->oid));
>  	}

The missing mechanism (for error, allow-any, print) should work without
needing to consult whether an object is a promisor object or not - it
should just print whatever is missing, so the "if
(!is_promisor_object..." line looks out of place.

In my original review [1], I suggested that we always show a tree if we
have its hash - if we don't have the object, we just recurse into it.
This would be the same as your patch, except that the 'die("bad tree
object...' is totally removed instead of merely moved. I still think
this solution has some merit - all the tests still pass (except that we
need to check for "unable to read" instead of "bad tree object" in error
messages), but I just realized that it might still be backwards
incompatible in that a basic "rev-list --objects" would now succeed
instead of fail if a tree was missing (I haven't tested this though).

We might need a flag called "do_not_die_on_missing_tree" (much like your
original idea of "show_missing_trees") so that callers that are prepared
to deal with missing trees can set this. Sorry for the churn. You can
document it as such:

 Blobs are shown without regard for their existence. But not so for
 trees: unless exclude_promisor_objects is set and the tree in question
 is a promisor object, or ignore_missing_links is set (and in this case,
 the tree in question may or may not be a promisor object), the revision
 walker dies with a "bad tree object" message when encountering a
 missing tree.

 For callers that can handle missing trees and want them to be
 filterable and showable, set this to true. The revision walker will
 filter and show such a missing tree as usual, but will not attempt to
 recurse into this tree object.

[1] https://public-inbox.org/git/20180810002411.13447-1-jonathantanmy@google.com/