Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix week number from Datetime::week() when weekstart=0 #84

Merged

Conversation

smemsh
Copy link
Contributor

@smemsh smemsh commented Oct 15, 2024

When working on GothenburgBitFactory/taskwarrior#3623, after modifying the code to pass the user-configured rc.weekstart down into Datetime::weekstart, it was discovered that the returned week number from Datetime::week() was one more than it should be when weekstart=0 (but not weekstart=1). The code was doing:

char weekStr[3];
struct tm* t = localtime (&_date);
strftime (weekStr, sizeof (weekStr), "%U", t);
int weekNumber = strtol (weekStr, NULL, 10);
if (weekstart == 0)
  weekNumber += 1; // <-- HERE

This increment was never activated before, because Datetime::weekstart was initialized to 1 in the library, and never altered by TW. Now that it can be set using configuration (in forthcoming patch), there was breakage seen with weekstart=0 (but not weekstart=1). This was unearthed by TW tests:

not ok 20 - dom2.test.py: DOM 3.due.week
# FAIL: AssertionError [...]
#       '35
# ' != '36
# '
#       - 35
#       + 36

I followed it in a debugger to the [apparently bogus] increment in Datetime::week(). Example that breaks without the patch:

task add testing
task mod 1 due:2011-09-01
task rc.weekstart=monday _get 1.due.week # --> 35 (correct)
task rc.weekstart=sunday _get 1.due.week # --> 36 (incorrect)

The week of 2011-09-01 should be 35 regardless of weekstart:

 $ env - ncal -w -M 09 2011
    September 2011
Mo     5 12 19 26
Tu     6 13 20 27
We     7 14 21 28
Th  1  8 15 22 29
Fr  2  9 16 23 30
Sa  3 10 17 24
Su  4 11 18 25
   35 36 37 38 39

 $ env - ncal -w -S 09 2011
    September 2011
Su     4 11 18 25
Mo     5 12 19 26
Tu     6 13 20 27
We     7 14 21 28
Th  1  8 15 22 29
Fr  2  9 16 23 30
Sa  3 10 17 24
   35 36 37 38 39

 $ date -d 2011-09-01 +%V
35

 $ date -d 2011-09-01 +%U
35

src/Datetime.cpp Outdated Show resolved Hide resolved
It's unclear why this week number increment was added, but it makes the
week number wrong when weekstart=0.  The condition was never true before
because weekstart was always 1, as initialized at the top of the file,
and Taskwarrior was not passing rc.weekstart in, so the [supposed] bug
never manifested.  We are now working on propagating rc.weekstart to
Datetime::weekstart, and sure enough, when weekstart=0, the week number
is ahead by one.
@smemsh smemsh force-pushed the fix-weekofyear-weekstart branch from c6e5fcb to f2fca6a Compare October 15, 2024 22:19
@djmitche
Copy link
Collaborator

Oops, I hadn't realized this wasn't merged!

@djmitche djmitche merged commit 04b46ce into GothenburgBitFactory:master Oct 17, 2024
9 checks passed
@smemsh smemsh deleted the fix-weekofyear-weekstart branch October 17, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants