Re: [PATCH] diff: correct newline in summary for renamed files
Ramsay Jones 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
On Wed, Sep 27, 2017 at 04:57:40PM -0700, Stefan Beller wrote: > >> + GIT_AUTHOR_DATE="2006-06-26 00:06:00 +" && > >> + GIT_COMMITTER_DATE="2006-06-26 00:06:00 +" && > >> + 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
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 > Helped-by: Jeff King > Signed-off-by: Stefan Beller > --- > > 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
>> + GIT_AUTHOR_DATE="2006-06-26 00:06:00 +" && >> + GIT_COMMITTER_DATE="2006-06-26 00:06:00 +" && >> + 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
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 > Helped-by: Jeff King > Signed-off-by: Stefan Beller > --- > > 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 +" && > + GIT_COMMITTER_DATE="2006-06-26 00:06:00 +" && > + 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 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 > Helped-by: Jeff King > Signed-off-by: Stefan Beller > --- > 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 Thanks.
[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 Helped-by: Jeff King Signed-off-by: Stefan Beller --- 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 +" && + GIT_COMMITTER_DATE="2006-06-26 00:06:00 +" && + 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 00..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 00..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 00..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 +Date: Mon Jun 26 00:06:00 2006 + + +update mode + commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange) Author: A U Thor Date: Mon Jun 26 00:06:00 2006 + 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 +Date: Mon Jun 26 00:06:00 2006 + + +update mode + commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange) Author: A U Thor Date: Mon Jun 26 00:06:00 2006 + -- 2.14.0.rc0.3.g6c2e499285
Re: [PATCH] diff: correct newline in summary for renamed files
On Wed, Sep 27, 2017 at 03:39:00PM -0700, Stefan Beller wrote: > On Wed, Sep 27, 2017 at 3:32 PM, Jeff King 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
Re: [PATCH] diff: correct newline in summary for renamed files
On Wed, Sep 27, 2017 at 3:32 PM, Jeff King 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
On Wed, Sep 27, 2017 at 3:09 PM, Jeff King wrote: > On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote: > >> From: Linus Torvalds >> >> 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 >> Signed-off-by: Stefan Beller > > 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: [PATCH] diff: correct newline in summary for renamed files
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 +" && + GIT_COMMITTER_DATE="2006-06-26 00:06:00 +" && + 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 00..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 00..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 00..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
On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote: > From: Linus Torvalds > > 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 > Signed-off-by: Stefan Beller 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
On Wed, Sep 27, 2017 at 2:58 PM, Stefan Beller 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
[PATCH] diff: correct newline in summary for renamed files
From: Linus Torvalds 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 Signed-off-by: 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. 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
