Re: [PATCH RFC 2/2] gitweb: Hyperlink multiple git hashes on the same commit message line
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Feb 18, 2009, 21:55 UTC
- Message-ID
- <200902182255.13983.jnareb@gmail.com>
- In-Reply-To
- <1234926043-7471-2-git-send-email-marcel@oak.homeunix.org>
On Wed, 18 Feb 2009, Marcel M. Cary wrote:
Show 6 quoted lines
> 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.
Show 16 quoted lines
> > 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.
[...]
Show 8 quoted lines
> 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?
Show 25 quoted lines
>
>
> 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.
Show 7 quoted lines
> 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