Re: [RFC/PATCH 1/4] show: obey --textconv for blobs

Michael J Gruber [off-list ref] writes:

Currently, "diff" and "cat-file" for blobs obey "--textconv" options
(with the former defaulting to "--textconv" and the latter to
"--no-textconv") whereas "show" does not obey this option, even though
it takes diff options.

Make "show" on blobs behave like "diff", i.e. obey "--textconv" by
default and "--no-textconv" when given.

What does "log -p" do currently, and what should it do?  Does/should
it also use --textconv?

The --textconv is a natural extension of what --ext-diff provides us,
so I think it should trigger the same way as how --ext-diff triggers.

We apply "--ext-diff" for "diff" by default but not for "log -p" and
"show"; I suspect this may have been for a good reason but I do not
recall the discussion that led to the current behaviour offhand.

On Wed, Feb 06, 2013 at 08:53:52AM -0800, Junio C Hamano wrote:

Michael J Gruber [off-list ref] writes:

quoted

Currently, "diff" and "cat-file" for blobs obey "--textconv" options
(with the former defaulting to "--textconv" and the latter to
"--no-textconv") whereas "show" does not obey this option, even though
it takes diff options.

Make "show" on blobs behave like "diff", i.e. obey "--textconv" by
default and "--no-textconv" when given.

What does "log -p" do currently, and what should it do?  Does/should
it also use --textconv?

The --textconv is a natural extension of what --ext-diff provides us,
so I think it should trigger the same way as how --ext-diff triggers.

We apply "--ext-diff" for "diff" by default but not for "log -p" and
"show"; I suspect this may have been for a good reason but I do not
recall the discussion that led to the current behaviour offhand.

I think Michael's commit message explains the situation badly.
--textconv is already on for "git show" (and for "git log") by default.
Diffs already use it.

This is more about the fact that when showing a single blob, we do not
bother to remember the context of the sha1 lookup, including its
pathname. Therefore we were not previously able to apply any
.gitattributes to the output. So this patch really does two things:

  1. Pass the information along to show_blob_object so that it can
     look up .gitattributes.

  2. Apply the textconv attribute (if ALLOW_TEXTCONV is on, of course).

And stating it that way makes it clear that there may be other missing
steps (3 and up) to apply other gitattributes. For example, should "git
show $blob" respect crlf attributes? Smudge filters?

-Peff

Junio C Hamano venit, vidit, dixit 06.02.2013 17:53:

Michael J Gruber [off-list ref] writes:

quoted

Currently, "diff" and "cat-file" for blobs obey "--textconv" options
(with the former defaulting to "--textconv" and the latter to
"--no-textconv") whereas "show" does not obey this option, even though
it takes diff options.

Make "show" on blobs behave like "diff", i.e. obey "--textconv" by
default and "--no-textconv" when given.

What does "log -p" do currently, and what should it do?  Does/should
it also use --textconv?

It invokes "--textconv" by default, and allows to override with
"--no-textconv. That's what I meant to say but seem to have failed to.

I think at some point we decided to have "human output" on by default
for all things diff (porcelain only, of course) unless the diff is meant
for machine consumption, such as in the case of format-patch.

The --textconv is a natural extension of what --ext-diff provides us,
so I think it should trigger the same way as how --ext-diff triggers.

This series' aim is to make the textconv behavior the same for all
"human output" commands. I don't see textconv and ext-diff to be
strongly related, and if then the other way round:

textconv is about converting blobs to a (possibly lossy) human
consumable text.

ext-diff is about computing diffs with an external diff "tool" (not in
the sense of diff-tool).

One way of diffing is textconving blobs then using internal diff on the
resulting text blobs, but ext-diff is more general and, really,
orthogonal in a way. (It used to be used often for what textconv does
when that didn't exist.)

We apply "--ext-diff" for "diff" by default but not for "log -p" and
"show"; I suspect this may have been for a good reason but I do not
recall the discussion that led to the current behaviour offhand.

I don't think I changed anything ext-diff related, did I? 1/4 is about
showing blobs, not diffs.

quoted

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/log.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8f0b2e8..f83870d 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	strbuf_release(&out);
 }
 
-static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
+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_TST(&rev->diffopt, ALLOW_TEXTCONV))
+		return stream_blob_to_fd(1, sha1, NULL, 0);
+
+	if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
+		die("Not a valid object name %s", obj_name);
+	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)

@@ -491,7 +509,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;