Re: [PATCH] gitweb: support the rel=vcs-* microformat
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jan 10, 2009, 00:52 UTC
- Message-ID
- <m3bpug0ypn.fsf@localhost.localdomain>
- In-Reply-To
- <20090107232427.GA18958@gnu.kitenet.net>
Joey Hess <joey@kitenet.net> writes:
Show 9 quoted lines
> 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.
Show 5 quoted lines
> > 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?
Show 5 quoted lines
> > 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.
Show 21 quoted lines
>
> 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;"?
Show 17 quoted lines
> 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...
Show 9 quoted lines
>
> 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?
Show 22 quoted lines
> 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;Show 7 quoted lines
> +
> + $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
Show 19 quoted lines
>
> 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)?
Show 10 quoted lines
> @@ -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 "/>").
Show 51 quoted lines
> + }
> + }
> + 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...
Show 8 quoted lines
> > -- > 1.5.6.5 > > > > -- > see shy jo
-- Jakub Narebski Poland ShadeHawk on #git