Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:53:20
I'm sorry I am only now answering, but unfortunately this patch has fallen thought crack. "W. Trevor King" [off-list ref] 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.
quoted hunk ↗ jump to hunk
Signed-off-by: W Trevor King <redacted> --- 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)
quoted hunk ↗ jump to hunk
+ -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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
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