Re: [PATCH RFC 2/2] gitweb: Hyperlink multiple git hashes on the same commit message line

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 18 Feb 2009, Marcel M. Cary wrote:

> The current implementation only hyperlinks the first hash on
> a given line of the commit message.  It seems sensible to
> highlight all of them if there are multiple, and it seems
> plausible that there would be multiple even with a tidy line
> length limit, because they can be abbreviated as short as 8
> characters.

That is a good catch. Code simply was not modified since we required
fill-length 40-characters SHA-1 id.

> 
> Benchmark:
> 
> I wanted to make sure that using the 'e' switch to the Perl regex
> wasn't going to kill performance, since this is called once per commit
> message line displayed.
> 
> In all three A/B scenarios I tried, the A and B yielded the same
> results within 2%, where A is the version of code before this patch
> and B is the version after.
> 
> 1: View a commit message containing the last 1000 commit hashes
> 2: View a commit message containing 1000 lines of 40 dots to avoid
>    hyperlinking at the same message length
> 3: View a short merge commit message with a few lines of text and
>    no hashes

I don't think we should worry about that; after all esc_path and
unescape subroutines also use 'e' switch to Perl regexp.

So the benchmark is nice addition, but I don't think it is really
necessary, especially that the change results in shorter and easier
(I think) to maintain code.

[...]
> So I think the patch has no noticeable effect on performance.
> 
> Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
> ---
> 
> And here's another.
> 
> Marcel

Do I understand correctly that those patches are not related at all
semantically or textually, only in that you have them one after other
(and blob sha-1 in the index line reflects state after former), isn't
it?

> 
> 
>  gitweb/gitweb.perl |   12 +++++-------
>  1 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 653f0be..51b7f56 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1384,13 +1384,11 @@ sub format_log_line_html {
>  	my $line = shift;
>  
>  	$line = esc_html($line, -nbsp=>1);
> -	if ($line =~ m/\b([0-9a-fA-F]{8,40})\b/) {
> -		my $hash_text = $1;
> -		my $link =
> -			$cgi->a({-href => href(action=>"object", hash=>$hash_text),
> -			        -class => "text"}, $hash_text);
> -		$line =~ s/$hash_text/$link/;
> -	}
> +	$line =~ s{\b([0-9a-fA-F]{8,40})\b}{
> +		return $cgi->a({-href => href(action=>"object", hash=>$1),
> +					   -class => "text"}, $1);
> +	}eg;
> +

Almost correct... but for this unnecessary 'return' statement.
Without it: ACK.

>  	return $line;
>  }
>  
> -- 
> 1.6.1
> 
> 

P.S. Why bare emails (without user names), e.g. "pasky@suse.cz"
and not "Petr Baudis <pasky@suse.cz>"? Just curious...

-- 
Jakub Narebski
Poland