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

On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote:

It might make more sense to call the helper *after* stuffing %latest_date
and pass the $latest_date{'rfc2822'} as parameter to the new helper
function, so that the helper function do not have to call parse_date().

I did something closer to your suggestion in my v1 patch, but

On Tue, Mar 20, 2012 at 04:55:45AM -0700, Jakub Narebski wrote:

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).

…

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.

Only passing in the epoch.  We could reduce computation at the expense
of complication by passing in both the epoch and a formatted time
string, but after Jakub's suggestions, I felt like a simpler interface
was the better approach.  I don't feel particularly committed to
either way, so just tell me which you'd like best ;).

The verb "die" everywhere else in the codebase of Git not just means an
immediate exit, but means an imediate _error_ exit.  The new helper
function is not about failure, but merely is an early return.  Perhaps
rename it to exit_if_unmodified_since() or something?

Will fix in v5.

Missing " &&" at the end (same in 3/3).

Oops.  I wonder why my tests still passed :p.  Will fix in v5.

Other than that, the patch looks cleanly done.

Thanks.

You're welcome.  Thanks for shepherding me through it ;).

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