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

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 28, 2012, 18:11 UTC
Message-ID: <201203282011.32148.jnareb@gmail.com>
In-Reply-To: <81004880a876bb9b9f607129956363d3167bff72.1332956550.git.wking@drexel.edu>

W. Trevor King wrote:

Show 14 quoted lines

> @@ -7060,9 +7064,15 @@ sub git_snapshot {
>  	}
>  
>  	$filename =~ s/(["\\])/\\$1/g;
> +	my %latest_date;
> +	if (%co) {
> +		%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 => (%co ? $latest_date{'rfc2822'} : ()),
>  		-status => '200 OK');

I'm sorry to be bearer of bad news, but this is still incorrect. It should be:

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

(The "fat comma" => operator has relatively low priority, lower than
ternary conditional operator ?:)

Show 9 quoted lines

> +test_expect_success 'modification: tree snapshot' '
> +	ID=`git rev-parse --verify HEAD^{tree}` &&
> +	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
> +	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
> +	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
> +	grep "Status: 200 OK" gitweb.output &&
> +	! grep "Last-Modified" gitweb.output
> +'
> +test_debug 'cat gitweb.headers'

And it was not caught by test because CGI.pm can output the last modified header as "Last-modified" (RFC 2616, sec 4.2 states "Field names are case-insensitive"), so the last check should be

  +	! grep -i "Last-Modified" gitweb.output

Hmmm... why we use gitweb.output and not gitweb.headers? Is it consistency with earlier tests?

-- 
Jakub Narebski
Poland