From: W. Trevor King <wking@drexel.edu>
Date: Wed, 28 Mar 2012 18:37:12 GMT
Subject: Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().
Message-ID: <20120328183712.GA5992@odin.tremily.us>
In-Reply-To: <201203282011.32148.jnareb@gmail.com>

On Wed, Mar 28, 2012 at 07:11:31PM +0100, Jakub Narebski wrote:
> W. Trevor King wrote:
> >  	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');

Grr.  Thanks.  I'm getting lots of rebase practice on this patch set,
but I'm still missing things…

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

Yes, but I can switch to `gitweb.headers` if you'd like.  Should I
adjust all the header tests in t9501 to use `gitweb.headers` and `grep
-i`?  It should probably be a separate patch for the tests that
existed before my i-m-s additions.

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy