Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 28 Mar 2012, W. Trevor King wrote:
> On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote:
> > On Mon, 26 Mar 2012, W. Trevor King wrote:
> > > +# ----------------------------------------------------------------------
> > > +# modification times (Last-Modified and If-Modified-Since)
> > > +
> > > +test_expect_success 'modification: feed last-modified' '
> > > +	gitweb_run "p=.git;a=atom;h=master" &&
> > > +	grep "Status: 200 OK" gitweb.output &&
> > > +	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
> > > +'
> > 
> > All right.
> > 
> > What's that date from?  Wouldn't it be better to read it from commit
> > object with `git show -s --pretty=%cD HEAD` or postprocessed from
> > '%ct' timestamp?
> 
> That's the date set by the first `test_tick`, which is hardcoded in
> `test-lib-functions.sh`.  Extracting the date dynamically seems
> unnecessary, since I can't imagine anyone changing the `test_tick`
> date.  

Ah, it's all right then.  I should have checked the test_tick function.

That of course assuming that nobody would add test_tick earlier, but
if he/she does, he/she can deal with fallout...

> It's easy enough to do if you think it is appropriate though… 

No, it is not needed.

> > > +test_debug 'cat gitweb.headers'
> > > +
> > > +test_expect_success 'modification: feed if-modified-since (modified)' '
> > > +	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
> > > +	gitweb_run "p=.git;a=atom;h=master" &&
> > > +	unset HTTP_IF_MODIFIED_SINCE &&
> > > +	grep "Status: 200 OK" gitweb.output
> > > +'
> > 
> > I think it *might* be better solution to use test_when_finished:
> > 
> >   +test_expect_success 'modification: feed if-modified-since (modified)' '
> >   +	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=atom;h=master" &&
> >   +	grep "Status: 200 OK" gitweb.output
> >   +'
> > 
> > I don't think we need sane_unset here.
> 
> Good point.  Sloppy me not reading `t/README` thoroughly enough ;).

-- 
Jakub Narebski
Poland