On Wed, Sep 27, 2017 at 2:02 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
>>
>> I disagree with this analysis, as the fix you propose adds the
>> new line unconditionally, i.e. this code path would be broken
>> regardless of "show filename or not".
>
> Right. Because it is what we want.
>
> The old code (before that commit) used to have two different cases:
>
> fprintf(file, "%s mode change %06o => %06o%c",
> line_prefix, p->one->mode,
> p->two->mode, show_name ? ' ' : '\n');
>
> ie if "show_name" was set, it would *not* print a newline, and print a
> space instead.
>
> But then on the very next line, it used to do:
>
> if (show_name) {
> write_name_quoted(p->two->path, file, '\n');
>
> ie now it prints the filename, and then prints the newline.
>
> End result: it used to *always* print the newline. Either it printed
> it at the end of the mode (for the non-show_name case), or it printed
> it at the end of the filename (for the show_name case).
>
> Your patch removed the '\n' entirely.
>
> My patch makes it unconditional, which it was before your patch (it
> was "conditional" only in where it was printed, not _whether_ it was
> printed).
I agree with this.
>
>> I wonder why our tests failed to tell us about this.
>>
>> Specifically we have t4100/t-apply-4.expect
>> mode change 100644 => 100755 t/t0000-basic.sh
>> mode change 100644 => 100755 t/test-lib.sh
>> which would seem to exercise this code path.
>
> That only tests "git apply --stat --summary".
>
> It doesn't test "git diff" at all.
>
> And the "mode change" printout is entirely different code (see apply.c
> vs diff.c).
Why doesn't this surprise me at all?
(Whenever I write emails I assume the best of Gits code base, such
as non-duplicated code. "It's all in the mighty diff machinery".)
In that case let me write a test for this and resubmit
your fix without whitespace mangling.
Thanks,
Stefan
