[PATCHv2 0/6] gitweb: feed metadata enhancements

[PATCHv2 0/6] gitweb: feed metadata enhancements

[Giuseppe Bilotta]

This second revision adds two patches to improve client-side rss
caching: the last-modified header we output is based on commit rather
than creation time, and we now act on if-modified-since request headers.

The last patch requires either HTTP::Date (from libwww-perl) or
Time::ParseDate

Giuseppe Bilotta (6):
  gitweb: channel image in rss feed
  gitweb: feed generator metadata
  gitweb: rss feed managingEditor
  gitweb: rss channel date
  gitweb: last-modified time should be commiter, not author
  gitweb: check if-modified-since for feeds

 gitweb/gitweb.perl |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

[PATCHv2 1/6] 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

[PATCHv2 2/6] 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

[PATCHv2 3/6] 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

[PATCHv2 4/6] 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

[PATCHv2 5/6] 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.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 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

[PATCHv2 6/6] gitweb: check if-modified-since for feeds

[Giuseppe Bilotta]

Offering Last-modified header for feeds is only half the work, even if
we bail out early on HEAD requests. We should also check that same date
against If-modified-since, and bail out early with 304 Not Modified if
that's the case.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8c49c75..f4defb0 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 Not Modified');
+				return;
+			}
+		}
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-- 
1.5.6.5

Re: [PATCHv2 0/6] gitweb: feed metadata enhancements

[Junio C Hamano]

This is independent from your other topic on PATH_INFO, right?  I am aware
that other one is being polished between you and Jakub, but is anobody
doing anything on this one, or is it already polished enough?

Re: [PATCHv2 0/6] gitweb: feed metadata enhancements

[Jakub Narebski]

On Wed, 28 Jan 2009, Junio C Hamano wrote:
> This is independent from your other topic on PATH_INFO, right?

Yes, it is independent on adding BASE for PATH_INFO.

> I am aware that other one is being polished between you and Jakub,

The code (in BASE for PATH_INFO) looks now good, only commit
message might be further improved.

> but is anobody doing anything on this one, or is it already polished enough?

I think it is good, but I do not use web feeds (Atom and RSS)
from gitweb myself, therefore I don't feel competent to tell
if it is good enough. With exception of last one there are
quite simple and feel good; the last one is a bit more involved,
but I think also good (it would be nice to say in commit
message that HTTP::Date can be found in perl-libwww-perl).

-- 
Jakub Narebski
Poland

Re: [PATCHv2 0/6] gitweb: feed metadata enhancements

[Giuseppe Bilotta]

On Wed, Jan 28, 2009 at 9:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This is independent from your other topic on PATH_INFO, right?  I am aware
> that other one is being polished between you and Jakub, but is anobody
> doing anything on this one, or is it already polished enough?

Yes it's a totally independent topic. For what it's worth, this series
is running on ruby-rbot.org gitweb and it fixed a bunch of rss polling
related issues we were having with the bot we use to watch it.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 0/6] gitweb: feed metadata enhancements

[Junio C Hamano]

Ok, let's apply it directly to 'master', then.

Thanks.

Re: [PATCHv2 1/6] gitweb: channel image in rss feed

[Jakub Narebski]

I'm sorry for the delay reviewing this series.

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> 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.

I think it is a very good idea.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I don't personally use either RSS feeds, or Atom feeds from gitweb,
therefore I don't feel like I am able to ack this changes... but I do
like them.

> ---
>  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";
> +		}

A question: how should URL, which is _contents_ of a tag (instead of
as _attribute_ value), quoted (escaped)? I don't think it is specified
in RSS. I guess that using esc_url() is a safe solution.

>  	} elsif ($format eq 'atom') {
>  		print <<XML;
>  <feed xmlns="http://www.w3.org/2005/Atom">
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv2 2/6] gitweb: feed generator metadata

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> 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.

Very good idea. I haven't examined either specification, so I don't
know what conventions are used, though... and what conventions _should_
be used.

By the way, gitweb uses in HTML header the following (see
git_header_html subroutine):

  <meta name="generator" content="gitweb/$version git/$git_version$mod_perl_version"/>

