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@xxxxxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxx"
and not "Petr Baudis <pasky@xxxxxxx>"? Just curious...
--
Jakub Narebski
Poland
--
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
