[PATCH 0/4] gitweb feed metadata tuneups

[PATCH 0/4] gitweb feed metadata tuneups

[Giuseppe Bilotta]

The next four patches add some metadata to gitweb generated feeds:
channel image, managing editor and last-update dates are added to RSS
feeds, and the feed generator (gitweb, with version specification) is
added to both RSS and Atom feeds.

Giuseppe Bilotta (4):
  gitweb: channel image in rss feed
  gitweb: feed generator metadata
  gitweb: rss feed managingEditor
  gitweb: rss channel date

 gitweb/gitweb.perl |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

[PATCH 1/4] gitweb: channel image in rss feed

[Giuseppe Bilotta]

Define the channel image for the rss feed when the logo or favicon are
defined, preferring the former to the latter. As suggested in the RSS
2.0 specifications, the image's title and link as set to the same as the
channel's.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 931db4f..f8a5d2e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6075,6 +6075,16 @@ XML
 		      "<link>$alt_url</link>\n" .
 		      "<description>$descr</description>\n" .
 		      "<language>en</language>\n";
+		if (defined $logo || defined $favicon) {
+			# prefer the logo to the favicon, since RSS
+			# doesn't allow both
+			my $img = esc_url($logo || $favicon);
+			print "<image>\n" .
+			      "<url>$img</url>\n" .
+			      "<title>$title</title>\n" .
+			      "<link>$alt_url</link>\n" .
+			      "</image>\n";
+		}
 	} elsif ($format eq 'atom') {
 		print <<XML;
 <feed xmlns="http://www.w3.org/2005/Atom">
-- 
1.5.6.5

[PATCH 2/4] gitweb: feed generator metadata

[Giuseppe Bilotta]

Add <generator> tag to RSS and Atom feed. Versioning info (gitweb/git
core versions, separated by a literal slash) is stored in the
appropriate attribute for the Atom feed, and in the tag content for the
RSS feed.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f8a5d2e..3d94f50 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6085,6 +6085,7 @@ XML
 			      "<link>$alt_url</link>\n" .
 			      "</image>\n";
 		}
+		print "<generator>gitweb v.$version/$git_version</generator>\n";
 	} elsif ($format eq 'atom') {
 		print <<XML;
 <feed xmlns="http://www.w3.org/2005/Atom">
@@ -6111,6 +6112,7 @@ XML
 		} else {
 			print "<updated>$latest_date{'iso-8601'}</updated>\n";
 		}
+		print "<generator version='$version/$git_version'>gitweb</generator>\n";
 	}
 
 	# contents
-- 
1.5.6.5

[PATCH 3/4] gitweb: rss feed managingEditor

[Giuseppe Bilotta]

The RSS 2.0 specification allows an optional managingEditor tag for the
channel, containing the "email address for person responsible for editorial
content", which is basically the project owner.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3d94f50..cc6d0fb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6074,7 +6074,9 @@ XML
 		print "<title>$title</title>\n" .
 		      "<link>$alt_url</link>\n" .
 		      "<description>$descr</description>\n" .
-		      "<language>en</language>\n";
+		      "<language>en</language>\n" .
+		      # project owner is responsible for 'editorial' content
+		      "<managingEditor>$owner</managingEditor>\n";
 		if (defined $logo || defined $favicon) {
 			# prefer the logo to the favicon, since RSS
 			# doesn't allow both
-- 
1.5.6.5

[PATCH 4/4] gitweb: rss channel date

[Giuseppe Bilotta]

The RSS 2.0 specifications defines not one but _two_ dates for its
channel element! Woohoo! Luckily, it seems that consensus seems to be
that if both are present they should be equal, except for some very
obscure and discouraged cases. Since lastBuildDate would make more sense
for us and pubDate seems to be the most commonly used, we defined both
and make them equal.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cc6d0fb..756868a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6087,6 +6087,10 @@ XML
 			      "<link>$alt_url</link>\n" .
 			      "</image>\n";
 		}