which tries to follow convention how _web servers_ like Apache return
version information in the 'Server:' HTTP response header (product
tokens). Because it was used on only one place, it was not put into
separate subroutine; should it now?

> 
> 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";

I'd rather use '"' for attributes, i.e.

+		print "<generator version=\"$version/$git_version\">gitweb</generator>\n";


>  	}
>  
>  	# contents
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv2 3/6] gitweb: rss feed managingEditor

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> 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.

Hmmm... does it make sense with gitweb? Perhaps it does...

> 
> 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";

Shouldn't we esc_html($owner), just in case it is for example
"O Wner <owner@example.com>" (either via gitweb.owner or via
projects_index being text file listing projects, because I don't
see GECOS having email in it...)?

>  		if (defined $logo || defined $favicon) {
>  			# prefer the logo to the favicon, since RSS
>  			# doesn't allow both
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv2 4/6] gitweb: rss channel date

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> 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.

Perhaps it would make sense to quote RSS 2.0 standard format here
in the commit message, e.g.:

  pubDate	 The publication date for the content in the channel.
  lastBuildDate	 The last time the content of the channel changed.

> 
> 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";
> +		}

I think it is good approximation of intended meaning of those two
elements.

>  		print "<generator>gitweb v.$version/$git_version</generator>\n";
>  	} elsif ($format eq 'atom') {
>  		print <<XML;
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> 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.

First, changing %latest_date from author time to committer time affects
not only Last-Modified HTTP header, but (after this series) also
various "publication dates" in the feed contents.  But I think that for
all those committer time is better approximation of publication date
and last change date than author time.

Second, author time reflects when change (commit) was made, according
to authors (perhaps skewed) clock.  Committer time reflects when given
commit (version of a commit) was entered into repository, or to be more
exact into some clone of given project.  But there is also an issue of
when changes got into given instance of repository (given clone): that
I guess might be found by stat-ing HEAD (if it arrived by commit),
FETCH_HEAD (if it arrived by fetch or pull) and ??? (if it arrived by
push)... err... it looks like it wouldn't work in most common case,
sorry, unless we want to stat all refs and packed-refs file. But while
this date might be better for Last-Modified, I'm not sure if it is good
at all for publication date.

So committer time is better than author time, and looks like good
middle ground.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  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'});

Nice and simple. Good.

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

-- 
Jakub Narebski
Poland

Re: [PATCHv2 6/6] gitweb: check if-modified-since for feeds

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> Offering Last-modified header for feeds is only half the work, even if
> we bail out early on HEAD requests. We should also check that same date
> against If-modified-since, and bail out early with 304 Not Modified if
> that's the case.

It looks now quite nice, but I'd like to see information about
dependencies for this feature in the commit message, something like:

   This feature (terminating early with '304 Not Modified' in response
   to 'If-Modified-Since' conditional request) requires having either
   HTTP::Date module (from libwww-perl) or Time::ParseDate module.
   If neither is present gitweb falls back to earlier behaviour of not
   reacting to 'If-Modified-Since'.

Note also (although I'm not sure if it is worth mentioning in commit
message) that it doesn't save gitweb as much work as one could think,
because at this place the whole list of commits is already generated
and parsed.  What we save is cost of running git-diff-tree for each
commit (we could do better here, I think), and of course bandwidth.


I wonder if it would be possible to separate this code into subroutine,
to make it possible to have support for "cache control" conditional
requests also in other cases where it might help (for the future of
course, not in this commit).

Perhaps if we run gitweb from Apache mod_perl in compatibility mode
(ModPerl::Registry) to use Apache Perl API to respond to 
'If-Modified-Since' ($r->is_fresh or something). But that is also
just idea for the future improvement.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Here I think:

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

> ---
>  gitweb/gitweb.perl |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8c49c75..f4defb0 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);

Nitpick: either follow aligning on '=' characters, or skip it
altogether and always use one space before '=' in this fragment.

> +		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 Not Modified');
> +				return;
> +			}
> +		}

Good.

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

-- 
Jakub Narebski
Poland

Re: [PATCHv2 1/6] gitweb: channel image in rss feed

[Giuseppe Bilotta]

