Re: textconv not invoked when viewing merge commit

From: Jeff King <peff@peff.net>
Date: Apr 16, 2011, 06:33 UTC
Message-ID: <20110416063353.GB28853@sigill.intra.peff.net>
In-Reply-To: <7v39lid8uz.fsf@alter.siamese.dyndns.org>

On Fri, Apr 15, 2011 at 11:10:44PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> >> And the ones that have been parsing cdiff wouldn't have done anything good
> >> before this change on such a binary blob anyway, no?
> >
> > No, but we can view the proposed change as fixing a bug for such a tool
> > Whereas turning it into:
> >
> >   --Binary blob XXX
> >   + Binary blob YYY
> >    +Binary blob ZZZ
> >
> > is codifying ambiguous output, and making the tool forever broken.
> 
> Of course, if we did this for a plumbing command and when the user did not
> ask for --textconv, I would agree with your argument.  Such an output
> makes it impossible to tell between the text files that had these lines
> and binary files.

OK, but what do you intend to do for a plumbing command _without_ --textconv? I think what it is doing now (pretending that lines in the binary file are relevant, and either truncating output on NUL or spewing NULs to the output stream) is just wrong.

The only reasonable thing I see there is inventing some combined-diff form of the "Binary files differ" message.

> What I am suggesting is to make any binary file use a fallback textconv
> "Binary blob $SHA-1", when the --textconv option is given from the command
> line and no textconv filter is configured for the path, in any textconv
> aware commands consistently, not limited to -c/--cc under discussion.

Ick, why? That pseudo-diff contains no additional interesting information that is not already there (since the "index" line already contains the blob sha1s). I suppose one could argue that it's more readable, but I don't find it so; I actually think it is less readable, because it makes you (even as a human, not a parsing script) think you are looking at a meaningful text diff.

And then on top of that is the fact that what we do now is consistent with other diff implementations, so people expect it.

Show 12 quoted lines

> With the current codebase, such a change *would* break a bog-standard,
> two-way "git diff" for a binary file; we do want to see the traditional
> "Binary files differ" by not using the fallback textconv, but we cannot
> tell if the --textconv option was explicitly given from the command line
> with the test used in Michael's patch (i.e. ALLOW_TEXTCONV), because we
> set the bit by default for Porcelain commands.  And showing "-Binary X"
> followed by "-Binary Y" is simply wrong and ambiguous, of course, in such
> a case.  We need to be able to tell if an explicit --textconv was given or
> we have ALLOW_TEXTCONV merely because we are running a Porcelain.
> 
> But I suspect that isn't something we cannot fix---we can just use another
> bit to record that in the command line parser.

Sure, it would take some code tweaking, but it wouldn't be hard to get the behavior you are mentioning.

Show 8 quoted lines

> Once that is fixed, I don't think giving "Binary files differ" when the
> line-counter script reads from a plumbing command that was invoked
> explicitly with the --textconv command is any better than giving the above
> three lines.  For a two-way merge, it does not matter much, but when
> viewing a merge with three or more parents, -c/--cc output that shows
> which sets of parents had the same blobs would be useful for humans (and
> tools) than a single "Binary files differ" output that does not tell any
> details.

Oh, sure. I am not proposing that "-c" should just say exactly "Binary files X and Y differ", only that we need a message _like_ that. I think it would be fine to represent which parents had which sha1, either in some structured format or even as text. I just think that making it look exactly like a text diff (even though, yes, that is a convenient structured format that we already have) is unnecessarily confusing to both humans and scripts.

-Peff