Re: [PATCH 2/2] cat-file: handle --batch format with missing type/size

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 11, 2013, 20:42 UTC
Message-ID: <20131211204200.GN2311@google.com>
In-Reply-To: <20131211115844.GB10594@sigill.intra.peff.net>

Jeff King wrote:

> We could do the same for the type. However, besides our
> consistency check, we also care about the type in deciding
> whether to stream or not. We therefore make sure to always
> trigger a type lookup when we are printing, so that

This "We make sure" is the behavior after this patch, not before, right?

[...]

Show 6 quoted lines

> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -211,7 +211,7 @@ static void print_object_or_die(int fd, struct expand_data *data)
>  			die("object %s disappeared", sha1_to_hex(sha1));
>  		if (type != data->type)
>  			die("object %s changed type!?", sha1_to_hex(sha1));

Maybe an assert(data.info.typep) or similar would make this more locally readable.

[...]

Show 9 quoted lines

> @@ -276,6 +276,13 @@ static int batch_objects(struct batch_options *opt)
>  	data.mark_query = 0;
>  
> +	/*
> +	 * If we are printing out the object, then always fill in the type,
> +	 * since we will want to decide whether or not to stream.
> +	 */
> +	if (opt->print_contents)
> +		data.info.typep = &data.type;

Oof. I guess this means that the optimization from 98e2092b wasn't being applied by 'git cat-file --batch' with format specifiers that don't include %(objecttype), but no one would have noticed because of the "changed type" thing. :)

Show 29 quoted lines

> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -85,6 +85,28 @@ $content"
>  		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
>  	test_cmp expect actual
>      '
> +
> +    test -z "$content" ||
> +    test_expect_success "--batch without type ($type)" '
> +	{
> +		echo "$size" &&
> +		maybe_remove_timestamp "$content" $no_ts
> +	} >expect &&
> +	echo $sha1 | git cat-file --batch="%(objectsize)" >actual.full &&
> +	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> +	test_cmp expect actual
> +    '
> +
> +    test -z "$content" ||
> +    test_expect_success "--batch without size ($type)" '
> +	{
> +		echo "$type" &&
> +		maybe_remove_timestamp "$content" $no_ts
> +	} >expect &&
> +	echo $sha1 | git cat-file --batch="%(objecttype)" >actual.full &&
> +	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
> +	test_cmp expect actual
> +    '
>  }

Looks good.

(not about this patch) I suspect a test_cmp_ignore_timestamp helper could simplify these tests somewhat. :)

For what it's worth, with or without commit message changes or the check that data->type is initialized,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>