Thread (2 messages) flat view 2 messages, 2 authors, 2016-06-15

Re: textconv not invoked when viewing merge commit

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:03

Jeff King [off-list ref] writes:
quoted
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help