Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 20, 2012, 11:55 UTC
- Message-ID
- <m37gyf4hc3.fsf@localhost.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" <wking@drexel.edu> writes:
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 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.
Show 16 quoted lines
> Signed-off-by: W Trevor King <wking@drexel.edu>
>
> ---
> 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.
Show 12 quoted lines
> + 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)
Show 22 quoted lines
> + -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.
Show 15 quoted lines
> +
> 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.
Show 5 quoted lines
> 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?
Show 24 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;
> - }
> + if (! modified_since($content_type, $latest_epoch,
> + $latest_commit{'comitter_tz'})) {
> + return;
> }
> print $cgi->header(
> -type => $content_type,
> --
> 1.7.3.4-- Jakub Narebski