On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller <sbel...@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 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
