Re: [RFC PATCH 1/6] gitweb: Hyperlink committags in a commit message by regex matching
- From
Petr Baudis <pasky@suse.cz>
- Date
- Nov 18, 2009, 08:20 UTC
- Message-ID
- <20091118082024.GD12890@machine.or.cz>
- In-Reply-To
- <1258525350-5528-2-git-send-email-marcel@oak.homeunix.org>
On Tue, Nov 17, 2009 at 10:22:25PM -0800, Marcel M. Cary wrote:
Show 5 quoted lines
> One additional thing that occured to me is that maybe the hyperlinks > added by committags should have 'rel="nofollow"' by default? And if > so, maybe that needs to be configurable? On the other hand, I'm not > sure how useful it is to hide real URLs in the commit messages from > search engines... ?
I don't think rel="nofollow" is necessary.
BTW, wouldn't it be useful to do the matching in blob bodies as well? And is it sensible to call these "committags" at all then? I already made the mistake of calling content tags "ctags" and I regret it; I think calling yet another thing tags after git tags and ctags is almost unbearable.
Show 16 quoted lines
> diff --git a/gitweb/INSTALL b/gitweb/INSTALL
> index b76a0cf..9081ed8 100644
> --- a/gitweb/INSTALL
> +++ b/gitweb/INSTALL
> @@ -132,6 +132,11 @@ adding the following lines to your $GITWEB_CONFIG:
> $known_snapshot_formats{'zip'}{'disabled'} = 1;
> $known_snapshot_formats{'tgz'}{'compressor'} = ['gzip','-6'];
>
> +To add a committag to the default list of commit tags, for example to
> +enable hyperlinking of bug numbers to a bug tracker for all projects:
> +
> + push @{$feature{'committags'}{'default'}}, 'bugzilla';
> +
>
> Gitweb repositories
> -------------------I think this is not useful at all, since:
(i) The user will also *always* need to override the URL. (ii) More importantly, the user has no idea what on earth commit tags are.
Could you please prepend this paragraph with a short committags description, e.g.:
"Gitweb can rewrite certain snippets of text in commit messages to hyperlinks, e.g. URL addresses or bug tracker references - we call these snippets 'committags'."
And you should also add
$committags{'buzilla'}{'options'}{'url'} = ...to the explanation, together with a reference to the appropriate part of gitweb.perl for more details.
Show 11 quoted lines
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index e4cbfc3..2d72202 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -213,6 +213,97 @@ our %avatar_size = ( > 'double' => 32 > ); > > +# In general, the site admin can enable/disable per-project > +# configuration of each committag. Only the 'options' part of the > +# committag is configurable per-project.
The exact same problem here - it is not explained what committag actually is and where it applies.
Show 35 quoted lines
> +# The site admin can of course add new tags to this hash or override
> +# the 'sub' key if necessary. But such changes may be fragile; this
> +# is not designed as a full-blown plugin architecture. The 'sub' must
> +# return a list of strings or string refs. The strings must contain
> +# plain text and the string refs must contain HTML. The string refs
> +# will not be processed further.
> +#
> +# For any committag, set the 'override' key to 1 to allow individual
> +# projects to override entries in the 'options' hash for that tag.
> +# For example, to match only commit hashes given in lowercase in one
> +# project, add this to the $GITWEB_CONFIG:
> +#
> +# $committags{'sha1'}{'override'} = 1;
> +#
> +# And in the project's config:
> +#
> +# gitweb.committags.sha1.pattern = \\b([0-9a-f]{8,40})\\b
> +#
> +# Some committags have additional options whose interpretation depends
> +# on the implementation of the 'sub' key. The hyperlink_committag
> +# value appends the first captured group to the 'url' option.
> +our %committags = (
> + # Link Git-style hashes to this gitweb
> + 'sha1' => {
> + 'options' => {
> + 'pattern' => qr/\b([0-9a-fA-F]{8,40})\b/,
> + },
> + 'override' => 0,
> + 'sub' => sub {
> + my ($opts, @match) = @_;
> + return \$cgi->a({-href => href(action=>"object", hash=>$match[1]),
> + -class => "text"},
> + esc_html($match[0], -nbsp=>1));
> + },
> + },Ideally, a link should be made only in case the object exists, but this is not trivial to implement without overhead of 1 exec per object, so I guess it's fine to leave this for later (after all this feature was already present). In that case, I think it would be useful to start matching ids from 5 characters up - I use these quite frequently ;) - but until then it would probably make for too much false positives.
Show 8 quoted lines
> @@ -417,6 +508,21 @@ our %feature = ( > 'sub' => \&feature_avatar, > 'override' => 0, > 'default' => ['']}, > + > + # The selection and ordering of committags that are enabled. > + # Committag transformations will be applied to commit log messages > + # in the order listed here if listed here.
You should add something like "See %committags definition above for explanation of committags and pre-defined committag classes."
Show 11 quoted lines
> + # To disable system wide have in $GITWEB_CONFIG
> + # $feature{'committags'}{'default'} = [];
> + # To have project specific config enable override in $GITWEB_CONFIG
> + # $feature{'committags'}{'override'} = 1;
> + # and in project config gitweb.committags = sha1, url, bugzilla
> + # to enable those three committags for that project
> + 'committags' => {
> + 'sub' => sub { feature_list('committags', @_) },
> + 'override' => 0,
> + 'default' => ['sha1']},
> );Would people consider it harmful to add 'url' to the default set?
Show 38 quoted lines
> @@ -463,16 +569,16 @@ sub feature_bool {
> }
> }
>
> -sub feature_snapshot {
> - my (@fmts) = @_;
> +sub feature_list {
> + my ($key, @defaults) = @_;
>
> - my ($val) = git_get_project_config('snapshot');
> + my ($cfg) = git_get_project_config($key);
>
> - if ($val) {
> - @fmts = ($val eq 'none' ? () : split /\s*[,\s]\s*/, $val);
> + if ($cfg) {
> + return ($cfg eq 'none' ? () : split(/\s*[,\s]\s*/, $cfg));
> }
>
> - return @fmts;
> + return @defaults;
> }
>
> sub feature_patches {
> @@ -886,6 +992,35 @@ if ($git_avatar eq 'gravatar') {
> $git_avatar = '';
> }
>
> +# ordering of committags
> +our @committags = gitweb_get_feature('committags');
> +
> +# whether we've loaded committags for the project yet
> +our $loaded_project_committags = 0;
> +
> +# Load committag configs from the repository config file and and
> +# incorporate them into the gitweb defaults where permitted by the
> +# site administrator.
> +sub gitweb_load_project_committags {
> + return if (!$git_dir || $loaded_project_committags);When can it happen that this is called and !$git_dir? In case it could ever happen, why not allow the configuration at least in global gitweb file?
Show 17 quoted lines
> + my %project_config = ();
> + my %raw_config = git_parse_project_config('gitweb\.committag');
> + foreach my $key (keys(%raw_config)) {
> + next if ($key !~ /gitweb\.committag\.[^.]+\.[^.]/);
> + my ($gitweb_prefix, $committag_prefix, $ctname, $option) =
> + split(/\./, $key, 4);
> + $project_config{$ctname}{$option} = $raw_config{$key};
> + }
> + foreach my $ctname (keys(%committags)) {
> + next if (!$committags{$ctname}{'override'});
> + foreach my $optname (keys %{$project_config{$ctname}}) {
> + $committags{$ctname}{'options'}{$optname} =
> + $project_config{$ctname}{$optname};
> + }
> + }
> + $loaded_project_committags = 1;
> +}For the next-conditions, I'd prefer unless formulation, but I guess that's purely a matter of taste.
Show 13 quoted lines
> +sub push_or_append (\@@) {
> + my $fragments = shift;
> +
> + if (ref $_[0] || ! @$fragments || ref $fragments->[-1]) {
> + push @$fragments, @_;
> + } else {
> + my $a = pop @$fragments;
> + my $b = shift @_;
> +
> + push @$fragments, $a . $b, @_;
> + }
> + # imitate push
> + return scalar @$fragments;This looks *quite* cryptic, a comment would be rather helpful.
-- Petr "Pasky" Baudis A lot of people have my books on their bookshelves. That's the problem, they need to read them. -- Don Knuth