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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Jan 26, 2009, 11:25 UTC
Message-ID: <cb7bb73a0901260325j74f07f3doee3aad9b0300ee88@mail.gmail.com>
In-Reply-To: <200901260318.05301.jnareb@gmail.com>

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.

Show 12 quoted lines

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

Show 18 quoted lines

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

Show 11 quoted lines

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