Re: [PATCH 0/25] detecting &&-chain breakage
On Sat, Mar 21, 2015 at 11:01:43AM -0700, Junio C Hamano wrote: > > Running: > > > > git diff origin origin/jk/test-chain-lint | > > perl diff-blame.pl jk/test-chain-lint | > > grep EOF > > > > was fun. At least I am not the only one. :) > > The parameter to diff-blame.pl should be origin, instead of > jk/test-chain-lint, I presume? You are grabbing the preimage line > numbers and asking blame to find out who wrote them. Yes, sorry, that was an error translating from what I actually ran in the shell into the email. It should be "origin". And if the script really wanted to be user-friendly, it should probably take two endpoints and just run the diff itself (when I started it, I assume that you could process any diff, but of course you must know the start point to get a reasonable blame). > > Nor the worst in the "severe" category. > > I do not quite get what this means---the script does not seem to > judge what is severe and what is not, so I presume that this is to > be judged by whoever is reading the output from the above pipeline > after replacing "grep EOF" with "less" or something? That was the exercise I left to the reader. :) In this case, it is possible because I have already split the patches into "severe", "moderate", and "trivial" cases, so you can blame only the severe patch (using its parent as the start-point). > > while () { > > if (m{^--- .*?/(.*)}) { > > This may match a removal of a line that begins with "^-- something/" ;-) True. I was trying to avoid being stateful in my diff parsing. I guess it would be enough to parse the hunk headers to know how many lines are in the hunk. Not worth it for this one-off, but a good thing if somebody wanted to pick this idea up for a "real" tool. > > # XXX coalesce blocks of adjacent lines into ranges? > > system(qw(git --no-pager blame), @ARGV, > > You may want to pass an option to always show the filename here. I left that to the user. I actually found "--line-porcelain" useful for gathering statistics (e.g., piped to "grep '^author ' | sort | uniq -c"). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
Jeff King writes: > Running: > > git diff origin origin/jk/test-chain-lint | > perl diff-blame.pl jk/test-chain-lint | > grep EOF > > was fun. At least I am not the only one. :) The parameter to diff-blame.pl should be origin, instead of jk/test-chain-lint, I presume? You are grabbing the preimage line numbers and asking blame to find out who wrote them. > Nor the worst in the "severe" category. I do not quite get what this means---the script does not seem to judge what is severe and what is not, so I presume that this is to be judged by whoever is reading the output from the above pipeline after replacing "grep EOF" with "less" or something? > # diff-blame.pl > use warnings FATAL => 'all'; > use strict; > > my $head = shift > or die "usage: git diff | $0 [blame-opts]"; > > my $file; > my @lines; > my $line_nr; > > while () { > if (m{^--- .*?/(.*)}) { This may match a removal of a line that begins with "^-- something/" ;-) > flush(); > $file = $1; > @lines = (); > } > elsif (m{^@@ -(\d+)}) { > $line_nr = $1; > } > elsif (m{^ }) { > $line_nr++; > } > elsif (m{^-}) { > push @lines, $line_nr++; > } > } > flush(); > exit 0; > > sub flush { > return unless defined $file && @lines > 0; > > # XXX coalesce blocks of adjacent lines into ranges? > system(qw(git --no-pager blame), @ARGV, You may want to pass an option to always show the filename here. > (map { "-L$_,$_" } @lines), $head, $file); > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
On Fri, Mar 20, 2015 at 07:18:47PM -0400, Eric Sunshine wrote: > Ironically, one of the broken here-doc &&-links you detected with > --chain-lint and fixed in 4/25 was from a patch from me: 5a9830cb > (t8001/t8002 (blame): add blame -L :funcname tests, 2013-07-17). Heh. I was afraid to look at my own. I wondered if we had a good way of finding who wrote the specific lines that were changed in a patch. I wrote the script below before realizing that contrib/contacts does something similar (though I think it blames whole hunks, including context, and I really want to care specifically only about touched lines; I wonder if that is a distinction git-contacts should make). Running: git diff origin origin/jk/test-chain-lint | perl diff-blame.pl jk/test-chain-lint | grep EOF was fun. At least I am not the only one. :) Nor the worst in the "severe" category. I will leave using the script to read the list of "severe" names as an exercise to the reader; reading such output is probably not overly healthy (and besides, to be fair it really should be normalized over the number of contributions). -- >8 -- # diff-blame.pl use warnings FATAL => 'all'; use strict; my $head = shift or die "usage: git diff | $0 [blame-opts]"; my $file; my @lines; my $line_nr; while () { if (m{^--- .*?/(.*)}) { flush(); $file = $1; @lines = (); } elsif (m{^@@ -(\d+)}) { $line_nr = $1; } elsif (m{^ }) { $line_nr++; } elsif (m{^-}) { push @lines, $line_nr++; } } flush(); exit 0; sub flush { return unless defined $file && @lines > 0; # XXX coalesce blocks of adjacent lines into ranges? system(qw(git --no-pager blame), @ARGV, (map { "-L$_,$_" } @lines), $head, $file); } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
On Fri, Mar 20, 2015 at 6:04 AM, Jeff King wrote: > [...] > There were a number of false positives, though as a percentage of the > test suite, probably not many (it's just that we have quite a lot of > tests). Most of them were in rather old tests, and IMHO the fixes I did > actually improved the readability of the result. So overall I think this > is a very positive change; I doubt it will get in people's way very > often, and I look forward to having one less thing to worry about > handling manually in review. The biggest downside is that I may have > automated Eric Sunshine out of a job. :) Heh. I won't mind. Thanks for doing a thorough job. Ironically, one of the broken here-doc &&-links you detected with --chain-lint and fixed in 4/25 was from a patch from me: 5a9830cb (t8001/t8002 (blame): add blame -L :funcname tests, 2013-07-17). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
Jeff King writes: > I'm actually about to send out a re-roll of that, as I think all of the > review comments have been addressed. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
On Fri, Mar 20, 2015 at 11:00:05AM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > I found 2026 and 5312 to be broken (there may be others that are > > excluded in my usual test set) in 'pu'. As to these topics in "git > > log --first-parent master..pu", my preference is to queue fixups on > > the broken-out topics (available at http://github.com/gitster/git) > > independently. > > > > For example, this will go on top of nd/multiple-work-trees. Heh, I just crossed emails with you over the same patch. > ... and this goes on top of jk/prune-with-corrupt-refs until it is > rerolled. I'm actually about to send out a re-roll of that, as I think all of the review comments have been addressed. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
Junio C Hamano writes: > I found 2026 and 5312 to be broken (there may be others that are > excluded in my usual test set) in 'pu'. As to these topics in "git > log --first-parent master..pu", my preference is to queue fixups on > the broken-out topics (available at http://github.com/gitster/git) > independently. > > For example, this will go on top of nd/multiple-work-trees. ... and this goes on top of jk/prune-with-corrupt-refs until it is rerolled. -- >8 -- Subject: [PATCH] SQUASH??? t5312: fix broken &&-chain Signed-off-by: Junio C Hamano --- t/t5312-prune-corruption.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 8b54d16..9633d97 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -80,7 +80,7 @@ test_expect_success 'pack-refs does not silently delete broken loose ref' ' # skip processing a broken ref test_expect_success 'create packed-refs file with broken ref' ' rm -f .git/refs/heads/master && - cat >.git/packed-refs <<-EOF + cat >.git/packed-refs <<-EOF && $missing refs/heads/master $recoverable refs/heads/other EOF -- 2.3.3-401-g402122f -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
On Fri, Mar 20, 2015 at 03:28:11PM +0100, Michael J Gruber wrote: > With 1/25 only, I get 163 dubious or failed on current next. > With 1/25 and only chain-lint without running the actual test loads, I > get 213. > > So, just as Jeff explained, we don't want a "chain-lint-only mode" > because it does not give correct results. Thanks for checking. I had assumed there would be some weirdness, but I didn't actually try a lint-only mode. I only ran the lint against master earlier. There are two trivial broken chains in pu. Patch is below (against nd/multiple-work-trees). Looks like that is in 'next', so we can't just squash it in. -- >8 -- Subject: t2026: fix broken &&-chains These are totally trivial (test_when_finished should never fail), but being complete with our &&-chaining makes the new --chain-lint feature more useful. Signed-off-by: Jeff King --- t/t2026-prune-linked-checkouts.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh index 2936d52..e885baf 100755 --- a/t/t2026-prune-linked-checkouts.sh +++ b/t/t2026-prune-linked-checkouts.sh @@ -65,7 +65,7 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' ' ' test_expect_success 'not prune locked checkout' ' - test_when_finished rm -r .git/worktrees + test_when_finished rm -r .git/worktrees && mkdir -p .git/worktrees/ghi && : >.git/worktrees/ghi/locked && git prune --worktrees && @@ -73,7 +73,7 @@ test_expect_success 'not prune locked checkout' ' ' test_expect_success 'not prune recent checkouts' ' - test_when_finished rm -r .git/worktrees + test_when_finished rm -r .git/worktrees && mkdir zz && mkdir -p .git/worktrees/jlm && echo "$(pwd)"/zz >.git/worktrees/jlm/gitdir && -- 2.3.3.520.g3cfbb5d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
Thanks. They applied cleanly on 'master' and all looked sensible. I found 2026 and 5312 to be broken (there may be others that are excluded in my usual test set) in 'pu'. As to these topics in "git log --first-parent master..pu", my preference is to queue fixups on the broken-out topics (available at http://github.com/gitster/git) independently. For example, this will go on top of nd/multiple-work-trees. diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh index 2936d52..e885baf 100755 --- a/t/t2026-prune-linked-checkouts.sh +++ b/t/t2026-prune-linked-checkouts.sh @@ -65,7 +65,7 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' ' ' test_expect_success 'not prune locked checkout' ' - test_when_finished rm -r .git/worktrees + test_when_finished rm -r .git/worktrees && mkdir -p .git/worktrees/ghi && : >.git/worktrees/ghi/locked && git prune --worktrees && @@ -73,7 +73,7 @@ test_expect_success 'not prune locked checkout' ' ' test_expect_success 'not prune recent checkouts' ' - test_when_finished rm -r .git/worktrees + test_when_finished rm -r .git/worktrees && mkdir zz && mkdir -p .git/worktrees/jlm && echo "$(pwd)"/zz >.git/worktrees/jlm/gitdir && -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/25] detecting &&-chain breakage
Jeff King venit, vidit, dixit 20.03.2015 11:04: > This is a cleanup of the &&-chain lint I posted earlier: > > http://thread.gmane.org/gmane.comp.version-control.git/265613/focus=265859 > > I don't know who came up with the idea for it originally, but the > concept certainly was floating in the back of my mind from Jonathan's > earlier version that is referenced in that thread. > > The general idea is to detect &&-chain breakage that can lead to our > tests yielding false success. The first patch implements and discusses > the lint-check itself, which is quite simple. The bulk of the work was > fixing all of the issues in the existing tests. :) > > That didn't all need to happen immediately. I mainly wanted to start on > it to answer two questions: > > 1. Are &&-chain breakages actually preventing us from seeing any test > failures? Or is it mostly just pedantry, and we miss out only on > knowing whether "cat >expect <<-\EOF" failed (which presumably it > never does). > > 2. How bad are the false positives? Both how common, and how bad to > work around. > > But after a few hours, I reached a zen state and just kept going. So at > the end of this series, the whole test suite is --chain-lint clean > (modulo any tests that are skipped on my platform). We could even switch > the checks on by default at the end of the series, but I didn't do that > here. I think it would be sane to run them all the time, though; in the > normal success case, they don't add any forks (the shell just runs > "(exit) && ...", and realizes that the whole thing is one big &&-chain). > I couldn't measure any time difference running the suite with and > without it. > > Anyway, to answer the questions: Yes, there were definitely tests whose > values were being thrown away, and we would not have noticed if they > failed. The good news is that all of them did pass once we started > checking their results. Hooray. > > There were a number of false positives, though as a percentage of the > test suite, probably not many (it's just that we have quite a lot of > tests). Most of them were in rather old tests, and IMHO the fixes I did > actually improved the readability of the result. So overall I think this > is a very positive change; I doubt it will get in people's way very > often, and I look forward to having one less thing to worry about > handling manually in review. The biggest downside is that I may have > automated Eric Sunshine out of a job. :) > > The patches are: > > [01/25]: t/test-lib: introduce --chain-lint option > [02/25]: t: fix severe &&-chain breakage > [03/25]: t: fix moderate &&-chain breakage > [04/25]: t: fix trivial &&-chain breakage > [05/25]: t: assume test_cmp produces verbose output > [06/25]: t: use verbose instead of hand-rolled errors > [07/25]: t: use test_must_fail instead of hand-rolled blocks > [08/25]: t: fix &&-chaining issues around setup which might fail > [09/25]: t: use test_might_fail for diff and grep > [10/25]: t: use test_expect_code instead of hand-rolled comparison > [11/25]: t: wrap complicated expect_code users in a block > [12/25]: t: avoid using ":" for comments > [13/25]: t3600: fix &&-chain breakage for setup commands > [14/25]: t7201: fix &&-chain breakage > [15/25]: t9502: fix &&-chain breakage > [16/25]: t6030: use modern test_* helpers > [17/25]: t0020: use modern test_* helpers > [18/25]: t1301: use modern test_* helpers > [19/25]: t6034: use modern test_* helpers > [20/25]: t4117: use modern test_* helpers > [21/25]: t9001: use test_when_finished > [22/25]: t0050: appease --chain-lint > [23/25]: t7004: fix embedded single-quotes > [24/25]: t0005: fix broken &&-chains > [25/25]: t4104: drop hand-rolled error reporting > > It's a lot of patches, and a few of them are long. I tried to group > them by functionality rather than file (though as you can see, some of > the tests were unique enough snowflakes that it made sense to discuss > their issues separately). I'm hoping it should be an easy review, if > not a short one. > > I'll spare you the full per-file diffstat, but the total is a very > satisfying: > >84 files changed, 397 insertions(+), 647 deletions(-) > > That's a lot of changes in a lot of hunks, but most of them are in files > that haven't been touched in a while. The series merges cleanly with > "pu", so I don't think I've stepped on anyone's topics in flight. > > -Peff Nice series! In fact, it's a great place to learn many of the test suite features that we have nowadays. With 1/25 only, I get 163 dubious or failed on current next. With 1/25 and only chain-lint without running the actual test loads, I get 213. So, just as Jeff explained, we don't want a "chain-lint-only mode" because it does not give correct results. I reviewed the first 15 and they all look good. I skimmed through the rest and it appears good to (though this is no review). You run without git svn tests, obvi
Re: [PATCH 0/25] detecting &&-chain breakage
On Fri, Mar 20, 2015 at 06:04:30AM -0400, Jeff King wrote: > It's a lot of patches, and a few of them are long. I tried to group > them by functionality rather than file (though as you can see, some of > the tests were unique enough snowflakes that it made sense to discuss > their issues separately). I'm hoping it should be an easy review, if > not a short one. I hoped that would make it easier to review, but I was also curious about the patterns I would see. Here are a few things I noticed: - the single most common place to forget an "&&" was at the start of a here-doc I complained earlier that I would prefer a way to put it at the end. And indeed, I found somebody who agreed with me and was more clever than I. They wrote in one test: (cat >expect < foo" redirects when I was touching those particular lines :) ). Which is all to say, please don't mention ugly style issues you see in the context lines during review, unless it is to point me to your patch series that comes on top of mine and cleans them up. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/25] detecting &&-chain breakage
This is a cleanup of the &&-chain lint I posted earlier: http://thread.gmane.org/gmane.comp.version-control.git/265613/focus=265859 I don't know who came up with the idea for it originally, but the concept certainly was floating in the back of my mind from Jonathan's earlier version that is referenced in that thread. The general idea is to detect &&-chain breakage that can lead to our tests yielding false success. The first patch implements and discusses the lint-check itself, which is quite simple. The bulk of the work was fixing all of the issues in the existing tests. :) That didn't all need to happen immediately. I mainly wanted to start on it to answer two questions: 1. Are &&-chain breakages actually preventing us from seeing any test failures? Or is it mostly just pedantry, and we miss out only on knowing whether "cat >expect <<-\EOF" failed (which presumably it never does). 2. How bad are the false positives? Both how common, and how bad to work around. But after a few hours, I reached a zen state and just kept going. So at the end of this series, the whole test suite is --chain-lint clean (modulo any tests that are skipped on my platform). We could even switch the checks on by default at the end of the series, but I didn't do that here. I think it would be sane to run them all the time, though; in the normal success case, they don't add any forks (the shell just runs "(exit) && ...", and realizes that the whole thing is one big &&-chain). I couldn't measure any time difference running the suite with and without it. Anyway, to answer the questions: Yes, there were definitely tests whose values were being thrown away, and we would not have noticed if they failed. The good news is that all of them did pass once we started checking their results. Hooray. There were a number of false positives, though as a percentage of the test suite, probably not many (it's just that we have quite a lot of tests). Most of them were in rather old tests, and IMHO the fixes I did actually improved the readability of the result. So overall I think this is a very positive change; I doubt it will get in people's way very often, and I look forward to having one less thing to worry about handling manually in review. The biggest downside is that I may have automated Eric Sunshine out of a job. :) The patches are: [01/25]: t/test-lib: introduce --chain-lint option [02/25]: t: fix severe &&-chain breakage [03/25]: t: fix moderate &&-chain breakage [04/25]: t: fix trivial &&-chain breakage [05/25]: t: assume test_cmp produces verbose output [06/25]: t: use verbose instead of hand-rolled errors [07/25]: t: use test_must_fail instead of hand-rolled blocks [08/25]: t: fix &&-chaining issues around setup which might fail [09/25]: t: use test_might_fail for diff and grep [10/25]: t: use test_expect_code instead of hand-rolled comparison [11/25]: t: wrap complicated expect_code users in a block [12/25]: t: avoid using ":" for comments [13/25]: t3600: fix &&-chain breakage for setup commands [14/25]: t7201: fix &&-chain breakage [15/25]: t9502: fix &&-chain breakage [16/25]: t6030: use modern test_* helpers [17/25]: t0020: use modern test_* helpers [18/25]: t1301: use modern test_* helpers [19/25]: t6034: use modern test_* helpers [20/25]: t4117: use modern test_* helpers [21/25]: t9001: use test_when_finished [22/25]: t0050: appease --chain-lint [23/25]: t7004: fix embedded single-quotes [24/25]: t0005: fix broken &&-chains [25/25]: t4104: drop hand-rolled error reporting It's a lot of patches, and a few of them are long. I tried to group them by functionality rather than file (though as you can see, some of the tests were unique enough snowflakes that it made sense to discuss their issues separately). I'm hoping it should be an easy review, if not a short one. I'll spare you the full per-file diffstat, but the total is a very satisfying: 84 files changed, 397 insertions(+), 647 deletions(-) That's a lot of changes in a lot of hunks, but most of them are in files that haven't been touched in a while. The series merges cleanly with "pu", so I don't think I've stepped on anyone's topics in flight. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html