Re: diffstat summary mode change bug

[Stefan Beller <sbeller@google.com>]

On Wed, Sep 27, 2017 at 2:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller <sbeller@google.com> 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