Junio C Hamano venit, vidit, dixit 20.05.2011 18:17:
> Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> Signed-off-by: Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html