Re: diffstat summary mode change bug

On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller [off-list ref] 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 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).

                 Linus