diffstat summary mode change bug

diffstat summary mode change bug

[Linus Torvalds]

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 <sbeller@google.com>
  Date:   Thu Jun 29 17:07:05 2017 -0700

      diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY

      Signed-off-by: Stefan Beller <sbeller@google.com>
      Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

Re: diffstat summary mode change bug

[Stefan Beller]

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

> 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

Re: diffstat summary mode change bug

[Linus Torvalds]

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

> 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

Re: diffstat summary mode change bug

[Stefan Beller]

On Wed, Sep 27, 2017 at 2:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> 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).

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

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

[PATCH] diff: correct newline in summary for renamed files

[Stefan Beller]

From: Linus Torvalds <torvalds@linux-foundation.org>

In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
2017-06-29), the conversion from direct printing to the symbol emission
dropped the new line character for renamed, copied and rewritten files.

Add the emission of a newline, add a test for this case.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Linus, I assumed your sign off for the original patch. Thanks for spotting.
 
 Adding the mode change to t4016 seems like the easiest way to test it.
 
 Thanks,
 Stefan
  
 diff.c                | 1 +
 t/t4016-diff-quote.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/diff.c b/diff.c
index 3c6a3e0faa..653bb2e72e 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);
diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh
index 9c48e5c2c9..514056dd10 100755
--- a/t/t4016-diff-quote.sh
+++ b/t/t4016-diff-quote.sh
@@ -30,6 +30,7 @@ test_expect_success setup '
 	git add . &&
 	git commit -m initial &&
 	git mv "$P0.0" "R$P0.0" &&
+	chmod a+x "R$P0.0" &&
 	git mv "$P0.1" "R$P1.0" &&
 	git mv "$P0.2" "R$P2.0" &&
 	git mv "$P0.3" "R$P3.0" &&
@@ -47,6 +48,7 @@ cat >expect <<\EOF
  rename pathname.2 => Rpathname with SP.0 (100%)
  rename "pathname\twith HT.2" => Rpathname with SP.1 (100%)
  rename pathname.0 => Rpathname.0 (100%)
+ mode change 100644 => 100755
  rename "pathname\twith HT.0" => Rpathname.1 (100%)
 EOF
 '
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH] diff: correct newline in summary for renamed files

[Linus Torvalds]

On Wed, Sep 27, 2017 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:
>
>  Linus, I assumed your sign off for the original patch. Thanks for spotting.
>
>  Adding the mode change to t4016 seems like the easiest way to test it.

Looks good to me, and you don't need to give me authorship credit.
Just a "Reported-by:" is fine by me.

But yes, you can also consider the patch signed off by me, although
with your test update, _most_ of the patch is yours so it feels kind
of stupid to mark me as author.

              Linus

Re: [PATCH] diff: correct newline in summary for renamed files

[Jeff King]

On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
> 2017-06-29), the conversion from direct printing to the symbol emission
> dropped the new line character for renamed, copied and rewritten files.
> 
> Add the emission of a newline, add a test for this case.
> 
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Stefan Beller <sbeller@google.com>

The overall substance looks good, but...

> diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh
> index 9c48e5c2c9..514056dd10 100755
> --- a/t/t4016-diff-quote.sh
> +++ b/t/t4016-diff-quote.sh
> @@ -30,6 +30,7 @@ test_expect_success setup '
>  	git add . &&
>  	git commit -m initial &&
>  	git mv "$P0.0" "R$P0.0" &&
> +	chmod a+x "R$P0.0" &&
>  	git mv "$P0.1" "R$P1.0" &&
>  	git mv "$P0.2" "R$P2.0" &&
>  	git mv "$P0.3" "R$P3.0" &&

Won't this chmod be a problem for platforms without an executable bit?
I think you'd need to use "update-index --chmod=+x" here, or require the
FILEMODE prereq.

The whole script is marked as !MINGW, so that makes it less of a
problem, but it's still possible have !FILEMODE on a Linux system, if
you're on a funny filesystem. That also seems like a good reason to make
sure this is in a script which is run more widely, since Windows folks
would want to run this test, too.

-Peff

Re: [PATCH] diff: correct newline in summary for renamed files

