Re: [PATCHv2 6/6] gitweb: check if-modified-since for feeds
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Feb 5, 2009, 02:03 UTC
- Message-ID
- <200902050303.43356.jnareb@gmail.com>
- In-Reply-To
- <1232970616-21167-7-git-send-email-giuseppe.bilotta@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>
Show 15 quoted lines
> ---
> 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.
Show 17 quoted lines
> + 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.
Show 7 quoted lines
> print $cgi->header( > -type => $content_type, > -charset => 'utf-8', > -- > 1.5.6.5 > >
-- Jakub Narebski Poland