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

[Jonathan Tan <jonathantanmy@google.com>]

> @@ -209,7 +210,8 @@ static inline void finish_object__ma(struct object *obj)
>  	 */
>  	switch (arg_missing_action) {
>  	case MA_ERROR:
> -		die("missing blob object '%s'", oid_to_hex(&obj->oid));
> +		die("missing %s object '%s'",
> +		    type_name(obj->type), oid_to_hex(&obj->oid));
>  		return;
>  
>  	case MA_ALLOW_ANY:
> @@ -222,8 +224,8 @@ static inline void finish_object__ma(struct object *obj)
>  	case MA_ALLOW_PROMISOR:
>  		if (is_promisor_object(&obj->oid))
>  			return;
> -		die("unexpected missing blob object '%s'",
> -		    oid_to_hex(&obj->oid));
> +		die("unexpected missing %s object '%s'",
> +		    type_name(obj->type), oid_to_hex(&obj->oid));
>  		return;

Once again, I'll do a fuller review tomorrow.

These are fine (obj->type is populated), because the types of objects
are known during traversal.

> -	if (obj->type == OBJ_BLOB && !has_object_file(&obj->oid)) {
> +	if (!has_object_file(&obj->oid)) {
>  		finish_object__ma(obj);
>  		return 1;

And this is also fine, because finish_object__ma can now handle any
object type.

> +	revs.show_missing_trees = 1;

(and elsewhere)

Could we just show missing trees all the time? We do that for blobs and
already rely on the caller (eventually, show_object() in
builtin/rev-list.c) to determine whether the object actually exists or
not; we could do the same for trees. This allows us to not include this
extra knob.

> -	if (parse_tree_gently(tree, gently) < 0) {
> +	parse_result = parse_tree_gently(tree, gently);
> +	if (parse_result < 0 && !revs->show_missing_trees) {
>  		if (revs->ignore_missing_links)
>  			return;
>  
> @@ -182,7 +185,8 @@ static void process_tree(struct traversal_context *ctx,
>  	if (base->len)
>  		strbuf_addch(base, '/');
>  
> -	process_tree_contents(ctx, tree, base);
> +	if (parse_result >= 0)
> +		process_tree_contents(ctx, tree, base);
>  
>  	if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {
>  		r = ctx->filter_fn(LOFS_END_TREE, obj,

Is it possible to call the appropriate callbacks and then return
immediately, instead of going through the whole function checking
parse_result when necessary? When doing the latter, the reader needs to
keep on checking if each function still works if the tree is
unparseable.