Re: diffstat summary mode change bug

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sep 27, 2017, 21:02 UTC
Message-ID: <CA+55aFxwNOLaoeg64CYb-a6c89ceVbfc0b5vG_WCzyNBwynwfQ@mail.gmail.com>
In-Reply-To: <CAGZ79kZF5w7ucbUCKJAuAbz2KBoMJjWneAxFcjOPeted8SBNmg@mail.gmail.com>

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).

Show 6 quoted lines

> 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