Re: textconv not invoked when viewing merge commit
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 16, 2011, 06:10 UTC
- Message-ID
- <7v39lid8uz.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110416014758.GB23306@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 14 quoted lines
>> > Well, I know no tool parsing combined diff actually, so it's indeed a >> > hypothetical case. >> >> 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.
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.
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.
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. The line-counter script would be counting "forever broken" data when you feed your JPEG collection with exif extracting textconv filter anyway, and I do not necessarily think it would make things worse to give a fallback textconv filter to binary files that do not have one defined.