On Wed, Feb 4, 2009 at 11:56 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> +             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";
>> +             }
>
> A question: how should URL, which is _contents_ of a tag (instead of
> as _attribute_ value), quoted (escaped)? I don't think it is specified
> in RSS. I guess that using esc_url() is a safe solution.

Indeed, it might not be needed, but it's doesn't do any damage to
escape the URL.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 2/6] gitweb: feed generator metadata

[Giuseppe Bilotta]

On Thu, Feb 5, 2009 at 12:15 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>
>> 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.
>
> Very good idea. I haven't examined either specification, so I don't
> know what conventions are used, though... and what conventions _should_
> be used.
>
> By the way, gitweb uses in HTML header the following (see
> git_header_html subroutine):
>
>  <meta name="generator" content="gitweb/$version git/$git_version$mod_perl_version"/>
>
> which tries to follow convention how _web servers_ like Apache return
> version information in the 'Server:' HTTP response header (product
> tokens). Because it was used on only one place, it was not put into
> separate subroutine; should it now?

RSS 2.0 spec for generator @
http://cyber.law.harvard.edu/rss/rss.html#optionalChannelElements
seems to suggest that the content for the tag in RSS feeds is pretty
much free-form and we might use the same string we have in HTML pages.
Requirements for Atom (see
http://www.atomenabled.org/developers/syndication/atom-format-spec.php#element.generator
) are rather more stringent, so it needs its own code anyway.

>> 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";
>
> I'd rather use '"' for attributes, i.e.
>
> +               print "<generator version=\"$version/$git_version\">gitweb</generator>\n";

I can do that.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 3/6] gitweb: rss feed managingEditor

[Giuseppe Bilotta]

On Thu, Feb 5, 2009 at 12:19 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>
>> 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.
>
> Hmmm... does it make sense with gitweb? Perhaps it does...

I must confess I had my own doubts about this 8-D

>> 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";
>
> Shouldn't we esc_html($owner), just in case it is for example
> "O Wner <owner@example.com>" (either via gitweb.owner or via
> projects_index being text file listing projects, because I don't
> see GECOS having email in it...)?
>

Indeed, itt's probably safer to do the escaping.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 4/6] gitweb: rss channel date

[Giuseppe Bilotta]

On Thu, Feb 5, 2009 at 12:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>
>> 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.
>
> Perhaps it would make sense to quote RSS 2.0 standard format here
> in the commit message, e.g.:
>
>  pubDate        The publication date for the content in the channel.
>  lastBuildDate  The last time the content of the channel changed.

That sort of spoils the very non-technical tone of the message, but
you're probably right 8-D

>> 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";
>> +             }
>
> I think it is good approximation of intended meaning of those two
> elements.

However, this is still not perfect. While this is absolutely fine for
rss feeds that point at an explicit commit hash, rss feeds that point
to  a dynamic ref (some branch head or whatever) can get an
interesting but confusing situation:

A updates his local clone of the repo.

B gets the feed of the repo, which is not updated to include A changes.

A pushes.

B gets the feed: the dates are actually BEFORE the date he last
retrieved the feed, although they are after the date shown the last
time he retrieved the feed.

Ideally, when the rss is publishing a dynamic ref, we should as
buildDate (and as Last-modified: HTTP header!) the date when the ref
was changed (e.g. the date of the push). However, I couldn't really
think of a robust way of getting such a date, which is why I'm using
the commit date which is what comes closest.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Thu, Feb 5, 2009 at 12:38 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>
>> 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.
>
> First, changing %latest_date from author time to committer time affects
> not only Last-Modified HTTP header, but (after this series) also
> various "publication dates" in the feed contents.  But I think that for
> all those committer time is better approximation of publication date
> and last change date than author time.
>
> Second, author time reflects when change (commit) was made, according
> to authors (perhaps skewed) clock.  Committer time reflects when given
> commit (version of a commit) was entered into repository, or to be more
> exact into some clone of given project.  But there is also an issue of
> when changes got into given instance of repository (given clone): that
> I guess might be found by stat-ing HEAD (if it arrived by commit),
> FETCH_HEAD (if it arrived by fetch or pull) and ??? (if it arrived by
> push)... err... it looks like it wouldn't work in most common case,
> sorry, unless we want to stat all refs and packed-refs file. But while
> this date might be better for Last-Modified, I'm not sure if it is good
> at all for publication date.
>
> So committer time is better than author time, and looks like good
> middle ground.

