Re: diffstat summary mode change bug
Linus Torvalds writes: > 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 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 Thanks for being extra gentle by mentioning "old code was very confusing", but it was not necessary. We should have caught this breakage during the review, but I somehow failed to spot it. My bad. Thanks for a fix.
Re: diffstat summary mode change bug
On Wed, Sep 27, 2017 at 2:02 PM, Linus Torvalds
wrote:
> On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller 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/t-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
Re: diffstat summary mode change bug
On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller 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/t-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
Re: diffstat summary mode change bug
On Wed, Sep 27, 2017 at 11:15 AM, Linus Torvalds 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/t-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 > 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
diffstat summary mode change bug
Current git shows file-mode changes incorrectly in the diffstat summary, as I just noted from a pull request I did on the kernel. The pull request *should* have resulted in a summary like this: ... 21 files changed, 247 insertions(+), 67 deletions(-) mode change 100644 => 100755 tools/testing/selftests/memfd/run_tests.sh create mode 100644 tools/testing/selftests/net/reuseaddr_conflict.c but instead that "mode change" line didn't have a newline at the end, and I got ... 21 files changed, 247 insertions(+), 67 deletions(-) mode change 100644 => 100755 tools/testing/selftests/memfd/run_tests.sh create mode 100644 tools/testing/selftests/net/reuseaddr_conflict.c (ok, linewrapping in this email may make that look wrong - but the "mode change" land the "create mode" are both on the same line. Bisecting it got me: 146fdb0dfe445464fa438f3835557c58a01d85d7 is the first bad commit commit 146fdb0dfe445464fa438f3835557c58a01d85d7 Author: Stefan Beller Date: Thu Jun 29 17:07:05 2017 -0700 diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano 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 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