[Jeff King]

On Wed, Sep 27, 2017 at 06:09:25PM -0400, Jeff King wrote:

> > diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh
> > index 9c48e5c2c9..514056dd10 100755
> > --- a/t/t4016-diff-quote.sh
> > +++ b/t/t4016-diff-quote.sh
> > @@ -30,6 +30,7 @@ test_expect_success setup '
> >  	git add . &&
> >  	git commit -m initial &&
> >  	git mv "$P0.0" "R$P0.0" &&
> > +	chmod a+x "R$P0.0" &&
> >  	git mv "$P0.1" "R$P1.0" &&
> >  	git mv "$P0.2" "R$P2.0" &&
> >  	git mv "$P0.3" "R$P3.0" &&
> 
> Won't this chmod be a problem for platforms without an executable bit?
> I think you'd need to use "update-index --chmod=+x" here, or require the
> FILEMODE prereq.
> 
> The whole script is marked as !MINGW, so that makes it less of a
> problem, but it's still possible have !FILEMODE on a Linux system, if
> you're on a funny filesystem. That also seems like a good reason to make
> sure this is in a script which is run more widely, since Windows folks
> would want to run this test, too.

The most appropriate place seems like t4013. I tried adding to its big
list of tested formats, but it's quite fragile. The patch below is what
I came up with, but it still needs updated to cover the cases which call
"log --all".

I think we'd do better to just do a set of new tests at the end (or even
a new test script for diffing mode changes in various formats).

-- >8 --
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index d09acfe48e..c515e3e53f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -90,6 +90,14 @@ test_expect_success setup '
 	git commit -m "Rearranged lines in dir/sub" &&
 	git checkout master &&
 
+	GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
+	GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
+	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
+	git checkout -b mode initial &&
+	git update-index --chmod=+x file0 &&
+	git commit -m "update mode" &&
+	git checkout -f master &&
+
 	git config diff.renames false &&
 
 	git show-branch
@@ -192,6 +200,10 @@ diff-tree --pretty side
 diff-tree --pretty -p side
 diff-tree --pretty --patch-with-stat side
 
+diff-tree initial mode
+diff-tree --stat initial mode
+diff-tree --summary initial mode
+
 diff-tree master
 diff-tree -p master
 diff-tree -p -m master
diff --git a/t/t4013/diff.diff-tree_--stat_initial_mode b/t/t4013/diff.diff-tree_--stat_initial_mode
new file mode 100644
index 0000000000..0e5943c2c6
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--stat_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree --stat initial mode
+ file0 | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_--summary_initial_mode b/t/t4013/diff.diff-tree_--summary_initial_mode
new file mode 100644
index 0000000000..25846b6af8
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--summary_initial_mode
@@ -0,0 +1,3 @@
+$ git diff-tree --summary initial mode
+ mode change 100644 => 100755 file0
+$
diff --git a/t/t4013/diff.diff-tree_initial_mode b/t/t4013/diff.diff-tree_initial_mode
new file mode 100644
index 0000000000..c47c09423e
--- /dev/null
+++ b/t/t4013/diff.diff-tree_initial_mode
@@ -0,0 +1,3 @@
+$ git diff-tree initial mode
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file0
+$

Re: [PATCH] diff: correct newline in summary for renamed files

[Stefan Beller]

On Wed, Sep 27, 2017 at 3:32 PM, Jeff King <peff@peff.net> wrote:

> The most appropriate place seems like t4013. I tried adding to its big
> list of tested formats, but it's quite fragile. The patch below is what
> I came up with, but it still needs updated to cover the cases which call
> "log --all".
>
> I think we'd do better to just do a set of new tests at the end (or even
> a new test script for diffing mode changes in various formats).

Thanks for providing this patch, I think it is actually reasonable to
do it here. I can fixup the rest if you don't mind.

Re: [PATCH] diff: correct newline in summary for renamed files

[Jeff King]

On Wed, Sep 27, 2017 at 03:39:00PM -0700, Stefan Beller wrote:

