{"thread":{"id":"27416","subject":"[PATCH maint] git-svn: Fix git svn log --show-commit","startedAt":"2011-05-20T07:49:12Z","lastAt":"2011-05-21T09:30:37Z","messageCount":6,"participants":["Michael J Gruber","Bernt Hansen","Junio C Hamano","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168304","messageId":"a9a8802cb8efa509991f7430ed7591d0b3946e88.1305877679.git.git@drmicha.warpmail.net","threadId":"27416","inReplyTo":"87y622doa7.fsf@norang.ca","subject":"[PATCH maint] git-svn: Fix git svn log --show-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-20T07:49:12Z","receivedAt":"2011-05-20T07:49:12Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"git svn log --show-commit had no tests and, consequently, no attention\nby the author of\n\nb1b4755 (git-log: put space after commit mark, 2011-03-10)\n\nwho kept git svn log working only without --show-commit.\n\nIntroduce a test and fix it.\n\nReported-by: Bernt Hansen \nSigned-off-by: Michael J Gruber \n---\ngit svn scares me. It exits with 0 even with these problems!\nConsequently, the tests in t9116 completely ignore the return codes\nbecause of the pipes.\n\nPatch directly on top of b1b4755. Thanks for the report!\n\nThe fix replaces a group \"(- )?\" by \"-? ?\" which works in this\ncase (though not being equivalent). It's easier than dealing\nwith the problem the grouping introduced.\n---\n git-svn.perl | 2 +-\n t/t9116-git-svn-log.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex a5857c1..b1eef85 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -5734,7 +5734,7 @@ sub cmd_show_log {\n \tmy (@k, $c, $d, $stat);\n \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n \twhile (<$log>) {\n-\t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n+\t\tif (/^${esc_color}commit -? ?($::sha1_short)/o) {\n \t\t\tmy $cmt = $1;\n \t\t\tif ($c && cmt_showable($c) && $c->{r} != $r_last) {\n \t\t\t\t$r_last = $c->{r};\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 5d477e4..cf4c052 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -60,6 +60,21 @@ test_expect_success 'test ascending revision range' \"\n \tgit svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 -\n \t\"\n \n+test_expect_success 'test ascending revision range with --show-commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 -\n+\t\"\n+\n+test_expect_success 'test ascending revision range with --show-commit (sha1)' \"\n+\tgit svn find-rev r1 >expected-range-r1-r2-r4-sha1 &&\n+\tgit svn find-rev r2 >>expected-range-r1-r2-r4-sha1 &&\n+\tgit svn find-rev r4 >>expected-range-r1-r2-r4-sha1 &&\n+\tgit reset --hard trunk &&\n+\tgit svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f2 >out &&\n+\tgit rev-parse \\$(cat out) >actual &&\n+\ttest_cmp expected-range-r1-r2-r4-sha1 actual\n+\t\"\n+\n printf 'r4 \\nr2 \\nr1 \\n' > expected-range-r4-r2-r1\n \n test_expect_success 'test descending revision range' \"\n-- \n1.7.5.1.558.gc8bec\n"},{"id":"168311","messageId":"3dd919897d4a5eca34f421457cc8da461574ee78.1305890184.git.git@drmicha.warpmail.net","threadId":"27416","inReplyTo":"87y622doa7.fsf@norang.ca","subject":"[PATCHv2 maint] git-svn: Fix git svn log --show-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-20T11:16:34Z","receivedAt":"2011-05-20T11:16:34Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"git svn log --show-commit had no tests and, consequently, no attention\nby the author of\n\nb1b4755 (git-log: put space after commit mark, 2011-03-10)\n\nwho kept git svn log working only without --show-commit.\n\nIntroduce a test and fix it.\n\nReported-by: Bernt Hansen \nSigned-off-by: Michael J Gruber \n---\ngit svn scares me. It exits with 0 even with these problems!\nConsequently, the tests in t9116 completely ignore the return codes\nbecause of the pipes.\n\nPatch directly on top of b1b4755. Thanks for the report!\n\nv2 uses the grouped regexp again. I've learned to count to 2 meanwhile.\n---\n git-svn.perl | 2 +-\n t/t9116-git-svn-log.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex a5857c1..0cee0e9 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -5735,7 +5735,7 @@ sub cmd_show_log {\n \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n \twhile (<$log>) {\n \t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n-\t\t\tmy $cmt = $1;\n+\t\t\tmy $cmt = $2;\n \t\t\tif ($c && cmt_showable($c) && $c->{r} != $r_last) {\n \t\t\t\t$r_last = $c->{r};\n \t\t\t\tprocess_commit($c, $r_min, $r_max, \\@k) or\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 5d477e4..cf4c052 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -60,6 +60,21 @@ test_expect_success 'test ascending revision range' \"\n \tgit svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 -\n \t\"\n \n+test_expect_success 'test ascending revision range with --show-commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 -\n+\t\"\n+\n+test_expect_success 'test ascending revision range with --show-commit (sha1)' \"\n+\tgit svn find-rev r1 >expected-range-r1-r2-r4-sha1 &&\n+\tgit svn find-rev r2 >>expected-range-r1-r2-r4-sha1 &&\n+\tgit svn find-rev r4 >>expected-range-r1-r2-r4-sha1 &&\n+\tgit reset --hard trunk &&\n+\tgit svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f2 >out &&\n+\tgit rev-parse \\$(cat out) >actual &&\n+\ttest_cmp expected-range-r1-r2-r4-sha1 actual\n+\t\"\n+\n printf 'r4 \\nr2 \\nr1 \\n' > expected-range-r4-r2-r1\n \n test_expect_success 'test descending revision range' \"\n-- \n1.7.5.1.558.gc8bec\n"},{"id":"168312","messageId":"1305892106-4584-1-git-send-email-bernt@norang.ca","threadId":"27416","inReplyTo":"3dd919897d4a5eca34f421457cc8da461574ee78.1305890184.git.git@drmicha.warpmail.net","subject":"[PATCHv2 maint] git-svn: Fix git svn log --show-commit","fromName":"Bernt Hansen","fromEmail":"bernt@norang.ca","sentAt":"2011-05-20T11:48:26Z","receivedAt":"2011-05-20T11:48:26Z","isPatch":false,"sender":{"key":"bernt@norang.ca","avatar":null},"body":"From: Michael J Gruber \n\nFrom: Michael J Gruber \n\ngit svn log --show-commit had no tests and, consequently, no attention\nby the author of\n\nb1b4755 (git-log: put space after commit mark, 2011-03-10)\n\nwho kept git svn log working only without --show-commit.\n\nIntroduce a test and fix it.\n\nReported-by: Bernt Hansen \nSigned-off-by: Michael J Gruber \nTested-by: Bernt Hansen \n---\nThis patch works for me. Thanks!\n\n git-svn.perl | 2 +-\n t/t9116-git-svn-log.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 0fd2fd2..28d037f 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -5766,7 +5766,7 @@ sub cmd_show_log {\n \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n \twhile (<$log>) {\n \t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n-\t\t\tmy $cmt = $1;\n+\t\t\tmy $cmt = $2;\n \t\t\tif ($c && cmt_showable($c) && $c->{r} != $r_last) {\n \t\t\t\t$r_last = $c->{r};\n \t\t\t\tprocess_commit($c, $r_min, $r_max, \\@k) or\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 5d477e4..cf4c052 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -60,6 +60,21 @@ test_expect_success 'test ascending revision range' \"\n \tgit svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 -\n \t\"\n \n+test_expect_success 'test ascending revision range with --show-commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | test_cmp expected-range-r1-r2-r4 -\n+\t\"\n+\n+test_expect_success 'test ascending revision range with --show-commit (sha1)' \"\n+\tgit svn find-rev r1 >expected-range-r1-r2-r4-sha1 &&\n+\tgit svn find-rev r2 >>expected-range-r1-r2-r4-sha1 &&\n+\tgit svn find-rev r4 >>expected-range-r1-r2-r4-sha1 &&\n+\tgit reset --hard trunk &&\n+\tgit svn log --show-commit -r 1:4 | grep '^r[0-9]' | cut -d'|' -f2 >out &&\n+\tgit rev-parse \\$(cat out) >actual &&\n+\ttest_cmp expected-range-r1-r2-r4-sha1 actual\n+\t\"\n+\n printf 'r4 \\nr2 \\nr1 \\n' > expected-range-r4-r2-r1\n \n test_expect_success 'test descending revision range' \"\n-- \n1.7.5.2.316.g687f\n"},{"id":"168319","messageId":"7vk4dl4a9f.fsf@alter.siamese.dyndns.org","threadId":"27416","inReplyTo":"3dd919897d4a5eca34f421457cc8da461574ee78.1305890184.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 maint] git-svn: Fix git svn log --show-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-20T16:17:16Z","receivedAt":"2011-05-20T16:17:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> git svn log --show-commit had no tests and, consequently, no attention\n> by the author of\n>\n> b1b4755 (git-log: put space after commit mark, 2011-03-10)\n>\n> who kept git svn log working only without --show-commit.\n>\n> Introduce a test and fix it.\n>\n> Reported-by: Bernt Hansen \n> Signed-off-by: Michael J Gruber \n> ---\n> git svn scares me.\n\nSorry about this breakage. \n\n> diff --git a/git-svn.perl b/git-svn.perl\n> index a5857c1..0cee0e9 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -5735,7 +5735,7 @@ sub cmd_show_log {\n> \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n> \twhile (<$log>) {\n> \t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n> -\t\t\tmy $cmt = $1;\n> +\t\t\tmy $cmt = $2;\n\nEven more defensive approach would be not to grab the grouping by doing:\n\n- \t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n+ \t\tif (/^${esc_color}commit (?:- )?($::sha1_short)/o) {\n\nand not to change anything else. I should have noticed the $1 reference\nthat was immediately on the next line when I saw and applied your patch,\nbut if there were more references in the scope that is outside of the\npatch context, the same bug would be likely to have gone unnoticed.\n\nI do not have enough bandwidth to read every single line of the patch from\neverybody, so small bugs in patches from known to be good people (you\nincluded) can slip through, unless marked with \"I am not familiar with\nthis codepath\" or \"I am not strong in Perl regexp\" or somesuch, in which\ncase I try to allocate more time to give it another pass of eyeballing.\n\nIn any case, thanks for the fix. I think being defensive with (?:) would\nbe a better idea, so I'll tweak the patch before applying with your test.\n"},{"id":"168336","messageId":"m2r57t86f9.fsf@igel.home","threadId":"27416","inReplyTo":"a9a8802cb8efa509991f7430ed7591d0b3946e88.1305877679.git.git@drmicha.warpmail.net","subject":"Re: [PATCH maint] git-svn: Fix git svn log --show-commit","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-05-20T20:26:34Z","receivedAt":"2011-05-20T20:26:34Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Michael J Gruber writes:\n\n> The fix replaces a group \"(- )?\" by \"-? ?\" which works in this\n> case (though not being equivalent). It's easier than dealing\n> with the problem the grouping introduced.\n\nYou could also use \"(?:- )?\" to make it a non-capturing group.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"168369","messageId":"4DD7863D.2010002@drmicha.warpmail.net","threadId":"27416","inReplyTo":"7vk4dl4a9f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 maint] git-svn: Fix git svn log --show-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-21T09:30:37Z","receivedAt":"2011-05-21T09:30:37Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 20.05.2011 18:17:\n> Michael J Gruber writes:\n> \n>> git svn log --show-commit had no tests and, consequently, no attention\n>> by the author of\n>>\n>> b1b4755 (git-log: put space after commit mark, 2011-03-10)\n>>\n>> who kept git svn log working only without --show-commit.\n>>\n>> Introduce a test and fix it.\n>>\n>> Reported-by: Bernt Hansen \n>> Signed-off-by: Michael J Gruber \n>> ---\n>> git svn scares me.\n> \n> Sorry about this breakage. \n\nI have to be sorry, I broke it!\nWhich is why I took responsibility to fix it asap.\n\n>> diff --git a/git-svn.perl b/git-svn.perl\n>> index a5857c1..0cee0e9 100755\n>> --- a/git-svn.perl\n>> +++ b/git-svn.perl\n>> @@ -5735,7 +5735,7 @@ sub cmd_show_log {\n>> \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n>> \twhile (<$log>) {\n>> \t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n>> -\t\t\tmy $cmt = $1;\n>> +\t\t\tmy $cmt = $2;\n> \n> Even more defensive approach would be not to grab the grouping by doing:\n> \n> - \t\tif (/^${esc_color}commit (- )?($::sha1_short)/o) {\n> + \t\tif (/^${esc_color}commit (?:- )?($::sha1_short)/o) {\n> \n> and not to change anything else. I should have noticed the $1 reference\n> that was immediately on the next line when I saw and applied your patch,\n> but if there were more references in the scope that is outside of the\n> patch context, the same bug would be likely to have gone unnoticed.\n> \n> I do not have enough bandwidth to read every single line of the patch from\n> everybody, so small bugs in patches from known to be good people (you\n> included) can slip through, unless marked with \"I am not familiar with\n> this codepath\" or \"I am not strong in Perl regexp\" or somesuch, in which\n> case I try to allocate more time to give it another pass of eyeballing.\n\nSo, for the record:\nI don't grok perl.\nI don't grok perl regexps\nI don't like either ;)\n\nI use perl when I have to (fixing others' cgi, hacking git-svn) and try\nmy best, or ask for help. Here, three things came together: I did not\nreally comprehend the full regexp with the ($::thingy), probably because\n/foo bar/ looks separated to my eyes, but was not aware of that lack of\nunderstanding; there was no test for the codepath which used $cmt (which\nends up being used as $foo->c somewhere else); I didn't even know about\n\"--show-commit\" (and that codepath).\n\n> In any case, thanks for the fix. I think being defensive with (?:) would\n> be a better idea, so I'll tweak the patch before applying with your test.\n\nInserting a few more \"?\" or even \"???\" definitely describes the issue well..\n\nTurns out that \"non-capturing groups\" are a regexp feature which is not\neven specific to perl.\n\nSo, thanks Bernt for teaching me a new \"git svn log\" option and Junio\nand Andreas for a new regexp feature ;)\n\nMichael\n"}]}