On my machine (Mac OS X 10.4 Tiger, GHC 6.6, darcs unstable as of February 26
22:09 CET) the match.pl test fails. The output of the scripts is attached.

Attachments

• matchtestfailure.txt

This is odd.  It's not entirely surprising, as this date-matching code has been
very tricky to get right, and the test script almost trickier.

Can you check by hand whether the following works for you?

Run darcs changes on a repository (any repository) and find one of the dates. 
Take that date and put it in the format yyyy-mm-dd, and run

darcs changes --match 'date "yyyy-mm-dd"'

and let us know what happens? It should display all changes made on that day, as
measured in your timezone.  darcs changes should also display dates and times in
your timezone, so everything should be consistent and beautiful.

If this doesn't work for you, then we know this is a real bug in darcs, not in
match.pl.  If it works fine, then we don't know whether it's a bug in darcs
itself or in match.pl.

Thanks for the bug report!

David

Just to be clear, and to demonstrate what you should see, I ran this test myself:

$ darcs changes --last 3
Tue Feb 26 17:36:12 EST 2008  David Roundy <droundy@darcs.net>
  * resolved issue701: fixed bug in commutation of certain conflicting patches.
  We were failing to treat a resolution patch properly when commuting a patch
  that depends on the resolution with one that conflicts with it.  :(

Tue Feb 26 17:35:19 EST 2008  David Roundy <droundy@darcs.net>
  * make conflict-fight.sh define patch author names according to repository.

Tue Feb 26 17:32:08 EST 2008  David Roundy <droundy@darcs.net>
  * add some assertions of consistency.
  This only affects darcs-2 repositories, and currently will slow darcs
  down.  Eventually, we should add a configure option to disable these checks,
  which are merely present to detect the case where there's a bug in darcs'
  internals.

$ darcs changes --match 'date "2008-02-26"'
Tue Feb 26 17:36:12 EST 2008  David Roundy <droundy@darcs.net>
  * resolved issue701: fixed bug in commutation of certain conflicting patches.
  We were failing to treat a resolution patch properly when commuting a patch
  that depends on the resolution with one that conflicts with it.  :(

Tue Feb 26 17:35:19 EST 2008  David Roundy <droundy@darcs.net>
  * make conflict-fight.sh define patch author names according to repository.

Tue Feb 26 17:32:08 EST 2008  David Roundy <droundy@darcs.net>
  * add some assertions of consistency.
  This only affects darcs-2 repositories, and currently will slow darcs
  down.  Eventually, we should add a configure option to disable these checks,
  which are merely present to detect the case where there's a bug in darcs'
  internals.

When I sort of repeated the test that match.pl is doing by hand, I saw nothing.
When I run your command on the darcs-unstable repository however, I get
something surprising:

$ darcs2 changes --match 'date "2008-02-26"'
get_changes_info: opts is [FixFilePath "/Users/reinier/Source/darcs-unstable"
"",SeveralPattern pattern "date \"2008-02-26\""], length ps is 38
Mon Feb 25 16:23:36 CET 2008  David Roundy <droundy@darcs.net>
  * resolved issue184: treat changes to superdirs as affecting directory contents.

Mon Feb 25 15:59:21 CET 2008  David Roundy <droundy@darcs.net>
  * add separate makefile target for just running known bugs tests.

Mon Feb 25 15:58:53 CET 2008  David Roundy <droundy@darcs.net>
  * mark issue279 as fixed.

Mon Feb 25 15:55:16 CET 2008  David Roundy <droundy@darcs.net>
  * mark apply-hunks.sh as passing.

Mon Feb 25 15:54:52 CET 2008  David Roundy <droundy@darcs.net>
  * increase env var size to 10k.

Some more commands:

$ darcs2 changes --last=3
get_changes_info: opts is [FixFilePath "/Users/reinier/Source/darcs-unstable"
"",LastN 3], length ps is 38
Mon Feb 25 16:23:36 CET 2008  David Roundy <droundy@darcs.net>
  * resolved issue184: treat changes to superdirs as affecting directory contents.

Mon Feb 25 15:59:21 CET 2008  David Roundy <droundy@darcs.net>
  * add separate makefile target for just running known bugs tests.

Mon Feb 25 15:58:53 CET 2008  David Roundy <droundy@darcs.net>
  * mark issue279 as fixed.

$ darcs pull
Pulling from "http://darcs.net/repos/unstable"...
No remote changes to pull in!

$ date
Wed Feb 27 00:14:23 CET 2008

On Tue, Feb 26, 2008 at 11:19:58PM -0000, Reinier Lamers wrote:
> When I sort of repeated the test that match.pl is doing by hand, I saw nothing.
> When I run your command on the darcs-unstable repository however, I get
> something surprising:
> 
> $ darcs2 changes --match 'date "2008-02-26"'
> get_changes_info: opts is [FixFilePath "/Users/reinier/Source/darcs-unstable"
> "",SeveralPattern pattern "date \"2008-02-26\""], length ps is 38

I think this debugging statement must have been removed, unless you've
added it yourself?

> Mon Feb 25 16:23:36 CET 2008  David Roundy <droundy@darcs.net>
>   * resolved issue184: treat changes to superdirs as affecting directory contents.
> 
> Mon Feb 25 15:59:21 CET 2008  David Roundy <droundy@darcs.net>
>   * add separate makefile target for just running known bugs tests.
> 
> Mon Feb 25 15:58:53 CET 2008  David Roundy <droundy@darcs.net>
>   * mark issue279 as fixed.
> 
> Mon Feb 25 15:55:16 CET 2008  David Roundy <droundy@darcs.net>
>   * mark apply-hunks.sh as passing.
> 
> Mon Feb 25 15:54:52 CET 2008  David Roundy <droundy@darcs.net>
>   * increase env var size to 10k.

This is odd.  It seems that the matching is not being done based on your
timezone, since these changes were made on the 25th, not on the 26th
according to darcs changes.  So something screwy is definitely going on
here, presumably involving your timezone.  CET is Central European Time, I
believe? That makes its time offset opposite in sign from mine.

Tommy, don't you live in CET? Can we get your help with this? It may be
that we're not handling positive timezone offsets properly.  I guess I
could actually test this myself by modifying my timezone
configuration... but not tonight.

> Some more commands:
> 
> $ darcs2 changes --last=3
> get_changes_info: opts is [FixFilePath "/Users/reinier/Source/darcs-unstable"
> "",LastN 3], length ps is 38
> Mon Feb 25 16:23:36 CET 2008  David Roundy <droundy@darcs.net>
>   * resolved issue184: treat changes to superdirs as affecting directory contents.
> 
> Mon Feb 25 15:59:21 CET 2008  David Roundy <droundy@darcs.net>
>   * add separate makefile target for just running known bugs tests.
> 
> Mon Feb 25 15:58:53 CET 2008  David Roundy <droundy@darcs.net>
>   * mark issue279 as fixed.
> 
> $ darcs pull
> Pulling from "http://darcs.net/repos/unstable"...
> No remote changes to pull in!
> 
> $ date
> Wed Feb 27 00:14:23 CET 2008

Oh, I see that you ran these tests right after midnight.  I bet if you ran
them an hour or two later they'd pass (which would have been a bummer).
Could you try running the same manual test again during the day (at a time
when it's the same day where you are and in England) to see if I'm right
about this?

Thanks again for the bug reporting!

Perhaps we should adjust match.pl to try to modify the timezone and rerun
the tests a bunch of times.  Then we would be less prone to the "it passes
during the middle of the day when droundy works on darcs, but fails when
he's asleep" sorts of issues.
-- 
David Roundy
Department of Physics
Oregon State University

I inserted some debug statements myself, and I see that samePartialDate in
DateMatcher.lhs is giving results that are surprising at least to me. The output
is however to copious to paste here. I'm also going to stay for a week without
internet access today, so I won't be able to answer any more questions. I'll
check this bug again after I come back, however.

Anyway, trying again during the day doesn't make much of a difference (I'm
sending stderr to the bit bucket to hide my own debug statements):
--
$ darcs2 changes --match 'date "2008-02-26"' 2> /dev/null
Mon Feb 25 16:23:36 CET 2008  David Roundy <droundy@darcs.net>
  * resolved issue184: treat changes to superdirs as affecting directory contents.

Mon Feb 25 15:59:21 CET 2008  David Roundy <droundy@darcs.net>
  * add separate makefile target for just running known bugs tests.

Mon Feb 25 15:58:53 CET 2008  David Roundy <droundy@darcs.net>
  * mark issue279 as fixed.

Mon Feb 25 15:55:16 CET 2008  David Roundy <droundy@darcs.net>
  * mark apply-hunks.sh as passing.

Mon Feb 25 15:54:52 CET 2008  David Roundy <droundy@darcs.net>
  * increase env var size to 10k.

$ darcs2 changes --match 'date "2008-02-27"' 2> /dev/null
Tue Feb 26 23:36:12 CET 2008  David Roundy <droundy@darcs.net>
  * resolved issue701: fixed bug in commutation of certain conflicting patches.
  We were failing to treat a resolution patch properly when commuting a patch
  that depends on the resolution with one that conflicts with it.  :(

Tue Feb 26 23:35:19 CET 2008  David Roundy <droundy@darcs.net>
  * make conflict-fight.sh define patch author names according to repository.

Tue Feb 26 23:32:08 CET 2008  David Roundy <droundy@darcs.net>
  * add some assertions of consistency.
  This only affects darcs-2 repositories, and currently will slow darcs
  down.  Eventually, we should add a configure option to disable these checks,
  which are merely present to detect the case where there's a bug in darcs'
  internals.

$ date
Wed Feb 27 09:28:17 CET 2008
--

CET is GMT+1 indeed, so your hypothesis about positive/negative timezones sounds
plausible.

And here's a result of trying this on a small test repository with one patch
(like match.pl does in its first test). This time it's small enough to include
the debugging output, that logs the input and output of samePartialDate:

$ darcs2 changes --match 'date "2008-02-26"'
get_changes_info: opts is [FixFilePath
"/Users/reinier/Source/darcs-unstable/tests/handmatig" "",SeveralPattern pattern
"date \"2008-02-26\""], length ps is 1
parseDateMatcher called
samePartialDate: is MCalendarTime {mctYear = Just 2008, mctMonth = Just
February, mctDay = Just 26, mctHour = Nothing, mctMin = Nothing, mctSec =
Nothing, mctPicosec = Nothing, mctWDay = Nothing, mctYDay = Nothing, mctTZName =
Nothing, mctTZ = Just 3600, mctIsDST = Nothing, mctWeek = False} and
CalendarTime {ctYear = 2008, ctMonth = February, ctDay = 27, ctHour = 9, ctMin =
34, ctSec = 15, ctPicosec = 520857000000, ctWDay = Wednesday, ctYDay = 57,
ctTZName = "CET", ctTZ = 3600, ctIsDST = False} the same partial date? False!
samePartialDate: is MCalendarTime {mctYear = Just 2008, mctMonth = Just
February, mctDay = Just 26, mctHour = Nothing, mctMin = Nothing, mctSec =
Nothing, mctPicosec = Nothing, mctWDay = Nothing, mctYDay = Nothing, mctTZName =
Nothing, mctTZ = Just 3600, mctIsDST = Nothing, mctWeek = False} and
CalendarTime {ctYear = 2008, ctMonth = February, ctDay = 26, ctHour = 21, ctMin
= 35, ctSec = 7, ctPicosec = 0, ctWDay = Sunday, ctYDay = 0, ctTZName = "GMT",
ctTZ = 0, ctIsDST = False} the same partial date? False!
length of filtered changes: 0

$ darcs2 changes
get_changes_info: opts is [FixFilePath
"/Users/reinier/Source/darcs-unstable/tests/handmatig" ""], length ps is 1
length of filtered changes: 1
Tue Feb 26 22:35:07 CET 2008  tux_rocker@reinier.de
  * blaattest

On Wed, Feb 27, 2008 at 12:30:39AM -0000, David Roundy wrote:
> Tommy, don't you live in CET? Can we get your help with this? It may be
> that we're not handling positive timezone offsets properly.  I guess I
> could actually test this myself by modifying my timezone
> configuration... but not tonight.

I looked at the new date matching code, and I think the logic in
it is all wrong. For one thing, the function
IsoDate.resetMCalendar does a very strange thing that can not be
right.

There are only two ways (I can think of) to test a partial date
match against a source date in a different time zone. The
easiest to understand way is to convert the source date to the
time zone of the partial date matcher, and then just compare the
resulting numbers for year month and day. The other way (which I
think is what the code tries to do) is to convert the source
date to UTC, and the partial date to a time interval in UTC. But
in that case the time interval must be constructed BEFORE its
start and end times are converted to UTC. To be explicit: the
month 2008-02 CET starts at 2008-02-01T00:00:00+0100 and ends
before 2008-03-01T00:00:00+0100, which converted to UTC is
2008-01-31T23:00:00+0000 and 2008-02-29T23:00:00+0000
respectively, so it should match patches whose dates in UTC
falls within these UTC times.

It looks like the current partial date matching code converts
the partial date to UTC before it constructs the time interval.

I don't think I'll get time to do anything about this anytime
soon. I'm barely keeping up reading the mailing lists and bug
reports. :-(

On Wed, Feb 27, 2008 at 11:31:52PM -0000, Tommy Pettersson wrote:
> I don't think I'll get time to do anything about this anytime
> soon. I'm barely keeping up reading the mailing lists and bug
> reports. :-(

No problem.  Thanks for the analysis! Hopefully it'll motivate (and enable)
some other contributor to take this on.  Since date matching has never
worked well, it's definitely not a release-critical issue.
-- 
David Roundy
Department of Physics
Oregon State University

I'm starting to work on this again. However, I find myself writing code that I
think should be in libraries. Code to convert timezones, which in turn involves
code to give the number of days in a month and the number of days in a year. 

Wouldn't it be better if all internal time processing in darcs were done using
timestamps?

On Sun, Mar 16, 2008 at 03:48:15PM -0000, Reinier Lamers wrote:
> I'm starting to work on this again. However, I find myself writing code that I
> think should be in libraries. Code to convert timezones, which in turn involves
> code to give the number of days in a month and the number of days in a year. 

Can't you do all these conversions with the following functions from System.Time?

toCalendarTime :: ClockTime -> IO CalendarTime
toUTCTime :: ClockTime -> CalendarTime
toClockTime :: CalendarTime -> ClockTime

> Wouldn't it be better if all internal time processing in darcs were done using
> timestamps?

What do you mean by timestamps?

David

Tommy's analysis sounds right.  Boy, I'm glad I don't live somewhere people
shoot you for shoddy programming/testing.

Note that we can test things easily by saying something like
 TZ=CET darcs changes --match="date 2008-02-26"

If my current understanding of things is correct, the date matchers sees this
and (correctly) thinks "ah! since the user is in CET, what he really means is
2008-02-25 at 23h".  What is incorrect is that the calendar reset stuff tries to
propagate the notion of partialness in a very clumsy fashion.  If the original
partial date was only precise up to the day, so is the reset day, and
"2008-02-25 at 23h" magically becomes plain old 2008-02-25.

To sum up, the incorrect part of the code is that timezone-shifting a partial
date also gives you partial date with the same precision.

From a design standpoint, my code seems overcomplicated : we still need some way
of communicating how much precision the user wants (I think MCalendar does
that).  But this whole business of tentative matching seems rather silly and
could be replaced by a single interval check once we have established the interval.

Another thing perhaps is that we should maybe ditch MCalendarTime and rethink. 
Maybe it would be cleaner and less error prone if we used just plain old
CalendarTime and passed around the precision stuff in a separate structure.

Anyway, the date tester should definitely try out all sorts of timezones.  I
think I may have tried setting my computer clock back, but not forward (what a dolt)

It looks like Eric's fixed this one.  Although I had to make one change (could
you read it over, Eric?) to fix a final failing test case (which perhaps only
fails certain times of the day in certain timezones).  So I'm marking this as
resolved-in-unstable.

David

That converts to UTC, but not to an arbitrary timezone. My new code, as Tommy 
suggested, converts the CalendarTime to the TZ of the MCalendarTime instead 
of doing an incorrect conversion of the MCalendarTime to UTC.

Unix timestamps, numbers of seconds since the beginning of 1970. That way, 
there are not many different ways of representing the same time point or 
interval, so hopefully less conversion trouble.

It seems that I fixed the timezone bug now - reducing the number of failures 
in match.pl from 18 to 5. Those 5 seem to fail for different reasons at first 
sight. But I haven't stress-tested it by setting my computer clock to the 
beginning of a new year, the moment DST starts, etc.

Tommy, what have you done already?

Reinier

That converts to UTC, but not to an arbitrary timezone. My new code, as Tommy 
suggested, converts the CalendarTime to the TZ of the MCalendarTime instead 
of doing an incorrect conversion of the MCalendarTime to UTC.

Unix timestamps, numbers of seconds since the beginning of 1970. That way, 
there are not many different ways of representing the same time point or 
interval, so hopefully less conversion trouble.

It seems that I fixed the timezone bug now - reducing the number of failures 
in match.pl from 18 to 5. Those 5 seem to fail for different reasons at first 
sight. But I haven't stress-tested it by setting my computer clock to the 
beginning of a new year, the moment DST starts, etc.

Tommy, what have you done already?

Reinier

On Tue, Mar 18, 2008 at 03:04:53PM -0000, Reinier Lamers wrote:
> Tommy, what have you done already?

It looks like Eric got this fixed yesterday... not the most efficient use
of contributor effort...  :(
-- 
David Roundy
Department of Physics
Oregon State University

Just to clear up,  I think (think!) I've fixed the bugs.  Sorry to
step on your toes/overlap your efforts.

The problem wasn't so much converting timezones, but truncating dates
to the wrong precision (this in the resetMCalendar stuff that Tommy
was pointing to).  I simplified the code and avoided this incorrect
truncation.  We still have to convert to UTC, though.

>  Unix timestamps, numbers of seconds since the beginning of 1970. That way,
>  there are not many different ways of representing the same time point or
>  interval, so hopefully less conversion trouble.

This is what the code does already actually.

Indeed, I saw that. But my mails to the bug tracker had been delayed  
by a day for some reason. Your code is much nicer than mine, so all  
the better that you fixed it :-)

Reinier

On Tue, Mar 18, 2008 at 03:24:29PM -0000, Reinier Lamers wrote:
> Indeed, I saw that. But my mails to the bug tracker had been delayed  
> by a day for some reason. Your code is much nicer than mine, so all  
> the better that you fixed it :-)

Yeah, they were stuck in the moderator queue, which I only check once a day
normally.  It won't happen again with this email address, though.
-- 
David Roundy
Department of Physics
Oregon State University