threads / rfc / 5642

RFC patchgitweb: Add committags support

Subject: [RFC/PATCH] gitweb: Add committags support

## tl;dr

17 messages between Sep 21, 2006 and Sep 26, 2006. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

Jakub Narebski· Sep 21, 2006, 21:56 UTC · lore

Below there is preliminary (hence RFC) committag support for gitweb, based on the idea introduced by Sham Chukoury to gitweb-xmms2.

The code has all the possible committags I could think of enabled; not all are tested, though. This includes existing commitsha tag support (full sha links to commit view, is sha is sha of commit), mantis bug and feature tags from gitweb-xmms2 (there is no release committag of gitweb-xmms2, but it should be fairly easy to add it), bugzilla committag for the Linux kernel, plain text URL committag (probably doesn't work that well, marking as links examples, and sometimes protocol specification), and Message-Id committag via GMane git mailing list (and not only) archive -- not tested.

Comments? Discussion?
This patch is rather not for inclusion; it is not in format-patch form.
P.S. I've corrected git_get_type for comparison
  git_get_type($hash) eq "commit"
to work without Perl syntax errors.
-- >8 --
Show changes to gitweb/gitweb.perl +157 −15
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7ed963c..5eb0dd0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -173,6 +173,118 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+# You define site-wide comittags defaults here; override them with
+# $GITWEB_CONFIG as necessary.
+our %committags = (
+	# 'committag' => {
+	# 	'pattern' => regexp (use 'qr' quote-like operator)
+	# 	'sub' => committag-sub (subroutine),
+	# 	'enabled' => is given committag enabled,
+	# fields below can be defined, but don't need to
+	# 	'options' => [ default options...] (array reference),
+	# 	'insubject' => should given committag be enabled in commit/tag subject,
+	# 	'islink' => if the result is hyperlink,
+	# }
+	#
+	# You should ensure that enabled committags cannot overlap
+	#
+	# The committag subroutine is called with match for pattern,
+	# and options if they are defined. Match is replaced by return
+	# value of committag-sub.
+
+	'commitsha' => {
+		'pattern' => qr/[0-9a-fA-F]{40}/,
+		'enabled' => 1,
+		'islink'  => 1,
+		'sub' => \&tag_commit_id},
+
+	'mantis' => {
+		'pattern' => qr/(BUG|FEATURE)\(\d+\)/,
+		'enabled' => 1,
+		'insubject' => 1,
+		'islink' => 1,
+		'options' => [ 'http://bugs.xmms2.xmms.se/view.php?id=' ],
+		'sub' => \&tag_bugtracker},
+
+	'bugzilla' => {
+		'pattern' => qr/bug( )+\(\d+\)/,
+		'enabled' => 1,
+		'insubject' => 1,
+		'islink' => 1,
+		'options' => [ 'http://bugzilla.kernel.org/show_bug.cgi?id=' ],
+		'sub' => \&tag_bugtracker},
+
+	'URL' => {
+		'pattern' => qr!(http|ftp)s?://[a-zA-Z0-9_%./]+!,
+		'enabled' => 1,
+		'islink' => 1,
+		'sub' => \&tag_url},
+
+	'message_id' => {
+		'pattern' => qr/(message|msg)[- ]?id <([^&]*)&rt;/i,
+		'enabled' => 1,
+		'options' => [
+			'http://news.gmane.org/find-root.php?message_id=',
+			\&quote_msgid_gmane ],
+		'sub' => \&tag_msgid},
+);
+
+sub tag_commit_id {
+	my $hash_text = shift;
+
+	if (git_get_type($hash_text) eq "commit") {
+		return $cgi->a({-href => href(action=>"commit", hash=>$hash_text),
+		               -class => "text"}, $hash_text);
+	}
+
+	return;
+}
+
+sub tag_bugtracker {
+	my $match = shift;
+	my $URL = shift || return $match;
+	my ($issue) = $match =~ m/(\d+)/;
+
+	return $match if (!defined $issue);
+
+	return $cgi->a({-href => "$URL$issue"}, $match);
+}
+
+sub tag_url {
+	my $url_text = shift;
+
+	return $cgi->a({-href => $url_text}, $url_text);
+}
+
+sub quote_msgid_gmane {
+	my $msgid = shift || return;
+
+	return '<'.(quotemeta $msgid).'>';
+}
+
+sub quote_msgid_marc {
+	my $msgid = shift || return;
+	my ($user, $host) = split(/\@/, $msgid, 2);
+	$host =~ s/\./ ! /g;
+
+	return $user.' () '.$host;
+}
+
+
+sub tag_msgid {
+	my $text = shift;
+	my $URL = shift || return $text;
+	my $repl = shift;
+
+	my ($msgid) =~ m/&lt;([^&]*)&rt;/;
+	my $msgid_url = (ref($repl) eq "CODE") ? $repl->($msgid) : $msgid;
+	my $link = $cgi->a({-href => "$URL$msgid_url"}, $msgid);
+
+	$text =~ s/$msgid/$link/;
+
+	return $text;
+}
+
 # rename detection options for git-diff and git-diff-tree
 # - default is '-M', with the cost proportional to
 #   (number of removed files) * (number of new files).
@@ -191,6 +303,10 @@ our $git_version = qx($GIT --version) =~
 
 $projects_list ||= $projectroot;
 
+# enabled committags, and committags enabled for subject
+our @committags  = grep { $committags{$_}{'enabled'} } keys %committags;
+our @subjecttags = grep { $committags{$_}{'insubject'} } @committags;
+
 # ======================================================================
 # input validation and dispatch
 our $action = $cgi->param('a');
@@ -577,18 +693,43 @@ ## which don't beling to other sections
 # format line of commit message or tag comment
 sub format_log_line_html {
 	my $line = shift;
+	my @tags = @_;
+	my $a_attr;
+	my %subst;
+
+	if (!@tags) {
+		@tags = @committags;
+	} else {
+		$a_attr = ref($tags[0]) eq "HASH" ? shift @tags : undef;
+	}
 
 	$line = esc_html($line);
 	$line =~ s/ /&nbsp;/g;
-	if ($line =~ m/([0-9a-fA-F]{40})/) {
-		my $hash_text = $1;
-		if (git_get_type($hash_text) eq "commit") {
-			my $link =
-				$cgi->a({-href => href(action=>"commit", hash=>$hash_text),
-				        -class => "text"}, $hash_text);
-			$line =~ s/$hash_text/$link/;
+
+	foreach my $ct (@tags) {
+		next unless exists $committags{$ct};
+		my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
+		my @opts =
+			exists $committags{$ct}{'options'} ?
+			@{$committags{$ct}{'options'}} :
+			();
+
+		while ($line =~ m/($committags{$ct}{'pattern'})/gc) {
+			my $match = $1;
+			my $repl = $committags{$ct}{'sub'}->($match, @opts);
+			next unless $repl;
+
+			if ($wrap) {
+				$repl = $cgi->end_a() . $repl . $cgi->start_a($a_attr);
+			}
+
+			$subst{quotemeta $match} = $repl;
 		}
 	}
+
+	while (my ($from, $to) = each %subst) {
+		$line =~ s/$from/$to/g;
+	}
 	return $line;
 }
 
@@ -626,12 +767,13 @@ sub format_subject_html {
 	$extra = '' unless defined($extra);
 
 	if (length($short) < length($long)) {
-		return $cgi->a({-href => $href, -class => "list subject",
-		                -title => $long},
-		       esc_html($short) . $extra);
+		my $a_attr = {-href => $href, -class => "list subject", -title => $long};
+		return $cgi->a($a_attr,
+		       format_log_line_html($short, $a_attr, @subjecttags) . $extra);
 	} else {
-		return $cgi->a({-href => $href, -class => "list subject"},
-		       esc_html($long)  . $extra);
+		my $a_attr = {-href => $href, -class => "list subject"};
+		return $cgi->a($a_attr,
+		       format_log_line_html($long,  $a_attr, @subjecttags) . $extra);
 	}
 }
 
@@ -693,9 +835,9 @@ sub git_get_type {
 
 	open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
 	my $type = <$fd>;
-	close $fd or return;
+	close $fd or return "unknown";
 	chomp $type;
-	return $type;
+	return ($type || "unknown");
 }
 
 sub git_get_project_config {
@@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
 			$empty = 0;
 		}
 
-		print format_log_line_html($line) . "<br/>\n";
+		print format_log_line_html($line, @committags) . "<br/>\n";
 	}
 
 	if ($opts{'-final_empty_line'}) {
Jakub Narebski· Sep 22, 2006, 17:55 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Jakub Narebski wrote:
Show 14 quoted lines
> Below there is preliminary (hence RFC) committag support for gitweb,
> based on the idea introduced by Sham Chukoury to gitweb-xmms2.
> 
> The code has all the possible committags I could think of enabled;
> not all are tested, though. This includes existing commitsha tag
> support (full sha links to commit view, is sha is sha of commit),
> mantis bug and feature tags from gitweb-xmms2 (there is no release
> committag of gitweb-xmms2, but it should be fairly easy to add it),
> bugzilla committag for the Linux kernel, plain text URL committag
> (probably doesn't work that well, marking as links examples, and
> sometimes protocol specification), and Message-Id committag via
> GMane git mailing list (and not only) archive -- not tested.
> 
> Comments? Discussion?

For example: is the committags code overengineered, too complex? Or is it not generic enough? Should all committags code be put into format_log_line_html, or should we add another subroutine, format_committags (or similar)?

What other committags would you like to have?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Petr Baudis· Sep 23, 2006, 03:29 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Dear diary, on Thu, Sep 21, 2006 at 11:56:31PM CEST, I got a letter where Jakub Narebski <jnareb@gmail.com> said that...

Show 12 quoted lines
> Below there is preliminary (hence RFC) committag support for gitweb,
> based on the idea introduced by Sham Chukoury to gitweb-xmms2.
> 
> The code has all the possible committags I could think of enabled;
> not all are tested, though. This includes existing commitsha tag
> support (full sha links to commit view, is sha is sha of commit),
> mantis bug and feature tags from gitweb-xmms2 (there is no release
> committag of gitweb-xmms2, but it should be fairly easy to add it),
> bugzilla committag for the Linux kernel, plain text URL committag
> (probably doesn't work that well, marking as links examples, and
> sometimes protocol specification), and Message-Id committag via
> GMane git mailing list (and not only) archive -- not tested.

I think that's a good test. People will certainly want more but that can be added over time.

Show 28 quoted lines
> -- >8 --
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7ed963c..5eb0dd0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -173,6 +173,118 @@ sub feature_pickaxe {
>  	return ($_[0]);
>  }
>  
> +# You define site-wide comittags defaults here; override them with
> +# $GITWEB_CONFIG as necessary.
> +our %committags = (
> +	# 'committag' => {
> +	# 	'pattern' => regexp (use 'qr' quote-like operator)
> +	# 	'sub' => committag-sub (subroutine),
> +	# 	'enabled' => is given committag enabled,
> +	# fields below can be defined, but don't need to
> +	# 	'options' => [ default options...] (array reference),
> +	# 	'insubject' => should given committag be enabled in commit/tag subject,
> +	# 	'islink' => if the result is hyperlink,
> +	# }
> +	#
> +	# You should ensure that enabled committags cannot overlap
> +	#
> +	# The committag subroutine is called with match for pattern,
> +	# and options if they are defined. Match is replaced by return
> +	# value of committag-sub.

You should note that the pattern matches in an already HTML-escaped text. Which is actually quite ugly especially wrt. the &nbsp;, perhaps it would be worth considering converting the spaces after this? (The Marc links would need fixing tho'.)

..snip..
Show 7 quoted lines
> +	'message_id' => {
> +		'pattern' => qr/(message|msg)[- ]?id&nbsp;&lt;([^&]*)&rt;/i,
> +		'enabled' => 1,
> +		'options' => [
> +			'http://news.gmane.org/find-root.php?message_id=',
> +			\&quote_msgid_gmane ],
> +		'sub' => \&tag_msgid},

Mention quote_msgid_marc() (and the URL) in a comment so that it's not completely unreferenced?

> +);
..snip..
Show 5 quoted lines
> +sub quote_msgid_gmane {
> +	my $msgid = shift || return;
> +
> +	return '<'.(quotemeta $msgid).'>';
> +}
<> should be HTML-escaped (unless CGI::a() does that).
> @@ -577,18 +693,43 @@ ## which don't beling to other sections
..snip..
Show 10 quoted lines
> -	if ($line =~ m/([0-9a-fA-F]{40})/) {
> -		my $hash_text = $1;
> -		if (git_get_type($hash_text) eq "commit") {
> -			my $link =
> -				$cgi->a({-href => href(action=>"commit", hash=>$hash_text),
> -				        -class => "text"}, $hash_text);
> -			$line =~ s/$hash_text/$link/;
> +
> +	foreach my $ct (@tags) {
> +		next unless exists $committags{$ct};

At this point, I'd complain (even die) rather than silently pass, that's definitely a bug.

> +		my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
$a_attr can't be but a hashref, that test is redundant.
Show 19 quoted lines
> @@ -626,12 +767,13 @@ sub format_subject_html {
>  	$extra = '' unless defined($extra);
>  
>  	if (length($short) < length($long)) {
> -		return $cgi->a({-href => $href, -class => "list subject",
> -		                -title => $long},
> -		       esc_html($short) . $extra);
> +		my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> +		return $cgi->a($a_attr,
> +		       format_log_line_html($short, $a_attr, @subjecttags) . $extra);
>  	} else {
> -		return $cgi->a({-href => $href, -class => "list subject"},
> -		       esc_html($long)  . $extra);
> +		my $a_attr = {-href => $href, -class => "list subject"};
> +		return $cgi->a($a_attr,
> +		       format_log_line_html($long,  $a_attr, @subjecttags) . $extra);
>  	}
>  }
>  
Subjects are often clickable and we don't want links in those.
Show 12 quoted lines
> @@ -693,9 +835,9 @@ sub git_get_type {
>  
>  	open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
>  	my $type = <$fd>;
> -	close $fd or return;
> +	close $fd or return "unknown";
>  	chomp $type;
> -	return $type;
> +	return ($type || "unknown");
>  }
>  
>  sub git_get_project_config {
> P.S. I've corrected git_get_type for comparison
>   git_get_type($hash) eq "commit"
> to work without Perl syntax errors.

Hey it's just a warning. ;-) And if you are calling git_get_type() on a non-existing object, don't you have a problem you should better know about? Couldn't this break git_get_refs_list()?

Show 9 quoted lines
> @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
>  			$empty = 0;
>  		}
>  
> -		print format_log_line_html($line) . "<br/>\n";
> +		print format_log_line_html($line, @committags) . "<br/>\n";
>  	}
>  
>  	if ($opts{'-final_empty_line'}) {
What about the tags? Or perhaps even blobs, for that matter?
-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
Jakub Narebski· Sep 23, 2006, 08:34 UTC · re: Petr Baudis · lore

Re: [RFC/PATCH] gitweb: Add committags support

Petr "Pasky" Baudis wrote:
> Dear diary, on Thu, Sep 21, 2006 at 11:56:31PM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
Show 8 quoted lines
> > +	# The committag subroutine is called with match for pattern,
> > +	# and options if they are defined. Match is replaced by return
> > +	# value of committag-sub.
> 
> You should note that the pattern matches in an already HTML-escaped
> text. Which is actually quite ugly especially wrt. the &nbsp;, perhaps
> it would be worth considering converting the spaces after this? (The
> Marc links would need fixing tho'.)
Perhaps we better use "white-space: pre;" for commit (and tag) messages
instead of 'manually' doing this via converting ' ' to '&nbsp;'.
 
Show 11 quoted lines
> ..snip..
> > +	'message_id' => {
> > +		'pattern' => qr/(message|msg)[- ]?id&nbsp;&lt;([^&]*)&rt;/i,
> > +		'enabled' => 1,
> > +		'options' => [
> > +			'http://news.gmane.org/find-root.php?message_id=',
> > +			\&quote_msgid_gmane ],
> > +		'sub' => \&tag_msgid},
> 
> Mention quote_msgid_marc() (and the URL) in a comment so that it's not
> completely unreferenced?

Well, that is RFC. I'm not sure if the final patch would have all those committags, all those committags but disabled with exception of commitsha/sha, or just commitsha tag in source and the rest in commit message as examples.

For MARC:
	'options' => [
		'http://marc.theaimsgroup.com/?i=',
		\&quote_msgid_marc ],
Show 9 quoted lines
> > +);
> ..snip..
> > +sub quote_msgid_gmane {
> > +	my $msgid = shift || return;
> > +
> > +	return '<'.(quotemeta $msgid).'>';
> > +}
> 
> <> should be HTML-escaped (unless CGI::a() does that).
CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
	return '%3c' . (quotemeta $msgid) . '%3e';
(Quotemeta is because Message-Id contains '@').
Show 15 quoted lines
> > @@ -577,18 +693,43 @@ ## which don't beling to other sections
> ..snip..
> > -	if ($line =~ m/([0-9a-fA-F]{40})/) {
> > -		my $hash_text = $1;
> > -		if (git_get_type($hash_text) eq "commit") {
> > -			my $link =
> > -				$cgi->a({-href => href(action=>"commit", hash=>$hash_text),
> > -				        -class => "text"}, $hash_text);
> > -			$line =~ s/$hash_text/$link/;
> > +
> > +	foreach my $ct (@tags) {
> > +		next unless exists $committags{$ct};
> 
> At this point, I'd complain (even die) rather than silently pass,
> that's definitely a bug.
I've programmed defensively. Perhaps too defensively.
> > +		my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
> 
> $a_attr can't be but a hashref, that test is redundant.

$a_attr can be undefined or be a hashref. It could theoretically be empty hashref, but that is a mistake (<a>...</a>).

Show 21 quoted lines
> > @@ -626,12 +767,13 @@ sub format_subject_html {
> >  	$extra = '' unless defined($extra);
> >  
> >  	if (length($short) < length($long)) {
> > -		return $cgi->a({-href => $href, -class => "list subject",
> > -		                -title => $long},
> > -		       esc_html($short) . $extra);
> > +		my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> > +		return $cgi->a($a_attr,
> > +		       format_log_line_html($short, $a_attr, @subjecttags) . $extra);
> >  	} else {
> > -		return $cgi->a({-href => $href, -class => "list subject"},
> > -		       esc_html($long)  . $extra);
> > +		my $a_attr = {-href => $href, -class => "list subject"};
> > +		return $cgi->a($a_attr,
> > +		       format_log_line_html($long,  $a_attr, @subjecttags) . $extra);
> >  	}
> >  }
> >  
> 
> Subjects are often clickable and we don't want links in those.

The extra code with $a_attr is to have links within links. It works quite well, I'd say. The subject link is broken, and the committag link is inserted in the break (gitweb-xmms2 committag code did the same, but did not preserved all the subject link attributes, like title or class, only the target of the link).

The result is somethink like:
  <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a> 
Show 20 quoted lines
> > @@ -693,9 +835,9 @@ sub git_get_type {
> >  
> >  	open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
> >  	my $type = <$fd>;
> > -	close $fd or return;
> > +	close $fd or return "unknown";
> >  	chomp $type;
> > -	return $type;
> > +	return ($type || "unknown");
> >  }
> >  
> >  sub git_get_project_config {
> 
> > P.S. I've corrected git_get_type for comparison
> >   git_get_type($hash) eq "commit"
> > to work without Perl syntax errors.
> 
> Hey it's just a warning. ;-) And if you are calling git_get_type() on a
> non-existing object, don't you have a problem you should better know
> about? Couldn't this break git_get_refs_list()?

There are many cherry-picked comments with commitsha of object which existed on some topic branch, was pruned, and does not exist anymore. Perhaps we should add "2> /dev/null", too...

I've not tested how this change works with git_get_refs_list(), and other subroutines that use git_get_type().

Show 11 quoted lines
> > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
> >  			$empty = 0;
> >  		}
> >  
> > -		print format_log_line_html($line) . "<br/>\n";
> > +		print format_log_line_html($line, @committags) . "<br/>\n";
> >  	}
> >  
> >  	if ($opts{'-final_empty_line'}) {
> 
> What about the tags? Or perhaps even blobs, for that matter?

What about? In commit messages you usually reference other commits (as: this correct some commit, this finishes what was started in commit, this reverts commit (!), cherry-picked commit).

Thanks for all the comments.
-- 
Jakub Narebski
Poland
Petr Baudis· Sep 23, 2006, 12:11 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter where Jakub Narebski <jnareb@gmail.com> said that...

> Perhaps we better use "white-space: pre;" for commit (and tag) messages
> instead of 'manually' doing this via converting ' ' to '&nbsp;'.
+1
Show 15 quoted lines
> > ..snip..
> > > +	'message_id' => {
> > > +		'pattern' => qr/(message|msg)[- ]?id&nbsp;&lt;([^&]*)&rt;/i,
> > > +		'enabled' => 1,
> > > +		'options' => [
> > > +			'http://news.gmane.org/find-root.php?message_id=',
> > > +			\&quote_msgid_gmane ],
> > > +		'sub' => \&tag_msgid},
> > 
> > Mention quote_msgid_marc() (and the URL) in a comment so that it's not
> > completely unreferenced?
> 
> Well, that is RFC. I'm not sure if the final patch would have all those
> committags, all those committags but disabled with exception of commitsha/sha,
> or just commitsha tag in source and the rest in commit message as examples.

I'd disable mantis and bugzilla by default (but leave them in the code commented out) since those are totally project-specific, but a lot of OSS projects use gmane and URL and commitsha are universally beneficial.

Also, there is a fundamental limitation for the multi-word patterns that they won't work if the line wraps at that point in the log message. This will likely be a problem especially for the msgids, because those are very long and are very likely to cause a linewrap immediately before.

By the way, I don't think taking just 40-digits sha1s is very useful, since that's insanely long and besides the linewrap issue, a lot of people just shorten that to some 8 to 12 digits now - I'd use {8-40} instead (the enforced minimum is 4 in the Git autocompletion code but we shouldn't encourage people to write so ambiguous sha1s to persistent records).

Show 13 quoted lines
> > > +);
> > ..snip..
> > > +sub quote_msgid_gmane {
> > > +	my $msgid = shift || return;
> > > +
> > > +	return '<'.(quotemeta $msgid).'>';
> > > +}
> > 
> > <> should be HTML-escaped (unless CGI::a() does that).
> 
> CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
> 
> 	return '%3c' . (quotemeta $msgid) . '%3e';
Ah, sillly me, I didn't notice that it goes into the URL.
> (Quotemeta is because Message-Id contains '@').
Hmm, and at which point would that be eaten?
Show 7 quoted lines
> > > @@ -577,18 +693,43 @@ ## which don't beling to other sections
> > > +		my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
> > 
> > $a_attr can't be but a hashref, that test is redundant.
> 
> $a_attr can be undefined or be a hashref. It could theoretically
> be empty hashref, but that is a mistake (<a>...</a>).
Yes so no point in testing the emptiness. But this is just nitpicking.
Show 31 quoted lines
> > > @@ -626,12 +767,13 @@ sub format_subject_html {
> > >  	$extra = '' unless defined($extra);
> > >  
> > >  	if (length($short) < length($long)) {
> > > -		return $cgi->a({-href => $href, -class => "list subject",
> > > -		                -title => $long},
> > > -		       esc_html($short) . $extra);
> > > +		my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> > > +		return $cgi->a($a_attr,
> > > +		       format_log_line_html($short, $a_attr, @subjecttags) . $extra);
> > >  	} else {
> > > -		return $cgi->a({-href => $href, -class => "list subject"},
> > > -		       esc_html($long)  . $extra);
> > > +		my $a_attr = {-href => $href, -class => "list subject"};
> > > +		return $cgi->a($a_attr,
> > > +		       format_log_line_html($long,  $a_attr, @subjecttags) . $extra);
> > >  	}
> > >  }
> > >  
> > 
> > Subjects are often clickable and we don't want links in those.
> 
> The extra code with $a_attr is to have links within links. It works
> quite well, I'd say. The subject link is broken, and the committag
> link is inserted in the break (gitweb-xmms2 committag code did the same,
> but did not preserved all the subject link attributes, like title or class,
> only the target of the link).
> 
> The result is somethink like:
> 
>   <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a> 

I don't think this is good idea though - if I'm clicking at links, I don't want to have to carefully watch where that bit of the link leads. IMHO this would be just annoying.

Show 23 quoted lines
> > > @@ -693,9 +835,9 @@ sub git_get_type {
> > >  
> > >  	open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
> > >  	my $type = <$fd>;
> > > -	close $fd or return;
> > > +	close $fd or return "unknown";
> > >  	chomp $type;
> > > -	return $type;
> > > +	return ($type || "unknown");
> > >  }
> > >  
> > >  sub git_get_project_config {
> > 
> > > P.S. I've corrected git_get_type for comparison
> > >   git_get_type($hash) eq "commit"
> > > to work without Perl syntax errors.
> > 
> > Hey it's just a warning. ;-) And if you are calling git_get_type() on a
> > non-existing object, don't you have a problem you should better know
> > about? Couldn't this break git_get_refs_list()?
> 
> There are many cherry-picked comments with commitsha of object which
> existed on some topic branch, was pruned, and does not exist anymore.

Then I think it's better to explicitly state that "in this case, this object may not exist" by checking the return value for undef first.

> Perhaps we should add "2> /dev/null", too...
Yes that would be sensible.
Show 15 quoted lines
> > > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
> > >  			$empty = 0;
> > >  		}
> > >  
> > > -		print format_log_line_html($line) . "<br/>\n";
> > > +		print format_log_line_html($line, @committags) . "<br/>\n";
> > >  	}
> > >  
> > >  	if ($opts{'-final_empty_line'}) {
> > 
> > What about the tags? Or perhaps even blobs, for that matter?
> 
> What about? In commit messages you usually reference other commits
> (as: this correct some commit, this finishes what was started in commit,
> this reverts commit (!), cherry-picked commit).

I meant that we should consider substituting the committags in those as well.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
Jakub Narebski· Sep 23, 2006, 13:33 UTC · re: Petr Baudis · lore

Re: [RFC/PATCH] gitweb: Add committags support

Petr Baudis wrote:
> Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
> Also, there is a fundamental limitation for the multi-word patterns that
> they won't work if the line wraps at that point in the log message. This
> will likely be a problem especially for the msgids, because those are
> very long and are very likely to cause a linewrap immediately before.

We do not wrap log messages in gitweb. So the problem is only when commit message is wrongly wrapped itself (pre imples nowrap).

Show 15 quoted lines
>> > > +);
>> > ..snip..
>> > > +sub quote_msgid_gmane {
>> > > +	my $msgid = shift || return;
>> > > +
>> > > +	return '<'.(quotemeta $msgid).'>';
>> > > +}
>> > 
>> > <> should be HTML-escaped (unless CGI::a() does that).
>> 
>> CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
>> 
>> 	return '%3c' . (quotemeta $msgid) . '%3e';
> 
> Ah, silly me, I didn't notice that it goes into the URL.

Should it be "params quoted" or not? What are valid characters in message-id? (And in which RFC it is defined?)

>> (Quotemeta is because Message-Id contains '@').
> 
> Hmm, and at which point would that be eaten?

In the substitution phase... but perhaps I was to defensive. s/$from/$to/g where $to can have '@'.

Show 35 quoted lines
>> > > @@ -626,12 +767,13 @@ sub format_subject_html {
>> > >  	$extra = '' unless defined($extra);
>> > >  
>> > >  	if (length($short) < length($long)) {
>> > > -		return $cgi->a({-href => $href, -class => "list subject",
>> > > -		                -title => $long},
>> > > -		       esc_html($short) . $extra);
>> > > +		my $a_attr = {-href => $href, -class => "list subject", -title => $long};
>> > > +		return $cgi->a($a_attr,
>> > > +		       format_log_line_html($short, $a_attr, @subjecttags) . $extra);
>> > >  	} else {
>> > > -		return $cgi->a({-href => $href, -class => "list subject"},
>> > > -		       esc_html($long)  . $extra);
>> > > +		my $a_attr = {-href => $href, -class => "list subject"};
>> > > +		return $cgi->a($a_attr,
>> > > +		       format_log_line_html($long,  $a_attr, @subjecttags) . $extra);
>> > >  	}
>> > >  }
>> > >  
>> > 
>> > Subjects are often clickable and we don't want links in those.
>> 
>> The extra code with $a_attr is to have links within links. It works
>> quite well, I'd say. The subject link is broken, and the committag
>> link is inserted in the break (gitweb-xmms2 committag code did the same,
>> but did not preserved all the subject link attributes, like title or class,
>> only the target of the link).
>> 
>> The result is somethink like:
>> 
>>   <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a> 
> 
> I don't think this is good idea though - if I'm clicking at links, I
> don't want to have to carefully watch where that bit of the link leads.
> IMHO this would be just annoying.

The committag links within subject link are clearly visually distinguished: first they have default link color (blue for not visited, dark red for visited links), second they are not bold width (as opposed to gitweb-xmms2, where bold font was due to <b>...</b> element and not CSS styling of a.subject).

I have just noticed that somehow git_log ("log" view) doesn't use format_subject_line (perhaps because it is not shortened) and that committags are not used there. And that is not only place where subjecttags (committags in clickable subject line) are not used.

By the way, you can easily disable some committags in subject, either removing 'insubject' field, or setting it to false.

Show 18 quoted lines
>> > > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
>> > >  			$empty = 0;
>> > >  		}
>> > >  
>> > > -		print format_log_line_html($line) . "<br/>\n";
>> > > +		print format_log_line_html($line, @committags) . "<br/>\n";
>> > >  	}
>> > >  
>> > >  	if ($opts{'-final_empty_line'}) {
>> > 
>> > What about the tags? Or perhaps even blobs, for that matter?
>> 
>> What about? In commit messages you usually reference other commits
>> (as: this correct some commit, this finishes what was started in commit,
>> this reverts commit (!), cherry-picked commit).
> 
> I meant that we should consider substituting the committags in those as
> well.

Ahh... For tags I guess it is a good idea (especially that I think that fixes for bugtracker tracked bugs and feature request should be marked by tags, e.g. b/<bugid>, to be used to link to commit/change/patch from bugtracker.

By the way, should we use some color for PGP signature block in signed tags?
-- 
Jakub Narebski
ShadeHawk on #git
Torun, Poland
Petr Baudis· Sep 23, 2006, 14:05 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Dear diary, on Sat, Sep 23, 2006 at 03:33:01PM CEST, I got a letter where Jakub Narebski <jnareb@gmail.com> said that...

Show 12 quoted lines
> Petr Baudis wrote:
> 
> > Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter
> > where Jakub Narebski <jnareb@gmail.com> said that...
> 
> > Also, there is a fundamental limitation for the multi-word patterns that
> > they won't work if the line wraps at that point in the log message. This
> > will likely be a problem especially for the msgids, because those are
> > very long and are very likely to cause a linewrap immediately before.
> 
> We do not wrap log messages in gitweb. So the problem is only when
> commit message is wrongly wrapped itself (pre imples nowrap).

The commit message is not "wrongly" wrapped but just wrapped to fit into 72 or whatever columns. It would be silly to mandate users to use msg-id: <200609231533.02455.jnareb@gmail.com> with the message id stretching far away just for the sake of some gitweb limitations when having the message wrapped such as msg-id: <200609231533.02455.jnareb@gmail.com> looks much more reasonable.

Show 18 quoted lines
> >> > > +);
> >> > ..snip..
> >> > > +sub quote_msgid_gmane {
> >> > > +	my $msgid = shift || return;
> >> > > +
> >> > > +	return '<'.(quotemeta $msgid).'>';
> >> > > +}
> >> > 
> >> > <> should be HTML-escaped (unless CGI::a() does that).
> >> 
> >> CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
> >> 
> >> 	return '%3c' . (quotemeta $msgid) . '%3e';
> > 
> > Ah, silly me, I didn't notice that it goes into the URL.
> 
> Should it be "params quoted" or not? What are valid characters
> in message-id? (And in which RFC it is defined?)

RFC2822. Unfortunately there can be all kind of crap inside if you put it inside quotes, so yes, it should.

What a pity that (apparently) noone supports RFC2392.
Show 6 quoted lines
> >> (Quotemeta is because Message-Id contains '@').
> > 
> > Hmm, and at which point would that be eaten?
> 
> In the substitution phase... but perhaps I was to defensive.
> s/$from/$to/g where $to can have '@'. 

$ perl -le 'my $x="a\@bc"; my $y="@"; $x =~ s/$y/$y.$y/g; print $x;' a@.@bc

You probably should regexp-quote $from if you don't yet, though, although a misbehaviour from that side is not very probable.

Show 21 quoted lines
> >> > Subjects are often clickable and we don't want links in those.
> >> 
> >> The extra code with $a_attr is to have links within links. It works
> >> quite well, I'd say. The subject link is broken, and the committag
> >> link is inserted in the break (gitweb-xmms2 committag code did the same,
> >> but did not preserved all the subject link attributes, like title or class,
> >> only the target of the link).
> >> 
> >> The result is somethink like:
> >> 
> >>   <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a> 
> > 
> > I don't think this is good idea though - if I'm clicking at links, I
> > don't want to have to carefully watch where that bit of the link leads.
> > IMHO this would be just annoying.
> 
> The committag links within subject link are clearly visually distinguished:
> first they have default link color (blue for not visited, dark red for
> visited links), second they are not bold width (as opposed to gitweb-xmms2,
> where bold font was due to <b>...</b> element and not CSS styling 
> of a.subject).

Ok, in that case it's better though I'm still feeling somewhat uncomfortable about it. What if the _whole_ subject is just "Bug 1324"? I can click the 'commit' link instead but it throws exception in my brain I need to handle and I need to move my mouse around.

Show 6 quoted lines
> > I meant that we should consider substituting the committags in those as
> > well.
> 
> Ahh... For tags I guess it is a good idea (especially that I think that
> fixes for bugtracker tracked bugs and feature request should be marked by tags,
> e.g. b/<bugid>, to be used to link to commit/change/patch from bugtracker. 

For blobs, the point is mainly comments, those can contain all kinds of stuff.

> By the way, should we use some color for PGP signature block in signed tags?
Sounds like a good idea.
-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
Jakub Narebski· Sep 25, 2006, 18:06 UTC · re: Petr Baudis · lore

Re: [RFC/PATCH] gitweb: Add committags support

Petr Baudis wrote:
Show 20 quoted lines
> Dear diary, on Sat, Sep 23, 2006 at 03:33:01PM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
>> Petr Baudis wrote:
>> 
>> > Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter
>> > where Jakub Narebski <jnareb@gmail.com> said that...
>> 
>> > Also, there is a fundamental limitation for the multi-word patterns that
>> > they won't work if the line wraps at that point in the log message. This
>> > will likely be a problem especially for the msgids, because those are
>> > very long and are very likely to cause a linewrap immediately before.
>> 
>> We do not wrap log messages in gitweb. So the problem is only when
>> commit message is wrongly wrapped itself (pre imples nowrap).
> 
> The commit message is not "wrongly" wrapped but just wrapped to fit into
> 72 or whatever columns. It would be silly to mandate users to use msg-id: <200609231533.02455.jnareb@gmail.com>
> with the message id stretching far away just for the sake of some
> gitweb limitations when having the message wrapped such as msg-id:
> <200609231533.02455.jnareb@gmail.com> looks much more reasonable.
So when putting message if into commit message, just put the commit
message in separate line, perhaps even with some indentation, like
below:
  Msg-Id: <200609231533.02455.jnareb@gmail.com>
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Petr Baudis· Sep 25, 2006, 20:15 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Dear diary, on Mon, Sep 25, 2006 at 08:06:44PM CEST, I got a letter where Jakub Narebski <jnareb@gmail.com> said that...

> So when putting message if into commit message, just put the commit
> message in separate line, perhaps even with some indentation, like
> below:
>   Msg-Id: <200609231533.02455.jnareb@gmail.com>
Oh please. :-) The tool should bend, not the user.

Are you going to mandate that for bug references as well? Or should I proofread all of my commits (if they aren't actually just autoformatted by fmt without my further review) to verify that they don't actually end up wrapped, and manually reformat the whole paragraph?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
Jakub Narebski· Sep 26, 2006, 11:52 UTC · re: Petr Baudis · lore

Re: [RFC/PATCH] gitweb: Add committags support

Petr Baudis wrote:
Show 8 quoted lines
> Dear diary, on Mon, Sep 25, 2006 at 08:06:44PM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
>> So when putting message if into commit message, just put the commit
>> message in separate line, perhaps even with some indentation, like
>> below:
>>   Msg-Id: <200609231533.02455.jnareb@gmail.com>
> 
> Oh please. :-) The tool should bend, not the user.

Still it is more (much more) readable to have Msg-Id "header" (or equivalent), and message id itself in one line.

> Are you going to mandate that for bug references as well? Or should I
> proofread all of my commits (if they aren't actually just autoformatted
> by fmt without my further review) to verify that they don't actually end
> up wrapped, and manually reformat the whole paragraph?

O.K. that convinces me (although wrapped hyperlink is not a pretty thing). Besides for tags we have natural multiline "tag", namely PGP signature, which we wpuld (most probably) want to syntax highlight wrapping it in <div class="GPG_signature">...</div> element.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· Sep 23, 2006, 19:58 UTC · re: Petr Baudis · lore

Re: [RFC/PATCH] gitweb: Add committags support

Petr Baudis <pasky@suse.cz> writes:
Show 11 quoted lines
> Also, there is a fundamental limitation for the multi-word patterns that
> they won't work if the line wraps at that point in the log message. This
> will likely be a problem especially for the msgids, because those are
> very long and are very likely to cause a linewrap immediately before.
>
> By the way, I don't think taking just 40-digits sha1s is very useful,
> since that's insanely long and besides the linewrap issue, a lot of
> people just shorten that to some 8 to 12 digits now - I'd use {8-40}
> instead (the enforced minimum is 4 in the Git autocompletion code but we
> shouldn't encourage people to write so ambiguous sha1s to persistent
> records).

True. Without looking at Jakub's code under discussion, off the top of my head, I wonder if we can do something like this:

 (1) take the whole commit log message, not line by line, into a
     Perl variable, say $log.  Note that this is _before_ HTML
     escaping.
 (2) each enabled tag-marking sub are expected to take a list of
     string or ref as its parameter.  The first one in the chain
     takes a single parameter which is ($log).  You call all the
     enabled ones, in some order, and feed the return value from
     the previous one as the input to the next one.
 (3) a tag-marking sub are expected to inspect the list it
     received as its input parameter, and build its return value
     by looking at each element in the list:
	(3-1) if the element is not a string, pass it down as-is
	in the output list.
	(3-2) if the element is a string, do the pattern match,
	and output one or more elements in the output list.
	Literal string you did not care about are placed as
	string for later taggers to be processed, the links you
	generate are placed as a ref to string.
    For example, commit_id tag-marker might look like this:
        sub tag_marker_commit_id {
                my @input = (@_);
                my @output;
                for (@input) {
                        if (!ref $_) {
                                push @output, $_;
                                next;
                        }
                        while ($_ ne '' && m/(.*?)\b([0-9a-fA-F]{6,})\b(.*)/s) {
                                my ($pre, $sha1, $post) = ($1, $2, $3);
                                if (!is_a_commit_id($sha1)) {
                                        push @output, "$pre$sha1";
                                }
                                else {
					my $subst =
                                          href(action => 'commit', hash => $2);
                                        push @output, $1;
                                        push @output, \$subst;
                                }
                                $_ = $post;
                        }
                }
                return @output;
        }
 (4) when you finished calling the chain, you would have a list
     of string and ref.  You html quote the strings and pass
     the ref's (which are supposed to be already HTML) as is,
     and concatenate the result:
	sub markup_all {
        	my ($log) = @_;
                my @current = $log;
                for my $tag_marker (@tag_markers) {
			@current = $tag_marker->(@current);
		}
                return join('',
                	map {
                        	(ref $_) ? $$_ : esc_html($_)
			} @current);
	}
Hmm?
     
Jakub Narebski· Sep 23, 2006, 20:18 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] gitweb: Add committags support

Junio C Hamano wrote:
> True.  Without looking at Jakub's code under discussion, off the
> top of my head, I wonder if we can do something like this:

The code was to analyse log message line after line, doing loop over all committags, building list (well, hash to be more exact) of _exact_ replacements (e.g. in the case of commit_id/commitsha committag we use exact hash as the replaced part, not generic regexp to find a sha1).

The idea of doing committag parsing, and preserving replacements from matching committag and being subject to HTML escaping by using reference to string has it's merits.

I'd have to think about it.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· Sep 23, 2006, 20:50 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Jakub Narebski <jnareb@gmail.com> writes:
Show 5 quoted lines
> The idea of doing committag parsing, and preserving replacements
> from matching committag and being subject to HTML escaping by using
> reference to string has it's merits.
>
> I'd have to think about it.

Good. My obviously buggy illustration would be easier to read with this patch ;-).

--- gomi 2006-09-23 13:47:52.000000000 -0700 +++ gomi 2006-09-23 13:48:23.000000000 -0700

Show changes to diff +4 −1
@@ -2,7 +2,7 @@
                 my @input = (@_);
                 my @output;
                 for (@input) {
-                        if (!ref $_) {
+                        if (ref $_) {
                                 push @output, $_;
                                 next;
                         }
@@ -19,6 +19,9 @@
                                 }
                                 $_ = $post;
                         }
+			if ($_ ne '') {
+				push @output, $_;
+			}
                 }
                 return @output;
         }
Jakub Narebski· Sep 24, 2006, 09:37 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] gitweb: Add committags support

Junio C Hamano wrote:
Show 10 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> The idea of doing committag parsing, and preserving replacements
>> from matching committag and being subject to HTML escaping by using
>> reference to string has it's merits.
>>
>> I'd have to think about it.
> 
> Good.  My obviously buggy illustration would be easier to read
> with this patch ;-).

