Re: [PATCHv2 maint] git-svn: Fix git svn log --show-commit

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2011, 16:17 UTC
Message-ID: <7vk4dl4a9f.fsf@alter.siamese.dyndns.org>
In-Reply-To: <3dd919897d4a5eca34f421457cc8da461574ee78.1305890184.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 13 quoted lines

> 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.

Show 10 quoted lines

> 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.