On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:

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

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

Here I think:

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

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

quoted text
> +		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.

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

-- 
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html