Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 21, 2012, 13:19 UTC
- Message-ID
- <m3sjh2ay6j.fsf@localhost.localdomain>
- In-Reply-To
- <20120321121126.GA27660@odin.tremily.us>
By the way, it is custom on this mailing list to usually Cc (send a copy) to all people participating in discussion, and not only to git mailing list.
"W. Trevor King" <wking@drexel.edu> writes:
> Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb
Perhaps a better title would be:
gitweb: Refactor If-Modified-Since handling, support in snapshot
to mention all that thispatch does. Though trouble with coming up with a short but fairly complete one-line summary might mean that this patch would be better split in two: refactoring and adding support for If-Modified-Since to snapshots.
Show 5 quoted lines
> The current gitweb only generates Last-Modified and handles > If-Modified-Since headers for the git_feed action. This patch breaks > the Last-Modified and If-Modified-Since handling code out from > git_feed into a new function die_if_unmodified. This makes the code > easy to reuse for other actions
O.K.
> where it is appropriate
^^^^^^^^^^^^^^^^^^^^^^^
This doesn't add any information. I think it the commit message would be better if you either remove this, or expand (in a separate paragraph) where support for If-Modified-Since might make sense, and where it could not.
> and adds the > code to do that to git_snapshot.
This would read better as a separate paragraph.
Show 5 quoted lines
> Signed-off-by: W Trevor King <wking@drexel.edu> > --- > Changed since v2: > - Shorter title. > - Fixed modified_since -> die_if_unmodified in commit message.
Nice.
BTW. what happened to diffstat?
Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)? We could use test_tick() and $test_tick for that (or extract formatted committer date from a commit).
Show 29 quoted lines
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a8b5fad..b944351 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -7003,6 +7003,28 @@ sub snapshot_name {
> return wantarray ? ($name, $name) : $name;
> }
>
> +sub die_if_unmodified {
> + my ($latest_epoch) = @_;
> + our $cgi;
> +
> + 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) {
> + my %latest_date = parse_date($latest_epoch);
> + print $cgi->header(
> + -last_modified => $latest_date{'rfc2822'},
> + -status => '304 Not Modified');
> + goto DONE_GITWEB;
> + }
> + }
> +}O.K.
die_if_unmodified() is good enough name, though I wonder if we could come up with a better name....
Show 12 quoted lines
> +
> sub git_snapshot {
> my $format = $input_params{'snapshot_format'};
> if (!@snapshot_fmts) {
> @@ -7029,6 +7051,10 @@ sub git_snapshot {
>
> my ($name, $prefix) = snapshot_name($project, $hash);
> my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
> +
> + my %co = parse_commit($hash) or die_error(404, "Unknown commit object");
> + die_if_unmodified($co{'committer_epoch'});
> +That unexplainably changes behavior of 'snapshot' action. Before we would accept snapshot of a tree given by its SHA-1, now we do not.
This might be a good idea from a security point of view wrt. leaking information (c.f. "git archive --remote" behavior), but it at least needs to be explained in commit message, and perhaps even in a comment above this line.
Alternative solution would be to skip If-Modified-Since handling if we request snapshot by tree id:
+
+ my %co = parse_commit($hash);
+ die_if_unmodified($co{'committer_epoch'}) if %co;
+Show 10 quoted lines
> @@ -7038,9 +7064,11 @@ sub git_snapshot {
> }
>
> $filename =~ s/(["\\])/\\$1/g;
> + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
> print $cgi->header(
> -type => $known_snapshot_formats{$format}{'type'},
> -content_disposition => 'inline; filename="' . $filename . '"',
> + -last_modified => $latest_date{'rfc2822'},
> -status => '200 OK');Of course this would need to be updated too.
Show 27 quoted lines
> open my $fd, "-|", $cmd
> @@ -7820,24 +7848,8 @@ sub git_feed {
> if (defined($commitlist[0])) {
> %latest_commit = %{$commitlist[0]};
> my $latest_epoch = $latest_commit{'committer_epoch'};
> + die_if_unmodified($latest_epoch);
> %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
> - 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',Nice.
-- Jakub Narebski