Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

[Jakub Narebski <jnareb@gmail.com>]

I'm sorry I am only now answering, but unfortunately this patch has
fallen thought crack.

"W. Trevor King" <wking@drexel.edu> 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 <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.

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