Oops should have finished reading your comments before my previous
reply. The solution would be to introduce a way to determine robustly
when a branch was last _physically_ updated. Checking the
corresponding entry in refs/ would work for non-packed refs, and maybe
one would hope that if the ref got packed, it means it hasn't updated
in a long time ... but I'm not enough of an expert on git's internal
to really know about this. Suggestions?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 6/6] gitweb: check if-modified-since for feeds

[Giuseppe Bilotta]

On Thu, Feb 5, 2009 at 3:03 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>
>> Offering Last-modified header for feeds is only half the work, even if
>> we bail out early on HEAD requests. We should also check that same date
>> against If-modified-since, and bail out early with 304 Not Modified if
>> that's the case.
>
> It looks now quite nice, but I'd like to see information about
> dependencies for this feature in the commit message, something like:
>
>   This feature (terminating early with '304 Not Modified' in response
>   to 'If-Modified-Since' conditional request) requires having either
>   HTTP::Date module (from libwww-perl) or Time::ParseDate module.
>   If neither is present gitweb falls back to earlier behaviour of not
>   reacting to 'If-Modified-Since'.

Good idea.

> Note also (although I'm not sure if it is worth mentioning in commit
> message) that it doesn't save gitweb as much work as one could think,
> because at this place the whole list of commits is already generated
> and parsed.  What we save is cost of running git-diff-tree for each
> commit (we could do better here, I think), and of course bandwidth.

If it's possible to get the first commit without generating and
parsing the entire commit list, this can probably be optimized
further.

> I wonder if it would be possible to separate this code into subroutine,
> to make it possible to have support for "cache control" conditional
> requests also in other cases where it might help (for the future of
> course, not in this commit).

Yes, I thought about it too.

>
> Perhaps if we run gitweb from Apache mod_perl in compatibility mode
> (ModPerl::Registry) to use Apache Perl API to respond to
> 'If-Modified-Since' ($r->is_fresh or something). But that is also
> just idea for the future improvement.

Also, it's way beyond my current knowledge of Apache, CGI and Perl 8-D

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 2/6] gitweb: feed generator metadata

[Jakub Narebski]

On Fri, 6 Feb 2009, Giuseppe Bilotta wrote:
> On Thu, Feb 5, 2009 at 12:15 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>>
>>> 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.
>>
>> Very good idea. I haven't examined either specification, so I don't
>> know what conventions are used, though... and what conventions _should_
>> be used.
>>
>> By the way, gitweb uses in HTML header the following (see
>> git_header_html subroutine):
>>
>>  <meta name="generator" content="gitweb/$version git/$git_version$mod_perl_version"/>
>>
>> which tries to follow convention how _web servers_ like Apache return
>> version information in the 'Server:' HTTP response header (product
>> tokens). Because it was used on only one place, it was not put into
>> separate subroutine; should it now?
> 
> RSS 2.0 spec for generator @
> http://cyber.law.harvard.edu/rss/rss.html#optionalChannelElements
> seems to suggest that the content for the tag in RSS feeds is pretty
> much free-form and we might use the same string we have in HTML pages.
> Requirements for Atom (see
> http://www.atomenabled.org/developers/syndication/atom-format-spec.php#element.generator
> ) are rather more stringent, so it needs its own code anyway.

I don't see there in given above Atom spec _how_ 'version' attribute
should be formatted. Here is relevant excerpt from mentioned page:

  4.2.4 The "atom:generator" Element

  The "atom:generator" element's content identifies the agent used
  to generate a feed, for debugging and other purposes.
  [...]

  The content of this element, when present, MUST be a string that
  is a human-readable name for the generating agent.
  [...]

  The atom:generator element MAY have a "version" attribute that
  indicates the version of the generating agent.

So why not use something like:

  <generator version="gitweb/$version git/$git_version$mod_perl_version">
  gitweb v$version</generator>
 