Don't forget about final s,\n,<br/>\n,gm if we parse it as a whole, and not line by line.

By the way, I think that only the driver (i.e. format_log_line_html) has to be changed...

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· Sep 24, 2006, 10:34 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Jakub Narebski <jnareb@gmail.com> writes:
> Don't forget about final s,\n,<br/>\n,gm if we parse it as a whole,
> and not line by line. 

Yes, the driver and individual tag markers need to have a convention on who is responsible for doing <br />. One reasonable convention would be to make the driver side responsible for taking \n and turns it into <br /> in plain strings, and make the tag markers responsible for doing so in their marked-up output.

For example, when bugzilla_tagger is annotating this commit message:

    This rewrites frotz() routine to work around Bug
    #22574.  The fix was confirmed by Xyzzy.
    Thanks for everybody who helped with this rewrite.

it would match with a regexp /\b(?i:bug)\s*#?(\d+)/s to pick up "Bug\n#22574", extract 22574, and produce:

    ('This rewrites frotz() routine to work around ',
     \'<a href="/cgi-bin/bugzilla.cgi?id=22574">Bug<br/>#22574</a>',
     '.  The fix was confirmed by Xyzzy.\n\nThanks for everybody...')
Jakub Narebski· Sep 24, 2006, 16:35 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Jakub Narebski wrote:
Show 18 quoted lines
> Junio C Hamano wrote:
> 
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> The idea of doing committag parsing, and preserving replacements
>>> from matching committag and being subject to HTML escaping by using
>>> reference to string has it's merits.
>>>
>>> I'd have to think about it.
>> 
>> Good.  My obviously buggy illustration would be easier to read
>> with this patch ;-).
> 
> Don't forget about final s,\n,<br/>\n,gm if we parse it as a whole,
> and not line by line. 
> 
> By the way, I think that only the driver (i.e. format_log_line_html) has to
> be changed...

