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

[Jakub Narebski <jnareb@gmail.com>]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes: 
> On Thursday 08 January 2009 00:24, Joey Hess wrote:
> 
> > 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/)
> 
> Have you considered submitting the microformat to microformats.org?
> That would make the microformat more official and would be an good
> first step to have wider coverage of it, and additional reviews.

Good thinking.  BTW. microformats.org is IIRC wiki (or at least part
of it is wiki), so it should be easy to do...

> 
> > 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.
> > 
> > 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.
> > 
> > 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.
> 
> I agree with this, although people with very large project lists may
> differ ... do we have timings on these?

I think while adding this microformat to 'summary' page is non-issue,
we might want to be able configure it out so it is not used for
projects_list page (which might be very large).

And what about OPML, RSS and Atom formats?

>  
> > 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.
> 
> You may want to consider splitting the patch into three: memoizing
> of git_get_project_description(), reworking of
> git_get_project_url_list(), and the actual rel=vc-* insertions.

Very good idea.  Small, single feature patches are nice.

[...]
> >  sub git_get_project_description {
> >       my $path = shift;
> >  
> > +     return $project_descriptions{$path} if exists $project_descriptions{$path};
> > +
> 
> This line is bordering on the 80 characters, so you may want to
> consider moving 'my $descr' here, with something such as
> 
> my $descr = $project_descriptions{$path};
> return $descr if exists $descr;
> 
> Also, I'm no perl guru so I'm not sure about exists vs defined here.

You might have undefined value in existing key, but I guess that we
can assume that those are equivalent for this.  While 'exists' seems
more up to what you check (does the key exosts in hash) you further on
rely on the fact that $descr is not undefined.

[...]
> >  ## ======================================================================
> >  ## ======================================================================
> >  ## 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);
> 
> This makes me wonder if it would be worth it to turn git_header_html
> into -param => value style, but I'm not really sure it's worth it.

It is git_header_html(STATUS, EXPIRES, EXTRA)

Hmmm... now I have checked we use either git_header_html() in gitweb
(which is most common), or git_header_html(STATUS) in die_error, or in
a few cases git_header_html(undef, $expires); and now
git_header_html(undef, undef, $extra), so named parameters might be a
good idea... I don't have opinion here...

-- 
Jakub Narebski
Poland
ShadeHawk on #git