Re: textconv not invoked when viewing merge commit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jeff King <peff@xxxxxxxx> writes:

>> > 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]