+		if (%latest_date) {
+			print "<pubDate>$latest_date{'rfc2822'}</pubDate>\n";
+			print "<lastBuildDate>$latest_date{'rfc2822'}</lastBuildDate>\n";
+		}
 		print "<generator>gitweb v.$version/$git_version</generator>\n";
 	} elsif ($format eq 'atom') {
 		print <<XML;
-- 
1.5.6.5

Re: [PATCH 0/4] gitweb feed metadata tuneups

[Jakub Narebski]

On Fri, 23 Jan 2009, Giuseppe Bilotta wrote:

> The next four patches add some metadata to gitweb generated feeds:
> channel image, managing editor and last-update dates are added to RSS
> feeds, and the feed generator (gitweb, with version specification) is
> added to both RSS and Atom feeds.
> 
> Giuseppe Bilotta (4):
>   gitweb: channel image in rss feed
>   gitweb: feed generator metadata
>   gitweb: rss feed managingEditor
>   gitweb: rss channel date
> 
>  gitweb/gitweb.perl |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)

I like this series; however I do not use gitweb feeds (Atom or RSS),
so I cannot say anything on their validity and usefullness.

P.S. I tried to look up who is responsible (who have added) RSS code,
but unfortunately it looks like it dates back to Kay Sievers.
Unfortunately because IIRC he is not active on the list...
-- 
Jakub Narebski
Poland

[PATCH] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

The last-modified time header added by RSS to increase cache hits from
readers should be set to the date the repository was last modified. The
author time in this respect is not a good guess because the last commit
might come from a oldish patch.

Use the committer time for the last-modified header to ensure a more
correct guess of the last time the repository was modified.
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 756868a..8c49c75 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6015,7 +6015,7 @@ sub git_feed {
 	}
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
-		%latest_date   = parse_date($latest_commit{'author_epoch'});
+		%latest_date   = parse_date($latest_commit{'committer_epoch'});
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-- 
1.5.6.5

[PATCH] gitweb: check if-modified-since for feeds

[Giuseppe Bilotta]

Offering Last-modified header for feeds is only half the work: we should
also check that same date against If-modified-since, and bail out early
with 304 Not Modified.
---
 gitweb/gitweb.perl |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8c49c75..0a5d229 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6015,7 +6015,25 @@ sub git_feed {
 	}
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
-		%latest_date   = parse_date($latest_commit{'committer_epoch'});
+		my $latest_epoch = $latest_commit{'committer_epoch'};
+		%latest_date   = parse_date($latest_epoch);
+		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+		if (defined $if_modified) {
+			my $since;
+			if (eval { require HTTP::Date; 1; }) {
+				$since = HTTP::Date::str2time($if_modified);
+			} elsif (eval { require Time::ParseDate; 1; }) {
+				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+			}
+			if (defined $since && $latest_epoch <= $since) {
+				print $cgi->header(
+					-type => $content_type,
+					-charset => 'utf-8',
+					-last_modified => $latest_date{'rfc2822'},
+					-status => 304);
+				return;
+			}
+		}
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-- 
1.5.6.5

Re: [PATCH] gitweb: last-modified time should be commiter, not author

[Jakub Narebski]

On Sun, 25 Jan 2009, Giuseppe Bilotta wrote:

> Subject: [PATCH] gitweb: last-modified time should be commiter, not author

Should be really either "[PATCH 1/2]" or "[PATCH 5/4]" or "[PATCH 5/6]"
just in case for the next patch, because next patch _depends_ on this
one, and just in case of threading problem it should be marked as it;
it also makes easier to apply patches from emails saved as individual
files each.

>
> The last-modified time header added by RSS to increase cache hits from
> readers should be set to the date the repository was last modified. The
> author time in this respect is not a good guess because the last commit
> might come from a oldish patch.
> 
> Use the committer time for the last-modified header to ensure a more
> correct guess of the last time the repository was modified.

Good catch, good thinking IMHO. Committer date has much better chance
to be monotonic than author date, and is more close related to 
_publishing_ date (author date is more of _creation_ date).

Lack signoff; if Junio forges it (or you reply that it should be
signed off), you can add from me

Acked-by: Jakub Narebski <jnareb@gmail.com>


P.S. I wonder what other web interfaces do, for example cgit. I guess
that web interfaces for other SCMs like SVN::Web, ViewVC etc. do not
have this problem because they have only one, single date.