Yet another question is how to deal with commit message specific "syntax highlighting". Currently, parsing commit message line by line, we treat specially signoff lines (syntax highlighting, and removing trailing empty lines after signoff), empty lines (we collapse consecutive empty lines); the rest goes through format_log_line_html... and committags.

Tag messages have another specific syntax highlighting, namely PGP/GPG signature part, which should be syntax highlighted I think (and not subject to committags replacements).

Some of the problems of marking replacements as not to be committags searched and not to be HTML escaped can be dealt with by changing the order of committags. For example having 'URL' tag as first tag to check we avoid invoking it on committag replacements links.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Junio C Hamano· Sep 24, 2006, 19:17 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] gitweb: Add committags support

Jakub Narebski <jnareb@gmail.com> writes:
Show 5 quoted lines
> Yet another question is how to deal with commit message specific
> "syntax highlighting". Currently, parsing commit message line by line,
> we treat specially signoff lines (syntax highlighting, and removing
> trailing empty lines after signoff), empty lines (we collapse consecutive
> empty lines); the rest goes through format_log_line_html... and committags.

Actually I was hoping that my bright idea(tm) of passing the whole message to a chain of tag markers would solve that automatically. You define a std marker whose purpose is _not_ to add anchor elements leading to other URLs but mark up sign-off lines and stuff. Put that at the beginning or at the end of the chain as you see fit and it would do its work just like other real tag markers would do.

For example, one of the things it would do is to unhighlight the Signed-off-by: lines. It would match and split with

	/(.*?)^(Signed-off-by: .*?)$(.*)/ms
then add:
	( $1, # pass as literal we did not touch
          \'<span class="signoff">', # unhighlight
          $2, # give chance to muck with e-mail address to downstream
	      # markers
          \'</span>'
        )

to its output buffer and process the rest ($3). A downstream tag maker may match e-mail address the above left and might mark it up with mailto: URL.

          
	

← back to recent threads