for Atom? Perhaps with 'Server:'-like version generation refactored
to its own subroutine?

>>> +             print "<generator>gitweb v.$version/$git_version</generator>\n";
>>>       } elsif ($format eq 'atom') {

>>> +             print "<generator version='$version/$git_version'>gitweb</generator>\n";

-- 
Jakub Narebski
Poland

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Jakub Narebski]

On Fri, 06 Feb 2009, Giuseppe Bilotta wrote:
> On Thu, Feb 5, 2009 at 12:38 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>>
>>> 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.

[...]
>> Second, author time reflects when change (commit) was made, according
>> to authors (perhaps skewed) clock.  Committer time reflects when given
>> commit (version of a commit) was entered into repository, or to be more
>> exact into some clone of given project.  But there is also an issue of
>> when changes got into given instance of repository (given clone): that
>> I guess might be found by stat-ing HEAD (if it arrived by commit),
>> FETCH_HEAD (if it arrived by fetch or pull) and ??? (if it arrived by
>> push)... err... it looks like it wouldn't work in most common case,
>> sorry, unless we want to stat all refs and packed-refs file. But while
>> this date might be better for Last-Modified, I'm not sure if it is good
>> at all for publication date.
>>
>> So committer time is better than author time, and looks like good
>> middle ground.
> 
> Oops should have finished reading your comments before my previous
> reply. The solution would be to introduce a way to determine robustly
> when a branch was last _physically_ updated. Checking the
> corresponding entry in refs/ would work for non-packed refs, and maybe
> one would hope that if the ref got packed, it means it hasn't updated
> in a long time ... but I'm not enough of an expert on git's internal
> to really know about this. Suggestions?

The point is I am not that sure if the date branch was _physically_
updated (it was updated in given clone of repository) is a good
choice for publish date in feed (RSS or Atom), whether it is good
choice for Last-Modified (and If-Modified-Since), and whether publish
date can be different from Last-Modified.  

Using committime doesn't ensure that it is monotonical (but it is
highly probable, much more than authortime), but it is the same for
every clone of repository, and for every gitweb installation that
hosts given repository.

Using update time does ensure that it is monotonical, and it
wouldn't ever be earlier than the time repository started to be
available, but it changes from clone to clone.

-- 
Jakub Narebski
Poland

Re: [PATCHv2 0/6] gitweb: feed metadata enhancements

[Jakub Narebski]

On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

> This second revision adds two patches to improve client-side rss
> caching: the last-modified header we output is based on commit rather
> than creation time, and we now act on if-modified-since request headers.
> 
> The last patch requires either HTTP::Date (from libwww-perl) or
> Time::ParseDate


Below there is summary of my comments.
 
> Giuseppe Bilotta (6):
>   gitweb: channel image in rss feed

O.K. esc_url is perhaps unnecessary, but safe

>   gitweb: feed generator metadata

Better to use the same convention for version info as in meta.generator
in HTML header.  Use '"' for attribute values.

>   gitweb: rss feed managingEditor

Need to esc_html (well, esc_xml ;-) $owner, because it might be set to
"O Wner <owner@example.com>".

>   gitweb: rss channel date

O.K. (with question if it would be better to quote relevant fragment
of RSS 2.0 standard with respect to meaning of those date elements).

>   gitweb: last-modified time should be commiter, not author

O.K. perhaps with clarification that it affects not only Last-Modified,
but also just introduced (and used earlier) "publish" dates for feed.

>   gitweb: check if-modified-since for feeds

O.K., with proposed addition to commit message.

> 
>  gitweb/gitweb.perl |   40 ++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 38 insertions(+), 2 deletions(-)
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Fri, Feb 6, 2009 at 10:12 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Using update time does ensure that it is monotonical, and it
> wouldn't ever be earlier than the time repository started to be
> available, but it changes from clone to clone.

I don't think it changing from clone to clone is a problem. Monotony
and the ability to properly catch when the underlying data really
changed are much more important factors. After all, there is no
_requirement_ that Last-Modified be used for If-modified-since headers
(although it is recommented, and clients that don't follow the
recommendation are warned to be aware of all possible odd problems
arising from not using it).

