Re: diffstat summary mode change bug

From: Stefan Beller <sbeller@google.com>
Date: Sep 27, 2017, 20:40 UTC
Message-ID: <CAGZ79kZF5w7ucbUCKJAuAbz2KBoMJjWneAxFcjOPeted8SBNmg@mail.gmail.com>
In-Reply-To: <CA+55aFzThd6cayUVuv39k=sb8KCphLkWGCV4n2HUjfn7Pxkd3g@mail.gmail.com>

On Wed, Sep 27, 2017 at 11:15 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> (ok, linewrapping in this email may make that look wrong - but the
> "mode change" land the "create mode" are both on the same line.

Thanks for the bug report.

> and the reason seems to be that the '\n' at the end got dropped as the
> old code was very confusing (the old code had two different '\n' cases
> for the "show filename or not").

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

Both search for "create mode" and "mode change" results in hits in the test suite, so we should have caught 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.

The code *looks* correct, but I am full of doubts now.

Stefan

Show 19 quoted lines

> I think the right fix is this whitespace-damaged trivial one-liner:
>
>   diff --git a/diff.c b/diff.c
>   index 3c6a3e0fa..653bb2e72 100644
>   --- a/diff.c
>   +++ b/diff.c
>   @@ -5272,6 +5272,7 @@ static void show_mode_change(struct
> diff_options *opt, struct diff_filepair *p,
>                           strbuf_addch(&sb, ' ');
>                           quote_c_style(p->two->path, &sb, NULL, 0);
>                   }
>   +               strbuf_addch(&sb, '\n');
>                   emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
>                                    sb.buf, sb.len, 0);
>                   strbuf_release(&sb);
>
> but somebody should double-check that.
>
>                         Linus