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 => 100755tools/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 <[email protected]>
Date: Thu Jun 29 17:07:05 2017 -0700
diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY
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(structdiff_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
Quoted 2 lines · Linus Torvalds
(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.
Quoted 3 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 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
Quoted 19 lines
I think the right fix is this whitespace-damaged trivial one-liner:
diff --git
a/diff.cb/diff.c
index3c6a3e0fa..653bb2e72100644
---a/diff.c
+++b/diff.c
@@ -5272,6 +5272,7 @@ static void show_mode_change(structdiff_options*opt, structdiff_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
Quoted 4 lines · Stefan Beller
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).
Quoted 6 lines
I wonder why our tests failed to tell us about this.
Specifically we have t4100/t-apply-4.expect
mode change 100644 => 100755t/t0000-basic.sh
mode change 100644 => 100755t/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
Quoted 28 lines · Linus Torvalds
Quoted 4 lines · Stefan Beller
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_namecase).
Quoted 5 lines
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.
Quoted 8 lines
Quoted 7 lines
I wonder why our tests failed to tell us about this.
Specifically we have t4100/t-apply-4.expect
mode change 100644 => 100755t/t0000-basic.sh
mode change 100644 => 100755t/test-lib.sh
which would seem to exercise this code path.
Quoted 6 lines
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.cvsdiff.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 1/1 diff: correct newline in summary for renamed files
From: Linus Torvalds <[email protected]>
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.
---
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.c +1 -0
@@ -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);
t/t4016-diff-quote.sh +2 -0
@@ -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 '
Signature · 1 line
2.14.0.rc0.3.g6c2e499285
Re: [PATCH] diff: correct newline in summary for renamed files
Quoted 4 lines · Stefan Beller
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
In reply to Stefan BellerQuoted 10 lines · Stefan Beller
From: Linus Torvalds <[email protected]>
In
146fdb0dfe(diff.c:emit_diff_symbollearns aboutDIFF_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 <[email protected]>Signed-off-by Stefan Beller <[email protected]>
The overall substance looks good, but...
t/t4016-diff-quote.sh test_expect_success setup ' · line 36
Quoted 12 lines
t/t4016-diff-quote.sh +1
-0@@ -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
Quoted 17 lines · Jeff King
t/t4016-diff-quote.sh test_expect_success setup ' · line 36
Quoted 12 lines
t/t4016-diff-quote.sh +1
-0@@ -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.
Quoted 5 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.
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 --
t/t4013-diff-various.sh +12 -0
@@ -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
t/t4013/diff.diff-tree_--stat_initial_mode new file +4 -0
@@ -0,0 +1,4 @@ +$ git diff-tree --stat initial mode + file0 | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) +$
t/t4013/diff.diff-tree_--summary_initial_mode new file +3 -0
@@ -0,0 +1,3 @@ +$ git diff-tree --summary initial mode + mode change 100644 => 100755 file0 +$
t/t4013/diff.diff-tree_initial_mode new file +3 -0
@@ -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
Quoted 7 lines · Jeff King
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
Quoted 7 lines · Stefan Beller
Quoted 5 lines · Jeff King
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".
Quoted 5 lines
Quoted 2 lines
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 1/1 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.
---
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.c +1 -0
@@ -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);
t/t4013-diff-various.sh +12 -0
@@ -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
t/t4013/diff.diff-tree_--stat_initial_mode new file +4 -0
@@ -0,0 +1,4 @@ +$ git diff-tree --stat initial mode + file0 | 0 + 1 file changed, 0 insertions(+), 0 deletions(-) +$
t/t4013/diff.diff-tree_--summary_initial_mode new file +3 -0
@@ -0,0 +1,3 @@ +$ git diff-tree --summary initial mode + mode change 100644 => 100755 file0 +$
t/t4013/diff.diff-tree_initial_mode new file +3 -0
@@ -0,0 +1,3 @@ +$ git diff-tree initial mode +:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M file0 +$
t/t4013/diff.log_--decorate=full_--all +6 -0
@@ -1,4 +1,10 @@ $ git log --decorate=full --all +commit b7e0bc69303b488b47deca799a7d723971dfa6cd (refs/heads/mode) +Author: A U Thor <[email protected]> +Date: Mon Jun 26 00:06:00 2006 +0000 + + update mode + commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange) Author: A U Thor <[email protected]> Date: Mon Jun 26 00:06:00 2006 +0000
t/t4013/diff.log_--decorate_--all +6 -0
@@ -1,4 +1,10 @@ $ git log --decorate --all +commit b7e0bc69303b488b47deca799a7d723971dfa6cd (mode) +Author: A U Thor <[email protected]> +Date: Mon Jun 26 00:06:00 2006 +0000 + + update mode + commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange) Author: A U Thor <[email protected]> Date: Mon Jun 26 00:06:00 2006 +0000
Signature · 1 line
2.14.0.rc0.3.g6c2e499285
Re: [PATCH] diff: correct newline in summary for renamed files
Quoted 9 lines · Stefan Beller
In
146fdb0dfe(diff.c:emit_diff_symbollearns aboutDIFF_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 <[email protected]>Helped-by Jeff King <[email protected]>Signed-off-by Stefan Beller <[email protected]>
Quoted 12 lines
--- 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
Thanks.
Re: [PATCH] diff: correct newline in summary for renamed files
In reply to Stefan BellerQuoted 47 lines · Stefan Beller
In
146fdb0dfe(diff.c:emit_diff_symbollearns aboutDIFF_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 <[email protected]>Helped-by Jeff King <[email protected]>Signed-off-by Stefan Beller <[email protected]>---
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,
Stefandiff.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_modediff.c +1
-0@@ -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);t/t4013-diff-various.sh +0
-0
t/t4013-diff-various.sh test_expect_success setup ' · line 97
Quoted 8 lines
t/t4013-diff-various.sh +5
-0@@ -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
Quoted 7 lines
t/t4013-diff-various.sh test_expect_success setup ' · line 97
Quoted 5 lines
t/t4013-diff-various.sh +5
-0+ 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
Quoted 7 lines · Stefan Beller
Quoted 7 lines
t/t4013-diff-various.sh test_expect_success setup ' · line 97
Quoted 5 lines
t/t4013-diff-various.sh +5
-0+ 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.
Quoted 3 lines
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.
Quoted 3 lines
So I would argue that
test_chmodmay 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
In reply to Ramsay JonesQuoted 4 lines · Ramsay Jones
t/t4013-diff-various.sh test_expect_success setup ' · line 97
Quoted 2 lines
t/t4013-diff-various.sh +2
-0+ 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
In reply to Stefan BellerQuoted 6 lines · Stefan Beller
In
146fdb0dfe(diff.c:emit_diff_symbollearns aboutDIFF_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.
Quoted 8 lines
Reported-by Linus Torvalds <[email protected]>Helped-by Jeff King <[email protected]>Signed-off-by Stefan Beller <[email protected]>---
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
In reply to Jeff KingQuoted 15 lines · Jeff King
Quoted 10 lines · Stefan Beller
From: Linus Torvalds <[email protected]>
In
146fdb0dfe(diff.c:emit_diff_symbollearns aboutDIFF_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 <[email protected]>Signed-off-by Stefan Beller <[email protected]>The overall substance looks good, but...
Quoted 16 lines
t/t4016-diff-quote.sh test_expect_success setup ' · line 36
Quoted 12 lines
t/t4016-diff-quote.sh +1
-0@@ -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.
Quoted 7 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
Quoted 20 lines · Linus Torvalds
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.cb/diff.c
index3c6a3e0fa..653bb2e72100644
---a/diff.c
+++b/diff.c
@@ -5272,6 +5272,7 @@ static void show_mode_change(structdiff_options*opt, structdiff_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);
Quoted 3 lines
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.