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 start end | $0 start [blame-opts];
my $file;
my @lines;
my $line_nr;
while (STDIN) {
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
Jeff King p...@peff.net 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 start end | $0 start [blame-opts];
my $file;
my @lines;
my $line_nr;
while (STDIN) {
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 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 (STDIN) {
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
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 EOF ... whatever ... EOF ) which I suspect would be less error-prone, but is also quite ugly. I actually pulled that out in favor of a more conventional form (ironically, I found it because the author screwed up the -operator a few lines above). With automated -checking, I don't think there's any need to contort our syntax. The test suite will remind us when we forget. - the other common place I noticed was at the trailing ) of a sub-shell - Running through old tests, I saw a lot of opportunity for cleanups that I resisted. Places where we could use verbose for better output, where test_cmp would be nicer than using test X = Y, where people had indented here-docs badly (mostly failing to use -, places where people had some arcane indentation style for the test titles and snippets themselves, etc. The series was already bordering on intractable, so I tried to leave those be (though I did fix cat 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
Re: [PATCH 0/25] detecting -chain breakage
On Fri, Mar 20, 2015 at 6:04 AM, Jeff King p...@peff.net 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 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, obviously... I'll follow up with two patches for those. Michael -- To unsubscribe from this list: send the line unsubscribe git in
Re: [PATCH 0/25] detecting -chain breakage
Jeff King p...@peff.net 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
Junio C Hamano gits...@pobox.com 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 gits...@pobox.com --- 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 p...@peff.net --- 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
On Fri, Mar 20, 2015 at 11:00:05AM -0700, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com 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