> On Wed, Sep 27, 2017 at 3:32 PM, Jeff King <peff@peff.net> wrote:
> 
> > The most appropriate place seems like t4013. I tried adding to its big
> > list of tested formats, but it's quite fragile. The patch below is what
> > I came up with, but it still needs updated to cover the cases which call
> > "log --all".
> >
> > I think we'd do better to just do a set of new tests at the end (or even
> > a new test script for diffing mode changes in various formats).
> 
> Thanks for providing this patch, I think it is actually reasonable to
> do it here. I can fixup the rest if you don't mind.

OK. Please go ahead, then.

-Peff

[PATCH] diff: correct newline in summary for renamed files

[Stefan Beller]

In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
2017-06-29), the conversion from direct printing to the symbol emission
dropped the new line character for renamed, copied and rewritten files.

Add the emission of a newline, add a test for this case.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Peff, I am undecided about the added 'diff --stat' call as that
 uses a completely different code path and would not show the mode
 change, but I guess we can just use it to document the current state.
 
 Thanks,
 Stefan

 diff.c                                        |  1 +
 t/t4013-diff-various.sh                       | 12 ++++++++++++
 t/t4013/diff.diff-tree_--stat_initial_mode    |  4 ++++
 t/t4013/diff.diff-tree_--summary_initial_mode |  3 +++
 t/t4013/diff.diff-tree_initial_mode           |  3 +++
 t/t4013/diff.log_--decorate=full_--all        |  6 ++++++
 t/t4013/diff.log_--decorate_--all             |  6 ++++++
 7 files changed, 35 insertions(+)
 create mode 100644 t/t4013/diff.diff-tree_--stat_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_--summary_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_initial_mode

diff --git a/diff.c b/diff.c
index 3c6a3e0faa..653bb2e72e 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);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index d09acfe48e..c515e3e53f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -90,6 +90,14 @@ test_expect_success setup '
 	git commit -m "Rearranged lines in dir/sub" &&
 	git checkout master &&
 
+	GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
+	GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
+	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
+	git checkout -b mode initial &&
+	git update-index --chmod=+x file0 &&
+	git commit -m "update mode" &&
+	git checkout -f master &&
+
 	git config diff.renames false &&
 
 	git show-branch
@@ -192,6 +200,10 @@ diff-tree --pretty side
 diff-tree --pretty -p side
 diff-tree --pretty --patch-with-stat side
 
+diff-tree initial mode
+diff-tree --stat initial mode
+diff-tree --summary initial mode
+
 diff-tree master
 diff-tree -p master
 diff-tree -p -m master
diff --git a/t/t4013/diff.diff-tree_--stat_initial_mode b/t/t4013/diff.diff-tree_--stat_initial_mode
new file mode 100644
index 0000000000..0e5943c2c6
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--stat_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree --stat initial mode
+ file0 | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
diff --git a/t/t4013/diff.diff-tree_--summary_initial_mode b/t/t4013/diff.diff-tree_--summary_initial_mode
new file mode 100644
index 0000000000..25846b6af8
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--summary_initial_mode
@@ -0,0 +1,3 @@
+$ git diff-tree --summary initial mode
+ mode change 100644 => 100755 file0
+$
diff --git a/t/t4013/diff.diff-tree_initial_mode b/t/t4013/diff.diff-tree_initial_mode
new file mode 100644
index 0000000000..c47c09423e
--- /dev/null
+++ b/t/t4013/diff.diff-tree_initial_mode
@@ -0,0 +1,3 @@
+$ git diff-tree initial mode
+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M	file0
+$
diff --git a/t/t4013/diff.log_--decorate=full_--all b/t/t4013/diff.log_--decorate=full_--all
index b345b2ebfa..2afe91f116 100644
--- a/t/t4013/diff.log_--decorate=full_--all
+++ b/t/t4013/diff.log_--decorate=full_--all
@@ -1,4 +1,10 @@
 $ git log --decorate=full --all
+commit b7e0bc69303b488b47deca799a7d723971dfa6cd (refs/heads/mode)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode
+
 commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange)
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:06:00 2006 +0000
diff --git a/t/t4013/diff.log_--decorate_--all b/t/t4013/diff.log_--decorate_--all
index 3aa16a9e42..d0f308ab2b 100644
--- a/t/t4013/diff.log_--decorate_--all
+++ b/t/t4013/diff.log_--decorate_--all
@@ -1,4 +1,10 @@
 $ git log --decorate --all