The real question is: _how_ do you get the (branch) update time?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 0/6] gitweb: feed metadata enhancements

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
>
>> This second revision adds two patches to improve client-side rss
>> caching: the last-modified header we output is based on commit rather
>> than creation time, and we now act on if-modified-since request headers.
>> 
>> The last patch requires either HTTP::Date (from libwww-perl) or
>> Time::ParseDate
>
>
> Below there is summary of my comments.

Thanks.

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Deskin Miller]

On Fri, Feb 6, 2009 at 18:00, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
>
> The real question is: _how_ do you get the (branch) update time?
>

Sorry the topic's cold, but...

git reflog?

Seems like one could find the oldest time the commit appears in the
reflog, for the branch one is interested in.  You can use the commit
time to limit the search through the reflog, but there would be clock
skew concerns.

Deskin Miller

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Wed, Feb 11, 2009 at 4:10 AM, Deskin Miller <deskinm@gmail.com> wrote:
> On Fri, Feb 6, 2009 at 18:00, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>>
>> The real question is: _how_ do you get the (branch) update time?
>>
>
> Sorry the topic's cold, but...
>
> git reflog?
>
> Seems like one could find the oldest time the commit appears in the
> reflog, for the branch one is interested in.  You can use the commit
> time to limit the search through the reflog, but there would be clock
> skew concerns.

Bingo! Thanks a lot

oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
--since="two days ago" master | cat
7324b32... master@{0}: push
e2dc08d... master@{1}: push
oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
--since="yesterday" master | cat
oblomov@rbot ~ $

I'll try to work it in the next review for this patchset.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Jakub Narebski]

On Wed, 11 Feb 2009, Giuseppe Bilotta wrote:
> On Wed, Feb 11, 2009 at 4:10 AM, Deskin Miller <deskinm@gmail.com> wrote:
>> On Fri, Feb 6, 2009 at 18:00, Giuseppe Bilotta
>> <giuseppe.bilotta@gmail.com> wrote:
>>>
>>> The real question is: _how_ do you get the (branch) update time?
>>>
>>
>> Sorry the topic's cold, but...
>>
>> git reflog?
>>
>> Seems like one could find the oldest time the commit appears in the
>> reflog, for the branch one is interested in.  You can use the commit
>> time to limit the search through the reflog, but there would be clock
>> skew concerns.
> 
> Bingo! Thanks a lot
> 
> oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
> --since="two days ago" master | cat
> 7324b32... master@{0}: push
> e2dc08d... master@{1}: push
> oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
> --since="yesterday" master | cat
> oblomov@rbot ~ $
> 
> I'll try to work it in the next review for this patchset.

Assuming that you have reflog enabled (yes, it is default now)...
So you would have to provide fallback in the case there is no reflog.

BTW. "git reflog" is porcelain; it would be better to parse reflog
directly, I think.
-- 
Jakub Narebski
Poland

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Wed, Feb 11, 2009 at 10:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 11 Feb 2009, Giuseppe Bilotta wrote:
>> On Wed, Feb 11, 2009 at 4:10 AM, Deskin Miller <deskinm@gmail.com> wrote:
>>> git reflog?
>>>
>>> Seems like one could find the oldest time the commit appears in the
>>> reflog, for the branch one is interested in.  You can use the commit
>>> time to limit the search through the reflog, but there would be clock
>>> skew concerns.
>>
>> Bingo! Thanks a lot
>>
>> oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
>> --since="two days ago" master | cat
>> 7324b32... master@{0}: push
>> e2dc08d... master@{1}: push
>> oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
>> --since="yesterday" master | cat
>> oblomov@rbot ~ $
>>
>> I'll try to work it in the next review for this patchset.
>
> Assuming that you have reflog enabled (yes, it is default now)...
> So you would have to provide fallback in the case there is no reflog.
>
> BTW. "git reflog" is porcelain; it would be better to parse reflog
> directly, I think.

Does disabling reflog remove old reflogs? IOW, can I check if reflog
is enabled just by opening the reflog file and assuming reflog isn't
enabled if it's not there? Falling back to the commit date would still
work decently.

Since we're only interested in the last reflog date, what we can do is
to read the last line and get the unix time which is held two places
before the tab separating the metadata from the log message. Correct?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Deskin Miller]

