* [PATCH maint] git-svn: Fix git svn log --show-commit [not found] <87y622doa7.fsf@norang.ca> @ 2011-05-20 7:49 ` Michael J Gruber 2011-05-20 20:26 ` Andreas Schwab 2011-05-20 11:16 ` [PATCHv2 " Michael J Gruber 1 sibling, 1 reply; 6+ messages in thread From: Michael J Gruber @ 2011-05-20 7:49 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Wong, Bernt Hansen git svn log --show-commit had no tests and, consequently, no attention by the author of b1b4755 (git-log: put space after commit mark, 2011-03-10) who kept git svn log working only without --show-commit. Introduce a test and fix it. Reported-by: Bernt Hansen <bernt@norang.ca> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> --- git svn scares me. It exits with 0 even with these problems! Consequently, the tests in t9116 completely ignore the return codes because of the pipes. Patch directly on top of b1b4755. Thanks for the report! The fix replaces a group "(- )?" by "-? ?" which works in this case (though not being equivalent). It's easier than dealing with the problem the grouping introduced. --- git-svn.perl | 2 +- t/t9116-git-svn-log.sh | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index a5857c1..b1eef85 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -5734,7 +5734,7 @@ sub cmd_show_log { my (@k, $c, $d, $stat); my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/; while (<$log>) { - if (/^${esc_color}commit (- )?($::sha1_short)/o) { + if (/^${esc_color}commit -? ?($::sha1_short)/o) { my $cmt = $1; if ($c && cmt_showable($c) && $c->{r} != $r_last) { $r_last = $c->{r}; diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh index 5d477e4..cf4c052 100755 --- a/t/t9116-git-svn-log.sh +++ b/t/t9116-git-svn-log.sh @@ -60,6 +60,21 @@ test_expect_success 'test ascending revision range' " git svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 - " +test_expect_success 'test ascending revision range with --show-commit' " + git reset --hard trunk && + git svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 - + " + +test_expect_success 'test ascending revision range with --show-commit (sha1)' " + git svn find-rev r1 >expected-range-r1-r2-r4-sha1 && + git svn find-rev r2 >>expected-range-r1-r2-r4-sha1 && + git svn find-rev r4 >>expected-range-r1-r2-r4-sha1 && + git reset --hard trunk && + git svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f2 >out && + git rev-parse \$(cat out) >actual && + test_cmp expected-range-r1-r2-r4-sha1 actual + " + printf 'r4 \nr2 \nr1 \n' > expected-range-r4-r2-r1 test_expect_success 'test descending revision range' " -- 1.7.5.1.558.gc8bec ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH maint] git-svn: Fix git svn log --show-commit 2011-05-20 7:49 ` [PATCH maint] git-svn: Fix git svn log --show-commit Michael J Gruber @ 2011-05-20 20:26 ` Andreas Schwab 0 siblings, 0 replies; 6+ messages in thread From: Andreas Schwab @ 2011-05-20 20:26 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Junio C Hamano, Eric Wong, Bernt Hansen Michael J Gruber <git@drmicha.warpmail.net> writes: > The fix replaces a group "(- )?" by "-? ?" which works in this > case (though not being equivalent). It's easier than dealing > with the problem the grouping introduced. You could also use "(?:- )?" to make it a non-capturing group. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2 maint] git-svn: Fix git svn log --show-commit [not found] <87y622doa7.fsf@norang.ca> 2011-05-20 7:49 ` [PATCH maint] git-svn: Fix git svn log --show-commit Michael J Gruber @ 2011-05-20 11:16 ` Michael J Gruber 2011-05-20 11:48 ` Bernt Hansen 2011-05-20 16:17 ` Junio C Hamano 1 sibling, 2 replies; 6+ messages in thread From: Michael J Gruber @ 2011-05-20 11:16 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Wong, Bernt Hansen git svn log --show-commit had no tests and, consequently, no attention by the author of b1b4755 (git-log: put space after commit mark, 2011-03-10) who kept git svn log working only without --show-commit. Introduce a test and fix it. Reported-by: Bernt Hansen <bernt@norang.ca> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> --- git svn scares me. It exits with 0 even with these problems! Consequently, the tests in t9116 completely ignore the return codes because of the pipes. Patch directly on top of b1b4755. Thanks for the report! v2 uses the grouped regexp again. I've learned to count to 2 meanwhile. --- git-svn.perl | 2 +- t/t9116-git-svn-log.sh | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index a5857c1..0cee0e9 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -5735,7 +5735,7 @@ sub cmd_show_log { my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/; while (<$log>) { if (/^${esc_color}commit (- )?($::sha1_short)/o) { - my $cmt = $1; + my $cmt = $2; if ($c && cmt_showable($c) && $c->{r} != $r_last) { $r_last = $c->{r}; process_commit($c, $r_min, $r_max, \@k) or diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh index 5d477e4..cf4c052 100755 --- a/t/t9116-git-svn-log.sh +++ b/t/t9116-git-svn-log.sh @@ -60,6 +60,21 @@ test_expect_success 'test ascending revision range' " git svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 - " +test_expect_success 'test ascending revision range with --show-commit' " + git reset --hard trunk && + git svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 - + " + +test_expect_success 'test ascending revision range with --show-commit (sha1)' " + git svn find-rev r1 >expected-range-r1-r2-r4-sha1 && + git svn find-rev r2 >>expected-range-r1-r2-r4-sha1 && + git svn find-rev r4 >>expected-range-r1-r2-r4-sha1 && + git reset --hard trunk && + git svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f2 >out && + git rev-parse \$(cat out) >actual && + test_cmp expected-range-r1-r2-r4-sha1 actual + " + printf 'r4 \nr2 \nr1 \n' > expected-range-r4-r2-r1 test_expect_success 'test descending revision range' " -- 1.7.5.1.558.gc8bec ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCHv2 maint] git-svn: Fix git svn log --show-commit 2011-05-20 11:16 ` [PATCHv2 " Michael J Gruber @ 2011-05-20 11:48 ` Bernt Hansen 2011-05-20 16:17 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Bernt Hansen @ 2011-05-20 11:48 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Eric Wong, Michael J Gruber From: Michael J Gruber <git@drmicha.warpmail.net> From: Michael J Gruber <git@drmicha.warpmail.net> git svn log --show-commit had no tests and, consequently, no attention by the author of b1b4755 (git-log: put space after commit mark, 2011-03-10) who kept git svn log working only without --show-commit. Introduce a test and fix it. Reported-by: Bernt Hansen <bernt@norang.ca> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Tested-by: Bernt Hansen <bernt@norang.ca> --- This patch works for me. Thanks! git-svn.perl | 2 +- t/t9116-git-svn-log.sh | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 0fd2fd2..28d037f 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -5766,7 +5766,7 @@ sub cmd_show_log { my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/; while (<$log>) { if (/^${esc_color}commit (- )?($::sha1_short)/o) { - my $cmt = $1; + my $cmt = $2; if ($c && cmt_showable($c) && $c->{r} != $r_last) { $r_last = $c->{r}; process_commit($c, $r_min, $r_max, \@k) or diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh index 5d477e4..cf4c052 100755 --- a/t/t9116-git-svn-log.sh +++ b/t/t9116-git-svn-log.sh @@ -60,6 +60,21 @@ test_expect_success 'test ascending revision range' " git svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 - " +test_expect_success 'test ascending revision range with --show-commit' " + git reset --hard trunk && + git svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 - + " + +test_expect_success 'test ascending revision range with --show-commit (sha1)' " + git svn find-rev r1 >expected-range-r1-r2-r4-sha1 && + git svn find-rev r2 >>expected-range-r1-r2-r4-sha1 && + git svn find-rev r4 >>expected-range-r1-r2-r4-sha1 && + git reset --hard trunk && + git svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f2 >out && + git rev-parse \$(cat out) >actual && + test_cmp expected-range-r1-r2-r4-sha1 actual + " + printf 'r4 \nr2 \nr1 \n' > expected-range-r4-r2-r1 test_expect_success 'test descending revision range' " -- 1.7.5.2.316.g687f ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2 maint] git-svn: Fix git svn log --show-commit 2011-05-20 11:16 ` [PATCHv2 " Michael J Gruber 2011-05-20 11:48 ` Bernt Hansen @ 2011-05-20 16:17 ` Junio C Hamano 2011-05-21 9:30 ` Michael J Gruber 1 sibling, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2011-05-20 16:17 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Eric Wong, Bernt Hansen Michael J Gruber <git@drmicha.warpmail.net> writes: > git svn log --show-commit had no tests and, consequently, no attention > by the author of > > b1b4755 (git-log: put space after commit mark, 2011-03-10) > > who kept git svn log working only without --show-commit. > > Introduce a test and fix it. > > Reported-by: Bernt Hansen <bernt@norang.ca> > Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> > --- > git svn scares me. Sorry about this breakage. > diff --git a/git-svn.perl b/git-svn.perl > index a5857c1..0cee0e9 100755 > --- a/git-svn.perl > +++ b/git-svn.perl > @@ -5735,7 +5735,7 @@ sub cmd_show_log { > my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/; > while (<$log>) { > if (/^${esc_color}commit (- )?($::sha1_short)/o) { > - my $cmt = $1; > + my $cmt = $2; Even more defensive approach would be not to grab the grouping by doing: - if (/^${esc_color}commit (- )?($::sha1_short)/o) { + if (/^${esc_color}commit (?:- )?($::sha1_short)/o) { and not to change anything else. I should have noticed the $1 reference that was immediately on the next line when I saw and applied your patch, but if there were more references in the scope that is outside of the patch context, the same bug would be likely to have gone unnoticed. I do not have enough bandwidth to read every single line of the patch from everybody, so small bugs in patches from known to be good people (you included) can slip through, unless marked with "I am not familiar with this codepath" or "I am not strong in Perl regexp" or somesuch, in which case I try to allocate more time to give it another pass of eyeballing. In any case, thanks for the fix. I think being defensive with (?:) would be a better idea, so I'll tweak the patch before applying with your test. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 maint] git-svn: Fix git svn log --show-commit 2011-05-20 16:17 ` Junio C Hamano @ 2011-05-21 9:30 ` Michael J Gruber 0 siblings, 0 replies; 6+ messages in thread From: Michael J Gruber @ 2011-05-21 9:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Wong, Bernt Hansen Junio C Hamano venit, vidit, dixit 20.05.2011 18:17: > Michael J Gruber <git@drmicha.warpmail.net> writes: > >> git svn log --show-commit had no tests and, consequently, no attention >> by the author of >> >> b1b4755 (git-log: put space after commit mark, 2011-03-10) >> >> who kept git svn log working only without --show-commit. >> >> Introduce a test and fix it. >> >> Reported-by: Bernt Hansen <bernt@norang.ca> >> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> >> --- >> git svn scares me. > > Sorry about this breakage. I have to be sorry, I broke it! Which is why I took responsibility to fix it asap. >> diff --git a/git-svn.perl b/git-svn.perl >> index a5857c1..0cee0e9 100755 >> --- a/git-svn.perl >> +++ b/git-svn.perl >> @@ -5735,7 +5735,7 @@ sub cmd_show_log { >> my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/; >> while (<$log>) { >> if (/^${esc_color}commit (- )?($::sha1_short)/o) { >> - my $cmt = $1; >> + my $cmt = $2; > > Even more defensive approach would be not to grab the grouping by doing: > > - if (/^${esc_color}commit (- )?($::sha1_short)/o) { > + if (/^${esc_color}commit (?:- )?($::sha1_short)/o) { > > and not to change anything else. I should have noticed the $1 reference > that was immediately on the next line when I saw and applied your patch, > but if there were more references in the scope that is outside of the > patch context, the same bug would be likely to have gone unnoticed. > > I do not have enough bandwidth to read every single line of the patch from > everybody, so small bugs in patches from known to be good people (you > included) can slip through, unless marked with "I am not familiar with > this codepath" or "I am not strong in Perl regexp" or somesuch, in which > case I try to allocate more time to give it another pass of eyeballing. So, for the record: I don't grok perl. I don't grok perl regexps I don't like either ;) I use perl when I have to (fixing others' cgi, hacking git-svn) and try my best, or ask for help. Here, three things came together: I did not really comprehend the full regexp with the ($::thingy), probably because /foo bar/ looks separated to my eyes, but was not aware of that lack of understanding; there was no test for the codepath which used $cmt (which ends up being used as $foo->c somewhere else); I didn't even know about "--show-commit" (and that codepath). > In any case, thanks for the fix. I think being defensive with (?:) would > be a better idea, so I'll tweak the patch before applying with your test. Inserting a few more "?" or even "???" definitely describes the issue well.. Turns out that "non-capturing groups" are a regexp feature which is not even specific to perl. So, thanks Bernt for teaching me a new "git svn log" option and Junio and Andreas for a new regexp feature ;) Michael ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-21 9:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <87y622doa7.fsf@norang.ca>
2011-05-20 7:49 ` [PATCH maint] git-svn: Fix git svn log --show-commit Michael J Gruber
2011-05-20 20:26 ` Andreas Schwab
2011-05-20 11:16 ` [PATCHv2 " Michael J Gruber
2011-05-20 11:48 ` Bernt Hansen
2011-05-20 16:17 ` Junio C Hamano
2011-05-21 9:30 ` Michael J Gruber
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).