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
Attachment:
signature.asc
Description: OpenPGP digital signature