Re: textconv not invoked when viewing merge commit

From: Jeff King <peff@peff.net>
Date: Apr 14, 2011, 21:30 UTC
Message-ID: <20110414213006.GA7709@sigill.intra.peff.net>
In-Reply-To: <7vwriwfssc.fsf@alter.siamese.dyndns.org>

On Thu, Apr 14, 2011 at 02:05:07PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> > 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.
>[...]
> 
> 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).

I disagree. Spewing binary contents in the middle of patch output is wrong and a bug, and we should fix it. Not to mention that the results are simply incomprehensible in many cases. Binary data isn't line-oriented, and treating it that way is just going to produce confusing and useless results. Not to mention that I wouldn't be surprised if embedded NULs in the data are not being handled properly by the diff code.

I would much rather have it say "Binary files differ". It's not that informative, but at least you don't waste a lot of time trying to figure out what in the world it means.

Show 6 quoted lines

> 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.

Weren't they needed originally for "git rev-list | git diff-tree"? Maybe they post-date the invention of actual C "git log"; I didn't look. At any rate, they've been around for a while, and it is not unreasonable for somebody to want to script around the generation of human-readable output, so I think they are a good addition.

I think the real argument to be made is that "--cc" was never parseable, because it can't be applied, and users of the format should know that. I sort of buy that. Though you could also potentially do other kinds of analysis on --cc output (e.g., something blame-ish but totally external to git). And for that you wouldn't want to pretend content was there that isn't. It's an edge case, certainly, but I don't see any reason not to be conservative in what we generate. The "Binary files differ" type of output is not that much harder to generate.

-Peff