Re: [PATCHv3 3/7] show: honor --textconv for blobs

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

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Currently, "diff" and "cat-file" for blobs honor "--textconv" options
> (with the former defaulting to "--textconv" and the latter to
> "--no-textconv") whereas "show" does not honor this option, even though
> it takes diff options.
>
> Make "show" on blobs behave like "diff", i.e. honor "--textconv" by
> default and "--no-textconv" when given.

Hmm...

> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
>  {
> +	unsigned char sha1c[20];
> +	struct object_context obj_context;
> +	char *buf;
> +	unsigned long size;
> +
>  	fflush(stdout);
> -	return stream_blob_to_fd(1, sha1, NULL, 0);
> +	if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
> +	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
> +		return stream_blob_to_fd(1, sha1, NULL, 0);

It is surprising that the necessary change is only this, but I think
it is correct ;-).  We ignore textconv when the command line did not
mention --[no-]textconv, or the command line said --no-textconv
explicitly.

This (especially the first condition) may deserve an in-code comment
for anybody who wonders where this default behaviour is implemented.

So "show" on blobs does show the raw contents by default, but the
user can explicitly ask to enable textconv with --[no-]textconv.  Is
the second paragraph in the log message still valid?

> +	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
> +		die("Not a valid object name %s", obj_name);

This looks somewhat unfortunate.

We already have sha1[]; actually we not just know sha1[] but have
the struct object for it.  How did we obtain it before we got here?

Will we always have a valid name in rev.pending.objects->name?  Will
that name convert back to the same sha1 we got in sha1[]?

I think the answers are "Yes (it is a command line argument), Yes
(that is what setup_revisions() got by feeding the name to give us
sha1[])".

I wonder if enriching rev_info->pending with the context information
might be a clean solution to avoid this redundant but unavoidable
conversion, but that is a separate and future topic, I think.

> +	if (!obj_context.path[0] ||
> +	    !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size))
> +		return stream_blob_to_fd(1, sha1, NULL, 0);
> +
> +	if (!buf)
> +		die("git show %s: bad file", obj_name);
> +
> +	write_or_die(1, buf, size);
> +	return 0;
>  }
>  
>  static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
> @@ -526,7 +545,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  		const char *name = objects[i].name;
>  		switch (o->type) {
>  		case OBJ_BLOB:
> -			ret = show_blob_object(o->sha1, NULL);
> +			ret = show_blob_object(o->sha1, &rev, name);
>  			break;
>  		case OBJ_TAG: {
>  			struct tag *t = (struct tag *)o;
> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
> index 3950fc9..0ebb028 100755
> --- a/t/t4030-diff-textconv.sh
> +++ b/t/t4030-diff-textconv.sh
> @@ -96,14 +96,14 @@ test_expect_success 'show blob produces binary' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'show --textconv blob produces text' '
> +test_expect_success 'show --textconv blob produces text' '
>  	git show --textconv HEAD:file >actual &&
>  	printf "0\\n1\\n" >expect &&
>  	test_cmp expect actual
>  '
>  
> -test_success 'show --no-textconv blob produces binary' '
> -	git show --textconv HEAD:file >actual &&
> +test_expect_success 'show --no-textconv blob produces binary' '
> +	git show --no-textconv HEAD:file >actual &&
>  	printf "\\0\\n\\01\\n" >expect &&
>  	test_cmp expect actual
>  '