+commit b7e0bc69303b488b47deca799a7d723971dfa6cd (mode)
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:06:00 2006 +0000
+
+    update mode
+
 commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange)
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:06:00 2006 +0000
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH] diff: correct newline in summary for renamed files

[Jonathan Nieder]

Stefan Beller wrote:

> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
> 2017-06-29), the conversion from direct printing to the symbol emission
> dropped the new line character for renamed, copied and rewritten files.
>
> Add the emission of a newline, add a test for this case.
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  diff.c                                        |  1 +
>  t/t4013-diff-various.sh                       | 12 ++++++++++++
>  t/t4013/diff.diff-tree_--stat_initial_mode    |  4 ++++
>  t/t4013/diff.diff-tree_--summary_initial_mode |  3 +++
>  t/t4013/diff.diff-tree_initial_mode           |  3 +++
>  t/t4013/diff.log_--decorate=full_--all        |  6 ++++++
>  t/t4013/diff.log_--decorate_--all             |  6 ++++++
>  7 files changed, 35 insertions(+)
>  create mode 100644 t/t4013/diff.diff-tree_--stat_initial_mode
>  create mode 100644 t/t4013/diff.diff-tree_--summary_initial_mode
>  create mode 100644 t/t4013/diff.diff-tree_initial_mode

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH] diff: correct newline in summary for renamed files

[Ramsay Jones]

On 27/09/17 23:51, Stefan Beller wrote:
> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
> 2017-06-29), the conversion from direct printing to the symbol emission
> dropped the new line character for renamed, copied and rewritten files.
> 
> Add the emission of a newline, add a test for this case.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>  Peff, I am undecided about the added 'diff --stat' call as that
>  uses a completely different code path and would not show the mode
>  change, but I guess we can just use it to document the current state.
>  
>  Thanks,
>  Stefan
> 
>  diff.c                                        |  1 +
>  t/t4013-diff-various.sh                       | 12 ++++++++++++
>  t/t4013/diff.diff-tree_--stat_initial_mode    |  4 ++++
>  t/t4013/diff.diff-tree_--summary_initial_mode |  3 +++
>  t/t4013/diff.diff-tree_initial_mode           |  3 +++
>  t/t4013/diff.log_--decorate=full_--all        |  6 ++++++
>  t/t4013/diff.log_--decorate_--all             |  6 ++++++
>  7 files changed, 35 insertions(+)
>  create mode 100644 t/t4013/diff.diff-tree_--stat_initial_mode
>  create mode 100644 t/t4013/diff.diff-tree_--summary_initial_mode
>  create mode 100644 t/t4013/diff.diff-tree_initial_mode
> 
> diff --git a/diff.c b/diff.c
> index 3c6a3e0faa..653bb2e72e 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);
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index d09acfe48e..c515e3e53f 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -90,6 +90,14 @@ test_expect_success setup '
>  	git commit -m "Rearranged lines in dir/sub" &&
>  	git checkout master &&
>  
> +	GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
> +	GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
> +	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
> +	git checkout -b mode initial &&
> +	git update-index --chmod=+x file0 &&

would 'test_chmod +x file0 &&' work here?

ATB,
Ramsay Jones

Re: [PATCH] diff: correct newline in summary for renamed files

[Stefan Beller]

>> +     GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
>> +     GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
>> +     export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
>> +     git checkout -b mode initial &&
>> +     git update-index --chmod=+x file0 &&
>
> would 'test_chmod +x file0 &&' work here?

That is what I was looking for in my previous solution,
this one doesn't care about the on-disk things at all (regarding
the mode of files).

So I would argue that test_chmod may be overkill
(well we could drop the force flag from the following
checkout... not sure if that is a good trade off)

Re: [PATCH] diff: correct newline in summary for renamed files

[Jeff King]

On Wed, Sep 27, 2017 at 04:57:40PM -0700, Stefan Beller wrote:

