Re: [PATCH v2] date: detect underflow when parsing dates with positive timezone offset
- From
darcy <acednes@gmail.com>
- Date
- Jun 3, 2024, 11:44 UTC
- Message-ID
- <2d771a72-3021-46db-ac32-e008a7ace34c@gmail.com>
- In-Reply-To
- <xmqqwmn66zzc.fsf@gitster.g>
> Without having a deep understanding of the code I don't quite see the > connection between this change and the problem description. Is it > necessary? If so, it might help to explain why it's needed in the commit > message or in the code.
Show 5 quoted lines
> ...I guess that > checking for tm_hour is assuming that TZ offset should always come > before the values necessary to compute the timestamp comes, but it > smells like an unwarranted assumption and not explaining the change > in the proposed log message is a bad sign.
This line has been reverted. The point was it would only parse the timezone offset if it occurs after the time part of the date, but I have realized that this is unrelated to the purpose of this change.
> You should also add at least one test.
Yep, thanks, added now to `t0006-date.sh`.
> Do we also want to check for overflows in the other direction (a large timestamp with a negative timezone offset)?
Is this something people want added? I am happy to implement this if so, though it wasn't my original intention.
Issues with the commit message should also be resolved. Thank you everyone for your patience :)
On 3/6/24 21:13, Junio C Hamano wrote:
Show 18 quoted lines
> "darcy via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> fix: prevent date underflow when using positive timezone offset >> >> cc: Patrick Steinhardt ps@pks.im cc: Phillip Wood >> phillip.wood123@gmail.com > You're expected to respond to review comments before you send in > updated patches. I didn't see the review comments responded to in > the thread: > > https://lore.kernel.org/git/pull.1726.git.git.1716801427015.gitgitgadget@gmail.com/ > > Please see "A typical life cycle of a patch series" section of the > SubmittingPatches document. > > https://git.github.io/htmldocs/SubmittingPatches.html#patch-flow > > Step #3 and Step #4 are distinct.