Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug

[Junio C Hamano <gitster@pobox.com>]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> When we use `--batch` with no atoms formatting and use
> `--batch-all-objects` at the same time (e.g.
> `git cat-file --batch="batman" --batch-all-objects`),
> Git will exit and report "object xxx changed type!?".
>
> This is because we have a format string which does not
> contain any atoms, so skip_object_info option will be
> set if we also use --batch-all-objects, and then
> `oid_object_info_extended()` will be skipped in
> `batch_object_write()`, it cause object type to not be
> collected. Therefore, it reported object type has changed.

The above analysis on how these die()s get hit makes sense, but ...

> So avoid checking changes in type and size when all_objects
> and skip_object_info options are set at the same time.

... it is not immediately clear how the above conclusion follows.

An obvious alternative, however, is to avoid skipping object info
when all objects is in use---but that goes directly against why this
"skip" mechanism was added at 845de33a (cat-file: avoid noop calls
to sha1_object_info_extended, 2016-05-18).

Which makes me suspect that the solution presented here would be
going in the right direction.

There however is one curious thing about this.  The log message of
the original commit that introduced this optimization does use the
batch-check and batch-all-objects at the same time.  Was this
breakage not there when the original was written and we broke it in
a later update?  If so, with what commit?  Can that commit have
broken other places in cat-file in a similar manner?

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 5ebf13359e83..5f9578f9b86b 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -345,11 +345,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
>  		contents = read_object_file(oid, &type, &size);
>  		if (!contents)
>  			die("object %s disappeared", oid_to_hex(oid));
> -		if (type != data->type)
> -			die("object %s changed type!?", oid_to_hex(oid));
> -		if (data->info.sizep && size != data->size)
> -			die("object %s changed size!?", oid_to_hex(oid));
> -
> +		if (!(opt->all_objects && data->skip_object_info)) {
> +			if (type != data->type)
> +				die("object %s changed type!?", oid_to_hex(oid));

When skip_object_info bit is set, we know data->type and date->size
are unusable and should not be checked, regardless of the reason why
skip_object_info bit is set, don't we?

> +			if (data->info.sizep && size != data->size)
> +				die("object %s changed size!?", oid_to_hex(oid));

Does this check need to be modified at all?  Would info.sizep ever
be set to non-NULL if skip_object_info is set (hence we are not
calling object-info)?

> +		}

In other words, shouldn't this patch just this one liner?

-		if (type != data->type)
+		if (data->skip_object_info && type != data->type)
			die("object %s changed type!?", oid_to_hex(oid));

> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index 5d2dc99b74ad..9b0f1ae5ef8b 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -586,4 +586,10 @@ test_expect_success 'cat-file --unordered works' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'cat-file --batch="batman" with --batch-all-objects will work' '
> +	git -C all-two cat-file --batch-all-objects --batch="%(objectname)" | wc -l >expect &&
> +	git -C all-two cat-file --batch-all-objects --batch="batman" | wc -l >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done
>
> base-commit: 5d5b1473453400224ebb126bf3947e0a3276bdf5