Re: [PATCHv3 3/7] show: honor --textconv for blobs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 10, 2013, 17:02 UTC
- Message-ID
- <7vy5bm22f8.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <c4ed1e0b67877e6453b8c269290e09e1672ce37d.1368197380.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 7 quoted lines
> 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...
Show 12 quoted lines
> +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.
Show 43 quoted lines
> + 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
> '