{"thread":{"id":"19866","subject":"[RFC PATCH 2/2] gitweb: Add second-stage matching of bug IDs in bugzilla committag","startedAt":"2009-06-19T14:13:50Z","lastAt":"2009-11-20T23:24:28Z","messageCount":13,"participants":["Marcel M. Cary","Jakub Narebski","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"116627","messageId":"1245420831-5103-1-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"200902180438.55081.jnareb@gmail.com","subject":"[RFC PATCH 1/2] gitweb: Hyperlink various committags in commit message with regex","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-06-19T14:13:50Z","receivedAt":"2009-06-19T14:13:50Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"I want gitweb to hyperlink commits to my bug tracking system so that\ninformation regarding the current status of a commit can be easily\ncross-referenced. For example, the QA and release status of a commit\ncannot be inserted into the comment. Maybe someday a \"git notes\"\nfeature will help with this, but for now, my organization has a\nseparate bug tracking system. Other repository browsers such as\nunfuddle and websvn support similar features.\n\nSince the bug hyperlinking feature was previously discussed as part of\n\"committags,\" a more general mechanism to embellish commit messages,\nimplement the more general mechanism instead, including the following\ncapabilities:\n\n* Hyperlinking mentions of bug IDs to Bugzilla\n* Hyperlinking URLs\n* Hyperlinking Message-Ids to a mailing list archive\n* Hyperlinking commit hashes as before by default, now with a\n configurable regex\n* Defining new committags per gitweb installation\n\nSince different repositories may use different bug tracking systems or\nmailing list archives, the URL parameter may be configured\nper-repository without reiterating the regexes. To accomodate\ndifferent conventions, regexes may also be configured per-project.\n\nThis patch is heavily based on discussions and code samples from the\nGit list:\n\n\t[RFC/PATCH] gitweb: Add committags support, Sep 2006\n\thttp://thread.gmane.org/gmane.comp.version-control.git/27504\n\n\t[RFC] gitweb: Add committags support (take 2), Dec 2006\n\thttp://thread.gmane.org/gmane.comp.version-control.git/33150\n\n\t[RFC] Configuring (future) committags support in gitweb, Nov 2008\n\thttp://thread.gmane.org/gmane.comp.version-control.git/100415\n\nSome issues I considered but punted:\n\n* Should this configuration try to follow the bugtraq spec?\n\n As far as I know, only subversion implements it. Separation of\n regexes by a newline would be a little awkward in the git config.\n And it is broader than just hyperlinking bugs: it also encompasses\n GUI bug ID form fields. So gitweb would only implement a subset.\n The gitweb configuration mechanism currently only reads\n keys starting with \"gitweb.\", but these parameters would be more\n broadly applicable, potentially to git-gui, for example.\n\n However, it *would* be useful for Git tools to standardize on\n config keys and interpretations of regexes and url formats. For\n example, git-gui might be able to hyperlink the same text as gitweb,\n and even show a separate bugID field when composing a commit\n message.\n\n* I would prefer the regex match against the whole commit message.\n\n This would allow the regex to insist that a bug reference occur\n on the first line or non-first line of the commit message. However,\n even if we concatenated the log lines for the first committag,\n subsequent committags would see the text broken up.\n\n Also, it would allow the regex to match a phrase split across a\n line boundary, as dicussed at some length in the first thread,\n but again, only if no prior committags had interfered.\n\n This could happen in a later patch.\n\n* I would prefer the site admin have a way to let a repository\n owner define new committags, which means having a way to specify\n the 'sub' key from the repo config or having a flexible default.\n\nThe bugtraq and some of the regex questions must be decided now to\navoid breaking gitweb configs later.\n\nSigned-off-by: Marcel M. Cary \n---\n gitweb/INSTALL | 4 +\n gitweb/gitweb.perl | 221 +++++++++++++++++++++++++++++++-\n t/t9500-gitweb-standalone-no-errors.sh | 150 +++++++++++++++++++++-\n 3 files changed, 367 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/INSTALL b/gitweb/INSTALL\nindex 18c9ce3..223e39e 100644\n--- a/gitweb/INSTALL\n+++ b/gitweb/INSTALL\n@@ -123,6 +123,10 @@ GITWEB_CONFIG file:\n \t$feature{'snapshot'}{'default'} = ['zip', 'tgz'];\n \t$feature{'snapshot'}{'override'} = 1;\n \n+\t$feature{'committags'}{'default'} = ['sha1', 'url', 'bugzilla'];\n+\t$feature{'committags'}{'override'} = 1;\n+\n+\n \n Gitweb repositories\n -------------------\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1e7e2d8..c66fdf3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -195,6 +195,81 @@ our %known_snapshot_format_aliases = (\n \t'x-zip' => undef, '' => undef,\n );\n \n+# Could call these something else besides committags... embellishments,\n+# patterns, rewrite rules, ?\n+#\n+# In general, the site admin can enable/disable per-project configuration\n+# of each committag. Only the 'options' part of the committag is configurable\n+# per-project.\n+#\n+# The site admin can of course add new tags to this hash or override the\n+# 'sub' key if necessary. But such changes may be fragile; this is not\n+# designed as a full-blown plugin architecture.\n+our %committags = (\n+\t# Link Git-style hashes to this gitweb\n+\t'sha1' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\t\\$cgi->a({-href => href(action=>\"object\", hash=>$match[1]),\n+\t\t\t -class => \"text\"}, esc_html($match[0], -nbsp=>1));\n+\t\t},\n+\t},\n+\t# Link bug/features to Mantis bug tracker using Mantis-style contextual cues\n+\t'mantis' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr/(?:BUG|FEATURE)\\((\\d+)\\)/,\n+\t\t\t'url' => 'http://bugs.xmms2.xmms.se/view.php?id=',\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => \\&hyperlink_committag,\n+\t},\n+\t# Link mentions of bug IDs to bugzilla\n+\t'bugzilla' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr/bug\\s+(\\d+)/,\n+\t\t\t'url' => 'http://bugzilla.kernel.org/show_bug.cgi?id=',\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => \\&hyperlink_committag,\n+\t},\n+\t# Link URLs\n+\t'url' => {\n+\t\t'options' => {\n+\t\t\t# Avoid matching punctuation that might immediately follow\n+\t\t\t# a url, is not part of the url, and is allowed in urls,\n+\t\t\t# like a full-stop ('.').\n+\t\t\t'pattern' => qr!(http|ftp)s?://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n+\t\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\treturn\n+\t\t\t\t\\$cgi->a({-href => $match[0],\n+\t\t\t\t -class => \"text\"},\n+\t\t\t\t esc_html($match[0], -nbsp=>1));\n+\t\t},\n+\t},\n+\t# Link Message-Id to mailing list archive\n+\t'messageid' => {\n+\t\t'options' => {\n+\t\t\t# The original pattern, which I don't really understand\n+\t\t\t#'pattern' => qr!(?:message|msg)-id:?\\s+<([^>]+)>;!i,\n+\t\t\t'pattern' => qr!(?:message|msg)-?id:?\\s+(<[^>]+>)!i,\n+\t\t\t'url' => 'http://news.gmane.org/find-root.php?message_id=',\n+\t\t},\n+\t\t'override' => 0,\n+\t\t# The original version didn't include the \"msg-id\" text in the\n+\t\t# link text, but this does. In general, I think a little more\n+\t\t# context makes for better link text.\n+\t\t'sub' => \\&hyperlink_committag,\n+\t},\n+);\n+\n # You define site-wide feature defaults here; override them with\n # $GITWEB_CONFIG as necessary.\n our %feature = (\n@@ -365,6 +440,21 @@ our %feature = (\n \t\t'sub' => \\&feature_patches,\n \t\t'override' => 0,\n \t\t'default' => [16]},\n+\n+\t# The selection and ordering of committags that are enabled.\n+\t# Committag transformations will be applied to commit log messages\n+\t# in this order if listed here.\n+\n+\t# To disable system wide have in $GITWEB_CONFIG\n+\t# $feature{'committags'}{'default'} = [];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'committags'}{'override'} = 1;\n+\t# and in project config gitweb.committags = sha1, url, bugzilla\n+\t# to enable those three committags for that project\n+\t'committags' => {\n+\t\t'sub' => \\&feature_committags,\n+\t\t'override' => 0,\n+\t\t'default' => ['sha1']},\n );\n \n sub gitweb_get_feature {\n@@ -433,6 +523,18 @@ sub feature_patches {\n \treturn ($_[0]);\n }\n \n+sub feature_committags {\n+\tmy (@defaults) = @_;\n+\n+\tmy ($cfg) = git_get_project_config('committags');\n+\n+\tif ($cfg) {\n+\t\treturn ($cfg eq 'none' ? () : split(/\\s*[,\\s]\\s*/, $cfg));\n+\t}\n+\n+\treturn @defaults;\n+}\n+\n # checking HEAD file with -e is fragile if the repository was\n # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n # and then pruned.\n@@ -814,6 +916,34 @@ $git_dir = \"$projectroot/$project\" if $project;\n our @snapshot_fmts = gitweb_get_feature('snapshot');\n @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n \n+# ordering of committags\n+our @committags = gitweb_get_feature('committags');\n+\n+# Merge project configs with default committag definitions\n+gitweb_load_project_committags();\n+\n+# Load committag configs from the repository config file and and\n+# incorporate them into the gitweb defaults where permitted by the\n+# site administrator.\n+sub gitweb_load_project_committags {\n+\treturn if (!$git_dir);\n+\tmy %project_config = ();\n+\tmy %raw_config = git_parse_project_config('gitweb\\.committag');\n+\tforeach my $key (keys(%raw_config)) {\n+\t\tnext if ($key !~ /gitweb\\.committag\\.[^.]+\\.[^.]/);\n+\t\tmy ($gitweb_prefix, $committag_prefix, $ctname, $option) =\n+\t\t\tsplit(/\\./, $key, 4);\n+\t\t$project_config{$ctname}{$option} = $raw_config{$key};\n+\t}\n+\tforeach my $ctname (keys(%committags)) {\n+\t\tnext if (!$committags{$ctname}{'override'});\n+\t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n+\t\t\t$committags{$ctname}{'options'}{$optname} =\n+\t\t\t\t$project_config{$ctname}{$optname};\n+\t\t}\n+\t}\n+}\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -1384,13 +1514,92 @@ sub file_type_long {\n sub format_log_line_html {\n \tmy $line = shift;\n \n-\t$line = esc_html($line, -nbsp=>1);\n-\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n-\t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n-\t\t\t\t\t-class => \"text\"}, $1);\n-\t}eg;\n+\t# In this list of log message fragments, a string ref indicates HTML,\n+\t# and a string indicates plain text\n+\tmy @list = ( $line );\n \n-\treturn $line;\n+COMMITTAG:\n+\tforeach my $ctname (@committags) {\n+\t\tnext COMMITTAG unless exists $committags{$ctname};\n+\t\tmy $committag = $committags{$ctname};\n+\n+\t\tnext COMMITTAG unless exists $committag->{'options'};\n+\t\tmy $opts = $committag->{'options'};\n+\n+\t\tnext COMMITTAG unless exists $opts->{'pattern'};\n+\t\tmy $pattern = $opts->{'pattern'};\n+\n+\t\tmy @newlist = ();\n+\n+\tPART:\n+\t\tforeach my $part (@list) {\n+\t\t\tnext PART if $part eq \"\";\n+\t\t\tif (ref($part)) {\n+\t\t\t\tpush @newlist, $part;\n+\t\t\t\tnext PART;\n+\t\t\t}\n+\n+\t\t\tmy $oldpos = 0;\n+\n+\t\tMATCH:\n+\t\t\twhile ($part =~ m/$pattern/gc) {\n+\t\t\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n+\t\t\t\tmy $repl = $committag->{'sub'}->($opts, $&, $1);\n+\t\t\t\t$repl = \"\" if (!defined $repl);\n+\n+\t\t\t\tmy $pre = substr($part, $oldpos, $prepos - $oldpos);\n+\t\t\t\tpush_or_append(\\@newlist, $pre);\n+\t\t\t\tpush_or_append(\\@newlist, $repl);\n+\n+\t\t\t\t$oldpos = $postpos;\n+\t\t\t} # end while [regexp matches]\n+\n+\t\t\tmy $rest = substr($part, $oldpos);\n+\t\t\tpush_or_append(\\@newlist, $rest);\n+\n+\t\t} # end foreach (@list)\n+\n+\t\t@list = @newlist;\n+\t} # end foreach (@committags)\n+\n+\t# Escape any remaining plain text and concatenate\n+\tmy $html = '';\n+\tfor my $part (@list) {\n+\t\tif (ref($part)) {\n+\t\t\t$html .= $$part;\n+\t\t} else {\n+\t\t\t$html .= esc_html($part, -nbsp=>1);\n+\t\t}\n+\t}\n+\n+\treturn $html;\n+}\n+\n+# Returns a ref to an HTML snippet that links the second\n+# parameter to a URL formed from the first and last parameters.\n+# This is a helper function used in %committags.\n+sub hyperlink_committag {\n+\tmy ($opts, @match) = @_;\n+\treturn\n+\t\t\\$cgi->a({-href => $opts->{url} . CGI::escape($match[1]),\n+\t\t\t\t -class => \"text\"},\n+\t\t\t\t esc_html($match[0], -nbsp=>1));\n+}\n+\n+\n+sub push_or_append (\\@@) {\n+\tmy $list = shift;\n+\n+\tif (ref $_[0] || ! @$list || ref $list->[-1]) {\n+\t\tpush @$list, @_;\n+\t} else {\n+\t\tmy $a = pop @$list;\n+\t\tmy $b = shift @_;\n+\n+\t\tpush @$list, $a . $b, @_;\n+\t}\n+\t# imitate push\n+\treturn scalar @$list;\n }\n \n # format marker of refs pointing to given object\ndiff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\nindex d539619..37a127c 100755\n--- a/t/t9500-gitweb-standalone-no-errors.sh\n+++ b/t/t9500-gitweb-standalone-no-errors.sh\n@@ -55,9 +55,9 @@ gitweb_run () {\n \t# some of git commands write to STDERR on error, but this is not\n \t# written to web server logs, so we are not interested in that:\n \t# we are interested only in properly formatted errors/warnings\n-\trm -f gitweb.log &&\n+\trm -f resp.http gitweb.log &&\n \tperl -- \"$SCRIPT_NAME\" \\\n-\t\t>/dev/null 2>gitweb.log &&\n+\t\t> resp.http 2>gitweb.log &&\n \tif grep \"^[[]\" gitweb.log >/dev/null 2>&1; then false; else true; fi\n \n \t# gitweb.log is left for debugging\n@@ -702,4 +702,150 @@ test_expect_success \\\n \t gitweb_run \"p=.git;a=summary\"'\n test_debug 'cat gitweb.log'\n \n+# ----------------------------------------------------------------------\n+# sha1 linking\n+#\n+echo hi > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <567890ab\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 567890ab resp.http'\n+\n+# ----------------------------------------------------------------------\n+# bugzilla commit tag\n+#\n+\n+echo foo > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <> gitweb_config.perl\n+test_expect_success 'bugzilla: enabled' '\n+\th=$(git rev-parse --verify HEAD) &&\n+\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 resp.http'\n+\n+git config gitweb.committag.bugzilla.url 'http://bts.example.com?bug='\n+test_expect_success 'bugzilla: url overridden but not permitted' '\n+\th=$(git rev-parse --verify HEAD) &&\n+\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 resp.http'\n+\n+echo '$committags{\"bugzilla\"}{\"override\"} = 1;' >> gitweb_config.perl\n+test_expect_success 'bugzilla: url overridden' '\n+\th=$(git rev-parse --verify HEAD) &&\n+\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 resp.http'\n+\n+git config gitweb.committag.bugzilla.pattern 'Fixes bug (\\d+)'\n+test_expect_success 'bugzilla: pattern overridden' '\n+\th=$(git rev-parse --verify HEAD) &&\n+\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 resp.http'\n+git config --unset gitweb.committag.bugzilla.pattern\n+\n+test_expect_success 'bugzilla: affects log view too' '\n+\tgitweb_run \"p=.git;a=log\" &&\n+\tgrep -F -q \\\n+\t\t\"bug 1234\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 resp.http'\n+\n+# ----------------------------------------------------------------------\n+# url linking\n+#\n+echo url_test > file.txt\n+git add file.txt\n+url='http://user@pass:example.com/foo.html?u=v&x=y#z'\n+url_esc=\"$(echo \"$url\" | sed 's/&/&/g')\"\n+git commit -q -F - file.txt <> gitweb_config.perl\n+git config gitweb.committags 'sha1, url'\n+test_expect_success 'url link: links when enabled' '\n+\th=$(git rev-parse --verify HEAD) &&\n+\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n+\tgrep -q -F \\\n+\t\t\"See also $url_esc.\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"$url\" resp.http'\n+\n+# ----------------------------------------------------------------------\n+# message id linking\n+#\n+echo msgid_test > file.txt\n+git add file.txt\n+url='http://news.gmane.org/find-root.php?message_id='\n+msgid=''\n+msgid_esc=\"$(echo \"$msgid\" | sed 's//\\>/g')\"\n+msgid_url=\"$url$(echo \"$msgid\" | sed 's//%3E/g')\"\n+git commit -q -F - file.txt <> gitweb_config.perl\n+git config gitweb.committags 'sha1, messageid'\n+test_expect_success 'msgid link: linked when enabled' '\n+\th=$(git rev-parse --verify HEAD) &&\n+\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n+\tgrep -q -F \\\n+\t\t\"See msg-id $msgid_esc.\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"y.z\" resp.http'\n+\n test_done\n-- \n1.6.2\n"},{"id":"116626","messageId":"1245420831-5103-2-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"1245420831-5103-1-git-send-email-marcel@oak.homeunix.org","subject":"[RFC PATCH 2/2] gitweb: Add second-stage matching of bug IDs in bugzilla committag","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-06-19T14:13:51Z","receivedAt":"2009-06-19T14:13:51Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"Match Bugzilla bug IDs with two regexes instead of one. The first is\na pre-filter that allows easy matching of multiple bug IDs on the same\nline, and the second easily picks out the individual big IDs for\nhyperlinking in the context of the first regex.\n\nFor example, it would help in matching these and hyperlinking each ID\nindividually.\n\n\t[#1234, #1235]\n\tResolves-bug: 1234, 1235\n\tbugs 1234, 1235, and 1236\n\nMaybe there's a better naming scheme for the two patterns?\n\nSigned-off-by: Marcel M. Cary \n---\n gitweb/gitweb.perl | 61 ++++++++++++++++++++++----------\n t/t9500-gitweb-standalone-no-errors.sh | 18 +++++++++\n 2 files changed, 60 insertions(+), 19 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c66fdf3..47c8cd5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -227,14 +227,28 @@ our %committags = (\n \t\t'override' => 0,\n \t\t'sub' => \\&hyperlink_committag,\n \t},\n-\t# Link mentions of bug IDs to bugzilla\n+\t# Link mentions of bugs to bugzilla, allowing for separate outer\n+\t# and inner regexes (see unit test for example)\n \t'bugzilla' => {\n \t\t'options' => {\n \t\t\t'pattern' => qr/bug\\s+(\\d+)/,\n+\t\t\t'innerpattern' => undef,\n \t\t\t'url' => 'http://bugzilla.kernel.org/show_bug.cgi?id=',\n \t\t},\n \t\t'override' => 0,\n-\t\t'sub' => \\&hyperlink_committag,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\tif (defined($opts->{'innerpattern'})) {\n+\t\t\t\tmy @list = ();\n+\t\t\t\tpush_or_append_replacements(\\@list, $opts->{innerpattern},\n+\t\t\t\t $match[0], sub {\n+\t\t\t\t\t\treturn hyperlink_committag($opts, @_);\n+\t\t\t\t\t});\n+\t\t\t\treturn @list;\n+\t\t\t} else {\n+\t\t\t\treturn hyperlink_committag(@_);\n+\t\t\t}\n+\t\t},\n \t},\n \t# Link URLs\n \t'url' => {\n@@ -1539,23 +1553,9 @@ COMMITTAG:\n \t\t\t\tnext PART;\n \t\t\t}\n \n-\t\t\tmy $oldpos = 0;\n-\n-\t\tMATCH:\n-\t\t\twhile ($part =~ m/$pattern/gc) {\n-\t\t\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n-\t\t\t\tmy $repl = $committag->{'sub'}->($opts, $&, $1);\n-\t\t\t\t$repl = \"\" if (!defined $repl);\n-\n-\t\t\t\tmy $pre = substr($part, $oldpos, $prepos - $oldpos);\n-\t\t\t\tpush_or_append(\\@newlist, $pre);\n-\t\t\t\tpush_or_append(\\@newlist, $repl);\n-\n-\t\t\t\t$oldpos = $postpos;\n-\t\t\t} # end while [regexp matches]\n-\n-\t\t\tmy $rest = substr($part, $oldpos);\n-\t\t\tpush_or_append(\\@newlist, $rest);\n+\t\t\tpush_or_append_replacements(\\@newlist, $opts->{'pattern'}, $part, sub {\n+\t\t\t\t\t$committag->{'sub'}->($opts, @_);\n+\t\t\t\t});\n \n \t\t} # end foreach (@list)\n \n@@ -1586,6 +1586,29 @@ sub hyperlink_committag {\n \t\t\t\t esc_html($match[0], -nbsp=>1));\n }\n \n+# Find $pattern in string $part, and push_or_append the parts between\n+# matches and the result of calling $sub with matched text to $newlist.\n+sub push_or_append_replacements {\n+\tmy ($newlist, $pattern, $part, $sub) = @_;\n+\n+\tmy $oldpos = 0;\n+\n+MATCH:\n+\twhile ($part =~ m/$pattern/gc) {\n+\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n+\n+\t\tmy @repl = $sub->($&, $1);\n+\n+\t\tmy $pre = substr($part, $oldpos, $prepos - $oldpos);\n+\t\tpush_or_append($newlist, $pre);\n+\t\tpush_or_append($newlist, @repl);\n+\n+\t\t$oldpos = $postpos;\n+\t} # end while [regexp matches]\n+\n+\tmy $rest = substr($part, $oldpos);\n+\tpush_or_append($newlist, $rest);\n+}\n \n sub push_or_append (\\@@) {\n \tmy $list = shift;\ndiff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\nindex 37a127c..573f03c 100755\n--- a/t/t9500-gitweb-standalone-no-errors.sh\n+++ b/t/t9500-gitweb-standalone-no-errors.sh\n@@ -798,6 +798,24 @@ test_expect_success 'bugzilla: affects log view too' '\n test_debug 'cat gitweb.log'\n test_debug 'grep 1234 resp.http'\n \n+echo hello > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <#123,#45]\" \\\n+\t\tresp.http\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 123 resp.http'\n+\n # ----------------------------------------------------------------------\n # url linking\n #\n-- \n1.6.2\n"},{"id":"116755","messageId":"200906221318.19598.jnareb@gmail.com","threadId":"19866","inReplyTo":"1245420831-5103-1-git-send-email-marcel@oak.homeunix.org","subject":"Re: [RFC PATCH 1/2] gitweb: Hyperlink various committags in commit message with regex","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-22T11:18:18Z","receivedAt":"2009-06-22T11:18:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 19 June 2009, Marcel M. Cary wrote:\n\nThanks for diligently working on this issue. Good work!\n\nI see that it is an RFC, and not final submission, but just in case\nI'd like to remind you that some of information below should go into\ncommit message, but some of it should I think go to comments (between\n\"---\" and diffstat).\n\n> I want gitweb to hyperlink commits to my bug tracking system so that\n> information regarding the current status of a commit can be easily\n> cross-referenced. For example, the QA and release status of a commit\n> cannot be inserted into the comment. Maybe someday a \"git notes\"\n> feature will help with this, but for now, my organization has a\n> separate bug tracking system. Other repository browsers such as\n> unfuddle and websvn support similar features.\n\nThe paragraph above should, I think, be made more clear. You don't\nneed to mention what you don't do; the comment about \"git notes\" should\nbe not in commit message but in comments section.\n\nWhat you want to have is to have some markers in commit message \n(committags) hyperlinked; namely you want notifications about bug/issue\nnumbers in the commit message hyperlinked to appropriate bugtracker/issue\ntracker URL. Do I understand this correctly?\n\nI tried here to reword what you said, to come up with better commit\nmessage for a future final submission.\n\n> \n> Since the bug hyperlinking feature was previously discussed as part of\n> \"committags,\" a more general mechanism to embellish commit messages,\n> implement the more general mechanism instead, including the following\n> capabilities:\n\nWell, I think that the fact that it would be not much harder to create\ngeneral mechanism for commit message transformation, than to add \nsuitably generic and well customizable support for bugtracker \n'committags'.\n\n> \n> * Hyperlinking mentions of bug IDs to Bugzilla\n> * Hyperlinking URLs\n> * Hyperlinking Message-Ids to a mailing list archive\n> * Hyperlinking commit hashes as before by default, now with a\n> configurable regex\n> * Defining new committags per gitweb installation\n\nWell, there is one _implicit_ (but important) commit message \ntransformation (filter) for display, which has to be always present[1],\nnamely HTML escaping. We make use of the fact that you can do\nHTML escaping before doing the only currently supported committag, \nnamely hyperlinking (shortened) SHA-1 to 'object' gitweb URL, but for\nother committags like mentioned \"Message-Id to mail archive\" committag\n(filter) it would make them more difficult.\n\nAlso one might consider vertical whitespace simplification (removing\nleading empty lines, compacting empty lines to single empty line \nbetween paragraphs), and syntax highlighting signoff lines to be\na kind of commit message filter like mentioned above committags\n(see git_print_log() subroutine).\n\nAlthough probably vertical whitespace simplification should be not\nmade into commit message filter, as it is used not for all views.\n\n> \n> Since different repositories may use different bug tracking systems or\n> mailing list archives, the URL parameter may be configured\n> per-repository without reiterating the regexes. To accomodate\n> different conventions, regexes may also be configured per-project.\n\nAlso list of supported committags is separated from the list of \ncommittags used[1]; just like it is done for snapshot formats.\nThis could be mentioned in final commit message.\n\n[1] well, sequence rather than list in this case, as here\n ordering does matter a bit\n\n> \n> This patch is heavily based on discussions and code samples from the\n> Git list:\n> \n> \t[RFC/PATCH] gitweb: Add committags support, Sep 2006\n> \thttp://thread.gmane.org/gmane.comp.version-control.git/27504\n> \n> \t[RFC] gitweb: Add committags support (take 2), Dec 2006\n> \thttp://thread.gmane.org/gmane.comp.version-control.git/33150\n> \n> \t[RFC] Configuring (future) committags support in gitweb, Nov 2008\n> \thttp://thread.gmane.org/gmane.comp.version-control.git/100415\n\nHmmm... should this be put in final commit message, or only in comment\nto the patch (should this be in commit history of git repository)?\n\n> \n> Some issues I considered but punted:\n> \n> * Should this configuration try to follow the bugtraq spec?\n> \n> As far as I know, only subversion implements it. Separation of\n> regexes by a newline would be a little awkward in the git config.\n> And it is broader than just hyperlinking bugs: it also encompasses\n> GUI bug ID form fields. So gitweb would only implement a subset.\n\nI didn't even know that there is such spec. Were you talking about\nhttp://tortoisesvn.net/issuetracker_integration or do you have different\nURL in mind?\n\n> The gitweb configuration mechanism currently only reads\n> keys starting with \"gitweb.\", but these parameters would be more\n> broadly applicable, potentially to git-gui, for example.\n\nActually the fact that gitweb reads only keys in the 'gitweb' section\nfrom config is just a convention. There were (are) no config variables\nin other places (other sections) which would be of interest to gitweb.\n\n> \n> However, it *would* be useful for Git tools to standardize on\n> config keys and interpretations of regexes and url formats. For\n> example, git-gui might be able to hyperlink the same text as gitweb,\n> and even show a separate bugID field when composing a commit\n> message.\n\nThis is I think a very good idea... but I think idea which \nimplementation can be left for later.\n\n> \n> * I would prefer the regex match against the whole commit message.\n> \n> This would allow the regex to insist that a bug reference occur\n> on the first line or non-first line of the commit message. However,\n> even if we concatenated the log lines for the first committag,\n> subsequent committags would see the text broken up.\n> \n> Also, it would allow the regex to match a phrase split across a\n> line boundary, as dicussed at some length in the first thread,\n> but again, only if no prior committags had interfered.\n> \n> This could happen in a later patch.\n\nWell, the change should be fairly easy: just concatenate lines before\npassing them as single element list to commit message filters. OTOH\nyou would have to take care of end of line characters in committags\nregexps.\n\n> \n> * I would prefer the site admin have a way to let a repository\n> owner define new committags, which means having a way to specify\n> the 'sub' key from the repo config or having a flexible default.\n\nPerhaps, following your earlier suggestion to make committags supported\nalso by other tools, gitweb (and e.g. git-gui / gitk) use config \nvariable committag.. (where can be 'pattern' or 'url';\nalthough I wonder if we can allow 'pattern' as malicious user can do\na DoS attack against gitweb / server using badly behaved regexp).\n\n> \n> The bugtraq and some of the regex questions must be decided now to\n> avoid breaking gitweb configs later.\n\nTrue.\n\n> \n> Signed-off-by: Marcel M. Cary \n> ---\n> gitweb/INSTALL | 4 +\n> gitweb/gitweb.perl | 221 +++++++++++++++++++++++++++++++-\n> t/t9500-gitweb-standalone-no-errors.sh | 150 +++++++++++++++++++++-\n> 3 files changed, 367 insertions(+), 8 deletions(-)\n> \n> diff --git a/gitweb/INSTALL b/gitweb/INSTALL\n> index 18c9ce3..223e39e 100644\n> --- a/gitweb/INSTALL\n> +++ b/gitweb/INSTALL\n> @@ -123,6 +123,10 @@ GITWEB_CONFIG file:\n> \t$feature{'snapshot'}{'default'} = ['zip', 'tgz'];\n> \t$feature{'snapshot'}{'override'} = 1;\n> \n> +\t$feature{'committags'}{'default'} = ['sha1', 'url', 'bugzilla'];\n> +\t$feature{'committags'}{'override'} = 1;\n> +\n> +\n> \n> Gitweb repositories\n> -------------------\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1e7e2d8..c66fdf3 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -195,6 +195,81 @@ our %known_snapshot_format_aliases = (\n> \t'x-zip' => undef, '' => undef,\n> );\n> \n\nI understand that comments such as one below would be not present in\na final submission, and they are here to provide running commentary for\ncode, isn't it?\n\n> +# Could call these something else besides committags... embellishments,\n> +# patterns, rewrite rules, ?\n\nThey are \"commit filters\", or \"commit message filters\" (or 'formatters',\nor 'processors'; they are not 'parsers'). \n\nThe name 'committag' was first introduced as far as I remember in xmms2\nfork of gitweb (in old times when gitweb was separate project, and not\npart of git repository).\n\n> +#\n> +# In general, the site admin can enable/disable per-project configuration\n> +# of each committag. Only the 'options' part of the committag is configurable\n> +# per-project.\n\nSee above caveat about allowing to customize 'regexp'/'pattern' part\nin untrusted environment; you can construct regexp which has exponential\nbehavior.\n\n> +#\n> +# The site admin can of course add new tags to this hash or override the\n> +# 'sub' key if necessary. But such changes may be fragile; this is not\n> +# designed as a full-blown plugin architecture.\n> +our %committags = (\n\n\nYou should put the comments here about supported keys, similar to the\none for %known_snapshot_formats and %feature hashes.\n\n> +\t# Link Git-style hashes to this gitweb\n> +\t'sha1' => {\n> +\t\t'options' => {\n> +\t\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n> +\t\t},\n> +\t\t'override' => 0,\n\nShouldn't 'override' key be better last?\n\n> +\t\t'sub' => sub {\n> +\t\t\tmy ($opts, @match) = @_;\n> +\t\t\t\\$cgi->a({-href => href(action=>\"object\", hash=>$match[1]),\n> +\t\t\t -class => \"text\"}, esc_html($match[0], -nbsp=>1));\n> +\t\t},\n\nStyle: although there is commonly used idiom to use 'sub { ; }'\nfor a wrapper subroutines (e.g. 'sub { [] }' in Moose examples), one\nshould use explicit \"return\" statement instead of relying on Perl \nbehavior of returning last statement in a block.\n\nSee Perl::Critic::Policy::Subroutines::RequireFinalReturn policy in\nPerl::Critic (perlcritic.com). \"Perl Best Practices\" says:\n\n Subroutines without explicit 'return' statements at their ends can be\n confusing. It can be challenging to deduce what the return value will be.\n\n> +\t},\n> +\t# Link bug/features to Mantis bug tracker using Mantis-style contextual cues\n> +\t'mantis' => {\n> +\t\t'options' => {\n> +\t\t\t'pattern' => qr/(?:BUG|FEATURE)\\((\\d+)\\)/,\n> +\t\t\t'url' => 'http://bugs.xmms2.xmms.se/view.php?id=',\n\nI don't think we want to put such URL here. Please check if Mantis \ndocumentation uses some specific links, or follow RFC conventions and\nuse 'example.com' as hostname (e.g. 'bugs.example.com').\n\nBy the way the bugtraq proposal you mentioned uses placeholder in URL\nfor putting issue number (%BUGID%). Perhaps gitweb should do the same\nhere.\n\n> +\t\t},\n> +\t\t'override' => 0,\n> +\t\t'sub' => \\&hyperlink_committag,\n> +\t},\n> +\t# Link mentions of bug IDs to bugzilla\n> +\t'bugzilla' => {\n> +\t\t'options' => {\n> +\t\t\t'pattern' => qr/bug\\s+(\\d+)/,\n> +\t\t\t'url' => 'http://bugzilla.kernel.org/show_bug.cgi?id=',\n\nThe same comment as above.\n\n> +\t\t},\n> +\t\t'override' => 0,\n> +\t\t'sub' => \\&hyperlink_committag,\n> +\t},\n> +\t# Link URLs\n> +\t'url' => {\n> +\t\t'options' => {\n> +\t\t\t# Avoid matching punctuation that might immediately follow\n> +\t\t\t# a url, is not part of the url, and is allowed in urls,\n> +\t\t\t# like a full-stop ('.').\n> +\t\t\t'pattern' => qr!(http|ftp)s?://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n> +\t\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n\nIf you took this regexp from some place (like blog), it would be good\nto mention URL here, to be able to check more detailed explanation of\nconstruction of this URL-catching regexp.\n\nShould we also support irc://, nntp:// (pseudo)protocols? What about\ngit:// ?\n\n> +\t\t},\n> +\t\t'override' => 0,\n> +\t\t'sub' => sub {\n> +\t\t\tmy ($opts, @match) = @_;\n> +\t\t\treturn\n> +\t\t\t\t\\$cgi->a({-href => $match[0],\n> +\t\t\t\t -class => \"text\"},\n> +\t\t\t\t esc_html($match[0], -nbsp=>1));\n> +\t\t},\n\nHere you use explicit return.\n\n> +\t},\n> +\t# Link Message-Id to mailing list archive\n> +\t'messageid' => {\n> +\t\t'options' => {\n> +\t\t\t# The original pattern, which I don't really understand\n> +\t\t\t#'pattern' => qr!(?:message|msg)-id:?\\s+<([^>]+)>;!i,\n> +\t\t\t'pattern' => qr!(?:message|msg)-?id:?\\s+(<[^>]+>)!i,\n\nErrr... how original patter is different from the one used? Also above\ncomment should be removed in final submission.\n\n> +\t\t\t'url' => 'http://news.gmane.org/find-root.php?message_id=',\n\nSame comment about generic URL... although on the other hand perhaps\nhaving a few examples of mail archive sites which support finding \nmessages by Message-Id could be a good idea.\n\nBTW. you can write 'http://mid.gmane.org/' instead...\n\n> +\t\t},\n> +\t\t'override' => 0,\n> +\t\t# The original version didn't include the \"msg-id\" text in the\n> +\t\t# link text, but this does. In general, I think a little more\n> +\t\t# context makes for better link text.\n\nI guess that is the result of using generic hyperlink_committag() \nsubroutine here. (This comment should be removed or reworded in final\nsubmitted version, I think.)\n\nBTW. it would be much easier with Perl6-ish (or Perl 5.10.x) named\ncaptures (named groups):\n\n\t'pattern' => qr!(?:message|msg)-?id:?\\s+(?P<[^>]+>)!i,\n\nor something like that.\n\n> +\t\t'sub' => \\&hyperlink_committag,\n> +\t},\n> +);\n> +\n> # You define site-wide feature defaults here; override them with\n> # $GITWEB_CONFIG as necessary.\n> our %feature = (\n> @@ -365,6 +440,21 @@ our %feature = (\n> \t\t'sub' => \\&feature_patches,\n> \t\t'override' => 0,\n> \t\t'default' => [16]},\n> +\n> +\t# The selection and ordering of committags that are enabled.\n> +\t# Committag transformations will be applied to commit log messages\n> +\t# in this order if listed here.\n\n/this/given/\n\nYou need to mention somewhere that committag subroutines return a list\nof mixed scalar and reference to scalar elements, where using reference\nto scalar removes value from the chain of filters (including implicit\nfinal esc_html filter).\n\n> +\n> +\t# To disable system wide have in $GITWEB_CONFIG\n> +\t# $feature{'committags'}{'default'} = [];\n> +\t# To have project specific config enable override in $GITWEB_CONFIG\n> +\t# $feature{'committags'}{'override'} = 1;\n> +\t# and in project config gitweb.committags = sha1, url, bugzilla\n> +\t# to enable those three committags for that project\n\nJust a thought: perhaps we should provide support for 'default' in\nconfig (which would currently be \"sha1\" or \"sha1, url\").\n\nSee also comment text for 'snapshot' feature, which says:\n\n and in project config, a comma-separated list of [...] or \n \"none\" to disable.\n\n> +\t'committags' => {\n> +\t\t'sub' => \\&feature_committags,\n> +\t\t'override' => 0,\n> +\t\t'default' => ['sha1']},\n> );\n> \n> sub gitweb_get_feature {\n> @@ -433,6 +523,18 @@ sub feature_patches {\n> \treturn ($_[0]);\n> }\n> \n> +sub feature_committags {\n> +\tmy (@defaults) = @_;\n> +\n> +\tmy ($cfg) = git_get_project_config('committags');\n> +\n> +\tif ($cfg) {\n> +\t\treturn ($cfg eq 'none' ? () : split(/\\s*[,\\s]\\s*/, $cfg));\n> +\t}\n> +\n> +\treturn @defaults;\n> +}\n\nAs this would be second feature which uses comma-separated (or for\nbackward compatibility space separated) list of options, perhaps\nwe should factor out this part into common helper subroutine named\nfor example 'feature_list' or 'feature_multi' (like 'feature_bool').\n\n> +\n> # checking HEAD file with -e is fragile if the repository was\n> # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n> # and then pruned.\n> @@ -814,6 +916,34 @@ $git_dir = \"$projectroot/$project\" if $project;\n> our @snapshot_fmts = gitweb_get_feature('snapshot');\n> @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> \n> +# ordering of committags\n> +our @committags = gitweb_get_feature('committags');\n> +\n> +# Merge project configs with default committag definitions\n> +gitweb_load_project_committags();\n\nGood idea... although gitweb first defines and then uses subroutine,\nsee evaluate_path_info().\n\n> +\n> +# Load committag configs from the repository config file and and\n> +# incorporate them into the gitweb defaults where permitted by the\n> +# site administrator.\n> +sub gitweb_load_project_committags {\n> +\treturn if (!$git_dir);\n> +\tmy %project_config = ();\n> +\tmy %raw_config = git_parse_project_config('gitweb\\.committag');\n\nWhy not do lazy-loading of a whole config here? We use committag\ninfo only for project-specific actions in gitweb.\n\n> +\tforeach my $key (keys(%raw_config)) {\n> +\t\tnext if ($key !~ /gitweb\\.committag\\.[^.]+\\.[^.]/);\n> +\t\tmy ($gitweb_prefix, $committag_prefix, $ctname, $option) =\n> +\t\t\tsplit(/\\./, $key, 4);\n> +\t\t$project_config{$ctname}{$option} = $raw_config{$key};\n> +\t}\n\nAnd use created subroutines to handle config?\n\n> +\tforeach my $ctname (keys(%committags)) {\n> +\t\tnext if (!$committags{$ctname}{'override'});\n> +\t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n> +\t\t\t$committags{$ctname}{'options'}{$optname} =\n> +\t\t\t\t$project_config{$ctname}{$optname};\n> +\t\t}\n> +\t}\n> +}\n> +\n> # dispatch\n> if (!defined $action) {\n> \tif (defined $hash) {\n> @@ -1384,13 +1514,92 @@ sub file_type_long {\n> sub format_log_line_html {\n> \tmy $line = shift;\n> \n> -\t$line = esc_html($line, -nbsp=>1);\n> -\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n> -\t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n> -\t\t\t\t\t-class => \"text\"}, $1);\n> -\t}eg;\n> +\t# In this list of log message fragments, a string ref indicates HTML,\n> +\t# and a string indicates plain text\n> +\tmy @list = ( $line );\n\nWell, to be more exact string ref means that the string referenced is\nnot to be processed by later filters, including final implicit esc_html.\n\nPerhaps it would be better to use less generic name than @list herem\ne.g. @process or something?\n\n> \n> -\treturn $line;\n> +COMMITTAG:\n> +\tforeach my $ctname (@committags) {\n> +\t\tnext COMMITTAG unless exists $committags{$ctname};\n> +\t\tmy $committag = $committags{$ctname};\n> +\n> +\t\tnext COMMITTAG unless exists $committag->{'options'};\n> +\t\tmy $opts = $committag->{'options'};\n> +\n> +\t\tnext COMMITTAG unless exists $opts->{'pattern'};\n> +\t\tmy $pattern = $opts->{'pattern'};\n> +\n> +\t\tmy @newlist = ();\n> +\n> +\tPART:\n> +\t\tforeach my $part (@list) {\n> +\t\t\tnext PART if $part eq \"\";\n> +\t\t\tif (ref($part)) {\n> +\t\t\t\tpush @newlist, $part;\n> +\t\t\t\tnext PART;\n> +\t\t\t}\n> +\n> +\t\t\tmy $oldpos = 0;\n> +\n> +\t\tMATCH:\n> +\t\t\twhile ($part =~ m/$pattern/gc) {\n> +\t\t\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n> +\t\t\t\tmy $repl = $committag->{'sub'}->($opts, $&, $1);\n> +\t\t\t\t$repl = \"\" if (!defined $repl);\n> +\n> +\t\t\t\tmy $pre = substr($part, $oldpos, $prepos - $oldpos);\n> +\t\t\t\tpush_or_append(\\@newlist, $pre);\n> +\t\t\t\tpush_or_append(\\@newlist, $repl);\n> +\n> +\t\t\t\t$oldpos = $postpos;\n> +\t\t\t} # end while [regexp matches]\n> +\n> +\t\t\tmy $rest = substr($part, $oldpos);\n> +\t\t\tpush_or_append(\\@newlist, $rest);\n> +\n> +\t\t} # end foreach (@list)\n> +\n> +\t\t@list = @newlist;\n> +\t} # end foreach (@committags)\n> +\n> +\t# Escape any remaining plain text and concatenate\n> +\tmy $html = '';\n> +\tfor my $part (@list) {\n> +\t\tif (ref($part)) {\n> +\t\t\t$html .= $$part;\n> +\t\t} else {\n> +\t\t\t$html .= esc_html($part, -nbsp=>1);\n> +\t\t}\n> +\t}\n> +\n> +\treturn $html;\n> +}\n\nNice.\n\n> +\n> +# Returns a ref to an HTML snippet that links the second\n> +# parameter to a URL formed from the first and last parameters.\n> +# This is a helper function used in %committags.\n> +sub hyperlink_committag {\n> +\tmy ($opts, @match) = @_;\n> +\treturn\n> +\t\t\\$cgi->a({-href => $opts->{url} . CGI::escape($match[1]),\n\n$opts->{'url'} not $opts->{url}\n\n'$cgi->escapeHTML' I think, not 'CGI::escape' (but I am not sure here).\nbesides, we can always import 'escape'.\n\n> +\t\t\t\t -class => \"text\"},\n> +\t\t\t\t esc_html($match[0], -nbsp=>1));\n> +}\n> +\n> +\n> +sub push_or_append (\\@@) {\n\nHmmm... this would be first use of Perl subroutine prototypes in gitweb.\nBut this is made to imitate 'push' built-in, so I think it is O.K.\n\n> +\tmy $list = shift;\n> +\n> +\tif (ref $_[0] || ! @$list || ref $list->[-1]) {\n> +\t\tpush @$list, @_;\n> +\t} else {\n> +\t\tmy $a = pop @$list;\n> +\t\tmy $b = shift @_;\n> +\n> +\t\tpush @$list, $a . $b, @_;\n> +\t}\n> +\t# imitate push\n> +\treturn scalar @$list;\n> }\n> \n> # format marker of refs pointing to given object\n> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\n> index d539619..37a127c 100755\n> --- a/t/t9500-gitweb-standalone-no-errors.sh\n> +++ b/t/t9500-gitweb-standalone-no-errors.sh\n> @@ -55,9 +55,9 @@ gitweb_run () {\n> \t# some of git commands write to STDERR on error, but this is not\n> \t# written to web server logs, so we are not interested in that:\n> \t# we are interested only in properly formatted errors/warnings\n> -\trm -f gitweb.log &&\n> +\trm -f resp.http gitweb.log &&\n> \tperl -- \"$SCRIPT_NAME\" \\\n> -\t\t>/dev/null 2>gitweb.log &&\n> +\t\t> resp.http 2>gitweb.log &&\n> \tif grep \"^[[]\" gitweb.log >/dev/null 2>&1; then false; else true; fi\n> \n\nWell, if you begin to check _output_ of gitweb, then it should be put \nin separate test, not t/t9500-gitweb-standalone-no-errors.sh which is\nonly about no-errors... or change name of gitweb test.\n\n> \t# gitweb.log is left for debugging\n> @@ -702,4 +702,150 @@ test_expect_success \\\n> \t gitweb_run \"p=.git;a=summary\"'\n> test_debug 'cat gitweb.log'\n> \n> +# ----------------------------------------------------------------------\n> +# sha1 linking\n> +#\n> +echo hi > file.txt\n> +git add file.txt\n> +git commit -q -F - file.txt < +Summary\n> +\n> +See also commit 567890ab\n> +END\n> +test_expect_success 'sha1 link: enabled by default' '\n> +\th=$(git rev-parse --verify HEAD) &&\n> +\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n\nActually you can just use \"h=HEAD\" or use query without 'h' parameter\n(which defaults to \"HEAD\") here.\n\n> +\tgrep -q \\\n> +\t\t\"commit 567890ab\" \\\n> +\t\tresp.http\n> +'\n> +test_debug 'cat gitweb.log'\n> +test_debug 'grep 567890ab resp.http'\n\nI'd rather use Test::* (e.g. Test::WWW::Mechanize::CGI) for that...\nbut having some output test for gitweb, even in such simple form would\ncertainly be nice.\n\n> +\n> +# ----------------------------------------------------------------------\n> +# bugzilla commit tag\n> +#\n> +\n> +echo foo > file.txt\n> +git add file.txt\n> +git commit -q -F - file.txt < +Fix foo\n> +\n> +Fixes bug 1234 involving foo.\n> +END\n> +git config gitweb.committags 'sha1, bugzilla'\n> +test_expect_success 'bugzilla: enabled but not permitted' '\n> +\th=$(git rev-parse --verify HEAD) &&\n> +\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n> +\tgrep -F -q \\\n> +\t\t\"Fixes bug 1234 involving\" \\\n> +\t\tresp.http\n> +'\n> +test_debug 'cat gitweb.log'\n> +test_debug 'grep 1234 resp.http'\n> +\n> +echo '$feature{\"committags\"}{\"override\"} = 1;' >> gitweb_config.perl\n> +test_expect_success 'bugzilla: enabled' '\n> +\th=$(git rev-parse --verify HEAD) &&\n> +\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n> +\tgrep -F -q \\\n> +\t\t\"Fixes bug 1234 involving\" \\\n> +\t\tresp.http\n> +'\n\nHmmm...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"127814","messageId":"1258525350-5528-1-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"200906221318.19598.jnareb@gmail.com","subject":"[RFC PATCH 0/6] Second round of committag series","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-11-18T06:22:24Z","receivedAt":"2009-11-18T06:22:24Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"Thanks for the feedback. I've added four more patches to the end of\nthe series and updated the first two. My replies are below.\n\nOn Mon, 22 Jun 2009, Jakub Narebski wrote:\n> On Fri, 19 June 2009, Marcel M. Cary wrote:\n> \n> Thanks for diligently working on this issue. Good work!\n> \n> I see that it is an RFC, and not final submission, but just in case\n> I'd like to remind you that some of information below should go into\n> commit message, but some of it should I think go to comments (between\n> \"---\" and diffstat).\n> \n> > I want gitweb to hyperlink commits to my bug tracking system so that\n> > information regarding the current status of a commit can be easily\n> > cross-referenced. For example, the QA and release status of a commit\n> > cannot be inserted into the comment. Maybe someday a \"git notes\"\n> > feature will help with this, but for now, my organization has a\n> > separate bug tracking system. Other repository browsers such as\n> > unfuddle and websvn support similar features.\n> \n> The paragraph above should, I think, be made more clear. You don't\n> need to mention what you don't do; the comment about \"git notes\" should\n> be not in commit message but in comments section.\n> \n> What you want to have is to have some markers in commit message \n> (committags) hyperlinked; namely you want notifications about bug/issue\n> numbers in the commit message hyperlinked to appropriate bugtracker/issue\n> tracker URL. Do I understand this correctly?\n\nI'm not sure I would call the context of the bug numbers\n\"notifications\". A good example of what I'm looking for is to\nhyperlink \"Resolves-bug: 1234\" to\nhttp://bugzilla.example.com/show_bug.cgi?bug_id=1234. I suppose this\ncould be seen as a notification of bug 1234's resolution, except that\nI expect the more complex bugs (bugs, issues, and features) to require\nseveral commits to resolve, each of which I'd like tagged with that\nbug number, and so the bug would not really be resolved after the\nfirst such commit.\n\n> I tried here to reword what you said, to come up with better commit\n> message for a future final submission.\n\nI've rewritten the commit message paragraph as you suggested. Is it\nnow more inline with what you'd like to see?\n\n> > Since the bug hyperlinking feature was previously discussed as part of\n> > \"committags,\" a more general mechanism to embellish commit messages,\n> > implement the more general mechanism instead, including the following\n> > capabilities:\n> \n> Well, I think that the fact that it would be not much harder to create\n> general mechanism for commit message transformation, than to add \n> suitably generic and well customizable support for bugtracker \n> 'committags'.\n\nSo far, the biggest difficulty in generalizing the bugzilla hyperlink\nfeature has been the interaction between two or more commit tag\ntransformations -- how to structure them so they are composable but\ndecoupled.\n\n> > * Hyperlinking mentions of bug IDs to Bugzilla\n> > * Hyperlinking URLs\n> > * Hyperlinking Message-Ids to a mailing list archive\n> > * Hyperlinking commit hashes as before by default, now with a\n> > configurable regex\n> > * Defining new committags per gitweb installation\n> \n> Well, there is one _implicit_ (but important) commit message \n> transformation (filter) for display, which has to be always present[1],\n> namely HTML escaping. We make use of the fact that you can do\n> HTML escaping before doing the only currently supported committag, \n> namely hyperlinking (shortened) SHA-1 to 'object' gitweb URL, but for\n> other committags like mentioned \"Message-Id to mail archive\" committag\n> (filter) it would make them more difficult.\n\nThe original patch still did the HTML escaping. I don't see much\nvalue in implementing this transformation as a committag since it\nwouldn't be useful to configure it, and I don't really see it making\nthe code more clear or concise. Did you have any particular reasons?\n\n> Also one might consider vertical whitespace simplification (removing\n> leading empty lines, compacting empty lines to single empty line \n> between paragraphs), and syntax highlighting signoff lines to be\n> a kind of commit message filter like mentioned above committags\n> (see git_print_log() subroutine).\n> \n> Although probably vertical whitespace simplification should be not\n> made into commit message filter, as it is used not for all views.\n> \n> > \n> > Since different repositories may use different bug tracking systems or\n> > mailing list archives, the URL parameter may be configured\n> > per-repository without reiterating the regexes. To accomodate\n> > different conventions, regexes may also be configured per-project.\n> \n> Also list of supported committags is separated from the list of \n> committags used[1]; just like it is done for snapshot formats.\n> This could be mentioned in final commit message.\n> \n> [1] well, sequence rather than list in this case, as here\n> ordering does matter a bit\n> \n> > \n> > This patch is heavily based on discussions and code samples from the\n> > Git list:\n> > \n> > \t[RFC/PATCH] gitweb: Add committags support, Sep 2006\n> > \thttp://thread.gmane.org/gmane.comp.version-control.git/27504\n> > \n> > \t[RFC] gitweb: Add committags support (take 2), Dec 2006\n> > \thttp://thread.gmane.org/gmane.comp.version-control.git/33150\n> > \n> > \t[RFC] Configuring (future) committags support in gitweb, Nov 2008\n> > \thttp://thread.gmane.org/gmane.comp.version-control.git/100415\n> \n> Hmmm... should this be put in final commit message, or only in comment\n> to the patch (should this be in commit history of git repository)?\n\nI'll exclude the mailing list references from the commit message since\nthey describe how I arrived at this set of changes rather than\ndescribing the changes themselves.\n\n> > Some issues I considered but punted:\n> > \n> > * Should this configuration try to follow the bugtraq spec?\n> > \n> > As far as I know, only subversion implements it. Separation of\n> > regexes by a newline would be a little awkward in the git config.\n> > And it is broader than just hyperlinking bugs: it also encompasses\n> > GUI bug ID form fields. So gitweb would only implement a subset.\n> \n> I didn't even know that there is such spec. Were you talking about\n> http://tortoisesvn.net/issuetracker_integration or do you have different\n> URL in mind?\n\nThat is the Bugtraq spec, although I had been looking at a text-only\nversion of it at the time. I can't seem to find the text-only\ndocument, but I think that one explains it just as well.\n\n> > The gitweb configuration mechanism currently only reads\n> > keys starting with \"gitweb.\", but these parameters would be more\n> > broadly applicable, potentially to git-gui, for example.\n> \n> Actually the fact that gitweb reads only keys in the 'gitweb' section\n> from config is just a convention. There were (are) no config variables\n> in other places (other sections) which would be of interest to gitweb.\n> \n> > \n> > However, it *would* be useful for Git tools to standardize on\n> > config keys and interpretations of regexes and url formats. For\n> > example, git-gui might be able to hyperlink the same text as gitweb,\n> > and even show a separate bugID field when composing a commit\n> > message.\n> \n> This is I think a very good idea... but I think idea which \n> implementation can be left for later.\n\nVery well then, I'll leave everything related to the other git tools\nfor later.\n\n> > \n> > * I would prefer the regex match against the whole commit message.\n> > \n> > This would allow the regex to insist that a bug reference occur\n> > on the first line or non-first line of the commit message. However,\n> > even if we concatenated the log lines for the first committag,\n> > subsequent committags would see the text broken up.\n> > \n> > Also, it would allow the regex to match a phrase split across a\n> > line boundary, as dicussed at some length in the first thread,\n> > but again, only if no prior committags had interfered.\n> > \n> > This could happen in a later patch.\n> \n> Well, the change should be fairly easy: just concatenate lines before\n> passing them as single element list to commit message filters. OTOH\n> you would have to take care of end of line characters in committags\n> regexps.\n\nYes, it seems manageable to process the whole commit message at once\nrather than line by line. I've made this change to support patch 4 of\nthis series.\n\n> > * I would prefer the site admin have a way to let a repository\n> > owner define new committags, which means having a way to specify\n> > the 'sub' key from the repo config or having a flexible default.\n> \n> Perhaps, following your earlier suggestion to make committags supported\n> also by other tools, gitweb (and e.g. git-gui / gitk) use config \n> variable committag.. (where can be 'pattern' or 'url';\n\nThis is a great example of the kind of decision I'd like to make\nup-front to avoid breaking existing configs later:\ngitweb.committag.. vs. committag... I'm not\nvery interested in adding the git-gui / gitk feature myself. Is\nanyone actually interested in this feature? If not, perhaps it should\nstay under gitweb.committag... And even if there is\ninterest in the git-gui / gitk features, perhaps it would make sense\nto start with the gitweb-specific version of the config variable names\nand once there is cross-tool support, keep those configs as overrides\nfor gitweb only?\n\n> although I wonder if we can allow 'pattern' as malicious user can do\n> a DoS attack against gitweb / server using badly behaved regexp).\n\nYes, I imagine there is a risk of abuse in allowing patterns to be\nconfigurable. That's one reason why the site config can disallow\nper-project configuration of regexes. Are you suggesting there be a\nseparate flag to allow override of regexes than to allow override of\nother parameters? For example, with a separate flag for regexes,\nrepo.or.cz could allow you to configure the bug tracker URL but not\nthe bug regex. I've added very fine grain support for allowing only\nsome options to be overridden in patch 3 of this series.\n\n> > The bugtraq and some of the regex questions must be decided now to\n> > avoid breaking gitweb configs later.\n> \n> True.\n> \n> > \n> > Signed-off-by: Marcel M. Cary \n> > ---\n> > gitweb/INSTALL | 4 +\n> > gitweb/gitweb.perl | 221 +++++++++++++++++++++++++++++++-\n> > t/t9500-gitweb-standalone-no-errors.sh | 150 +++++++++++++++++++++-\n> > 3 files changed, 367 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/gitweb/INSTALL b/gitweb/INSTALL\n> > index 18c9ce3..223e39e 100644\n> > --- a/gitweb/INSTALL\n> > +++ b/gitweb/INSTALL\n> > @@ -123,6 +123,10 @@ GITWEB_CONFIG file:\n> > \t$feature{'snapshot'}{'default'} = ['zip', 'tgz'];\n> > \t$feature{'snapshot'}{'override'} = 1;\n> > \n> > +\t$feature{'committags'}{'default'} = ['sha1', 'url', 'bugzilla'];\n> > +\t$feature{'committags'}{'override'} = 1;\n> > +\n> > +\n> > \n> > Gitweb repositories\n> > -------------------\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 1e7e2d8..c66fdf3 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -195,6 +195,81 @@ our %known_snapshot_format_aliases = (\n> > \t'x-zip' => undef, '' => undef,\n> > );\n> > \n> \n> I understand that comments such as one below would be not present in\n> a final submission, and they are here to provide running commentary for\n> code, isn't it?\n\nYes. I've removed the running commentary, since it seems to be in the\nway.\n\n> > +# Could call these something else besides committags... embellishments,\n> > +# patterns, rewrite rules, ?\n> \n> They are \"commit filters\", or \"commit message filters\" (or 'formatters',\n> or 'processors'; they are not 'parsers'). \n> \n> The name 'committag' was first introduced as far as I remember in xmms2\n> fork of gitweb (in old times when gitweb was separate project, and not\n> part of git repository).\n\nSounds as though there is no interest in changing the name to\nsomething that more clearly describes the feature (asside from my own,\nbut I'd like a little more validation...). Let's stick with\ncommittag.\n\n> > +#\n> > +# In general, the site admin can enable/disable per-project configuration\n> > +# of each committag. Only the 'options' part of the committag is configurable\n> > +# per-project.\n> \n> See above caveat about allowing to customize 'regexp'/'pattern' part\n> in untrusted environment; you can construct regexp which has exponential\n> behavior.\n> \n> > +#\n> > +# The site admin can of course add new tags to this hash or override the\n> > +# 'sub' key if necessary. But such changes may be fragile; this is not\n> > +# designed as a full-blown plugin architecture.\n> > +our %committags = (\n> \n> \n> You should put the comments here about supported keys, similar to the\n> one for %known_snapshot_formats and %feature hashes.\n\nI believe I've addressed your request by adding a comment that\napplies to all tags. Because of the similarity of the tags, I don't\nthink it would be very useful to do this for each tag. Does the 'url'\noption used on two of the tags need further explanation?\n\n> > +\t# Link Git-style hashes to this gitweb\n> > +\t'sha1' => {\n> > +\t\t'options' => {\n> > +\t\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n> > +\t\t},\n> > +\t\t'override' => 0,\n> \n> Shouldn't 'override' key be better last?\n\nI liked 'override' close to the 'options' hash because that is what it\ncontrols. The 'sub' key was not overridable per-project no matter how\nyou configure gitweb. Now it is, so maybe now 'override' could go\nlast. Or first. But really, I don't think the order matters much.\n\n> > +\t\t'sub' => sub {\n> > +\t\t\tmy ($opts, @match) = @_;\n> > +\t\t\t\\$cgi->a({-href => href(action=>\"object\", hash=>$match[1]),\n> > +\t\t\t -class => \"text\"}, esc_html($match[0], -nbsp=>1));\n> > +\t\t},\n> \n> Style: although there is commonly used idiom to use 'sub { ; }'\n> for a wrapper subroutines (e.g. 'sub { [] }' in Moose examples), one\n> should use explicit \"return\" statement instead of relying on Perl \n> behavior of returning last statement in a block.\n> \n> See Perl::Critic::Policy::Subroutines::RequireFinalReturn policy in\n> Perl::Critic (perlcritic.com). \"Perl Best Practices\" says:\n> \n> Subroutines without explicit 'return' statements at their ends can be\n> confusing. It can be challenging to deduce what the return value will be.\n> \n> > +\t},\n> > +\t# Link bug/features to Mantis bug tracker using Mantis-style contextual cues\n> > +\t'mantis' => {\n> > +\t\t'options' => {\n> > +\t\t\t'pattern' => qr/(?:BUG|FEATURE)\\((\\d+)\\)/,\n> > +\t\t\t'url' => 'http://bugs.xmms2.xmms.se/view.php?id=',\n> \n> I don't think we want to put such URL here. Please check if Mantis \n> documentation uses some specific links, or follow RFC conventions and\n> use 'example.com' as hostname (e.g. 'bugs.example.com').\n\nOk, I'll use a neutral URL for the mantis default.\n\n> By the way the bugtraq proposal you mentioned uses placeholder in URL\n> for putting issue number (%BUGID%). Perhaps gitweb should do the same\n> here.\n\nI'd rather use a regex-style or sprintf-style syntax that could be\nused by all committags. I started with just an opaque string to\nprepend to the bug ID because it was the simplest way to fulfill my\nrequirements, but it's certainly plausible that a BTS would want urls\nlike \"example.com/bug/1234/detail\" in which case the prepending\nstrategy isn't sufficient. Again, ideally we'd decide now whether to\nuse '$1' or '%s' so we don't break configs later. If the bugtraq\nfeature is implemented at some point, those config parameters can\nstill use the %BUGID% placeholder.\n\n> > +\t\t},\n> > +\t\t'override' => 0,\n> > +\t\t'sub' => \\&hyperlink_committag,\n> > +\t},\n> > +\t# Link mentions of bug IDs to bugzilla\n> > +\t'bugzilla' => {\n> > +\t\t'options' => {\n> > +\t\t\t'pattern' => qr/bug\\s+(\\d+)/,\n> > +\t\t\t'url' => 'http://bugzilla.kernel.org/show_bug.cgi?id=',\n> \n> The same comment as above.\n> \n> > +\t\t},\n> > +\t\t'override' => 0,\n> > +\t\t'sub' => \\&hyperlink_committag,\n> > +\t},\n> > +\t# Link URLs\n> > +\t'url' => {\n> > +\t\t'options' => {\n> > +\t\t\t# Avoid matching punctuation that might immediately follow\n> > +\t\t\t# a url, is not part of the url, and is allowed in urls,\n> > +\t\t\t# like a full-stop ('.').\n> > +\t\t\t'pattern' => qr!(http|ftp)s?://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n> > +\t\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n> \n> If you took this regexp from some place (like blog), it would be good\n> to mention URL here, to be able to check more detailed explanation of\n> construction of this URL-catching regexp.\n\nActually, I think I started with a URL regex mentioned on-list and\nthen tweaked it until I thought it worked well enough.\n\nMost of the regexes I found on the web had issues. They tended to be\ntoo long because they were trying to precisely validate the URL or\nbecause they were trying to parse all the different parts of the url,\nor they did not deal with the trailing punctuation problem: I don't\nwant the period (\".\") to be highlighted in this context: \"See\nhttp://example.com/foo.\" I think it's a valid URL even with the\nperiod, but typically the period will not be part of the URL. On the\nother hand, \"http://example.com/foo.html\" should be completely\nhighlighted in spite of the period.\n\n> Should we also support irc://, nntp:// (pseudo)protocols? What about\n> git:// ?\n\nI'va added more schemes, but it's also possible to leave those as a\ncustomization. In my organization, it would be very uncommon for\nsomeone to follow a URL with any of those schemes. I'd be willing to\nallow any scheme that matches \"[a-z]+://\". I don't care about stuff\nlike \"data:literal+text\", and \"mailto:\" is also less interesting to\nme. (I'd rather add a committag to match the email address without a\n\"mailto:\".) And then there's those pesky windows file sharing\nthingies like '\\\\server\\directory' that work like URLs in Internet\nExplorer. I'd rather not include them in the URL committag... but a\nsite admin could certainly configure a committag for it. Here's a\nlist schemes for which KDE allegedly supports retreival of metadata:\n\nbluetooth, fish, ftp, imap(s), invitation, iso, ldap(s), mac, mdns,\nnfs, nntp(s), nxfish, obex, pop3(s), print, printdb, sdp, service,\nsftp, slp, smb, smtp(s), webdav(s)\n\nI wouldn't want to enumerate all of those, but, in addition to the\nthree you suggest, these look most useful to me: sftp, smb, webdav(s),\nnfs. So really I think it comes down to whether we want to enumerate\nthem or just match any scheme, and risk matching something that was\nnot intended as a URL. And if we enumerate, how many do we want to\nlist.\n\nWhat do you think of the new list of schemes in patch 1? I've\nincluded several more than before.\n\n> > +\t\t},\n> > +\t\t'override' => 0,\n> > +\t\t'sub' => sub {\n> > +\t\t\tmy ($opts, @match) = @_;\n> > +\t\t\treturn\n> > +\t\t\t\t\\$cgi->a({-href => $match[0],\n> > +\t\t\t\t -class => \"text\"},\n> > +\t\t\t\t esc_html($match[0], -nbsp=>1));\n> > +\t\t},\n> \n> Here you use explicit return.\n> \n> > +\t},\n> > +\t# Link Message-Id to mailing list archive\n> > +\t'messageid' => {\n> > +\t\t'options' => {\n> > +\t\t\t# The original pattern, which I don't really understand\n> > +\t\t\t#'pattern' => qr!(?:message|msg)-id:?\\s+<([^>]+)>;!i,\n> > +\t\t\t'pattern' => qr!(?:message|msg)-?id:?\\s+(<[^>]+>)!i,\n> \n> Errr... how original patter is different from the one used? Also above\n> comment should be removed in final submission.\n\nThe most important change is that the semicolon is removed. I\nalso made the dash optional. It wasn't clear to me whether this was\nintended to match a header-style reference:\n\n Message-Id: \n\nOr a casual mention:\n\n In msgid , John Doe says...\n\nBut I like the latter, and would like the former to still be\nsupported.\n\n> > +\t\t\t'url' => 'http://news.gmane.org/find-root.php?message_id=',\n> \n> Same comment about generic URL... although on the other hand perhaps\n> having a few examples of mail archive sites which support finding \n> messages by Message-Id could be a good idea.\n> \n> BTW. you can write 'http://mid.gmane.org/' instead...\n\nThanks for the tip. Is there another common mail archive site\nthat allows looking up emails by message-id? If so, I'd love to\nmention the url in the comment for that committag. I think we need to\nstrike a balance between having things work with minimal configuration\nand avoiding the promotion of specific web sites that won't make sense\nfor most sites using a particular committag. So, unless there is a\nwhole slew of web sites that provide this feature, I'd suggest leaving\nthe specific URL in.\n\n> > +\t\t},\n> > +\t\t'override' => 0,\n> > +\t\t# The original version didn't include the \"msg-id\" text in the\n> > +\t\t# link text, but this does. In general, I think a little more\n> > +\t\t# context makes for better link text.\n> \n> I guess that is the result of using generic hyperlink_committag() \n> subroutine here. (This comment should be removed or reworded in final\n> submitted version, I think.)\n\nYes, although if we decided it was sub-optimal link text, I'd be\nhappy to use a less generic mechanism.\n\n> BTW. it would be much easier with Perl6-ish (or Perl 5.10.x) named\n> captures (named groups):\n> \n> \t'pattern' => qr!(?:message|msg)-?id:?\\s+(?P<[^>]+>)!i,\n> \n> or something like that.\n\nI like the notion of names rather than numberic indices, but I'm\nhesitant to require site admins to use unfamiliar Perl regex syntax to\nconfigure committags. Of two admins I asked, neither could make sense\nof the sample regex. If wouldn't want a site admin to *have* to learn\nnew syntax to configure regexes.\n\n> > +\t\t'sub' => \\&hyperlink_committag,\n> > +\t},\n> > +);\n> > +\n> > # You define site-wide feature defaults here; override them with\n> > # $GITWEB_CONFIG as necessary.\n> > our %feature = (\n> > @@ -365,6 +440,21 @@ our %feature = (\n> > \t\t'sub' => \\&feature_patches,\n> > \t\t'override' => 0,\n> > \t\t'default' => [16]},\n> > +\n> > +\t# The selection and ordering of committags that are enabled.\n> > +\t# Committag transformations will be applied to commit log messages\n> > +\t# in this order if listed here.\n> \n> /this/given/\n> \n> You need to mention somewhere that committag subroutines return a list\n> of mixed scalar and reference to scalar elements, where using reference\n> to scalar removes value from the chain of filters (including implicit\n> final esc_html filter).\n\nI chose to add that explanation in the section that describes the\nconfiguration of committags rather than in this section that defines\nthe list of active committags. Sound ok?\n\n> > +\n> > +\t# To disable system wide have in $GITWEB_CONFIG\n> > +\t# $feature{'committags'}{'default'} = [];\n> > +\t# To have project specific config enable override in $GITWEB_CONFIG\n> > +\t# $feature{'committags'}{'override'} = 1;\n> > +\t# and in project config gitweb.committags = sha1, url, bugzilla\n> > +\t# to enable those three committags for that project\n> \n> Just a thought: perhaps we should provide support for 'default' in\n> config (which would currently be \"sha1\" or \"sha1, url\").\n\nI didn't understand how this would be very useful until I added\nthe signoff committag. The use case I see is that it allows the\ngitweb distribution to adjust the base functionality, in this case by\npushing some functionality into a committag, without project owners\nneeding to reconfigure their repositories. Site admins don't need\nthis because they can use \"push\" or \"unshift\" to preserve the\ndistributed committags, right? The project owner should get the\ndistribution default, not the site default, when they write \"default\",\nright? Are there other use cases you had in mind? A potential\nimplementation is in patch 6.\n\n> See also comment text for 'snapshot' feature, which says:\n> \n> and in project config, a comma-separated list of [...] or \n> \"none\" to disable.\n> \n> > +\t'committags' => {\n> > +\t\t'sub' => \\&feature_committags,\n> > +\t\t'override' => 0,\n> > +\t\t'default' => ['sha1']},\n> > );\n> > \n> > sub gitweb_get_feature {\n> > @@ -433,6 +523,18 @@ sub feature_patches {\n> > \treturn ($_[0]);\n> > }\n> > \n> > +sub feature_committags {\n> > +\tmy (@defaults) = @_;\n> > +\n> > +\tmy ($cfg) = git_get_project_config('committags');\n> > +\n> > +\tif ($cfg) {\n> > +\t\treturn ($cfg eq 'none' ? () : split(/\\s*[,\\s]\\s*/, $cfg));\n> > +\t}\n> > +\n> > +\treturn @defaults;\n> > +}\n> \n> As this would be second feature which uses comma-separated (or for\n> backward compatibility space separated) list of options, perhaps\n> we should factor out this part into common helper subroutine named\n> for example 'feature_list' or 'feature_multi' (like 'feature_bool').\n\nThanks for pointing out the opportunity to fold the feature list\nfunctions together.\n\n> > +\n> > # checking HEAD file with -e is fragile if the repository was\n> > # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n> > # and then pruned.\n> > @@ -814,6 +916,34 @@ $git_dir = \"$projectroot/$project\" if $project;\n> > our @snapshot_fmts = gitweb_get_feature('snapshot');\n> > @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> > \n> > +# ordering of committags\n> > +our @committags = gitweb_get_feature('committags');\n> > +\n> > +# Merge project configs with default committag definitions\n> > +gitweb_load_project_committags();\n> \n> Good idea... although gitweb first defines and then uses subroutine,\n> see evaluate_path_info().\n\nSounds like you're asking me to move the function call after the\nfunction definition. I've made the change, but I'm also curious to\nknow why you have that preference. I sometimes find it less readable,\nas in this case.\n\n> > +\n> > +# Load committag configs from the repository config file and and\n> > +# incorporate them into the gitweb defaults where permitted by the\n> > +# site administrator.\n> > +sub gitweb_load_project_committags {\n> > +\treturn if (!$git_dir);\n> > +\tmy %project_config = ();\n> > +\tmy %raw_config = git_parse_project_config('gitweb\\.committag');\n> \n> Why not do lazy-loading of a whole config here? We use committag\n> info only for project-specific actions in gitweb.\n\nI thought the check for $git_dir would prevent loading it for\nnon-project-specific pages. Even so, I suppose we might load the\nconfig on a page like the shortlog page that doesn't need it.\n\n> > +\tforeach my $key (keys(%raw_config)) {\n> > +\t\tnext if ($key !~ /gitweb\\.committag\\.[^.]+\\.[^.]/);\n> > +\t\tmy ($gitweb_prefix, $committag_prefix, $ctname, $option) =\n> > +\t\t\tsplit(/\\./, $key, 4);\n> > +\t\t$project_config{$ctname}{$option} = $raw_config{$key};\n> > +\t}\n> \n> And use created subroutines to handle config?\n\nAre you saying I should be using other subroutines that already\nexist in gitweb rather than implementing my own? Are you thinking of\ngit_get_project_config? If I understand correctly, it requires me to\nenumerate all the config keys I want. I'd rather not require the\ncommittags to have a predetermined set of possible config keys. I see\nnow that I could still call git_get_project_config to load data into\n%config and access that directly... I didn't do it before because it\nseems to violate some kind of abstraction.\n\n> > +\tforeach my $ctname (keys(%committags)) {\n> > +\t\tnext if (!$committags{$ctname}{'override'});\n> > +\t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n> > +\t\t\t$committags{$ctname}{'options'}{$optname} =\n> > +\t\t\t\t$project_config{$ctname}{$optname};\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > # dispatch\n> > if (!defined $action) {\n> > \tif (defined $hash) {\n> > @@ -1384,13 +1514,92 @@ sub file_type_long {\n> > sub format_log_line_html {\n> > \tmy $line = shift;\n> > \n> > -\t$line = esc_html($line, -nbsp=>1);\n> > -\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n> > -\t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n> > -\t\t\t\t\t-class => \"text\"}, $1);\n> > -\t}eg;\n> > +\t# In this list of log message fragments, a string ref indicates HTML,\n> > +\t# and a string indicates plain text\n> > +\tmy @list = ( $line );\n> \n> Well, to be more exact string ref means that the string referenced is\n> not to be processed by later filters, including final implicit esc_html.\n\nWell... it means both not processing and HTML escaping. The string\nrefs will also not be escaped in the final processing step.\n\n> Perhaps it would be better to use less generic name than @list herem\n> e.g. @process or something?\n\nSure. I chose @message_fragments as the less generic name for @list.\n\n> > -\treturn $line;\n> > +COMMITTAG:\n> > +\tforeach my $ctname (@committags) {\n> > +\t\tnext COMMITTAG unless exists $committags{$ctname};\n> > +\t\tmy $committag = $committags{$ctname};\n> > +\n> > +\t\tnext COMMITTAG unless exists $committag->{'options'};\n> > +\t\tmy $opts = $committag->{'options'};\n> > +\n> > +\t\tnext COMMITTAG unless exists $opts->{'pattern'};\n> > +\t\tmy $pattern = $opts->{'pattern'};\n> > +\n> > +\t\tmy @newlist = ();\n> > +\n> > +\tPART:\n> > +\t\tforeach my $part (@list) {\n> > +\t\t\tnext PART if $part eq \"\";\n> > +\t\t\tif (ref($part)) {\n> > +\t\t\t\tpush @newlist, $part;\n> > +\t\t\t\tnext PART;\n> > +\t\t\t}\n> > +\n> > +\t\t\tmy $oldpos = 0;\n> > +\n> > +\t\tMATCH:\n> > +\t\t\twhile ($part =~ m/$pattern/gc) {\n> > +\t\t\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n> > +\t\t\t\tmy $repl = $committag->{'sub'}->($opts, $&, $1);\n> > +\t\t\t\t$repl = \"\" if (!defined $repl);\n> > +\n> > +\t\t\t\tmy $pre = substr($part, $oldpos, $prepos - $oldpos);\n> > +\t\t\t\tpush_or_append(\\@newlist, $pre);\n> > +\t\t\t\tpush_or_append(\\@newlist, $repl);\n> > +\n> > +\t\t\t\t$oldpos = $postpos;\n> > +\t\t\t} # end while [regexp matches]\n> > +\n> > +\t\t\tmy $rest = substr($part, $oldpos);\n> > +\t\t\tpush_or_append(\\@newlist, $rest);\n> > +\n> > +\t\t} # end foreach (@list)\n> > +\n> > +\t\t@list = @newlist;\n> > +\t} # end foreach (@committags)\n> > +\n> > +\t# Escape any remaining plain text and concatenate\n> > +\tmy $html = '';\n> > +\tfor my $part (@list) {\n> > +\t\tif (ref($part)) {\n> > +\t\t\t$html .= $$part;\n> > +\t\t} else {\n> > +\t\t\t$html .= esc_html($part, -nbsp=>1);\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn $html;\n> > +}\n> \n> Nice.\n\nHehe, that bit of code is mostly yours, which you posted on this list\nages ago. (: I suppose we should try to get your signoff into the\nfirst patch of the series?\n\n> > +\n> > +# Returns a ref to an HTML snippet that links the second\n> > +# parameter to a URL formed from the first and last parameters.\n> > +# This is a helper function used in %committags.\n> > +sub hyperlink_committag {\n> > +\tmy ($opts, @match) = @_;\n> > +\treturn\n> > +\t\t\\$cgi->a({-href => $opts->{url} . CGI::escape($match[1]),\n> \n> $opts->{'url'} not $opts->{url}\n> \n> '$cgi->escapeHTML' I think, not 'CGI::escape' (but I am not sure here).\n> besides, we can always import 'escape'.\n> \n> > +\t\t\t\t -class => \"text\"},\n> > +\t\t\t\t esc_html($match[0], -nbsp=>1));\n> > +}\n> > +\n> > +\n> > +sub push_or_append (\\@@) {\n> \n> Hmmm... this would be first use of Perl subroutine prototypes in gitweb.\n> But this is made to imitate 'push' built-in, so I think it is O.K.\n\nOk, I guess I'll leave the subroutine prototype in then. I don't\nunderstand all the implications, so if you think it'll work well\nwithout the prototype, I'm happy to remove it. This is code\nthat I think you posted to the list.\n\n> > +\tmy $list = shift;\n> > +\n> > +\tif (ref $_[0] || ! @$list || ref $list->[-1]) {\n> > +\t\tpush @$list, @_;\n> > +\t} else {\n> > +\t\tmy $a = pop @$list;\n> > +\t\tmy $b = shift @_;\n> > +\n> > +\t\tpush @$list, $a . $b, @_;\n> > +\t}\n> > +\t# imitate push\n> > +\treturn scalar @$list;\n> > }\n> > \n> > # format marker of refs pointing to given object\n> > diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\n> > index d539619..37a127c 100755\n> > --- a/t/t9500-gitweb-standalone-no-errors.sh\n> > +++ b/t/t9500-gitweb-standalone-no-errors.sh\n> > @@ -55,9 +55,9 @@ gitweb_run () {\n> > \t# some of git commands write to STDERR on error, but this is not\n> > \t# written to web server logs, so we are not interested in that:\n> > \t# we are interested only in properly formatted errors/warnings\n> > -\trm -f gitweb.log &&\n> > +\trm -f resp.http gitweb.log &&\n> > \tperl -- \"$SCRIPT_NAME\" \\\n> > -\t\t>/dev/null 2>gitweb.log &&\n> > +\t\t> resp.http 2>gitweb.log &&\n> > \tif grep \"^[[]\" gitweb.log >/dev/null 2>&1; then false; else true; fi\n> > \n> \n> Well, if you begin to check _output_ of gitweb, then it should be put \n> in separate test, not t/t9500-gitweb-standalone-no-errors.sh which is\n> only about no-errors... or change name of gitweb test.\n\nI see there is some precedent for adding lib-foo.sh files. Perhaps it\nwould be appropriate to move parts of the no-errors test into a\nlib-gitweb.sh? Like gitweb_init, most of gitweb_run, and maybe the\nperl checks (which are that way for lib-git-svn.sh)? I'm all for\nseparating the tests. I don't like having to keep telling the test to\nskip the first 85 cases when all I want to run is the committag stuff.\n\nActually, looks like Mark Rada already made a gitweb-lib.sh which was\nexactly the same as the one I'd made except the name of the lib (vs.\nlib-gitweb.sh) and the name of the file holding gitweb output. I've\nrebased onto those changes.\n\n> I think I'm gonna leave this and the other feedback\n> > \t# gitweb.log is left for debugging\n> > @@ -702,4 +702,150 @@ test_expect_success \\\n> > \t gitweb_run \"p=.git;a=summary\"'\n> > test_debug 'cat gitweb.log'\n> > \n> > +# ----------------------------------------------------------------------\n> > +# sha1 linking\n> > +#\n> > +echo hi > file.txt\n> > +git add file.txt\n> > +git commit -q -F - file.txt < > +Summary\n> > +\n> > +See also commit 567890ab\n> > +\th=$(git rev-parse --verify HEAD) &&\n> > +\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n> \n> Actually you can just use \"h=HEAD\" or use query without 'h' parameter\n> (which defaults to \"HEAD\") here.\n \nThanks for the tip, I'll use h=HEAD.\n \n> > +\tgrep -q \\\n> > +\t\t\"commit 567890ab\" \\\n> > +\t\tresp.http\n> > +'\n> > +test_debug 'cat gitweb.log'\n> > +test_debug 'grep 567890ab resp.http'\n> \n> I'd rather use Test::* (e.g. Test::WWW::Mechanize::CGI) for that...\n> but having some output test for gitweb, even in such simple form would\n> certainly be nice.\n\nWould this mean people need to install some Mechanize stuff before\nthey can run the tests? I think I'd rather avoid the dependency as\nlong as grep seems to do the trick.\n\n> > +\n> > +# ----------------------------------------------------------------------\n> > +# bugzilla commit tag\n> > +#\n> > +\n> > +echo foo > file.txt\n> > +git add file.txt\n> > +git commit -q -F - file.txt < > +Fix foo\n> > +\n> > +Fixes bug 1234 involving foo.\n> > +END\n> > +git config gitweb.committags 'sha1, bugzilla'\n> > +test_expect_success 'bugzilla: enabled but not permitted' '\n> > +\th=$(git rev-parse --verify HEAD) &&\n> > +\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n> > +\tgrep -F -q \\\n> > +\t\t\"Fixes bug 1234 involving\" \\\n> > +\t\tresp.http\n> > +'\n> > +test_debug 'cat gitweb.log'\n> > +test_debug 'grep 1234 resp.http'\n> > +\n> > +echo '$feature{\"committags\"}{\"override\"} = 1;' >> gitweb_config.perl\n> > +test_expect_success 'bugzilla: enabled' '\n> > +\th=$(git rev-parse --verify HEAD) &&\n> > +\tgitweb_run \"p=.git;a=commit;h=$h\" &&\n> > +\tgrep -F -q \\\n> > +\t\t\"Fixes bug 1234 involving\" \\\n> > +\t\tresp.http\n> > +'\n> \n> Hmmm...\n> \n\n?\n\n\n\nMarcel M. Cary (6):\n gitweb: Hyperlink various committags in commit message with regex\n gitweb: Add second-stage matching of bug IDs in bugzilla committag\n gitweb: Allow finer-grained override controls for committags\n gitweb: Allow committag pattern matches to span multiple lines\n gitweb: Allow per-repository definition of new committags\n gitweb: Add _defaults_ keyword for feature lists in project config\n\n gitweb/INSTALL | 16 ++\n gitweb/gitweb.perl | 404 +++++++++++++++++++++++++++++++++++++-----\n t/t9502-gitweb-committags.sh | 309 ++++++++++++++++++++++++++++++++\n 3 files changed, 681 insertions(+), 48 deletions(-)\n create mode 100755 t/t9502-gitweb-committags.sh\n"},{"id":"127817","messageId":"1258525350-5528-2-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"1258525350-5528-1-git-send-email-marcel@oak.homeunix.org","subject":"[RFC PATCH 1/6] gitweb: Hyperlink committags in a commit message by regex matching","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-11-18T06:22:25Z","receivedAt":"2009-11-18T06:22:25Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"I want gitweb to hyperlink commits to my bug tracking system so that\ninformation regarding the current status of a commit can be easily\ncross-referenced. The QA and release status of a commit cannot be\ndirectly inserted into the commit message because they change over\ntime. But if the commit message mentions a bug number, gitweb could\ndetect the bug reference in the message and hyperlink it to the bug\ntracking system. Other repository browsers such as unfuddle and\nwebsvn support similar features.\n\nCurrently only commit hashes are hyperlinked in this manner.\n\nSince the bug hyperlinking feature was previously discussed as part of\n\"committags,\" a more general mechanism to embellish commit messages,\nimplement the more general mechanism instead, including the following\ncapabilities:\n\n* Hyperlinking mentions of bug IDs to Bugzilla\n* Hyperlinking URLs\n* Hyperlinking Message-Ids to a mailing list archive\n* Hyperlinking commit hashes, as before by default, now with a\n configurable regex\n* Defining new committags per gitweb installation\n\nSince different repositories may use different bug tracking systems or\nmailing list archives, the URL parameter may be configured\nper-repository without reiterating the regexes. To accomodate\ndifferent conventions, regexes may also be configured per-project.\n\nThe order in which gitweb applies committags may be configured\nper-project as well, because one committag may affect subsequent ones.\nInclusion in this sequence determines whether a committag is enabled\nor not.\n\nSigned-off-by: Marcel M. Cary \n---\n\nOne additional thing that occured to me is that maybe the hyperlinks\nadded by committags should have 'rel=\"nofollow\"' by default? And if\nso, maybe that needs to be configurable? On the other hand, I'm not\nsure how useful it is to hide real URLs in the commit messages from\nsearch engines... ?\n\n\n gitweb/INSTALL | 5 +\n gitweb/gitweb.perl | 247 +++++++++++++++++++++++++++++++++++++++---\n t/t9502-gitweb-committags.sh | 150 +++++++++++++++++++++++++\n 3 files changed, 389 insertions(+), 13 deletions(-)\n create mode 100755 t/t9502-gitweb-committags.sh\n\ndiff --git a/gitweb/INSTALL b/gitweb/INSTALL\nindex b76a0cf..9081ed8 100644\n--- a/gitweb/INSTALL\n+++ b/gitweb/INSTALL\n@@ -132,6 +132,11 @@ adding the following lines to your $GITWEB_CONFIG:\n \t$known_snapshot_formats{'zip'}{'disabled'} = 1;\n \t$known_snapshot_formats{'tgz'}{'compressor'} = ['gzip','-6'];\n \n+To add a committag to the default list of commit tags, for example to\n+enable hyperlinking of bug numbers to a bug tracker for all projects:\n+\n+\tpush @{$feature{'committags'}{'default'}}, 'bugzilla';\n+\n \n Gitweb repositories\n -------------------\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e4cbfc3..2d72202 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -213,6 +213,97 @@ our %avatar_size = (\n \t'double' => 32\n );\n \n+# In general, the site admin can enable/disable per-project\n+# configuration of each committag. Only the 'options' part of the\n+# committag is configurable per-project.\n+#\n+# The site admin can of course add new tags to this hash or override\n+# the 'sub' key if necessary. But such changes may be fragile; this\n+# is not designed as a full-blown plugin architecture. The 'sub' must\n+# return a list of strings or string refs. The strings must contain\n+# plain text and the string refs must contain HTML. The string refs\n+# will not be processed further.\n+#\n+# For any committag, set the 'override' key to 1 to allow individual\n+# projects to override entries in the 'options' hash for that tag.\n+# For example, to match only commit hashes given in lowercase in one\n+# project, add this to the $GITWEB_CONFIG:\n+#\n+# $committags{'sha1'}{'override'} = 1;\n+#\n+# And in the project's config:\n+#\n+# gitweb.committags.sha1.pattern = \\\\b([0-9a-f]{8,40})\\\\b\n+#\n+# Some committags have additional options whose interpretation depends\n+# on the implementation of the 'sub' key. The hyperlink_committag\n+# value appends the first captured group to the 'url' option.\n+our %committags = (\n+\t# Link Git-style hashes to this gitweb\n+\t'sha1' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\treturn \\$cgi->a({-href => href(action=>\"object\", hash=>$match[1]),\n+\t\t\t -class => \"text\"},\n+\t\t\t esc_html($match[0], -nbsp=>1));\n+\t\t},\n+\t},\n+\t# Link bug/features to Mantis bug tracker using Mantis-style\n+\t# contextual cues\n+\t'mantis' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr/(?:BUG|FEATURE)\\((\\d+)\\)/,\n+\t\t\t'url' => 'http://www.example.com/mantisbt/view.php?id=',\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => \\&hyperlink_committag,\n+\t},\n+\t# Link mentions of bug IDs to bugzilla\n+\t'bugzilla' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr/bug\\s+(\\d+)/,\n+\t\t\t'url' => 'http://bugzilla.example.com/show_bug.cgi?id=',\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => \\&hyperlink_committag,\n+\t},\n+\t# Link URLs\n+\t'url' => {\n+\t\t'options' => {\n+\t\t\t# Avoid matching punctuation that might immediately follow\n+\t\t\t# a url, is not part of the url, and is allowed in urls,\n+\t\t\t# like a full-stop ('.').\n+\t\t\t'pattern' => qr!(https?|ftps?|git|ssh|ssh+git|sftp|smb|webdavs?|\n+\t\t\t nfs|irc|nntp|rsync)\n+\t\t\t ://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n+\t\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\treturn \\$cgi->a({-href => $match[0],\n+\t\t\t -class => \"text\"},\n+\t\t\t esc_html($match[0], -nbsp=>1));\n+\t\t},\n+\t},\n+\t# Link Message-Id to mailing list archive\n+\t'messageid' => {\n+\t\t'options' => {\n+\t\t\t'pattern' => qr!(?:message|msg)-?id:?\\s+(<[^>]+>)!i,\n+\t\t\t'url' => 'http://mid.gmane.org/',\n+\t\t},\n+\t\t'override' => 0,\n+\t\t# Includes the \"msg-id\" text in the link text.\n+\t\t# Since we don't support linking multiple msg-ids in one match, we\n+\t\t# can include the \"msg-id\" in the link text for better context.\n+\t\t'sub' => \\&hyperlink_committag,\n+\t},\n+);\n+\n # You define site-wide feature defaults here; override them with\n # $GITWEB_CONFIG as necessary.\n our %feature = (\n@@ -258,7 +349,7 @@ our %feature = (\n \t# and in project config, a comma-separated list of formats or \"none\"\n \t# to disable. Example: gitweb.snapshot = tbz2,zip;\n \t'snapshot' => {\n-\t\t'sub' => \\&feature_snapshot,\n+\t\t'sub' => sub { feature_list('snapshot', @_) },\n \t\t'override' => 0,\n \t\t'default' => ['tgz']},\n \n@@ -417,6 +508,21 @@ our %feature = (\n \t\t'sub' => \\&feature_avatar,\n \t\t'override' => 0,\n \t\t'default' => ['']},\n+\n+\t# The selection and ordering of committags that are enabled.\n+\t# Committag transformations will be applied to commit log messages\n+\t# in the order listed here if listed here.\n+\n+\t# To disable system wide have in $GITWEB_CONFIG\n+\t# $feature{'committags'}{'default'} = [];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'committags'}{'override'} = 1;\n+\t# and in project config gitweb.committags = sha1, url, bugzilla\n+\t# to enable those three committags for that project\n+\t'committags' => {\n+\t\t'sub' => sub { feature_list('committags', @_) },\n+\t\t'override' => 0,\n+\t\t'default' => ['sha1']},\n );\n \n sub gitweb_get_feature {\n@@ -463,16 +569,16 @@ sub feature_bool {\n \t}\n }\n \n-sub feature_snapshot {\n-\tmy (@fmts) = @_;\n+sub feature_list {\n+\tmy ($key, @defaults) = @_;\n \n-\tmy ($val) = git_get_project_config('snapshot');\n+\tmy ($cfg) = git_get_project_config($key);\n \n-\tif ($val) {\n-\t\t@fmts = ($val eq 'none' ? () : split /\\s*[,\\s]\\s*/, $val);\n+\tif ($cfg) {\n+\t\treturn ($cfg eq 'none' ? () : split(/\\s*[,\\s]\\s*/, $cfg));\n \t}\n \n-\treturn @fmts;\n+\treturn @defaults;\n }\n \n sub feature_patches {\n@@ -886,6 +992,35 @@ if ($git_avatar eq 'gravatar') {\n \t$git_avatar = '';\n }\n \n+# ordering of committags\n+our @committags = gitweb_get_feature('committags');\n+\n+# whether we've loaded committags for the project yet\n+our $loaded_project_committags = 0;\n+\n+# Load committag configs from the repository config file and and\n+# incorporate them into the gitweb defaults where permitted by the\n+# site administrator.\n+sub gitweb_load_project_committags {\n+\treturn if (!$git_dir || $loaded_project_committags);\n+\tmy %project_config = ();\n+\tmy %raw_config = git_parse_project_config('gitweb\\.committag');\n+\tforeach my $key (keys(%raw_config)) {\n+\t\tnext if ($key !~ /gitweb\\.committag\\.[^.]+\\.[^.]/);\n+\t\tmy ($gitweb_prefix, $committag_prefix, $ctname, $option) =\n+\t\t\tsplit(/\\./, $key, 4);\n+\t\t$project_config{$ctname}{$option} = $raw_config{$key};\n+\t}\n+\tforeach my $ctname (keys(%committags)) {\n+\t\tnext if (!$committags{$ctname}{'override'});\n+\t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n+\t\t\t$committags{$ctname}{'options'}{$optname} =\n+\t\t\t\t$project_config{$ctname}{$optname};\n+\t\t}\n+\t}\n+\t$loaded_project_committags = 1;\n+}\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -1458,13 +1593,99 @@ sub file_type_long {\n sub format_log_line_html {\n \tmy $line = shift;\n \n-\t$line = esc_html($line, -nbsp=>1);\n-\t$line =~ s{\\b([0-9a-fA-F]{8,40})\\b}{\n-\t\t$cgi->a({-href => href(action=>\"object\", hash=>$1),\n-\t\t\t\t\t-class => \"text\"}, $1);\n-\t}eg;\n+\t# Merge project configs with site default committag definitions if\n+\t# it hasn't been done yet\n+\tgitweb_load_project_committags();\n \n-\treturn $line;\n+\t# In this list of log message fragments, a string ref indicates\n+\t# HTML, and a string indicates plain text. The string refs are\n+\t# also currently not processed by subsequent committags.\n+\tmy @message_fragments = ( $line );\n+\n+COMMITTAG:\n+\tforeach my $ctname (@committags) {\n+\t\tnext COMMITTAG unless exists $committags{$ctname};\n+\t\tmy $committag = $committags{$ctname};\n+\n+\t\tnext COMMITTAG unless exists $committag->{'sub'};\n+\t\tmy $sub = $committag->{'sub'};\n+\n+\t\tnext COMMITTAG unless exists $committag->{'options'};\n+\t\tmy $opts = $committag->{'options'};\n+\n+\t\tnext COMMITTAG unless exists $opts->{'pattern'};\n+\t\tmy $pattern = $opts->{'pattern'};\n+\n+\t\tmy @new_message_fragments = ();\n+\n+\tPART:\n+\t\tforeach my $fragment (@message_fragments) {\n+\t\t\tnext PART if $fragment eq \"\";\n+\t\t\tif (ref($fragment)) {\n+\t\t\t\tpush @new_message_fragments, $fragment;\n+\t\t\t\tnext PART;\n+\t\t\t}\n+\n+\t\t\tmy $oldpos = 0;\n+\n+\t\tMATCH:\n+\t\t\twhile ($fragment =~ m/$pattern/gc) {\n+\t\t\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n+\t\t\t\tmy $repl = $sub->($opts, $&, $1);\n+\t\t\t\t$repl = \"\" if (!defined $repl);\n+\n+\t\t\t\tmy $pre = substr($fragment, $oldpos, $prepos - $oldpos);\n+\t\t\t\tpush_or_append(\\@new_message_fragments, $pre);\n+\t\t\t\tpush_or_append(\\@new_message_fragments, $repl);\n+\n+\t\t\t\t$oldpos = $postpos;\n+\t\t\t} # end while [regexp matches]\n+\n+\t\t\tmy $rest = substr($fragment, $oldpos);\n+\t\t\tpush_or_append(\\@new_message_fragments, $rest);\n+\n+\t\t} # end foreach (@message_fragments)\n+\n+\t\t@message_fragments = @new_message_fragments;\n+\t} # end foreach (@committags)\n+\n+\t# Escape any remaining plain text and concatenate\n+\tmy $html = '';\n+\tfor my $fragment (@message_fragments) {\n+\t\tif (ref($fragment)) {\n+\t\t\t$html .= $$fragment;\n+\t\t} else {\n+\t\t\t$html .= esc_html($fragment, -nbsp=>1);\n+\t\t}\n+\t}\n+\n+\treturn $html;\n+}\n+\n+# Returns a ref to an HTML snippet that links the whole match to a URL\n+# formed from the 'url' option and the first captured subgroup. This\n+# is a helper function used in %committags.\n+sub hyperlink_committag {\n+\tmy ($opts, @match) = @_;\n+\treturn \\$cgi->a({-href => $opts->{'url'} . $cgi->escape($match[1]),\n+\t -class => \"text\"},\n+\t esc_html($match[0], -nbsp=>1));\n+}\n+\n+\n+sub push_or_append (\\@@) {\n+\tmy $fragments = shift;\n+\n+\tif (ref $_[0] || ! @$fragments || ref $fragments->[-1]) {\n+\t\tpush @$fragments, @_;\n+\t} else {\n+\t\tmy $a = pop @$fragments;\n+\t\tmy $b = shift @_;\n+\n+\t\tpush @$fragments, $a . $b, @_;\n+\t}\n+\t# imitate push\n+\treturn scalar @$fragments;\n }\n \n # format marker of refs pointing to given object\ndiff --git a/t/t9502-gitweb-committags.sh b/t/t9502-gitweb-committags.sh\nnew file mode 100755\nindex 0000000..f86cb3d\n--- /dev/null\n+++ b/t/t9502-gitweb-committags.sh\n@@ -0,0 +1,150 @@\n+#!/bin/sh\n+\n+test_description='gitweb committag tests.\n+\n+This test runs gitweb (git web interface) as CGI script from\n+commandline, and checks that committags perform the expected\n+transformations on log messages.'\n+\n+. ./gitweb-lib.sh\n+\n+# ----------------------------------------------------------------------\n+# sha1 linking\n+#\n+echo sha1_test > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <567890ab\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 567890ab gitweb.output'\n+\n+# ----------------------------------------------------------------------\n+# bugzilla commit tag\n+#\n+\n+echo bugzilla_test > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <> gitweb_config.perl\n+test_expect_success 'bugzilla: enabled' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 gitweb.output'\n+\n+git config gitweb.committag.bugzilla.url 'http://bts.example.com?bug='\n+test_expect_success 'bugzilla: url overridden but not permitted' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 gitweb.output'\n+\n+echo '$committags{\"bugzilla\"}{\"override\"} = 1;' >> gitweb_config.perl\n+test_expect_success 'bugzilla: url overridden' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 gitweb.output'\n+\n+git config gitweb.committag.bugzilla.pattern 'Fixes bug (\\d+)'\n+test_expect_success 'bugzilla: pattern overridden' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 gitweb.output'\n+git config --unset gitweb.committag.bugzilla.pattern\n+\n+test_expect_success 'bugzilla: affects log view too' '\n+\tgitweb_run \"p=.git;a=log\" &&\n+\tgrep -F -q \\\n+\t\t\"bug 1234\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 gitweb.output'\n+\n+# ----------------------------------------------------------------------\n+# url linking\n+#\n+echo url_test > file.txt\n+git add file.txt\n+url='http://user@pass:example.com/foo.html?u=v&x=y#z'\n+url_esc=\"$(echo \"$url\" | sed 's/&/&/g')\"\n+git commit -q -F - file.txt <> gitweb_config.perl\n+git config gitweb.committags 'sha1, url'\n+test_expect_success 'url link: links when enabled' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -q -F \\\n+\t\t\"See also $url_esc.\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"$url\" gitweb.output'\n+\n+# ----------------------------------------------------------------------\n+# message id linking\n+#\n+echo msgid_test > file.txt\n+git add file.txt\n+url='http://mid.gmane.org/'\n+msgid=''\n+msgid_esc=\"$(echo \"$msgid\" | sed 's//\\>/g')\"\n+msgid_url=\"$url$(echo \"$msgid\" | sed 's//%3E/g')\"\n+git commit -q -F - file.txt <> gitweb_config.perl\n+git config gitweb.committags 'sha1, messageid'\n+test_expect_success 'msgid link: linked when enabled' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -q -F \\\n+\t\t\"See msg-id $msgid_esc.\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"y.z\" gitweb.output'\n+\n+\n+test_done\n-- \n1.6.4.4\n"},{"id":"127813","messageId":"1258525350-5528-3-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"1258525350-5528-2-git-send-email-marcel@oak.homeunix.org","subject":"[RFC PATCH 2/6] gitweb: Add second-stage matching of bug IDs in bugzilla committag","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-11-18T06:22:26Z","receivedAt":"2009-11-18T06:22:26Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"Currently it's not easy to capture an unbounded number of items\nin a committag phrase to hyperlink individually.\n\nFor example, I would like to match and hyperlinking each bug ID\nindividually in these situations:\n\n\t[#1234, #1235]\n\tResolves-bug: 1234, 1235\n\tbugs 1234, 1235, and 1236\n\nMatch Bugzilla bug IDs with two regexes instead of one. The first is\na pre-filter that allows easy matching of multiple bug IDs and\ncontextual queues like \"bugs ___ and ___\" in a phrase, and the second\neasily picks out the individual big IDs for hyperlinking.\n\nSigned-off-by: Marcel M. Cary \n---\n gitweb/gitweb.perl | 68 +++++++++++++++++++++++++++++------------\n t/t9502-gitweb-committags.sh | 69 ++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 112 insertions(+), 25 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 2d72202..032b1c5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -262,14 +262,31 @@ our %committags = (\n \t\t'override' => 0,\n \t\t'sub' => \\&hyperlink_committag,\n \t},\n-\t# Link mentions of bug IDs to bugzilla\n+\t# Link mentions of bugs to bugzilla, allowing for separate outer\n+\t# and inner regexes (see unit test for example)\n \t'bugzilla' => {\n \t\t'options' => {\n-\t\t\t'pattern' => qr/bug\\s+(\\d+)/,\n+\t\t\t'pattern' => qr/(?i:bugs?):?\\s+\n+\t\t\t [#]?\\d+(?:(?:,\\s*|,?\\sand\\s|,?\\sn?or\\s|\\s+)\n+\t\t\t [#]?\\d+\\b)*/x,\n+\t\t\t'innerpattern' => qr/#?(\\d+)/,\n \t\t\t'url' => 'http://bugzilla.example.com/show_bug.cgi?id=',\n \t\t},\n \t\t'override' => 0,\n-\t\t'sub' => \\&hyperlink_committag,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\tif ($opts->{'innerpattern'}) {\n+\t\t\t\tmy @message_fragments = ();\n+\t\t\t\tpush_or_append_replacements(\\@message_fragments,\n+\t\t\t\t $opts->{'innerpattern'},\n+\t\t\t\t $match[0], sub {\n+\t\t\t\t\t\treturn hyperlink_committag($opts, @_);\n+\t\t\t\t\t});\n+\t\t\t\treturn @message_fragments;\n+\t\t\t} else {\n+\t\t\t\treturn hyperlink_committag(@_);\n+\t\t\t}\n+\t\t},\n \t},\n \t# Link URLs\n \t'url' => {\n@@ -1626,23 +1643,10 @@ COMMITTAG:\n \t\t\t\tnext PART;\n \t\t\t}\n \n-\t\t\tmy $oldpos = 0;\n-\n-\t\tMATCH:\n-\t\t\twhile ($fragment =~ m/$pattern/gc) {\n-\t\t\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n-\t\t\t\tmy $repl = $sub->($opts, $&, $1);\n-\t\t\t\t$repl = \"\" if (!defined $repl);\n-\n-\t\t\t\tmy $pre = substr($fragment, $oldpos, $prepos - $oldpos);\n-\t\t\t\tpush_or_append(\\@new_message_fragments, $pre);\n-\t\t\t\tpush_or_append(\\@new_message_fragments, $repl);\n-\n-\t\t\t\t$oldpos = $postpos;\n-\t\t\t} # end while [regexp matches]\n-\n-\t\t\tmy $rest = substr($fragment, $oldpos);\n-\t\t\tpush_or_append(\\@new_message_fragments, $rest);\n+\t\t\tpush_or_append_replacements(\\@new_message_fragments,\n+\t\t\t $pattern, $fragment, sub {\n+\t\t\t\t\t$sub->($opts, @_);\n+\t\t\t\t});\n \n \t\t} # end foreach (@message_fragments)\n \n@@ -1672,6 +1676,30 @@ sub hyperlink_committag {\n \t esc_html($match[0], -nbsp=>1));\n }\n \n+# Find $pattern in string $fragment, and push_or_append the parts\n+# between matches and the result of calling $sub with matched text to\n+# $new_fragments.\n+sub push_or_append_replacements {\n+\tmy ($new_fragments, $pattern, $fragment, $sub) = @_;\n+\n+\tmy $oldpos = 0;\n+\n+MATCH:\n+\twhile ($fragment =~ m/$pattern/gc) {\n+\t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n+\n+\t\tmy @repl = $sub->($&, $1);\n+\n+\t\tmy $pre = substr($fragment, $oldpos, $prepos - $oldpos);\n+\t\tpush_or_append($new_fragments, $pre);\n+\t\tpush_or_append($new_fragments, @repl);\n+\n+\t\t$oldpos = $postpos;\n+\t} # end while [regexp matches]\n+\n+\tmy $rest = substr($fragment, $oldpos);\n+\tpush_or_append($new_fragments, $rest);\n+}\n \n sub push_or_append (\\@@) {\n \tmy $fragments = shift;\ndiff --git a/t/t9502-gitweb-committags.sh b/t/t9502-gitweb-committags.sh\nindex f86cb3d..718e763 100755\n--- a/t/t9502-gitweb-committags.sh\n+++ b/t/t9502-gitweb-committags.sh\n@@ -52,7 +52,7 @@ echo '$feature{\"committags\"}{\"override\"} = 1;' >> gitweb_config.perl\n test_expect_success 'bugzilla: enabled' '\n \tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n \tgrep -F -q \\\n-\t\t\"Fixes bug 1234 involving\" \\\n+\t\t\"Fixes bug 1234 involving\" \\\n \t\tgitweb.output\n '\n test_debug 'cat gitweb.log'\n@@ -62,7 +62,7 @@ git config gitweb.committag.bugzilla.url 'http://bts.example.com?bug='\n test_expect_success 'bugzilla: url overridden but not permitted' '\n \tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n \tgrep -F -q \\\n-\t\t\"Fixes bug 1234 involving\" \\\n+\t\t\"Fixes bug 1234 involving\" \\\n \t\tgitweb.output\n '\n test_debug 'cat gitweb.log'\n@@ -72,12 +72,13 @@ echo '$committags{\"bugzilla\"}{\"override\"} = 1;' >> gitweb_config.perl\n test_expect_success 'bugzilla: url overridden' '\n \tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n \tgrep -F -q \\\n-\t\t\"Fixes bug 1234 involving\" \\\n+\t\t\"Fixes bug 1234 involving\" \\\n \t\tgitweb.output\n '\n test_debug 'cat gitweb.log'\n test_debug 'grep 1234 gitweb.output'\n \n+git config gitweb.committag.bugzilla.innerpattern ''\n git config gitweb.committag.bugzilla.pattern 'Fixes bug (\\d+)'\n test_expect_success 'bugzilla: pattern overridden' '\n \tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n@@ -87,17 +88,75 @@ test_expect_success 'bugzilla: pattern overridden' '\n '\n test_debug 'cat gitweb.log'\n test_debug 'grep 1234 gitweb.output'\n-git config --unset gitweb.committag.bugzilla.pattern\n \n+git config --unset gitweb.committag.bugzilla.innerpattern\n+git config --unset gitweb.committag.bugzilla.pattern\n test_expect_success 'bugzilla: affects log view too' '\n \tgitweb_run \"p=.git;a=log\" &&\n \tgrep -F -q \\\n-\t\t\"bug 1234\" \\\n+\t\t\"1234\" \\\n \t\tgitweb.output\n '\n test_debug 'cat gitweb.log'\n test_debug 'grep 1234 gitweb.output'\n \n+echo more_bugzilla > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <#123,#45]\" \\\n+ gitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 123 gitweb.output'\n+\n+echo even_more_bugzilla > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <]*>123.\" gitweb.output\n+'\n+test_expect_success 'bugzilla: fancy defaults: comma-separated list' '\n+\tgrep -q \\\n+\t\t\"bugs ]*>223, ]*>224, and ]*>225,\" \\\n+\t\tgitweb.output\n+'\n+test_expect_success 'bugzilla: fancy defaults: or-pair' '\n+\tgrep -q \"bug ]*>323 or ]*>324.\" \\\n+\t\tgitweb.output\n+'\n+test_expect_success 'bugzilla: fancy defaults: comma-separated, caps, >10' '\n+\tgrep -q \\\n+\t\t\"Bug: ]*>423,]*>424,.*,]*>435\" \\\n+\t\tgitweb.output\n+'\n+test_expect_success 'bugzilla: fancy defaults: space-separated with hash' '\n+\tgrep -q -e \\\n+\t\t\"-bugs: ]*>#523 ]*>#524\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 23 gitweb.output'\n+\n # ----------------------------------------------------------------------\n # url linking\n #\n-- \n1.6.4.4\n"},{"id":"127812","messageId":"1258525350-5528-4-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"1258525350-5528-3-git-send-email-marcel@oak.homeunix.org","subject":"[RFC PATCH 3/6] gitweb: Allow finer-grained override controls for committags","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-11-18T06:22:27Z","receivedAt":"2009-11-18T06:22:27Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"Currently, a site administrator must choose between allowing all or\nnone of a committag's options to be overridden in the project config.\nHowever, a site admin may wish to permit specifying a bugzilla URL\nwithout risking a maliciously resource hungry regular expression.\n\nAllow the site admin to specify which committag parameters may be\noverridden. Preserve the behavior of the original 0 and 1 override\nspecifications.\n\nSigned-off-by: Marcel M. Cary \n---\n gitweb/INSTALL | 8 +++++++-\n gitweb/gitweb.perl | 24 ++++++++++++++++++------\n t/t9502-gitweb-committags.sh | 13 +++++++++++++\n 3 files changed, 38 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/INSTALL b/gitweb/INSTALL\nindex 9081ed8..15c0128 100644\n--- a/gitweb/INSTALL\n+++ b/gitweb/INSTALL\n@@ -133,9 +133,15 @@ adding the following lines to your $GITWEB_CONFIG:\n \t$known_snapshot_formats{'tgz'}{'compressor'} = ['gzip','-6'];\n \n To add a committag to the default list of commit tags, for example to\n-enable hyperlinking of bug numbers to a bug tracker for all projects:\n+enable hyperlinking of bug numbers to a bug tracker for all projects, while\n+allowing each project to choose only the base URL for its bug tracker:\n \n \tpush @{$feature{'committags'}{'default'}}, 'bugzilla';\n+\t$committags{\"bugzilla\"}{\"override\"} = [\"url\"];\n+\n+And then let each project configure its bug tracker URL:\n+\n+\tgit config gitweb.committag.bugzilla.url 'http://bts.example.com?bug='\n \n \n Gitweb repositories\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 032b1c5..8f4480e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -225,11 +225,13 @@ our %avatar_size = (\n # will not be processed further.\n #\n # For any committag, set the 'override' key to 1 to allow individual\n-# projects to override entries in the 'options' hash for that tag.\n-# For example, to match only commit hashes given in lowercase in one\n-# project, add this to the $GITWEB_CONFIG:\n+# projects to override any entry in the 'options' hash for that tag.\n+# Leave 'override' as 0 to disallow all overriding of all entries.\n+# Set 'override' to an array of 'option' key names to allow overriding\n+# specific keys. For example, to match only commit hashes given in\n+# lowercase in one project, add this to the $GITWEB_CONFIG:\n #\n-# $committags{'sha1'}{'override'} = 1;\n+# $committags{'sha1'}{'override'} = 1; # or [\"pattern\"]\n #\n # And in the project's config:\n #\n@@ -237,7 +239,8 @@ our %avatar_size = (\n #\n # Some committags have additional options whose interpretation depends\n # on the implementation of the 'sub' key. The hyperlink_committag\n-# value appends the first captured group to the 'url' option.\n+# value appends the first captured group to the 'url' option, for example.\n+#\n our %committags = (\n \t# Link Git-style hashes to this gitweb\n \t'sha1' => {\n@@ -1029,8 +1032,17 @@ sub gitweb_load_project_committags {\n \t\t$project_config{$ctname}{$option} = $raw_config{$key};\n \t}\n \tforeach my $ctname (keys(%committags)) {\n-\t\tnext if (!$committags{$ctname}{'override'});\n+\t\tmy $override = $committags{$ctname}{'override'};\n+\t\tnext if (!$override);\n+\t\tmy $override_keys = undef;\n+\t\tif (ref($override) eq \"ARRAY\") {\n+\t\t\t$override_keys = {};\n+\t\t\tforeach my $optname (@$override) {\n+\t\t\t\t$override_keys->{$optname} = 1;\n+\t\t\t}\n+\t\t}\n \t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n+\t\t\tnext if ($override_keys && !$override_keys->{$optname});\n \t\t\t$committags{$ctname}{'options'}{$optname} =\n \t\t\t\t$project_config{$ctname}{$optname};\n \t\t}\ndiff --git a/t/t9502-gitweb-committags.sh b/t/t9502-gitweb-committags.sh\nindex 718e763..e13ac47 100755\n--- a/t/t9502-gitweb-committags.sh\n+++ b/t/t9502-gitweb-committags.sh\n@@ -68,6 +68,19 @@ test_expect_success 'bugzilla: url overridden but not permitted' '\n test_debug 'cat gitweb.log'\n test_debug 'grep 1234 gitweb.output'\n \n+echo '$committags{\"bugzilla\"}{\"override\"} = [\"url\"];' >> gitweb_config.perl\n+git config gitweb.committag.bugzilla.url 'http://bts.example.com?bug='\n+git config gitweb.committag.bugzilla.pattern 'slow DoS regex'\n+test_expect_success 'bugzilla: url overridden but regex not permitted' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -F -q \\\n+\t\t\"Fixes bug 1234 involving\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep 1234 gitweb.output'\n+git config --unset gitweb.committag.bugzilla.pattern\n+\n echo '$committags{\"bugzilla\"}{\"override\"} = 1;' >> gitweb_config.perl\n test_expect_success 'bugzilla: url overridden' '\n \tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n-- \n1.6.4.4\n"},{"id":"127816","messageId":"1258525350-5528-5-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"1258525350-5528-4-git-send-email-marcel@oak.homeunix.org","subject":"[RFC PATCH 4/6] gitweb: Allow committag pattern matches to span multiple lines","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-11-18T06:22:28Z","receivedAt":"2009-11-18T06:22:28Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"Committags cannot currently span multiple lines. Since some committag\npatterns match multiple words. If some of those words wrap to the\nnext line, the committag would miss an opportunity to match.\n\nEliminate the for-loop over @log and pull the signoff transformation\nfrom that loop into a committag.\n\nThe message will still get cut into pieces as committags are applied,\nbut at least newlines no longer force a cut.\n\nSigned-off-by: Marcel M. Cary \n---\n gitweb/gitweb.perl | 67 +++++++++++++++++++-----------------------\n t/t9502-gitweb-committags.sh | 8 +++++\n 2 files changed, 38 insertions(+), 37 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8f4480e..7f7d3a3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -255,6 +255,21 @@ our %committags = (\n \t\t\t esc_html($match[0], -nbsp=>1));\n \t\t},\n \t},\n+\t# Facilitate styling of common header/footer lines, suppressing\n+\t# any trailing newlines\n+\t'signoff' => {\n+\t\t'options' => {\n+\t\t\t'pattern' =>\n+\t\t\t\tqr/^( *(?:signed[ \\-]off[ \\-]by|acked[ \\-]by|cc)[ :].*)\\n*$/mi,\n+\t\t},\n+\t\t'override' => 0,\n+\t\t'sub' => sub {\n+\t\t\tmy ($opts, @match) = @_;\n+\t\t\treturn (\\$cgi->span({'class' => 'signoff'},\n+\t\t\t esc_html($match[1], -nbsp=>1)),\n+\t\t\t \"\\n\");\n+\t\t},\n+\t},\n \t# Link bug/features to Mantis bug tracker using Mantis-style\n \t# contextual cues\n \t'mantis' => {\n@@ -542,7 +557,7 @@ our %feature = (\n \t'committags' => {\n \t\t'sub' => sub { feature_list('committags', @_) },\n \t\t'override' => 0,\n-\t\t'default' => ['sha1']},\n+\t\t'default' => ['signoff', 'sha1']},\n );\n \n sub gitweb_get_feature {\n@@ -1619,8 +1634,8 @@ sub file_type_long {\n ## which don't belong to other sections\n \n # format line of commit message.\n-sub format_log_line_html {\n-\tmy $line = shift;\n+sub format_log_html {\n+\tmy $text = shift;\n \n \t# Merge project configs with site default committag definitions if\n \t# it hasn't been done yet\n@@ -1629,7 +1644,7 @@ sub format_log_line_html {\n \t# In this list of log message fragments, a string ref indicates\n \t# HTML, and a string indicates plain text. The string refs are\n \t# also currently not processed by subsequent committags.\n-\tmy @message_fragments = ( $line );\n+\tmy @message_fragments = ( $text );\n \n COMMITTAG:\n \tforeach my $ctname (@committags) {\n@@ -1671,7 +1686,9 @@ COMMITTAG:\n \t\tif (ref($fragment)) {\n \t\t\t$html .= $$fragment;\n \t\t} else {\n-\t\t\t$html .= esc_html($fragment, -nbsp=>1);\n+\t\t\t# Don't let esc_html turn \"\\n\" into \"\\\\n\"\n+\t\t\t$html .= join(\"
\\n\", map { esc_html($_, -nbsp=>1) }\n+\t\t\t split(\"\\n\", $fragment, -1));\n \t\t}\n \t}\n \n@@ -3776,40 +3793,16 @@ sub git_print_log {\n \t\tshift @$log;\n \t}\n \n-\t# print log\n-\tmy $signoff = 0;\n-\tmy $empty = 0;\n-\tforeach my $line (@$log) {\n-\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|acked[ \\-]by[ :]|cc[ :])/i) {\n-\t\t\t$signoff = 1;\n-\t\t\t$empty = 0;\n-\t\t\tif (! $opts{'-remove_signoff'}) {\n-\t\t\t\tprint \"\" . esc_html($line) . \"
\\n\";\n-\t\t\t\tnext;\n-\t\t\t} else {\n-\t\t\t\t# remove signoff lines\n-\t\t\t\tnext;\n-\t\t\t}\n-\t\t} else {\n-\t\t\t$signoff = 0;\n-\t\t}\n-\n-\t\t# print only one empty line\n-\t\t# do not print empty line after signoff\n-\t\tif ($line eq \"\") {\n-\t\t\tnext if ($empty || $signoff);\n-\t\t\t$empty = 1;\n-\t\t} else {\n-\t\t\t$empty = 0;\n-\t\t}\n-\n-\t\tprint format_log_line_html($line) . \"
\\n\";\n-\t}\n-\n \tif ($opts{'-final_empty_line'}) {\n-\t\t# end with single empty line\n-\t\tprint \"
\\n\" unless $empty;\n+\t\t# If we already have a trailing newline, this will be\n+\t\t# coalesced with it later.\n+\t\tpush @$log, \"\";\n \t}\n+\n+\t# print log\n+\tmy $text = join(\"\\n\", @$log) . \"\\n\";\n+\t$text =~ s{\\n\\n+}{\\n\\n}g;\n+\tprint format_log_html($text);\n }\n \n # return link target (what link points to)\ndiff --git a/t/t9502-gitweb-committags.sh b/t/t9502-gitweb-committags.sh\nindex e13ac47..0753630 100755\n--- a/t/t9502-gitweb-committags.sh\n+++ b/t/t9502-gitweb-committags.sh\n@@ -138,6 +138,10 @@ Fix memory leak in confabulator from bug 123.\n Based on history from bugs 223, 224, and 225,\n fix bug 323 or 324.\n \n+Bugs:\n+1234,\n+1235\n+\n Bug: 423,424,425,426,427,428,429,430,431,432,435\n Resolves-bugs: #523 #524\n END\n@@ -167,6 +171,10 @@ test_expect_success 'bugzilla: fancy defaults: space-separated with hash' '\n \t\t\"-bugs: ]*>#523 ]*>#524\" \\\n \t\tgitweb.output\n '\n+test_expect_success 'bugzilla: fancy defaults: spanning newlines' '\n+\tgrep -q -e \"]*>1234,]*>1235\n---\n gitweb/gitweb.perl | 135 ++++++++++++++++++++++++++++--------------\n t/t9502-gitweb-committags.sh | 50 +++++++++++++++\n 2 files changed, 141 insertions(+), 44 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7f7d3a3..d413f22 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -214,8 +214,7 @@ our %avatar_size = (\n );\n \n # In general, the site admin can enable/disable per-project\n-# configuration of each committag. Only the 'options' part of the\n-# committag is configurable per-project.\n+# configuration of each committag.\n #\n # The site admin can of course add new tags to this hash or override\n # the 'sub' key if necessary. But such changes may be fragile; this\n@@ -241,12 +240,18 @@ our %avatar_size = (\n # on the implementation of the 'sub' key. The hyperlink_committag\n # value appends the first captured group to the 'url' option, for example.\n #\n+# The project configuration can define new committags. Although the\n+# project configuration cannot supply code defining a new 'sub' key,\n+# the project configuration can choose from a list of pre-approved\n+# subroutines named in the 'allowed_committag_subs' feature. Both a\n+# 'sub' key and 'pattern' key must be defined for the committag to be\n+# used. If the 'allowed_committag_subs' feature is empty, no new\n+# committags can be defined in the project config.\n+#\n our %committags = (\n \t# Link Git-style hashes to this gitweb\n \t'sha1' => {\n-\t\t'options' => {\n-\t\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n-\t\t},\n+\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n \t\t'override' => 0,\n \t\t'sub' => sub {\n \t\t\tmy ($opts, @match) = @_;\n@@ -258,10 +263,8 @@ our %committags = (\n \t# Facilitate styling of common header/footer lines, suppressing\n \t# any trailing newlines\n \t'signoff' => {\n-\t\t'options' => {\n-\t\t\t'pattern' =>\n-\t\t\t\tqr/^( *(?:signed[ \\-]off[ \\-]by|acked[ \\-]by|cc)[ :].*)\\n*$/mi,\n-\t\t},\n+\t\t'pattern' =>\n+\t\t\tqr/^( *(?:signed[ \\-]off[ \\-]by|acked[ \\-]by|cc)[ :].*)\\n*$/mi,\n \t\t'override' => 0,\n \t\t'sub' => sub {\n \t\t\tmy ($opts, @match) = @_;\n@@ -273,23 +276,19 @@ our %committags = (\n \t# Link bug/features to Mantis bug tracker using Mantis-style\n \t# contextual cues\n \t'mantis' => {\n-\t\t'options' => {\n-\t\t\t'pattern' => qr/(?:BUG|FEATURE)\\((\\d+)\\)/,\n-\t\t\t'url' => 'http://www.example.com/mantisbt/view.php?id=',\n-\t\t},\n+\t\t'pattern' => qr/(?:BUG|FEATURE)\\((\\d+)\\)/,\n+\t\t'url' => 'http://www.example.com/mantisbt/view.php?id=',\n \t\t'override' => 0,\n \t\t'sub' => \\&hyperlink_committag,\n \t},\n \t# Link mentions of bugs to bugzilla, allowing for separate outer\n \t# and inner regexes (see unit test for example)\n \t'bugzilla' => {\n-\t\t'options' => {\n-\t\t\t'pattern' => qr/(?i:bugs?):?\\s+\n-\t\t\t [#]?\\d+(?:(?:,\\s*|,?\\sand\\s|,?\\sn?or\\s|\\s+)\n-\t\t\t [#]?\\d+\\b)*/x,\n-\t\t\t'innerpattern' => qr/#?(\\d+)/,\n-\t\t\t'url' => 'http://bugzilla.example.com/show_bug.cgi?id=',\n-\t\t},\n+\t\t'pattern' => qr/(?i:bugs?):?\\s+\n+\t\t [#]?\\d+(?:(?:,\\s*|,?\\sand\\s|,?\\sn?or\\s|\\s+)\n+\t\t [#]?\\d+\\b)*/x,\n+\t\t'innerpattern' => qr/#?(\\d+)/,\n+\t\t'url' => 'http://bugzilla.example.com/show_bug.cgi?id=',\n \t\t'override' => 0,\n \t\t'sub' => sub {\n \t\t\tmy ($opts, @match) = @_;\n@@ -308,15 +307,13 @@ our %committags = (\n \t},\n \t# Link URLs\n \t'url' => {\n-\t\t'options' => {\n-\t\t\t# Avoid matching punctuation that might immediately follow\n-\t\t\t# a url, is not part of the url, and is allowed in urls,\n-\t\t\t# like a full-stop ('.').\n-\t\t\t'pattern' => qr!(https?|ftps?|git|ssh|ssh+git|sftp|smb|webdavs?|\n-\t\t\t nfs|irc|nntp|rsync)\n-\t\t\t ://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n-\t\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n-\t\t},\n+\t\t# Avoid matching punctuation that might immediately follow\n+\t\t# a url, is not part of the url, and is allowed in urls,\n+\t\t# like a full-stop ('.').\n+\t\t'pattern' => qr!(https?|ftps?|git|ssh|ssh+git|sftp|smb|webdavs?|\n+\t\t nfs|irc|nntp|rsync)\n+\t\t ://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n+\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n \t\t'override' => 0,\n \t\t'sub' => sub {\n \t\t\tmy ($opts, @match) = @_;\n@@ -327,10 +324,8 @@ our %committags = (\n \t},\n \t# Link Message-Id to mailing list archive\n \t'messageid' => {\n-\t\t'options' => {\n-\t\t\t'pattern' => qr!(?:message|msg)-?id:?\\s+(<[^>]+>)!i,\n-\t\t\t'url' => 'http://mid.gmane.org/',\n-\t\t},\n+\t\t'pattern' => qr!(?:message|msg)-?id:?\\s+(<[^>]+>)!i,\n+\t\t'url' => 'http://mid.gmane.org/',\n \t\t'override' => 0,\n \t\t# Includes the \"msg-id\" text in the link text.\n \t\t# Since we don't support linking multiple msg-ids in one match, we\n@@ -558,6 +553,22 @@ our %feature = (\n \t\t'sub' => sub { feature_list('committags', @_) },\n \t\t'override' => 0,\n \t\t'default' => ['signoff', 'sha1']},\n+\n+\t# The list of committag callbacks that are permitted to be used\n+\t# from within a repository configuration. These are interpretted\n+\t# as perl subrouting names. If none are listed, no new committags\n+\t# can be defined in the project config, which is the default.\n+\n+\t# To enable system wide have in $GITWEB_CONFIG\n+\t# $feature{'allowed_committag_subs'}{'default'} = [\n+\t#\t\t'hyperlink_committag',\n+\t#\t\t'markup_committag',\n+\t#\t\t'transform_committag',\n+\t#\t\t];\n+\t# It would not make sense to allow per-project overrides of this.\n+\t'allowed_committag_subs' => {\n+\t\t'override' => 0,\n+\t\t'default' => []},\n );\n \n sub gitweb_get_feature {\n@@ -1030,6 +1041,9 @@ if ($git_avatar eq 'gravatar') {\n # ordering of committags\n our @committags = gitweb_get_feature('committags');\n \n+# ordering of committags\n+our @allowed_committag_subs = gitweb_get_feature('allowed_committag_subs');\n+\n # whether we've loaded committags for the project yet\n our $loaded_project_committags = 0;\n \n@@ -1046,9 +1060,18 @@ sub gitweb_load_project_committags {\n \t\t\tsplit(/\\./, $key, 4);\n \t\t$project_config{$ctname}{$option} = $raw_config{$key};\n \t}\n-\tforeach my $ctname (keys(%committags)) {\n-\t\tmy $override = $committags{$ctname}{'override'};\n+\n+\tmy %allowed_subs = ();\n+\tforeach my $sub (@allowed_committag_subs) {\n+\t\t$allowed_subs{$sub} = 1;\n+\t}\n+\n+\tforeach my $ctname (keys(%project_config)) {\n+\t\tmy $override = $committags{$ctname}\n+\t\t\t? $committags{$ctname}{'override'}\n+\t\t\t: 1;\n \t\tnext if (!$override);\n+\n \t\tmy $override_keys = undef;\n \t\tif (ref($override) eq \"ARRAY\") {\n \t\t\t$override_keys = {};\n@@ -1056,12 +1079,19 @@ sub gitweb_load_project_committags {\n \t\t\t\t$override_keys->{$optname} = 1;\n \t\t\t}\n \t\t}\n+\n \t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n \t\t\tnext if ($override_keys && !$override_keys->{$optname});\n-\t\t\t$committags{$ctname}{'options'}{$optname} =\n-\t\t\t\t$project_config{$ctname}{$optname};\n+\t\t\tmy $value = $project_config{$ctname}{$optname};\n+\t\t\tif ($optname eq 'sub') {\n+\t\t\t\tif (!$allowed_subs{$value}) {\n+\t\t\t\t\tnext;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\t$committags{$ctname}{$optname} = $value;\n \t\t}\n \t}\n+\n \t$loaded_project_committags = 1;\n }\n \n@@ -1654,11 +1684,8 @@ COMMITTAG:\n \t\tnext COMMITTAG unless exists $committag->{'sub'};\n \t\tmy $sub = $committag->{'sub'};\n \n-\t\tnext COMMITTAG unless exists $committag->{'options'};\n-\t\tmy $opts = $committag->{'options'};\n-\n-\t\tnext COMMITTAG unless exists $opts->{'pattern'};\n-\t\tmy $pattern = $opts->{'pattern'};\n+\t\tnext COMMITTAG unless exists $committag->{'pattern'};\n+\t\tmy $pattern = $committag->{'pattern'};\n \n \t\tmy @new_message_fragments = ();\n \n@@ -1672,7 +1699,8 @@ COMMITTAG:\n \n \t\t\tpush_or_append_replacements(\\@new_message_fragments,\n \t\t\t $pattern, $fragment, sub {\n-\t\t\t\t\t$sub->($opts, @_);\n+\t\t\t\t\tno strict \"refs\"; # for custome committags\n+\t\t\t\t\t$sub->($committag, @_);\n \t\t\t\t});\n \n \t\t} # end foreach (@message_fragments)\n@@ -1705,6 +1733,25 @@ sub hyperlink_committag {\n \t esc_html($match[0], -nbsp=>1));\n }\n \n+# Returns a ref to an HTML snippet formed from the 'replacement'\n+# option and match data. The match data is HTML-escaped, and the\n+# 'replacement' option is used as a sprintf format. This is a helper\n+# function used in %committags.\n+sub markup_committag {\n+\tmy ($opts, @match) = @_;\n+\treturn \\sprintf($opts->{'replacement'},\n+\t map { esc_html($_, -nbsp=>1) if defined } @match);\n+}\n+\n+# Returns a text snippet formed from the 'replacement' option and\n+# match data. The 'replacement' option is used as a sprintf format.\n+# Because the result is text, it can be re-processed by subsequent\n+# committags. This is a helper function used in %committags.\n+sub transform_committag {\n+\tmy ($opts, @match) = @_;\n+\treturn sprintf($opts->{'replacement'}, @match);\n+}\n+\n # Find $pattern in string $fragment, and push_or_append the parts\n # between matches and the result of calling $sub with matched text to\n # $new_fragments.\n@@ -1717,7 +1764,7 @@ MATCH:\n \twhile ($fragment =~ m/$pattern/gc) {\n \t\tmy ($prepos, $postpos) = ($-[0], $+[0]);\n \n-\t\tmy @repl = $sub->($&, $1);\n+\t\tmy @repl = $sub->($&, $1, $2);\n \n \t\tmy $pre = substr($fragment, $oldpos, $prepos - $oldpos);\n \t\tpush_or_append($new_fragments, $pre);\ndiff --git a/t/t9502-gitweb-committags.sh b/t/t9502-gitweb-committags.sh\nindex 0753630..cbe607b 100755\n--- a/t/t9502-gitweb-committags.sh\n+++ b/t/t9502-gitweb-committags.sh\n@@ -226,5 +226,55 @@ test_expect_success 'msgid link: linked when enabled' '\n test_debug 'cat gitweb.log'\n test_debug 'grep -F \"y.z\" gitweb.output'\n \n+# ----------------------------------------------------------------------\n+# custom committags\n+#\n+echo custom_test > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <\n+END\n+echo '$feature{\"allowed_committag_subs\"}{\"default\"} = [\n+\t\"hyperlink_committag\",\n+\t\"markup_committag\",\n+\t\"transform_committag\",\n+\t];' >> gitweb_config.perl\n+git config gitweb.committags 'sha1, obfuscate'\n+git config gitweb.committag.obfuscate.pattern '([a-z&]+@)[a-z]+(.com)'\n+git config gitweb.committag.obfuscate.sub 'transform_committag'\n+git config gitweb.committag.obfuscate.replacement '%2$sXXX%3$s'\n+test_expect_success 'custom committags: transform_committag' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -q -F \\\n+\t\t\"foo&bar@XXX.com\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"foo\" gitweb.output'\n+\n+git config gitweb.committags 'sha1, linkemail'\n+git config gitweb.committag.linkemail.pattern '<([a-z&]+@[a-z]+.com)>'\n+git config gitweb.committag.linkemail.sub 'markup_committag'\n+git config gitweb.committag.linkemail.replacement '%1$s'\n+test_expect_success 'custom committags: markup_committag' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -q -F \\\n+\t\t\"<foo&bar@bar.com>\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"foo\" gitweb.output'\n+\n+echo '$feature{\"allowed_committag_subs\"}{\"default\"} = [\n+\t];' >> gitweb_config.perl\n+test_expect_success 'custom committags: ignored when disabled' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -q -F \\\n+\t\t\"Something for <foo&bar@bar.com>\" \\\n+\t\tgitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'grep -F \"foo\" gitweb.output'\n+\n \n test_done\n-- \n1.6.4.4\n"},{"id":"127815","messageId":"1258525350-5528-7-git-send-email-marcel@oak.homeunix.org","threadId":"19866","inReplyTo":"1258525350-5528-6-git-send-email-marcel@oak.homeunix.org","subject":"[RFC PATCH 6/6] gitweb: Add _defaults_ keyword for feature lists in project config","fromName":"Marcel M. Cary","fromEmail":"marcel@oak.homeunix.org","sentAt":"2009-11-18T06:22:30Z","receivedAt":"2009-11-18T06:22:30Z","isPatch":true,"sender":{"key":"marcel@oak.homeunix.org","avatar":"https://gravatar.com/avatar/2bb524e4f383167b7e256bb93256c88353748d9873c34cde0fd461f1165baa0f?d=mp&s=160"},"body":"If the site admin configures the list of committags, there's no way\nfor a project to get the defaults back short of enumerating them\nexplicitly. Worse yet, when the distribution upgrades the default\nlist, perhaps to push more pre-existing functionality into committags,\nthe project would have to discover this and upgrade its configuration\nto match the new defaults.\n\nAdd a special _defaults_ list entry which, in the project config,\nexpands to the build-time default list configured for that variable.\nA project may use this to append or prepend to the default\nconfiguration, even as the default configuration changes with new\nreleases.\n---\n gitweb/INSTALL | 5 +++++\n gitweb/gitweb.perl | 18 +++++++++++++-----\n t/t9502-gitweb-committags.sh | 29 +++++++++++++++++++++++++++++\n 3 files changed, 47 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/INSTALL b/gitweb/INSTALL\nindex 15c0128..83e6a5e 100644\n--- a/gitweb/INSTALL\n+++ b/gitweb/INSTALL\n@@ -143,6 +143,11 @@ And then let each project configure its bug tracker URL:\n \n \tgit config gitweb.committag.bugzilla.url 'http://bts.example.com?bug='\n \n+In a project config, the build-time list of committags can be accessed\n+with the special _defaults_ entry.\n+\n+\tgit config gitweb.committags '_defaults_, bugzilla'\n+\n \n Gitweb repositories\n -------------------\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d413f22..707e76e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -334,6 +334,13 @@ our %committags = (\n \t},\n );\n \n+sub make_list_feature {\n+\tmy ($name, $hash) = @_;\n+\t$hash->{'build_default'} = [@{$hash->{'default'}}];\n+\t$hash->{'sub'} = sub { feature_list($name, @_) };\n+\treturn @_;\n+}\n+\n # You define site-wide feature defaults here; override them with\n # $GITWEB_CONFIG as necessary.\n our %feature = (\n@@ -378,8 +385,7 @@ our %feature = (\n \t# $feature{'snapshot'}{'override'} = 1;\n \t# and in project config, a comma-separated list of formats or \"none\"\n \t# to disable. Example: gitweb.snapshot = tbz2,zip;\n-\t'snapshot' => {\n-\t\t'sub' => sub { feature_list('snapshot', @_) },\n+\tmake_list_feature 'snapshot' => {\n \t\t'override' => 0,\n \t\t'default' => ['tgz']},\n \n@@ -549,8 +555,7 @@ our %feature = (\n \t# $feature{'committags'}{'override'} = 1;\n \t# and in project config gitweb.committags = sha1, url, bugzilla\n \t# to enable those three committags for that project\n-\t'committags' => {\n-\t\t'sub' => sub { feature_list('committags', @_) },\n+\tmake_list_feature 'committags' => {\n \t\t'override' => 0,\n \t\t'default' => ['signoff', 'sha1']},\n \n@@ -621,7 +626,10 @@ sub feature_list {\n \tmy ($cfg) = git_get_project_config($key);\n \n \tif ($cfg) {\n-\t\treturn ($cfg eq 'none' ? () : split(/\\s*[,\\s]\\s*/, $cfg));\n+\t\treturn () if $cfg eq 'none';\n+\t\treturn map {\n+\t\t\t\t$_ eq '_defaults_' ? @{$feature{$key}{'build_default'}} : $_\n+\t\t\t} split(/\\s*[,\\s]\\s*/, $cfg);\n \t}\n \n \treturn @defaults;\ndiff --git a/t/t9502-gitweb-committags.sh b/t/t9502-gitweb-committags.sh\nindex cbe607b..7d16329 100755\n--- a/t/t9502-gitweb-committags.sh\n+++ b/t/t9502-gitweb-committags.sh\n@@ -276,5 +276,34 @@ test_expect_success 'custom committags: ignored when disabled' '\n test_debug 'cat gitweb.log'\n test_debug 'grep -F \"foo\" gitweb.output'\n \n+# ----------------------------------------------------------------------\n+# default keyword\n+#\n+echo default_test > file.txt\n+git add file.txt\n+git commit -q -F - file.txt <\n+\n+Signed-off-by: A U Thor \n+END\n+echo '\n+$feature{\"committags\"}{\"default\"} = [\"sha1\", \"messageid\"];\n+$feature{\"committags\"}{\"override\"} = 1;\n+' >> gitweb_config.perl\n+git config gitweb.committags '_defaults_, bugzilla'\n+# All these committags should be in effect except messageid\n+test_expect_success '_defaults_ keyword: restores build-time default' '\n+\tgitweb_run \"p=.git;a=commit;h=HEAD\" &&\n+\tgrep -q \"Bug ]*>1234\" gitweb.output &&\n+\tgrep -q \"]*>567890ab\" gitweb.output &&\n+\tgrep -q \"See msg-id <x@y.z>\" gitweb.output &&\n+\tgrep -q \"]*>Signed-off-by:\" gitweb.output\n+'\n+test_debug 'cat gitweb.log'\n+test_debug 'for i in Bug 5678 msg-id Signed-off; do grep $i gitweb.output; done'\n \n test_done\n-- \n1.6.4.4\n"},{"id":"127825","messageId":"20091118082024.GD12890@machine.or.cz","threadId":"19866","inReplyTo":"1258525350-5528-2-git-send-email-marcel@oak.homeunix.org","subject":"Re: [RFC PATCH 1/6] gitweb: Hyperlink committags in a commit message by regex matching","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2009-11-18T08:20:24Z","receivedAt":"2009-11-18T08:20:24Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Tue, Nov 17, 2009 at 10:22:25PM -0800, Marcel M. Cary wrote:\n> One additional thing that occured to me is that maybe the hyperlinks\n> added by committags should have 'rel=\"nofollow\"' by default? And if\n> so, maybe that needs to be configurable? On the other hand, I'm not\n> sure how useful it is to hide real URLs in the commit messages from\n> search engines... ?\n\nI don't think rel=\"nofollow\" is necessary.\n\nBTW, wouldn't it be useful to do the matching in blob bodies as well?\nAnd is it sensible to call these \"committags\" at all then? I already\nmade the mistake of calling content tags \"ctags\" and I regret it; I\nthink calling yet another thing tags after git tags and ctags is almost\nunbearable.\n\n> diff --git a/gitweb/INSTALL b/gitweb/INSTALL\n> index b76a0cf..9081ed8 100644\n> --- a/gitweb/INSTALL\n> +++ b/gitweb/INSTALL\n> @@ -132,6 +132,11 @@ adding the following lines to your $GITWEB_CONFIG:\n> \t$known_snapshot_formats{'zip'}{'disabled'} = 1;\n> \t$known_snapshot_formats{'tgz'}{'compressor'} = ['gzip','-6'];\n> \n> +To add a committag to the default list of commit tags, for example to\n> +enable hyperlinking of bug numbers to a bug tracker for all projects:\n> +\n> +\tpush @{$feature{'committags'}{'default'}}, 'bugzilla';\n> +\n> \n> Gitweb repositories\n> -------------------\n\nI think this is not useful at all, since:\n\n\t(i) The user will also *always* need to override the URL.\n\t(ii) More importantly, the user has no idea what on earth commit\n\ttags are.\n\nCould you please prepend this paragraph with a short committags\ndescription, e.g.:\n\n\t\"Gitweb can rewrite certain snippets of text in commit messages\n\tto hyperlinks, e.g. URL addresses or bug tracker references - we\n\tcall these snippets 'committags'.\"\n\nAnd you should also add\n\n\t$committags{'buzilla'}{'options'}{'url'} = ...\n\nto the explanation, together with a reference to the appropriate part of\ngitweb.perl for more details.\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e4cbfc3..2d72202 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -213,6 +213,97 @@ our %avatar_size = (\n> \t'double' => 32\n> );\n> \n> +# In general, the site admin can enable/disable per-project\n> +# configuration of each committag. Only the 'options' part of the\n> +# committag is configurable per-project.\n\n The exact same problem here - it is not explained what committag\nactually is and where it applies.\n\n> +# The site admin can of course add new tags to this hash or override\n> +# the 'sub' key if necessary. But such changes may be fragile; this\n> +# is not designed as a full-blown plugin architecture. The 'sub' must\n> +# return a list of strings or string refs. The strings must contain\n> +# plain text and the string refs must contain HTML. The string refs\n> +# will not be processed further.\n> +#\n> +# For any committag, set the 'override' key to 1 to allow individual\n> +# projects to override entries in the 'options' hash for that tag.\n> +# For example, to match only commit hashes given in lowercase in one\n> +# project, add this to the $GITWEB_CONFIG:\n> +#\n> +# $committags{'sha1'}{'override'} = 1;\n> +#\n> +# And in the project's config:\n> +#\n> +# gitweb.committags.sha1.pattern = \\\\b([0-9a-f]{8,40})\\\\b\n> +#\n> +# Some committags have additional options whose interpretation depends\n> +# on the implementation of the 'sub' key. The hyperlink_committag\n> +# value appends the first captured group to the 'url' option.\n> +our %committags = (\n> +\t# Link Git-style hashes to this gitweb\n> +\t'sha1' => {\n> +\t\t'options' => {\n> +\t\t\t'pattern' => qr/\\b([0-9a-fA-F]{8,40})\\b/,\n> +\t\t},\n> +\t\t'override' => 0,\n> +\t\t'sub' => sub {\n> +\t\t\tmy ($opts, @match) = @_;\n> +\t\t\treturn \\$cgi->a({-href => href(action=>\"object\", hash=>$match[1]),\n> +\t\t\t -class => \"text\"},\n> +\t\t\t esc_html($match[0], -nbsp=>1));\n> +\t\t},\n> +\t},\n\nIdeally, a link should be made only in case the object exists, but this\nis not trivial to implement without overhead of 1 exec per object, so I\nguess it's fine to leave this for later (after all this feature was\nalready present). In that case, I think it would be useful to start\nmatching ids from 5 characters up - I use these quite frequently ;) -\nbut until then it would probably make for too much false positives.\n\n> @@ -417,6 +508,21 @@ our %feature = (\n> \t\t'sub' => \\&feature_avatar,\n> \t\t'override' => 0,\n> \t\t'default' => ['']},\n> +\n> +\t# The selection and ordering of committags that are enabled.\n> +\t# Committag transformations will be applied to commit log messages\n> +\t# in the order listed here if listed here.\n\nYou should add something like \"See %committags definition above for\nexplanation of committags and pre-defined committag classes.\"\n\n> +\t# To disable system wide have in $GITWEB_CONFIG\n> +\t# $feature{'committags'}{'default'} = [];\n> +\t# To have project specific config enable override in $GITWEB_CONFIG\n> +\t# $feature{'committags'}{'override'} = 1;\n> +\t# and in project config gitweb.committags = sha1, url, bugzilla\n> +\t# to enable those three committags for that project\n> +\t'committags' => {\n> +\t\t'sub' => sub { feature_list('committags', @_) },\n> +\t\t'override' => 0,\n> +\t\t'default' => ['sha1']},\n> );\n\nWould people consider it harmful to add 'url' to the default set?\n\n> @@ -463,16 +569,16 @@ sub feature_bool {\n> \t}\n> }\n> \n> -sub feature_snapshot {\n> -\tmy (@fmts) = @_;\n> +sub feature_list {\n> +\tmy ($key, @defaults) = @_;\n> \n> -\tmy ($val) = git_get_project_config('snapshot');\n> +\tmy ($cfg) = git_get_project_config($key);\n> \n> -\tif ($val) {\n> -\t\t@fmts = ($val eq 'none' ? () : split /\\s*[,\\s]\\s*/, $val);\n> +\tif ($cfg) {\n> +\t\treturn ($cfg eq 'none' ? () : split(/\\s*[,\\s]\\s*/, $cfg));\n> \t}\n> \n> -\treturn @fmts;\n> +\treturn @defaults;\n> }\n> \n> sub feature_patches {\n> @@ -886,6 +992,35 @@ if ($git_avatar eq 'gravatar') {\n> \t$git_avatar = '';\n> }\n> \n> +# ordering of committags\n> +our @committags = gitweb_get_feature('committags');\n> +\n> +# whether we've loaded committags for the project yet\n> +our $loaded_project_committags = 0;\n> +\n> +# Load committag configs from the repository config file and and\n> +# incorporate them into the gitweb defaults where permitted by the\n> +# site administrator.\n> +sub gitweb_load_project_committags {\n> +\treturn if (!$git_dir || $loaded_project_committags);\n\nWhen can it happen that this is called and !$git_dir? In case it could\never happen, why not allow the configuration at least in global gitweb\nfile?\n\n> +\tmy %project_config = ();\n> +\tmy %raw_config = git_parse_project_config('gitweb\\.committag');\n> +\tforeach my $key (keys(%raw_config)) {\n> +\t\tnext if ($key !~ /gitweb\\.committag\\.[^.]+\\.[^.]/);\n> +\t\tmy ($gitweb_prefix, $committag_prefix, $ctname, $option) =\n> +\t\t\tsplit(/\\./, $key, 4);\n> +\t\t$project_config{$ctname}{$option} = $raw_config{$key};\n> +\t}\n> +\tforeach my $ctname (keys(%committags)) {\n> +\t\tnext if (!$committags{$ctname}{'override'});\n> +\t\tforeach my $optname (keys %{$project_config{$ctname}}) {\n> +\t\t\t$committags{$ctname}{'options'}{$optname} =\n> +\t\t\t\t$project_config{$ctname}{$optname};\n> +\t\t}\n> +\t}\n> +\t$loaded_project_committags = 1;\n> +}\n\nFor the next-conditions, I'd prefer unless formulation, but I guess\nthat's purely a matter of taste.\n\n> +sub push_or_append (\\@@) {\n> +\tmy $fragments = shift;\n> +\n> +\tif (ref $_[0] || ! @$fragments || ref $fragments->[-1]) {\n> +\t\tpush @$fragments, @_;\n> +\t} else {\n> +\t\tmy $a = pop @$fragments;\n> +\t\tmy $b = shift @_;\n> +\n> +\t\tpush @$fragments, $a . $b, @_;\n> +\t}\n> +\t# imitate push\n> +\treturn scalar @$fragments;\n\nThis looks *quite* cryptic, a comment would be rather helpful.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nA lot of people have my books on their bookshelves.\nThat's the problem, they need to read them. -- Don Knuth\n"},{"id":"127827","messageId":"20091118082636.GE12890@machine.or.cz","threadId":"19866","inReplyTo":"1258525350-5528-2-git-send-email-marcel@oak.homeunix.org","subject":"Re: [RFC PATCH 1/6] gitweb: Hyperlink committags in a commit message by regex matching","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2009-11-18T08:26:36Z","receivedAt":"2009-11-18T08:26:36Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Tue, Nov 17, 2009 at 10:22:25PM -0800, Marcel M. Cary wrote:\n> +\t\t\t# Avoid matching punctuation that might immediately follow\n> +\t\t\t# a url, is not part of the url, and is allowed in urls,\n> +\t\t\t# like a full-stop ('.').\n> +\t\t\t'pattern' => qr!(https?|ftps?|git|ssh|ssh+git|sftp|smb|webdavs?|\n> +\t\t\t nfs|irc|nntp|rsync)\n> +\t\t\t ://[-_a-zA-Z0-9\\@/&=+~#<>;%:.?]+\n> +\t\t\t [-_a-zA-Z0-9\\@/&=+~#<>]!x,\n\nYou meant ssh\\+git here. ;-)\n\n\t\t\t\tPetr \"Pasky\" Baudis\n"},{"id":"128055","messageId":"200911210024.29725.jnareb@gmail.com","threadId":"19866","inReplyTo":"1258525350-5528-1-git-send-email-marcel@oak.homeunix.org","subject":"Re: [RFC PATCH 0/6] Second round of committag series","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-11-20T23:24:28Z","receivedAt":"2009-11-20T23:24:28Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 18 Nov 2009, Marcel M. Cary wrote:\n\n> Thanks for the feedback.  I've added four more patches to the end of\n> the series and updated the first two.  My replies are below.\n> \n> On Mon, 22 Jun 2009, Jakub Narebski wrote:\n> > On Fri, 19 June 2009, Marcel M. Cary wrote:\n\nThanks for working on this. I'll try to reply soon.\n\n-- \nJakub Narebski\nPoland\n"}]}