parse_datetime fix relative month dates result in wrong dates#253
parse_datetime fix relative month dates result in wrong dates#253cerdelen wants to merge 7 commits intouutils:mainfrom
Conversation
|
could you please add tests to cover this ? |
|
Are these tests sufficient? @sylvestre |
|
@sylvestre This would result in 12.Jan - 2 months -> 11 Sep. In the testsuite for this repo there is 1 testcase which explicitly tests that such overflows/underflows happen and are not handled correctly (correctly is subjective in this case i guess?). Eg 31.Jan + 1 Month. If we say it should overflow because thats what GNU does this whole issue and the source issue (uutils/coreutils#10185) can be closed with the argument of thats the GNU way of not guarding overflows/underflows. Just want a guiding opinion before i remove existing testcases explicitly for these under/overflows. For me logically i would try to make it so that the day of the month always stays the same, unless the target month has less day (eg 31 jan -> 29 feb). |
|
i think we want to do just like GNU :) |
In GNU if using date with relative Months with the -d flag and the target Month does not have the source day (for example 31) GNU overflows the diff in days.
|
Fixed it to work the GNU way and also added tests for relative Year overflow to ensure no regression. |
|
@sylvestre hope its okay to ping again but is there anything to be added here? |
|
pings are welcome, thanks :) |
src/items/builder.rs
Outdated
| let desired_day = dt.day(); | ||
| dt = dt.checked_add::<Span>(rel.try_into()?)?; | ||
| if desired_day != dt.day() { | ||
| dt = dt.checked_add((desired_day - dt.day()).days())?; |
There was a problem hiding this comment.
maybe needs checked arithmetic
Potential integer underflow in subtraction (desired_day - dt.day()) ?
There was a problem hiding this comment.
Logically dt.day() should only ever return a day that is smaller that original day.
If orig_day exists in the target month it will both be same day.
If orig_day does NOT exist in target month since original day was eg 31 dt will be the next closest day, for example 30, but it will be less so it should never underflow
If i do an checked_sub and then what would be the error propogation? An unwrap_or(0) ?
There was a problem hiding this comment.
logically i think a checked sub should'nt be necessary but im happy to add it if requested
|
@sylvestre sorry for another ping |
2 participants
fixed: #252
Using "X months ago" or "X months" (into the future) resulted in wrong dates if the months had different amount of days. Previously parse_datetime took the days of the current month and multiplied it times x to get the day delta between now and the desired X months ago. If this month had 31 days but the previous month had only 30 days this would result in -1 month and -1 day.
New implementation loops x times and checks the days in the months to calculate the correct day delta.