On Wed, Feb 11, 2009 at 04:54, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> On Wed, Feb 11, 2009 at 10:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Assuming that you have reflog enabled (yes, it is default now)...
>> So you would have to provide fallback in the case there is no reflog.
>>
>> BTW. "git reflog" is porcelain; it would be better to parse reflog
>> directly, I think.

Agreed on a fallback being necessary; as for parsing the log directly,
that seems fine, but I might be inclined to use git log -g --date=foo
and then parse that.  But that's still using porcelain at some level.

In my copious free time I'm working on a patch to expose reflog
information through git log -g using new --pretty=format: specifiers,
but even my initial attempts to carry reflog information through to
the point in the code where it could be output broke some testcases;
it'll be a long while before I grok what's going on enough to make it
work.  But I digress.

> Does disabling reflog remove old reflogs? IOW, can I check if reflog
> is enabled just by opening the reflog file and assuming reflog isn't
> enabled if it's not there? Falling back to the commit date would still
> work decently.

This seems easy enough to test.  If switching core.logAllRefUpdates
from true -> false doesn't remove the log, though (and why would
git-config delete reflog files?), you could have an old log which just
hasn't been updated for some time, where the branch head might not
appear at all.  But that's just idle speculation without testing the
actual behaviour.

> Since we're only interested in the last reflog date, what we can do is
> to read the last line and get the unix time which is held two places
> before the tab separating the metadata from the log message. Correct?

Looking at the log format I'd say that seems fine.

Glad to see I was thinking clearly, suggesting reflog :)

Deskin Miller

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Jakub Narebski]

On Wed, 11 Feb 2009, Giuseppe Bilotta wrote:
> On Wed, Feb 11, 2009 at 10:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 11 Feb 2009, Giuseppe Bilotta wrote:
>>> On Wed, Feb 11, 2009 at 4:10 AM, Deskin Miller <deskinm@gmail.com> wrote:
>>>>
>>>> git reflog?
>>>>
>>>> Seems like one could find the oldest time the commit appears in the
>>>> reflog, for the branch one is interested in.  You can use the commit
>>>> time to limit the search through the reflog, but there would be clock
>>>> skew concerns.
>>>
>>> Bingo! Thanks a lot
>>>
>>> oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
>>> --since="two days ago" master | cat
>>> 7324b32... master@{0}: push
>>> e2dc08d... master@{1}: push
>>> oblomov@rbot ~ $ GIT_DIR=/var/git/rbot.git/ git reflog show
>>> --since="yesterday" master | cat
>>> oblomov@rbot ~ $
>>>
>>> I'll try to work it in the next review for this patchset.
>>
>> Assuming that you have reflog enabled (yes, it is default now)...
>> So you would have to provide fallback in the case there is no reflog.
>>
>> BTW. "git reflog" is porcelain; it would be better to parse reflog
>> directly, I think.
> 
> Does disabling reflog remove old reflogs? IOW, can I check if reflog
> is enabled just by opening the reflog file and assuming reflog isn't
> enabled if it's not there? Falling back to the commit date would still
> work decently.

You can disable reflog by setting core.logAllRefUpdates to false...
which of course do not remove reflog.  But checking for this config
variable in gitweb shouldn't be too hard, although you would need
to somehow change assumption that we are interested only in ^gitweb\.
section of config.

There is other side of this issue: reflog is expired, so you can
have empty reflog if branch was updated long time ago.

>
> Since we're only interested in the last reflog date, what we can do is
> to read the last line and get the unix time which is held two places
> before the tab separating the metadata from the log message. Correct?

Almost correct.  Some tools (old StGit for example) didn't provide
reflog message, and with empty message they forgot to append TAB
separator.  So you would have to provide for legacy

  <old sha-1> <new-sha-1> <committer> <timestamp> <timezone>

instead of correct (for empty reflog message)

  <old sha-1> <new-sha-1> <committer> <timestamp> <timezone> TAB


