Re: [PATCH] gitweb: support the rel=vcs-* microformat

[Jakub Narebski <jnareb@gmail.com>]

Joey Hess <joey@kitenet.net> writes:

> The rel=vcs-* microformat allows a web page to indicate the locations of
> repositories related to it in a machine-parseable manner.
> (See http://kitenet.net/~joey/rfc/rel-vcs/)
> 
> Make gitweb use the microformat if it has been configured with project url
> information in any of the usual ways. On the project summary page, the
> repository URL display is simply marked up using the microformat. On the
> project list page and forks list page, the microformat is embedded in the
> header, since the URLs do not appear on the page.

I think having LINK elements also for 'summary' page would be a good
idea. This microformat is I think mainly for machines, and machines
can I guess read better a few LINK elements in fairly small HEAD of
page, than scan all of many link (A) elements on the page for those
matching vcs-* microformat.

Beside I am not sure if for example hyperlinking SCP-style repository
URL makes sense at all; I am also not sure if hyperlinking links on
which you cannot click on makes good sense (unless you use SPAN or
ABBR instead of A to mark repo links...)

> 
> The microformat could be included on other pages too, but I've skipped
> doing so for now, since it would mean reading another file for every page
> displayed.

Also it is not necessary: if some tool want to get repo links for
given project, it can get 'summary' page; if some tool want to get
list of all repos, it can access one of projects list actions.

> 
> There is a small overhead in including the microformat on project list
> and forks list pages, but getting the project descriptions for those pages
> already incurs a similar overhead, and the ability to get every repo url
> in one place seems worthwhile.

By the way, do you have any benchmarks for that?

> 
> This changes git_get_project_url_list() to not check wantarray, and only
> return in list context -- the only way it is used AFAICS. It memoizes
> both that function and git_get_project_description(), to avoid redundant
> file reads.

I would also add that, from what I understand, you have made
git_get_project_url_list() subroutine to be self-sufficient: it now
considers both per-repository configuration (gitweb.url in config,
cloneurl file in $GIT_DIR) and global gitweb configuration
(@git_base_url_list variable).

Simplification of code so it always return list and does nto check
contents is a side issue, orthogonal to issue mentioned above.

> 
> Signed-off-by: Joey Hess <joey@gnu.kitenet.net>
> ---
>  gitweb/gitweb.perl |   78 +++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 62 insertions(+), 16 deletions(-)
> 
> This incorporates Giuseppe Bilotta's feedback, and uses new features
> of the microformat. You can see this version running at
> http://git.ikiwiki.info/
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 99f71b4..c238717 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2020,9 +2020,14 @@ sub git_get_path_by_hash {
>  ## ......................................................................
>  ## git utility functions, directly accessing git repository
>  
> +{
> +my %project_descriptions; # cache
> +

Won't we get warnings (and perhaps errors) from mod_perl? Shouldn't
this be "our %project_descriptions;"?

>  sub git_get_project_description {
>  	my $path = shift;
>  
> +	return $project_descriptions{$path} if exists $project_descriptions{$path};
> +
>  	$git_dir = "$projectroot/$path";
>  	open my $fd, "$git_dir/description"
>  		or return git_get_project_config('description');
> @@ -2031,7 +2036,9 @@ sub git_get_project_description {
>  	if (defined $descr) {
>  		chomp $descr;
>  	}
> -	return $descr;
> +	return $project_descriptions{$path}=$descr;
> +}
> +
>  }

If we use 'title="$project git repository" for 'rel="vcs-git"' links,
is it still worth it extra complication to avoid double calculation of
project description in the case of 'summary' view for a project?
Because IIRC for 'projects_list' view it is already cached in
@projects list as 'descr' key...

>  
>  sub git_get_project_ctags {
> @@ -2099,18 +2106,30 @@ sub git_show_project_tagcloud {
>  	}
>  }
>  
> +{
> +my %project_url_lists; # cache
> +

Same question: would it work correctly for mod_perl?

>  sub git_get_project_url_list {
> +	# use per project git URL list in $projectroot/$path/cloneurl
> +	# or make project git URL from git base URL and project name
>  	my $path = shift;
>  
> +	return @{$project_url_lists{$path}} if exists $project_url_lists{$path};
> +
> +	my @ret;
>  	$git_dir = "$projectroot/$path";
> -	open my $fd, "$git_dir/cloneurl"
> -		or return wantarray ?
> -		@{ config_to_multi(git_get_project_config('url')) } :
> -		   config_to_multi(git_get_project_config('url'));
> -	my @git_project_url_list = map { chomp; $_ } <$fd>;
> -	close $fd;
> +	if (open my $fd, "$git_dir/cloneurl") {
> +		@ret = map { chomp; $_ } <$fd>;
> +		close $fd;
> +	} else {
> +	       @ret = @{ config_to_multi(git_get_project_config('url')) };
> +	}
> +	@ret=map { "$_/$project" } @git_base_url_list if ! @ret;

Style: 

+	@ret = map { "$_/$project" } @git_base_url_list if !@ret;

or even

+	@ret = map { "$_/$project" } @git_base_url_list unless @ret;

> +
> +	$project_url_lists{$path}=\@ret;
> +	return @ret;
> +}
>  
> -	return wantarray ? @git_project_url_list : \@git_project_url_list;
>  }

Again: is it worth caching? It is only for 'summary'; for
'projects_list' it might be better to extend @projects list instead

>  
>  sub git_get_projects_list {
> @@ -2856,6 +2875,7 @@ sub blob_contenttype {
>  sub git_header_html {
>  	my $status = shift || "200 OK";
>  	my $expires = shift;
> +	my $extraheader = shift;
>  
>  	my $title = "$site_name";
>  	if (defined $project) {
> @@ -2953,6 +2973,8 @@ EOF
>  		print qq(<link rel="shortcut icon" href="$favicon" type="image/png" />\n);
>  	}
>  
> +	print $extraheader if defined $extraheader;
> +
>  	print "</head>\n" .
>  	      "<body>\n";
>  

Good solution, but shouldn't this be better put into separate commit,
simply extending git_header_html to allow to add extra data (no need
to name it $extraheader I think, $extra would be enough) to the HTML
header (HEAD element contents)?

> @@ -4365,6 +4387,26 @@ sub git_search_grep_body {
>  	print "</table>\n";
>  }
>  
> +sub git_link_title {
> +	my $project=shift;
> +	
> +	my $description=git_get_project_description($project);
> +	return $project.(length $description ? " - $description" : "");
> +}

Style (whitespace around '='), and the fact that IMHO "$project git
repository" is better than "$project - $description", also because of
  "Unnamed repository; edit this file to name it for gitweb." 
default template

> +
> +# generates header with links to the specified projects
> +sub git_links_header {

Good abstraction, but I'm not so sure about subroutine name.

> +	my $ret='';
> +	foreach my $project (@_) {

Style: I'd rather use named variables, like "my @projects = @_";
also everywhere else we use spaces around '=' usually.

> +		# rel=vcs-* microformat
> +		my $title=git_link_title($project);

Good abstraction.

> +		foreach my $url git_get_project_url_list($project) {
> +			$ret.=qq{<link rel="vcs-git" href="$url" title="$title"/>\n}

To be HTML compatibile, it is better to use 

> +			$ret.=qq{<link rel="vcs-git" href="$url" title="$title" />\n}

(note the space before "/>").

> +		}
> +	}
> +	return $ret;
> +}
> +
>  ## ======================================================================
>  ## ======================================================================
>  ## actions
> @@ -4380,7 +4422,9 @@ sub git_project_list {
>  		die_error(404, "No projects found");
>  	}
>  
> -	git_header_html();
> +	my $extraheader=git_links_header(map { $_->{path} } @list);
> +
> +	git_header_html(undef, undef, $extraheader);
>  	if (-f $home_text) {
>  		print "<div class=\"index_include\">\n";
>  		insert_file($home_text);
> @@ -4405,8 +4449,10 @@ sub git_forks {
>  	if (!@list) {
>  		die_error(404, "No forks found");
>  	}
> +	
> +	my $extraheader=git_links_header(map { $_->{path} } @list);
>  
> -	git_header_html();
> +	git_header_html(undef, undef, $extraheader);
>  	git_print_page_nav('','');
>  	git_print_header_div('summary', "$project forks");
>  	git_project_list_body(\@list, $order);
> @@ -4468,14 +4514,14 @@ sub git_summary {
>  		print "<tr id=\"metadata_lchange\"><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
>  	}
>  
> -	# use per project git URL list in $projectroot/$project/cloneurl
> -	# or make project git URL from git base URL and project name
>  	my $url_tag = "URL";
> -	my @url_list = git_get_project_url_list($project);
> -	@url_list = map { "$_/$project" } @git_base_url_list unless @url_list;
> -	foreach my $git_url (@url_list) {
> +	my $title=git_link_title($project);
> +	foreach my $git_url (git_get_project_url_list($project)) {
>  		next unless $git_url;
> -		print "<tr class=\"metadata_url\"><td>$url_tag</td><td>$git_url</td></tr>\n";
> +		print "<tr class=\"metadata_url\"><td>$url_tag</td><td>".
> +		      # rel=vcs-* microformat
> +		      "<a rel=\"vcs-git\" href=\"$git_url\" title=\"$title\">$git_url</a>".
> +		      "</td></tr>\n";
>  		$url_tag = "";
>  	}

Non clickable hyperlink... hmmm...

>  
> -- 
> 1.5.6.5
> 
> 
> 
> -- 
> see shy jo

-- 
Jakub Narebski
Poland
ShadeHawk on #git