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

From: W. Trevor King <wking@drexel.edu>
Date: Mar 26, 2012, 17:36 UTC
Message-ID: <20120326173646.GA6524@odin.tremily.us>
In-Reply-To: <7v8vinqoaj.fsf@alter.siamese.dyndns.org>

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:

Show 15 quoted lines

> 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