Re: textconv not invoked when viewing merge commit

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2011, 21:05 UTC
Message-ID: <7vwriwfssc.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110414202356.GB6525@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

> On Thu, Apr 14, 2011 at 01:06:19PM -0700, Junio C Hamano wrote:
>
>> Instead, I think we should just use "Binary blob $SHA-1\n" as if that is
>> the textconv of a binary file without textconv filter.  That would
>> certainly make the code much simpler, and more importantly, the output
>> would become more pleasant. We would show something like:
>> 
>>     - Binary blob bc3c57058faba66f6a7a947e1e9642f47053b5bb
>>      -Binary blob 536e55524db72bd2acf175208aef4f3dfc148d42
>>     ++Binary blob 67cfeb2016b24df1cb406c18145efd399f6a1792
>> 
>> if we did so.
>
> Yeah, I think that is pretty readable. But it gives me a funny feeling
> to encode magic strings inside actual diff output. That is, the output
> is indistinguishable from a file which contained the "Binary blob..."
> strings.
>
> I can't think of a case where it matters, though, so maybe it is just
> paranoia.
>
> We do something similar for textconv, of course, but we always knew that
> was a human-only thing, and it isn't enabled for plumbing commands. This
> would be.

Yeah, that may be a sensible concern.

If we really cared, I would say that plumbing should keep the current behaviour (line-by-line even for binaries, and not using textconv unless it is asked). If the command line asked for --textconv, we can use that "Binary blob $SHA-1" string as a fallback textconv result for binary blobs that do not have any textconv filter configured. So the additional logic to convert the final image and parent images (two places to patch) would become more like:

	if (if we are a Porcelain or --textconv option given) {
		if (path has textconv)
                	use textconv;
		else if (path is binary)
                	use "Binary blob $SHA-1";
	}

Having said all that, I don't think we made -c/--cc available to plumbing on purpose; rather they happen to be available because we thought people with common sense wouldn't run things like "diff-tree --c" that are meant for human consumption and expect the result to be parsable by their scripts. In other words, making the parser barf only for plumbing was not worth doing.