> ---
>  gitweb/gitweb.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 756868a..8c49c75 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -6015,7 +6015,7 @@ sub git_feed {
>  	}
>  	if (defined($commitlist[0])) {
>  		%latest_commit = %{$commitlist[0]};
> -		%latest_date   = parse_date($latest_commit{'author_epoch'});
> +		%latest_date   = parse_date($latest_commit{'committer_epoch'});
>  		print $cgi->header(
>  			-type => $content_type,
>  			-charset => 'utf-8',
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: check if-modified-since for feeds

[Jakub Narebski]

Should be "[PATCH 2/2]" or similar, just in case.

On Sun, 25 Jun 2009, Giuseppe Bilotta wrote:

> Offering Last-modified header

And skipping generating the body if client uses 'HEAD' request to
get only Last-Modified header.

>                              for feeds is only half the work: we should 
> also check that same date against If-modified-since, and bail out early
> with 304 Not Modified.

Lacks signoff.

> ---
>  gitweb/gitweb.perl |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8c49c75..0a5d229 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -6015,7 +6015,25 @@ sub git_feed {
>  	}
>  	if (defined($commitlist[0])) {
>  		%latest_commit = %{$commitlist[0]};
> -		%latest_date   = parse_date($latest_commit{'committer_epoch'});
> +		my $latest_epoch = $latest_commit{'committer_epoch'};
> +		%latest_date   = parse_date($latest_epoch);
> +		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
> +		if (defined $if_modified) {
> +			my $since;
> +			if (eval { require HTTP::Date; 1; }) {
> +				$since = HTTP::Date::str2time($if_modified);
> +			} elsif (eval { require Time::ParseDate; 1; }) {
> +				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
> +			}

I'd really like to fallback on hand-parsing, as we have to parse date
in well defined HTTP-date format (RFC-1123, update to RFC-822), which
I think is what we send in Last-Modified header (or is it RFC-2822?).

But that might be too much work. I like the checking for modules,
and the fallback cascade, but could you explain why in this order?

> +			if (defined $since && $latest_epoch <= $since) {
> +				print $cgi->header(
> +					-type => $content_type,
> +					-charset => 'utf-8',
> +					-last_modified => $latest_date{'rfc2822'},
> +					-status => 304);

I think we spell HTTP status messages in full (even if it is hidden
in die_error subroutine), i.e.

+					-status => '304 Not Modified');

> +				return;
> +			}
> +		}
>  		print $cgi->header(
>  			-type => $content_type,
>  			-charset => 'utf-8',
> -- 
> 1.5.6.5
> 
> 

P.S. It would be nice to have this mechanism (responding to
cache-control headers such as If-Modified-Since) for all of gitweb,
but I guess it is most critical for feeds, which are _polled_.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: check if-modified-since for feeds

[Giuseppe Bilotta]

On Mon, Jan 26, 2009 at 3:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Should be "[PATCH 2/2]" or similar, just in case.

Yeah, but I was just hacking up ideas as they came. I'm planning a
resend of the whole series, properly numbered and all.

> On Sun, 25 Jun 2009, Giuseppe Bilotta wrote:
>
>> Offering Last-modified header
>
> And skipping generating the body if client uses 'HEAD' request to
> get only Last-Modified header.
>
>>                              for feeds is only half the work: we should
>> also check that same date against If-modified-since, and bail out early
>> with 304 Not Modified.
>
> Lacks signoff.

Oh yeah.

>> -             %latest_date   = parse_date($latest_commit{'committer_epoch'});
>> +             my $latest_epoch = $latest_commit{'committer_epoch'};
>> +             %latest_date   = parse_date($latest_epoch);
>> +             my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
>> +             if (defined $if_modified) {
>> +                     my $since;
>> +                     if (eval { require HTTP::Date; 1; }) {
>> +                             $since = HTTP::Date::str2time($if_modified);
>> +                     } elsif (eval { require Time::ParseDate; 1; }) {
>> +                             $since = Time::ParseDate::parsedate($if_modified, GMT => 1);
>> +                     }
>
> I'd really like to fallback on hand-parsing, as we have to parse date
> in well defined HTTP-date format (RFC-1123, update to RFC-822), which
> I think is what we send in Last-Modified header (or is it RFC-2822?).
>
> But that might be too much work. I like the checking for modules,
> and the fallback cascade, but could you explain why in this order?

Of course, if we have our own parsing code, we don't need the other
modules. I'm way too lazy to write the parsing code myself, although a
copypaste from existing GPL code would do it.

(BTW, I asked on #perl and they think gitweb non-reliance on CPAN
makes for some very horrible code. Of course, IMO the real problem is
that perl's stdlib is way too limited, but that is likely to causes a
language war so I refrained from discussing the thing.)

The order is almost casual, but I suspect that HTTP::Date, from
libwww-perl, is more likely to be available on a webserver than the
other.

>> +                     if (defined $since && $latest_epoch <= $since) {
>> +                             print $cgi->header(
>> +                                     -type => $content_type,
>> +                                     -charset => 'utf-8',
>> +                                     -last_modified => $latest_date{'rfc2822'},
>> +                                     -status => 304);
>
> I think we spell HTTP status messages in full (even if it is hidden
> in die_error subroutine), i.e.
>
> +                                       -status => '304 Not Modified');

Can do that.

> P.S. It would be nice to have this mechanism (responding to
> cache-control headers such as If-Modified-Since) for all of gitweb,
> but I guess it is most critical for feeds, which are _polled_.

I thought so too, but then again I couldn't see where last-modified
was used. (Its usage could be added, of course.)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Mon, Jan 26, 2009 at 2:54 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sun, 25 Jan 2009, Giuseppe Bilotta wrote:
>
>> Subject: [PATCH] gitweb: last-modified time should be commiter, not author
>
> Should be really either "[PATCH 1/2]" or "[PATCH 5/4]" or "[PATCH 5/6]"
> just in case for the next patch, because next patch _depends_ on this
> one, and just in case of threading problem it should be marked as it;
> it also makes easier to apply patches from emails saved as individual
> files each.

I'll resend the whole 6-patch series cc'ing Junio too

>> Use the committer time for the last-modified header to ensure a more
>> correct guess of the last time the repository was modified.
>
> Good catch, good thinking IMHO. Committer date has much better chance
> to be monotonic than author date, and is more close related to
> _publishing_ date (author date is more of _creation_ date).

BTW, it is still not good enough. Consider a remote repo to which you
just pushed some changes you commited last week. The last-modified
date would be last-week, even if you just pushed those changes.

This used to be a problem for us on ruby-rbot because even when
watching the feed we would get no updates on the irc channel (the bot
thought the feed could still be cached). the if-modified thing seems
to have fixed this though.

> Lack signoff; if Junio forges it (or you reply that it should be
> signed off), you can add from me
>
> Acked-by: Jakub Narebski <jnareb@gmail.com>
>
>
> P.S. I wonder what other web interfaces do, for example cgit. I guess
> that web interfaces for other SCMs like SVN::Web, ViewVC etc. do not
> have this problem because they have only one, single date.

HEAD'ing a cgit atom url gives a last-modified date one hour from the
request, it seems. This is actually a typical (and not nice) behaviour
of many CGI scripts.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: check if-modified-since for feeds

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
> On Mon, Jan 26, 2009 at 3:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Sun, 25 Jun 2009, Giuseppe Bilotta wrote:

>>> -             %latest_date   = parse_date($latest_commit{'committer_epoch'});
>>> +             my $latest_epoch = $latest_commit{'committer_epoch'};
>>> +             %latest_date   = parse_date($latest_epoch);
>>> +             my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
>>> +             if (defined $if_modified) {
>>> +                     my $since;
>>> +                     if (eval { require HTTP::Date; 1; }) {
>>> +                             $since = HTTP::Date::str2time($if_modified);
>>> +                     } elsif (eval { require Time::ParseDate; 1; }) {
>>> +                             $since = Time::ParseDate::parsedate($if_modified, GMT => 1);
>>> +                     }
>>
>> I'd really like to fallback on hand-parsing, as we have to parse date
>> in well defined HTTP-date format (RFC-1123, update to RFC-822), which
>> I think is what we send in Last-Modified header (or is it RFC-2822?).
>>
>> But that might be too much work. I like the checking for modules,
>> and the fallback cascade, but could you explain why in this order?
> 
> Of course, if we have our own parsing code, we don't need the other
> modules. I'm way too lazy to write the parsing code myself, although a
> copypaste from existing GPL code would do it.
> 
> (BTW, I asked on #perl and they think gitweb non-reliance on CPAN
> makes for some very horrible code. Of course, IMO the real problem is
> that perl's stdlib is way too limited, but that is likely to causes a
> language war so I refrained from discussing the thing.)

The problem is (which was discussed on git mailing list) that gitweb
is to be run on servers.  And admins are very reluctant on putting
anything but well tested software on server.  The order is: in standard
installation, in core distribution, in extras package of distribution,
in trusted extras repository... random CPAN package is usually out of
the question.

> 
> The order is almost casual, but I suspect that HTTP::Date, from
> libwww-perl, is more likely to be available on a webserver than the
> other.

True.

BTW. I have thought that we already require libwww-perl with the
modules we use in gitweb, but they are all packaged with Perl.

While I don't think that there would be many complaints requiring
libwww-perl, I'd rather have it optional...

-- 
Jakub Narebski
Poland