* [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
* [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: [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
* 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).