> >> +     GIT_AUTHOR_DATE="2006-06-26 00:06:00 +0000" &&
> >> +     GIT_COMMITTER_DATE="2006-06-26 00:06:00 +0000" &&
> >> +     export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
> >> +     git checkout -b mode initial &&
> >> +     git update-index --chmod=+x file0 &&
> >
> > would 'test_chmod +x file0 &&' work here?

Oh, I forgot we had that.

> That is what I was looking for in my previous solution,
> this one doesn't care about the on-disk things at all (regarding
> the mode of files).

It still wouldn't have helped there. It does an on-disk chmod and an
index update, with the assumption that the chmod will be a noop on some
systems. So it's good if you want to put the executable bit in the index
and you'd like the working tree to match if it can, or have its change
ignored otherwise.

But if you care about seeing a diff between the working tree and index,
that still wouldn't work.

> So I would argue that test_chmod may be overkill
> (well we could drop the force flag from the following
> checkout... not sure if that is a good trade off)

The "-f" certainly caught me (I only added it after seeing the test
fail). So maybe it's worth doing. I doubt it's all that big a deal
either way.

-Peff

Re: [PATCH] diff: correct newline in summary for renamed files

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> +	git checkout -b mode initial &&
>> +	git update-index --chmod=+x file0 &&
>
> would 'test_chmod +x file0 &&' work here?

This one wants to set +x only in the index, leaving the working tree
version without the executable bit.  test_chmod is about setting
both, so it would do something different from what we want to do
here.

Re: [PATCH] diff: correct newline in summary for renamed files

[Jeff King]

On Wed, Sep 27, 2017 at 03:51:26PM -0700, Stefan Beller wrote:

> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
> 2017-06-29), the conversion from direct printing to the symbol emission
> dropped the new line character for renamed, copied and rewritten files.
> 
> Add the emission of a newline, add a test for this case.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>  Peff, I am undecided about the added 'diff --stat' call as that
>  uses a completely different code path and would not show the mode
>  change, but I guess we can just use it to document the current state.

Yeah, I agree it isn't showing much. I was mostly thinking it just made
sense to test a bunch of formats across a mode change to make sure they
all showed something reasonable. But it's a rather unlikely bug that
"--stat" would _start_ showing something that it's not supposed.

I suppose that "--patch" should also possibly be included, though I'd be
surprised if that isn't covered elsewhere (but then, I was surprised
that --summary wasn't covered either).

-Peff

Re: [PATCH] diff: correct newline in summary for renamed files

[Stefan Beller]

On Wed, Sep 27, 2017 at 3:09 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote:
>
>> From: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
>> 2017-06-29), the conversion from direct printing to the symbol emission
>> dropped the new line character for renamed, copied and rewritten files.
>>
>> Add the emission of a newline, add a test for this case.
>>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> The overall substance looks good, but...
>
>> diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh
>> index 9c48e5c2c9..514056dd10 100755
>> --- a/t/t4016-diff-quote.sh
>> +++ b/t/t4016-diff-quote.sh
>> @@ -30,6 +30,7 @@ test_expect_success setup '
>>       git add . &&
>>       git commit -m initial &&
>>       git mv "$P0.0" "R$P0.0" &&
>> +     chmod a+x "R$P0.0" &&
>>       git mv "$P0.1" "R$P1.0" &&
>>       git mv "$P0.2" "R$P2.0" &&
>>       git mv "$P0.3" "R$P3.0" &&
>
> Won't this chmod be a problem for platforms without an executable bit?
> I think you'd need to use "update-index --chmod=+x" here, or require the
> FILEMODE prereq.

I was experimenting with git add --chmod=+x for this patch, but as this
test runs "diff --summary -M HEAD", we need it change don the fs.

So let's find another test. Changing the setup of t4013 is a lot of work,
but maybe worth it? Looking into t4031, that would also work.

To be fs agnostic, we can only compare commits against each other.

> The whole script is marked as !MINGW, so that makes it less of a
> problem, but it's still possible have !FILEMODE on a Linux system, if
> you're on a funny filesystem. That also seems like a good reason to make
> sure this is in a script which is run more widely, since Windows folks
> would want to run this test, too.
>
> -Peff

Re: diffstat summary mode change bug

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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.