# [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. 3 messages from 2012-03-15 to 2012-03-20. Participants: W. Trevor King, Jakub Narebski. Thread: https://gitlist.dev/t/29955 ## W. Trevor King, 2012-03-15 07:54 Subject: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. Message-ID: <20120315075407.GA18246@odin.tremily.us> URL: https://gitlist.dev/e/20120315075407.GA18246%40odin.tremily.us ``` 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 modified_since. This makes the code easy to reuse for other actions where it is appropriate, and I've added the code to do that in git_snapshot. Signed-off-by: W Trevor King --- gitweb/gitweb.perl | 54 ++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 38 insertions(+), 16 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a8b5fad..96a13e7 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,31 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub modified_since { + my ($content_type, $latest_epoch, $timezone) = @_; + 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, $timezone); + print $cgi->header( + -type => $content_type, + -charset => 'utf-8', + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + return 0; + } + } + return 1; +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -7029,6 +7054,14 @@ 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"); + if (! modified_since($known_snapshot_formats{$format}{'type'}, + $co{'committer_epoch'}, + $co{'committer_tz'})) { + return; + } + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7038,9 +7071,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'); open my $fd, "-|", $cmd @@ -7821,22 +7856,9 @@ sub git_feed { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_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; - } + if (! modified_since($content_type, $latest_epoch, + $latest_commit{'comitter_tz'})) { + return; } print $cgi->header( -type => $content_type, -- 1.7.3.4 ``` ## Jakub Narebski, 2012-03-20 11:55 Subject: Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. Message-ID: URL: https://gitlist.dev/e/m37gyf4hc3.fsf%40localhost.localdomain In-Reply-To: <20120315075407.GA18246@odin.tremily.us> ``` I'm sorry I am only now answering, but unfortunately this patch has fallen thought crack. "W. Trevor King" writes: > 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 modified_since. This makes the code easy > to reuse for other actions where it is appropriate, That is a very good idea. Of course adding support for If-Modified-Since has sense only if we can calculate modification time without doing all the work (and calculating modification time separately doesn;t add to work to be done), or at least we are to send large amount of data so even if we cannot save CPU time and I/O hit we can save network bandwidth. > and I've added the code to do that in git_snapshot. Nitpick about grammar: here you change from impersonal to personal form in commit message, unnecessarily I think. > Signed-off-by: W Trevor King > > --- > gitweb/gitweb.perl | 54 ++++++++++++++++++++++++++++++++++++--------------- > 1 files changed, 38 insertions(+), 16 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index a8b5fad..96a13e7 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -7003,6 +7003,31 @@ sub snapshot_name { > return wantarray ? ($name, $name) : $name; > } > > +sub modified_since { > + my ($content_type, $latest_epoch, $timezone) = @_; Passing $content_type to function named modified_since() looks quite strange to me. > + 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, $timezone); Shouldn't we pass \%latest_date (or rather \%modification_date to be more generic)? Ah, I see that parse_date() subroutine does not store original epoch not original timezone. By the way, for $latest_date{'rfc2822'} we don't need $timezone argument, and I think we should not pass it to generic modified_since() function (or whatever it will be called). > + print $cgi->header( > + -type => $content_type, > + -charset => 'utf-8', Do we need -charset here? There is no HTTP body, and -charset => 'utf-8' is incorrect for snapshots (with e.g. 'application/x-zip' $content_type) > + -last_modified => $latest_date{'rfc2822'}, > + -status => '304 Not Modified'); > + return 0; > + } > + } > + return 1; > +} > + > sub git_snapshot { > my $format = $input_params{'snapshot_format'}; > if (!@snapshot_fmts) { > @@ -7029,6 +7054,14 @@ 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"); > + if (! modified_since($known_snapshot_formats{$format}{'type'}, > + $co{'committer_epoch'}, > + $co{'committer_tz'})) { > + return; > + } For a function named modified_since(), it does too many things. I think a better idea would be to make this function/subroutine (named e.g. if_modified_since() or something like that) to work more like die_error() -- it should simply end request instead of having caller to use complcated calling convention, and care about eraly termination by itself. This means turning modified_since() into subroutine (and renaming it), and ending it with goto DONE_GITWEB; or goto DONE_REQUEST; depending on the code base you are applying to. > + > my $cmd = quote_command( > git_cmd(), 'archive', > "--format=$known_snapshot_formats{$format}{'format'}", > @@ -7038,9 +7071,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'); > Have you run gitweb tests? This simply has no way of working, as %co variable you use here is not defined anywhere in git_snapshot() subroutine. What if there is no commit, for example if we are rewuesting snapshot of a tree by its SHA-1? We need to be able to deal with sich situation, if only by not handling Last-Modified / If-Modified-Since. > open my $fd, "-|", $cmd > @@ -7821,22 +7856,9 @@ sub git_feed { > %latest_commit = %{$commitlist[0]}; > my $latest_epoch = $latest_commit{'committer_epoch'}; > %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); Do we use %latest_date beside handling If-Modified-Since? Is there a need for separate $latest_epoch and %;atest_date variables? > - 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; > - } > + if (! modified_since($content_type, $latest_epoch, > + $latest_commit{'comitter_tz'})) { > + return; > } > print $cgi->header( > -type => $content_type, > -- > 1.7.3.4 -- Jakub Narebski ``` ## W. Trevor King, 2012-03-20 16:40 Subject: [PATCH v2] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. Message-ID: <20120320164056.GA23093@odin.tremily.us> URL: https://gitlist.dev/e/20120320164056.GA23093%40odin.tremily.us In-Reply-To: ``` 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 modified_since. This makes the code easy to reuse for other actions where it is appropriate and adds the code to do that to git_snapshot. Signed-off-by: W Trevor King --- Thanks for the feedback, Jakub. I believe I've addressed your points in this revised patch. Changed since v1: - Fixed impersonal/personal commit message - Simplified modified_since() -> die_if_unmodified(). Now it only takes a $latest_epoch argument and we set fewer headers. It also jump straight to DONE_GITWEB if the page is unmodified. - Ran tests (which passed this time). Somehow I missed t/README earlier. I'd add tests for my patch, but I'd have to have commit timestamps from the test repository, and I don't understand the test framework well enough to know how to do that cleanly. Things that haven't changed: > > @@ -7038,9 +7071,11 @@ sub git_snapshot { > > } > > > > $filename =~ s/(["\\])/\\$1/g; > > + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > ... > This simply has no way of working, as %co variable you use here is > not defined anywhere in git_snapshot() subroutine. I defined it earlier: > > @@ -7029,6 +7054,14 @@ 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"); > > + if (! modified_since($known_snapshot_formats{$format}{'type'}, > > + $co{'committer_epoch'}, While modified_since() (now die_if_unmodified()) handles the 304 case, we also want to set Last-Modified for 200s. That's what the latter code is trying to do. > What if there is no commit, for example if we are rewuesting snapshot > of a tree by its SHA-1? We need to be able to deal with sich > situation, if only by not handling Last-Modified / If-Modified-Since. The parse_commit() call should handle these cases. 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; + } + } +} + 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'}); + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -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'); 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', -- 1.7.3.4 ```