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

[Jakub Narebski <jnareb@gmail.com>]

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