"snoozing" with notmuch?
[notmuch-archives.git] / 1e / 67ffc31d079fb03353e15c8e93cfbcf3793731
1 Return-Path: <jani@nikula.org>\r
2 X-Original-To: notmuch@notmuchmail.org\r
3 Delivered-To: notmuch@notmuchmail.org\r
4 Received: from localhost (localhost [127.0.0.1])\r
5         by olra.theworths.org (Postfix) with ESMTP id DF0EE431FB6\r
6         for <notmuch@notmuchmail.org>; Wed,  3 Oct 2012 13:32:23 -0700 (PDT)\r
7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
8 X-Spam-Flag: NO\r
9 X-Spam-Score: -0.7\r
10 X-Spam-Level: \r
11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
13 Received: from olra.theworths.org ([127.0.0.1])\r
14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
15         with ESMTP id vBCmkDSndEtk for <notmuch@notmuchmail.org>;\r
16         Wed,  3 Oct 2012 13:32:22 -0700 (PDT)\r
17 Received: from mail-la0-f53.google.com (mail-la0-f53.google.com\r
18         [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
19         (No client certificate requested)\r
20         by olra.theworths.org (Postfix) with ESMTPS id 34DC5431FAE\r
21         for <notmuch@notmuchmail.org>; Wed,  3 Oct 2012 13:32:22 -0700 (PDT)\r
22 Received: by lahl5 with SMTP id l5so3745573lah.26\r
23         for <notmuch@notmuchmail.org>; Wed, 03 Oct 2012 13:32:20 -0700 (PDT)\r
24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
25         d=google.com; s=20120113;\r
26         h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
27         :message-id:mime-version:content-type:x-gm-message-state;\r
28         bh=7kuxDjMOydSS+WAnh7FkBvjru2hZcUGcz73BHGWdpn4=;\r
29         b=JVT8v4KJFDYYbssh/DsJjqQdpcD8b/ucyLlC8vLH264unZk4+nALpCoJ3HBz3x0qtI\r
30         aD2es4LfTmZP/ZPEoz35X2P5M0ZClOpiR2c0H+ZGnHUf6lwD7cY/twumuDNHOHZ99aFX\r
31         qhyprID2zDq1WcPREPtsn3/HVixxvXT3AXJXIgvLtFtjy3gyEigxnPZBgPuD7jTUW4k1\r
32         j2BB1SIbwq8KiMg08OF2xpG3r5KnrbJOsQQq9KbE1ndbFQZZUwmiY462qQafn0q3ILp/\r
33         Wk2cgZfbD2YsjQw6NRNphe9h2hWNhh2eA0sDg19U0IbsAIf2WCh9SJ+tPvjfgq54jVH6\r
34         +RWQ==\r
35 Received: by 10.152.144.2 with SMTP id si2mr2572296lab.26.1349296340620;\r
36         Wed, 03 Oct 2012 13:32:20 -0700 (PDT)\r
37 Received: from localhost (dsl-hkibrasgw4-fe51df00-27.dhcp.inet.fi.\r
38         [80.223.81.27])\r
39         by mx.google.com with ESMTPS id xw14sm1649379lab.15.2012.10.03.13.32.18\r
40         (version=SSLv3 cipher=OTHER); Wed, 03 Oct 2012 13:32:19 -0700 (PDT)\r
41 From: Jani Nikula <jani@nikula.org>\r
42 To: Michal Sojka <sojkam1@fel.cvut.cz>, notmuch@notmuchmail.org,\r
43         David Bremner <david@tethera.net>\r
44 Subject: Re: [PATCH] test: Improve tests for the date/time parser module\r
45 In-Reply-To: <87zk4e1f5k.fsf@steelpick.2x.cz>\r
46 References: <cover.1347484177.git.jani@nikula.org>\r
47         <24186aafbdcb967b8f66c2390c928f3788ab6cbf.1347484177.git.jani@nikula.org>\r
48         <87zk4e1f5k.fsf@steelpick.2x.cz>\r
49 User-Agent: Notmuch/0.14+34~g2c0277c (http://notmuchmail.org) Emacs/23.3.1\r
50         (i686-pc-linux-gnu)\r
51 Date: Wed, 03 Oct 2012 23:32:16 +0300\r
52 Message-ID: <87lifn47qn.fsf@nikula.org>\r
53 MIME-Version: 1.0\r
54 Content-Type: text/plain; charset=us-ascii\r
55 X-Gm-Message-State:\r
56  ALoCoQl0iVlBwmLJfZf2vY7ACP1yrUNqNXjUyqlgf5peAuEPlVouO/EAaa1bRS75nZdz7AaNM9jz\r
57 X-BeenThere: notmuch@notmuchmail.org\r
58 X-Mailman-Version: 2.1.13\r
59 Precedence: list\r
60 List-Id: "Use and development of the notmuch mail system."\r
61         <notmuch.notmuchmail.org>\r
62 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
63         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
64 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
65 List-Post: <mailto:notmuch@notmuchmail.org>\r
66 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
67 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
68         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
69 X-List-Received-Date: Wed, 03 Oct 2012 20:32:24 -0000\r
70 \r
71 On Tue, 25 Sep 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:\r
72 > This patch reworks date/time parser library test program to make it\r
73 > easier to to write the actual tests. It also modifies the notmuch test\r
74 > script and adds several new tests to it.\r
75 \r
76 Cool!\r
77 \r
78 > The INPUT file for the test contains both the dates to be parsed as well\r
79 > as the "expected" results. The test program outputs the results in the\r
80 > same format and replaces expected results with real results. Currently,\r
81 > the "expected" results in the INPUT file correspond to the real results,\r
82 > so the test passes. Some results are, however, different from what I\r
83 > would expect - this is mentioned in the comments after '#'.\r
84 \r
85 Please see comments inline next to tests.\r
86 \r
87 >\r
88 > This patch applies on top of Jani's patchset.\r
89 > ---\r
90 > It can be seen that there are several errors and unexpected results.\r
91 > As I've already written, I'm not sure that the approach taken by this\r
92 > library is the right one. I tend to agree with mina86, that using a\r
93 > more systematic approach (such as bison) would be beneficial.\r
94 \r
95 Then we just have to agree to disagree here. :)\r
96 \r
97 > This is however not to say to throw this patchset away. Either Jani\r
98 > will be able to fix all the corner cases. Or we can work together to\r
99 > develop a better solution - add support for ranges to the bison\r
100 > parser.\r
101 \r
102 I think I've got the cases pretty much covered, and they're mostly not\r
103 about syntax and parsing, but rather semantics; what to do with the\r
104 parsed data.\r
105 \r
106 > -Michal\r
107 >\r
108 > diff --git a/test/Makefile.local b/test/Makefile.local\r
109 > index 9ae130a..b9105c7 100644\r
110 > --- a/test/Makefile.local\r
111 > +++ b/test/Makefile.local\r
112 > @@ -20,7 +20,7 @@ $(dir)/symbol-test: $(dir)/symbol-test.o\r
113 >       $(call quiet,CXX) $^ -o $@ -Llib -lnotmuch $(XAPIAN_LDFLAGS)\r
114 >  \r
115 >  $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o\r
116 > -     $(call quiet,CC) $^ -o $@\r
117 > +     $(call quiet,CC) $^ -o $@ -lrt\r
118 >  \r
119 >  .PHONY: test check\r
120 >  \r
121 > diff --git a/test/parse-time-string b/test/parse-time-string\r
122 > index 34b80d7..265437c 100755\r
123 > --- a/test/parse-time-string\r
124 > +++ b/test/parse-time-string\r
125 > @@ -14,13 +14,48 @@ _parse_time ()\r
126 >      ${TEST_DIRECTORY}/parse-time --format=%s "$*"\r
127 >  }\r
128 >  \r
129 > -test_begin_subtest "date(1) default format without TZ code"\r
130 > -test_expect_equal "$(_parse_time Fri Aug 3 23:06:06 2012)" "$(_date Fri Aug 3 23:06:06 2012)"\r
131 > +test_begin_subtest "Date parser tests"\r
132 > +cat <<EOF > INPUT\r
133 > +now          -> Tue Jan 11 11:11:00 +0000 2011\r
134 > +2010-1-1     -> parse_time_string() error: 5\r
135 \r
136 I think that's invalid per ISO 8601.\r
137 \r
138 > +Jan 2        -> Sat Jan 02 11:11:00 +0000 2010   # Why 2010?\r
139 \r
140 This is an interesting bug. The idea was that specifying a month without\r
141 year would always refer to past. So when you give Jan 2011 in the\r
142 reference time, Jan refers to the previous year. Seems simple, but\r
143 looking closer, also "Jan 2 this year" would end up in 2010. Not good.\r
144 \r
145 It would probably be possible to fix this, but per the simplicity of\r
146 implementation and unambiguity goals, I think I'll just make them refer\r
147 to current year, at least for now. This may mean having to look into\r
148 supporting "last {monthname,weekday}" for better usability, but I'll\r
149 leave that as a future improvement.\r
150 \r
151 > +Mon          -> Mon Jan 10 11:11:00 +0000 2011\r
152 > +last Friday  -> parse_time_string() error: 4\r
153 \r
154 "last <weekday>" is not supported.\r
155 \r
156 > +2 hours ago  -> parse_time_string() error: 1\r
157 \r
158 "ago" is not supported.\r
159 \r
160 > +last month   -> Sat Dec 11 11:11:00 +0000 2010\r
161 > +month ago    -> parse_time_string() error: 1\r
162 \r
163 Ditto.\r
164 \r
165 > +8am          -> Tue Jan 11 08:00:00 +0000 2011\r
166 > +9:15         -> Tue Jan 11 09:15:00 +0000 2011\r
167 > +12:34        -> Tue Jan 11 12:34:00 +0000 2011\r
168 > +monday       -> Mon Jan 10 11:11:00 +0000 2011\r
169 > +yesterday    -> Mon Jan 10 11:11:00 +0000 2011\r
170 > +tomorrow     -> parse_time_string() error: 1\r
171 \r
172 "tomorrow" is not supported (do you get a lot of mail from the future?\r
173 ;)\r
174 \r
175 > +             -> Tue Jan 11 11:11:00 +0000 2011 # Shouldn't empty string return an error???\r
176 \r
177 *shrug* It just starts with the reference time, and finds nothing to add\r
178 or remove. Let's call it a feature.\r
179 \r
180 >  \r
181 > -test_begin_subtest "date(1) --rfc-2822 format"\r
182 > -test_expect_equal "$(_parse_time Fri, 03 Aug 2012 23:07:46 +0100)" "$(_date Fri, 03 Aug 2012 23:07:46 +0100)"\r
183 > +Aug 3 23:06:06 2012             -> Fri Aug 03 23:06:06 +0000 2012 # date(1) default format without TZ code\r
184 > +Fri, 03 Aug 2012 23:07:46 +0100 -> Fri Aug 03 22:07:46 +0000 2012 # rfc-2822\r
185 > +2012-08-03 23:09:37+03:00       -> Fri Aug 03 20:09:37 +0000 2012 # rfc-3339 seconds\r
186 >  \r
187 > -test_begin_subtest "date(1) --rfc=3339=seconds format"\r
188 > -test_expect_equal "$(_parse_time 2012-08-03 23:09:37+03:00)" "$(_date 2012-08-03 23:09:37+03:00)"\r
189 > +10s           -> Tue Jan 11 11:10:50 +0000 2011\r
190 > +19701223s     -> Wed Dec 23 11:10:59 +0000 1970 # Surprising - number is parsed as date and 's' as '1 second'\r
191 \r
192 Will be fixed.\r
193 \r
194 > +19701223      -> Wed Dec 23 11:11:00 +0000 1970\r
195 > +\r
196 > +19701223 +0100 -> Wed Dec 23 11:11:00 +0000 1970 # Timezone is ignored without an error\r
197 \r
198 It's not ignored. Date is specified, but the time comes from the\r
199 reference time. It's the same absolute time regardless of the timezone.\r
200 \r
201 > +\r
202 > +today ^-> Wed Jan 12 00:00:00 +0000 2011 # This should be 11 23:59:59\r
203 \r
204 See my previous mail. Can be fixed.\r
205 \r
206 > +today v-> Tue Jan 11 00:00:00 +0000 2011\r
207 > +\r
208 > +thisweek ^-> Sun Jan 16 00:00:00 +0000 2011  # This should be Sunday 23:59:59\r
209 > +thisweek v-> Sun Jan 09 00:00:00 +0000 2011  # This should be Monday 00:00:00\r
210 \r
211 Implementation simplicity and dodging a localization issue. Start of the\r
212 week is based on the tm_mday field of struct tm, where 0 == Sunday. Even\r
213 if I personally agree Monday is the 1st day of the week.\r
214 \r
215 > +\r
216 > +two months ago-> parse_time_string() error: 1 # Comments in the code suggest that this is supported\r
217 \r
218 When in doubt, trust code over comments. ;)\r
219 \r
220 > +two months -> Thu Nov 11 11:11:00 +0000 2010\r
221 > +\r
222 > +1348569850 -> parse_time_string() error: 4 # Seconds since epoch not yet supported? Backward compatibility in notmuch???\r
223 > +10 -> parse_time_string() error: 4 # Seconds since epoch?\r
224 \r
225 Indeed, seconds since epoch not yet supported. There is no backwards\r
226 compatibility issue, as you can still use the\r
227 <initial-timestamp>..<final-timestamp> format as described in\r
228 notmuch-search-terms(7) man page. The new date:<since>..<until> just\r
229 doesn't support seconds since epoch yet. And I think I'll make the\r
230 syntax "@<timestamp>" to let you have "<really-big-number> seconds"\r
231 without surprises.\r
232 \r
233 > +EOF\r
234 > +\r
235 > +${TEST_DIRECTORY}/parse-time --now="Tue Jan 11 11:11:00 +0000 2011" < INPUT > OUTPUT\r
236 > +test_expect_equal_file INPUT OUTPUT\r
237 >  \r
238 >  test_done\r
239 > diff --git a/test/parse-time.c b/test/parse-time.c\r
240 > index b4de76b..0415f49 100644\r
241 > --- a/test/parse-time.c\r
242 > +++ b/test/parse-time.c\r
243 > @@ -18,59 +18,47 @@\r
244 >   * Author: Jani Nikula <jani@nikula.org>\r
245 >   */\r
246 >  \r
247 > +\r
248 > +#define _XOPEN_SOURCE 500       /* for strptime() and snprintf() */\r
249 >  #include <getopt.h>\r
250 >  #include <stdio.h>\r
251 >  #include <stdlib.h>\r
252 >  #include <string.h>\r
253 > +#include <time.h>\r
254 >  \r
255 >  #include "parse-time-string.h"\r
256 >  \r
257 > -/*\r
258 > - * concat argv[start]...argv[end - 1], separating them by a single\r
259 > - * space, to a malloced string\r
260 > - */\r
261 > -static char *\r
262 > -concat_args (int start, int end, char *argv[])\r
263 > -{\r
264 > -    int i;\r
265 > -    size_t len = 1;\r
266 > -    char *p;\r
267 > -\r
268 > -    for (i = start; i < end; i++)\r
269 > -     len += strlen (argv[i]) + 1;\r
270 > -\r
271 > -    p = malloc (len);\r
272 > -    if (!p)\r
273 > -     return NULL;\r
274 > -\r
275 > -    *p = 0;\r
276 > -\r
277 > -    for (i = start; i < end; i++) {\r
278 > -     if (i != start)\r
279 > -         strcat (p, " ");\r
280 > -     strcat (p, argv[i]);\r
281 > -    }\r
282 > -\r
283 > -    return p;\r
284 > -}\r
285 > -\r
286 >  #define DEFAULT_FORMAT "%a %b %d %T %z %Y"\r
287 >  \r
288 >  static void\r
289 >  usage (const char *name)\r
290 >  {\r
291 > -    printf ("Usage: %s [options ...] <date/time>\n\n", name);\r
292 > +    printf ("Usage: %s [options ...]\n\n", name);\r
293 >      printf (\r
294 > -     "Parse <date/time> and display it in given format.\n\n"\r
295 > -     "  -f, --format=FMT output format, FMT according to strftime(3)\n"\r
296 > -     "                   (default: \"%s\")\n"\r
297 > -     "  -n, --now=N      use N seconds since epoch as now (default: now)\n"\r
298 > -     "  -u, --up         round result up (default: no rounding)\n"\r
299 > -     "  -d, --down       round result down (default: no rounding)\n"\r
300 > -     "  -h, --help       print this help\n",\r
301 > +     "Parse date/time read from stdin and display it in given format.\n\n"\r
302 > +     "  -f, --format=FMT output format for dates and input format for --now,\n"\r
303 > +        "                   FMT according to strftime(3) (default: \"%s\")\n"\r
304 > +     "  -n, --now=N      reference date in FMT (default: now)\n"\r
305 > +     "  -h, --help       print this help\n"\r
306 > +     "\n"\r
307 > +     "stdin should contain one date/time per line in the following format:\n"\r
308 > +     "  <date/time> [ <arrow> [ comment ] ]\n"\r
309 > +     "where <arrow> determines the operation performed on the <date/time>.\n"\r
310 > +     "It can be one of '->', '^->', 'v->' meaning convert, convert and round\n"\r
311 > +     "up, convert and round down, respectively.\n",\r
312 >       DEFAULT_FORMAT);\r
313 >  }\r
314 >  \r
315 > +static const char *\r
316 > +get_round_str (int round)\r
317 > +{\r
318 > +    switch (round) {\r
319 > +    case PARSE_TIME_ROUND_UP:   return "^";\r
320 > +    case PARSE_TIME_ROUND_DOWN: return "v";\r
321 > +    default:                 return "";\r
322 > +    }\r
323 > +}\r
324 > +\r
325 >  int\r
326 >  main (int argc, char *argv[])\r
327 >  {\r
328 > @@ -79,14 +67,10 @@ main (int argc, char *argv[])\r
329 >      time_t result;\r
330 >      time_t now;\r
331 >      time_t *nowp = NULL;\r
332 > -    char *argstr;\r
333 >      int round = PARSE_TIME_NO_ROUND;\r
334 > -    char buf[1024];\r
335 >      const char *format = DEFAULT_FORMAT;\r
336 >      struct option options[] = {\r
337 >       { "help",       no_argument,            NULL,   'h' },\r
338 > -     { "up",         no_argument,            NULL,   'u' },\r
339 > -     { "down",       no_argument,            NULL,   'd' },\r
340 >       { "format",     required_argument,      NULL,   'f' },\r
341 >       { "now",        required_argument,      NULL,   'n' },\r
342 >       { NULL, 0, NULL, 0 },\r
343 > @@ -111,8 +95,13 @@ main (int argc, char *argv[])\r
344 >           round = PARSE_TIME_ROUND_DOWN;\r
345 >           break;\r
346 >       case 'n':\r
347 > -         /* specify now in seconds since epoch */\r
348 > -         now = (time_t) strtol (optarg, NULL, 10);\r
349 > +         memset (&tm, 0, sizeof (tm));\r
350 > +         char *parsed = strptime (optarg, format, &tm);\r
351 \r
352 One of the problems with strptime is that it doesn't support time zones,\r
353 which is why I chose not to use it here. (You can specify %z in the\r
354 format to ignore it, but it looks like it's ignored no matter\r
355 what. *shrug*) Combined with mktime below, you introduce possible TZ and\r
356 DST variations in the tests, which can be problematic. So perhaps we\r
357 should keep the reference time as a timestamp here.\r
358 \r
359 I didn't look at this test tool patch very closely yet, but in general I\r
360 like the very much increased clarity in the tests. I'll look into this\r
361 more when I have a moment.\r
362 \r
363 Thanks for the tests, comments, and corner cases. They've been helpful.\r
364 \r
365 \r
366 BR,\r
367 Jani.\r
368 \r
369 \r
370 > +         if (!parsed) {\r
371 > +             fprintf (stderr, "Cannot parse reference date: %s\n", optarg);\r
372 > +             return 1;\r
373 > +         }\r
374 > +         now = mktime (&tm);\r
375 >           if (now >= (time_t) 0)\r
376 >               nowp = &now;\r
377 >           break;\r
378 > @@ -124,22 +113,47 @@ main (int argc, char *argv[])\r
379 >       }\r
380 >      }\r
381 >  \r
382 > -    argstr = concat_args (optind, argc, argv);\r
383 > -    if (!argstr)\r
384 > -     return 1;\r
385 > -\r
386 > -    r = parse_time_string (argstr, &result, nowp, round);\r
387 > -\r
388 > -    free (argstr);\r
389 > -\r
390 > -    if (r)\r
391 > -     return 1;\r
392 > -\r
393 > -    if (!localtime_r (&result, &tm))\r
394 > -     return 1;\r
395 > -\r
396 > -    strftime (buf, sizeof (buf), format, &tm);\r
397 > -    printf ("%s\n", buf);\r
398 > +    char input[BUFSIZ];\r
399 > +    while (fgets (input, BUFSIZ, stdin) && input[0]) {\r
400 > +     if (input[0] == '\n') {\r
401 > +         printf ("\n");\r
402 > +         continue;\r
403 > +     }\r
404 > +     char *arrow;\r
405 > +     char *comment = strrchr (input, '#');\r
406 > +     arrow = strstr (input, "->");\r
407 > +     round = PARSE_TIME_NO_ROUND;\r
408 > +     if (arrow > input) {\r
409 > +         switch (arrow[-1]) {\r
410 > +         case '^': round = PARSE_TIME_ROUND_UP; arrow--; break;\r
411 > +         case 'v': round = PARSE_TIME_ROUND_DOWN; arrow--; break;\r
412 > +         default: break;\r
413 > +         }\r
414 > +     }\r
415 > +     if (arrow)\r
416 > +         *arrow = 0;\r
417 > +     else\r
418 > +         arrow = input + strlen (input); /* XXX: comment is not handled */\r
419 > +     while (arrow > input && arrow[-1] == '\n')\r
420 > +         arrow--;\r
421 > +     *arrow-- = 0;\r
422 > +\r
423 > +     r = parse_time_string (input, &result, nowp, round);\r
424 > +     char resstr[BUFSIZ];\r
425 > +     if (r)\r
426 > +         snprintf (resstr, sizeof(resstr), "parse_time_string() error: %d", r);\r
427 > +     else if (!localtime_r (&result, &tm))\r
428 > +         snprintf (resstr, sizeof(resstr), "localtime(result) error");\r
429 > +     else\r
430 > +         strftime (resstr, sizeof (resstr), format, &tm);\r
431 > +\r
432 > +     char buf[BUFSIZ];\r
433 > +     snprintf (buf, sizeof(buf), "%s%s-> %s", input, get_round_str (round), resstr);\r
434 > +     if (!comment)\r
435 > +         printf ("%s\n", buf);\r
436 > +     else\r
437 > +         printf ("%-*s%s", (int)(comment - input), buf, comment);\r
438 > +    }\r
439 >  \r
440 >      return 0;\r
441 >  }\r