threads / bug / 46846

diffstat summary mode change bug

Subject: diffstat summary mode change bug

## tl;dr

19 messages between Sep 27, 2017 and Sep 28, 2017.

replies: 18people: 6as markdown or json

Linus Torvalds· Sep 27, 2017, 18:15 UTC · lore

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
Stefan Beller· Sep 27, 2017, 20:40 UTC · re: Linus Torvalds · lore

Re: diffstat summary mode change bug

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
Linus Torvalds· Sep 27, 2017, 21:02 UTC · re: Stefan Beller · lore

Re: diffstat summary mode change bug

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
Stefan Beller· Sep 27, 2017, 21:09 UTC · re: Linus Torvalds · lore

Re: diffstat summary mode change bug

On Wed, Sep 27, 2017 at 2:02 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 33 quoted lines
> 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.
Show 14 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).

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

Stefan Beller· Sep 27, 2017, 21:58 UTC · re: Stefan Beller · lore

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

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
Linus Torvalds· Sep 27, 2017, 22:04 UTC · re: Stefan Beller · lore

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

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
Jeff King· Sep 27, 2017, 22:09 UTC · re: Stefan Beller · lore

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

On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote:
Show 10 quoted lines
> 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...
Show 12 quoted lines
> 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
Jeff King· Sep 27, 2017, 22:32 UTC · re: Jeff King · lore

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

On Wed, Sep 27, 2017 at 06:09:25PM -0400, Jeff King wrote:
Show 22 quoted lines
> > 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
+$
Stefan Beller· Sep 27, 2017, 22:39 UTC · re: Jeff King · lore

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

On Wed, Sep 27, 2017 at 3:32 PM, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> 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.

Jeff King· Sep 27, 2017, 22:49 UTC · re: Stefan Beller · lore

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

On Wed, Sep 27, 2017 at 03:39:00PM -0700, Stefan Beller wrote:
Show 12 quoted lines
> 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
Stefan Beller· Sep 27, 2017, 22:51 UTC · re: Jeff King · lore

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

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
Jonathan Nieder· Sep 27, 2017, 23:15 UTC · re: Stefan Beller · lore

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

Stefan Beller wrote:
Show 21 quoted lines
> 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.
Ramsay Jones· Sep 27, 2017, 23:49 UTC · re: Stefan Beller · lore

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

On 27/09/17 23:51, Stefan Beller wrote:
Show 55 quoted lines
> 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

Stefan Beller· Sep 27, 2017, 23:57 UTC · re: Ramsay Jones · lore

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

Show 7 quoted lines
>> +     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)

Jeff King· Sep 28, 2017, 00:39 UTC · re: Stefan Beller · lore

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

On Wed, Sep 27, 2017 at 04:57:40PM -0700, Stefan Beller wrote:
Show 7 quoted lines
> >> +     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
Junio C Hamano· Sep 28, 2017, 04:14 UTC · re: Ramsay Jones · lore

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

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.

Jeff King· Sep 28, 2017, 00:35 UTC · re: Stefan Beller · lore

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

On Wed, Sep 27, 2017 at 03:51:26PM -0700, Stefan Beller wrote:
Show 14 quoted lines
> 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
Stefan Beller· Sep 27, 2017, 22:34 UTC · re: Jeff King · lore

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

On Wed, Sep 27, 2017 at 3:09 PM, Jeff King <peff@peff.net> wrote:
Show 31 quoted lines
> 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.
Show 7 quoted lines
> 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
Junio C Hamano· Sep 28, 2017, 04:12 UTC · re: Linus Torvalds · lore

Re: diffstat summary mode change bug

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 23 quoted lines
> 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.

← back to recent threads