Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Jakub Narebski <jnareb@gmail.com>]

On Mon, 26 Mar 2012, W. Trevor King wrote:

> Because snapshots can be large, you can save some bandwidth by
> supporting caching via If-Modified-Since.  This patch adds support for
> the i-m-s request to git_snapshot() if the requested hash is a commit.

"if the requested hash is a commit" means here "if we request snapshot
of a commit", isn't it?

> Requests for snapshots of tree-ishes, which lack well defined
> timestamps, are still handled as they were before.

s/tree-ishes/trees/.  Commit is tree-ish but not a tree; it is tree
that lacks timestamp that commit has.

> 
> Signed-off-by: W Trevor King <wking@drexel.edu>
> ---
>  gitweb/gitweb.perl                       |   21 +++++++++++++++---
>  t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 229f3da..be9ad5d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -7051,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);
> +	exit_if_unmodified_since($co{'committer_epoch'}) if %co;
> +

O.K.  Nice.

>  	my $cmd = quote_command(
>  		git_cmd(), 'archive',
>  		"--format=$known_snapshot_formats{$format}{'format'}",
> @@ -7060,10 +7064,19 @@ sub git_snapshot {
>  	}
>  
>  	$filename =~ s/(["\\])/\\$1/g;
> -	print $cgi->header(
> -		-type => $known_snapshot_formats{$format}{'type'},
> -		-content_disposition => 'inline; filename="' . $filename . '"',
> -		-status => '200 OK');
> +	if (%co) {
> +		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');
> +	} else {
> +		print $cgi->header(
> +			-type => $known_snapshot_formats{$format}{'type'},
> +			-content_disposition => 'inline; filename="' . $filename . '"',
> +			-status => '200 OK');
> +	}

This can be written shorter, and with less code repetition using ?: 
conditional operator:

> +	print $cgi->header(
> +		-type => $known_snapshot_formats{$format}{'type'},
> +		-content_disposition => 'inline; filename="' . $filename . '"',
> +		 (%co ? -last_modified => $latest_date{'rfc2822'} : ()),
> +		-status => '200 OK');

...or something like that.

>  
>  	open my $fd, "-|", $cmd
>  		or die_error(500, "Execute git-archive failed");
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index 0e49f29..38e90bd 100755
> --- a/t/t9501-gitweb-standalone-http-status.sh
> +++ b/t/t9501-gitweb-standalone-http-status.sh

Same comments as for 2/3.

-- 
Jakub Narebski
Poland