P.S. It is a pity that due to packed refs stat-ing branch tip file
$GIT_DIR/refs/heads/<branch> is not enough...
-- 
Jakub Narebski
Poland

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Thu, Feb 12, 2009 at 10:07 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 11 Feb 2009, Giuseppe Bilotta wrote:
>> On Wed, Feb 11, 2009 at 10:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:

>>> Assuming that you have reflog enabled (yes, it is default now)...
>>> So you would have to provide fallback in the case there is no reflog.
>>>
>>> BTW. "git reflog" is porcelain; it would be better to parse reflog
>>> directly, I think.
>>
>> Does disabling reflog remove old reflogs? IOW, can I check if reflog
>> is enabled just by opening the reflog file and assuming reflog isn't
>> enabled if it's not there? Falling back to the commit date would still
>> work decently.
>
> You can disable reflog by setting core.logAllRefUpdates to false...
> which of course do not remove reflog.  But checking for this config
> variable in gitweb shouldn't be too hard, although you would need
> to somehow change assumption that we are interested only in ^gitweb\.
> section of config.
>
> There is other side of this issue: reflog is expired, so you can
> have empty reflog if branch was updated long time ago.

I'm thinking that what I could do (that should always work as
expected) is to consider as 'last modified' the most recent date
between the commit date and the reflog date (if the reflog is found).
This basically implements an 'automatic' fallback for
disabled/obsolete/expired reflog to commit date. (Of course, assuming
the last commit doesn't suffer from a ridicously long clockskew in the
future.)

>> Since we're only interested in the last reflog date, what we can do is
>> to read the last line and get the unix time which is held two places
>> before the tab separating the metadata from the log message. Correct?
>
> Almost correct.  Some tools (old StGit for example) didn't provide
> reflog message, and with empty message they forgot to append TAB
> separator.  So you would have to provide for legacy
>
>  <old sha-1> <new-sha-1> <committer> <timestamp> <timezone>
>
> instead of correct (for empty reflog message)
>
>  <old sha-1> <new-sha-1> <committer> <timestamp> <timezone> TAB

That's the easy part: split at TAB, get the first array entry, split
at spaces, get the last two. But thanks for making me aware of the
possibly missing TAB

> P.S. It is a pity that due to packed refs stat-ing branch tip file
> $GIT_DIR/refs/heads/<branch> is not enough...

Yeah, that's what the other rbot developer suggested initially (then I
found out that cloning the repo was enough to NOT have branch heads).

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Jakub Narebski]

On Thu, 12 Feb 2009, Giuseppe Bilotta wrote:
> On Thu, Feb 12, 2009 at 10:07 AM, Jakub Narebski <jnareb@gmail.com> 
> wrote: 

>> You can disable reflog by setting core.logAllRefUpdates to false...
>> which of course do not remove reflog.  But checking for this config
>> variable in gitweb shouldn't be too hard, although you would need
>> to somehow change assumption that we are interested only in ^gitweb\.
>> section of config.
>>
>> There is other side of this issue: reflog is expired, so you can
>> have empty reflog if branch was updated long time ago.
> 
> I'm thinking that what I could do (that should always work as
> expected) is to consider as 'last modified' the most recent date
> between the commit date and the reflog date (if the reflog is found).
> This basically implements an 'automatic' fallback for
> disabled/obsolete/expired reflog to commit date. (Of course, assuming
> the last commit doesn't suffer from a ridicously long clockskew in the
> future.)

That is, I think, a very good idea. It also covers situation where we
use non-head 'h'/'hb', for example explicit SHA-1 (not that it makes
sense, but...) 
 
Well, that beside my little doubt whether using publish date is a good
idea or not...


P.S. What would you do for explicit and implicit HEAD? HEAD reflog,
or 'current branch' reflog?
-- 
Jakub Narebski
Poland

Re: [PATCHv2 5/6] gitweb: last-modified time should be commiter, not author

[Giuseppe Bilotta]

On Thu, Feb 12, 2009 at 11:11 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>
> Well, that beside my little doubt whether using publish date is a good
> idea or not...
>

I'm open to suggestion for better ideas 8-)

> P.S. What would you do for explicit and implicit HEAD? HEAD reflog,
> or 'current branch' reflog?

I would say the HEAD reflog.


-- 
Giuseppe "Oblomov" Bilotta