ae/ded1f49d79be3de12d692d1f8990636dceaaaf

   1 Return-Path: <amdragon@mit.edu>\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 12369431FBF\r
   6         for <notmuch@notmuchmail.org>; Tue, 10 Jul 2012 12:13:37 -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 ho7jLnyld7BR for <notmuch@notmuchmail.org>;\r
  16         Tue, 10 Jul 2012 12:13:35 -0700 (PDT)\r
  17 Received: from dmz-mailsec-scanner-2.mit.edu (DMZ-MAILSEC-SCANNER-2.MIT.EDU\r
  18         [18.9.25.13])\r
  19         by olra.theworths.org (Postfix) with ESMTP id AA571431FAE\r
  20         for <notmuch@notmuchmail.org>; Tue, 10 Jul 2012 12:13:35 -0700 (PDT)\r
  21 X-AuditID: 1209190d-b7fd56d000000933-0d-4ffc7edec63f\r
  22 Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
  23         by dmz-mailsec-scanner-2.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 65.B0.02355.EDE7CFF4; Tue, 10 Jul 2012 15:13:34 -0400 (EDT)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id q6AJDXmL009316; \r
  27         Tue, 10 Jul 2012 15:13:34 -0400\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q6AJDWY5012664\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Tue, 10 Jul 2012 15:13:33 -0400 (EDT)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1SofsK-0005gp-2V; Tue, 10 Jul 2012 15:13:32 -0400\r
  37 Date: Tue, 10 Jul 2012 15:13:31 -0400\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: craven@gmx.net\r
  40 Subject: Re: [PATCH] v2: Added better support for multiple structured output\r
  41         formats.\r
  42 Message-ID: <20120710191331.GE7332@mit.edu>\r
  43 References: <87liisue70.fsf@nexoid.at> <87fw8zvqmm.fsf@nexoid.at>\r
  44         <87liir944f.fsf@qmul.ac.uk> <87a9z7vj3p.fsf@nexoid.at>\r
  45 MIME-Version: 1.0\r
  46 Content-Type: text/plain; charset=us-ascii\r
  47 Content-Disposition: inline\r
  48 In-Reply-To: <87a9z7vj3p.fsf@nexoid.at>\r
  49 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFprAKsWRmVeSWpSXmKPExsUixCmqrXuv7o+/wa79whZ7G9oZLa7fnMns\r
  52         wOSxeNN+No9nq24xBzBFcdmkpOZklqUW6dslcGW82/KBraA3veLx0yOMDYwtgV2MnBwSAiYS\r
  53         j47MZ4ewxSQu3FvP1sXIxSEksI9R4ufTNiYIZwOjxKddD9khnJNMEk+utEA5Sxglds6/zgzS\r
  54         zyKgKnH201ewWWwCGhLb9i9nBLFFBIQkJn15xQJiMwtIS3z73cwEYgsLhEv8bYXo5RXQljj9\r
  55         6hFYXEigTmLB7iuMEHFBiZMzn0D1aknc+PcSqIYDbM7yfxwgYU4BdYmP+16AjREVUJGYcnIb\r
  56         2wRGoVlIumch6Z6F0L2AkXkVo2xKbpVubmJmTnFqsm5xcmJeXmqRrpFebmaJXmpK6SZGcGBL\r
  57         8u5gfHdQ6RCjAAejEg/vjLQ//kKsiWXFlbmHGCU5mJREeadUAoX4kvJTKjMSizPii0pzUosP\r
  58         MUpwMCuJ8D50BsrxpiRWVqUW5cOkpDlYlMR5r6Tc9BcSSE8sSc1OTS1ILYLJynBwKEnwRgAj\r
  59         WEiwKDU9tSItM6cEIc3EwQkynAdouB5IDW9xQWJucWY6RP4Uoy7HujdHbjAKseTl56VKifNa\r
  60         gRQJgBRllObBzYElpFeM4kBvCfNag1TxAJMZ3KRXQEuYgJa09/wCWVKSiJCSamBcNeu7nHTK\r
  61         wWknWFRDjk1LWSBitcmrhjn0VhbPghqNZ5br3LW2neKfp8W38cl9c8GPvRLZhrxLL38Qm56V\r
  62         amD3vZGnRfDvbhHVxjTP7xlcE573+67WTTQ6piv7/mPYhyz1/e8CN2l8Otb+MNbEIMNvTd3n\r
  63         vLhoywCFAI+VaROr5p5vN0mVn6fEUpyRaKjFXFScCAC+NB5/IwMAAA==\r
  64 Cc: notmuch@notmuchmail.org\r
  65 X-BeenThere: notmuch@notmuchmail.org\r
  66 X-Mailman-Version: 2.1.13\r
  67 Precedence: list\r
  68 List-Id: "Use and development of the notmuch mail system."\r
  69         <notmuch.notmuchmail.org>\r
  70 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  72 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  73 List-Post: <mailto:notmuch@notmuchmail.org>\r
  74 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  75 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  76         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  77 X-List-Received-Date: Tue, 10 Jul 2012 19:13:37 -0000\r
  78 \r
  79 Since it would be great to use the structure printer for show as well,\r
  80 it would make sense to put it in its own source file, where it can\r
  81 easily be shared between commands.\r
  82 \r
  83 There are a few systematic code formatting problems in your patch.  To\r
  84 be consistent with other notmuch code, there should be a space between\r
  85 function names and parameter lists (in both declarations and calls)\r
  86 and there should be a space between a keyword and a paren (e.g., "if\r
  87 (" and "for (").  In a function definition, the function name should\r
  88 start on a new line (this makes it easy to grep for a function's\r
  89 definition).  Also, in general, try to keep lines under 80 characters\r
  90 wide (we're not very consistent about this one, but it would be nice\r
  91 to not become even less consistent about it).\r
  92 \r
  93 More detailed comments below.\r
  94 \r
  95 Quoth craven@gmx.net on Jul 10 at  3:30 pm:\r
  96 > As discussed in <id:20120121220407.GK16740@mit.edu>, this patch adds\r
  97 > support for new structured output formats (like s-expressions) by using\r
  98 > stateful structure_printers. An implementation of the JSON structure\r
  99 > printer that passes all tests is included. The output for JSON (and\r
 100 > text) is identical to the current output. S-Expressions will be added in\r
 101 > a later patch.\r
 102 > \r
 103 > A large part of this patch just implements the differentiation between\r
 104 > structured and non-structured output (all the code within \r
 105 > "if(format == unstructured_text_printer)").\r
 106 > \r
 107 > In a second patch, the structured output code should be isolated in a\r
 108 > separate file, and also used in all other parts of notmuch.\r
 109 > \r
 110 > The interface is a structure structure_printer, which contains the following methods:\r
 111 > \r
 112 > - initial_state: is called to create a state object, that is passed to all invocations. This should be used to keep track of the output file and everything else necessary to correctly format output.\r
 113 > - map: is called when a new map (associative array, dictionary) is started. map_key and the primitives (string, number, bool) are used alternatingly to add key/value pairs. pop is used to close the map (see there). This function must return a nesting level identifier that can be used to close all nested structures (maps and lists), backing out to the returned nesting level.\r
 114 > - list: is called when a new list (array, vector) is started. the primitives (string, number, bool) are used consecutively to add values to the list. pop is used to close the list. This function must return a nesting level identifier that can be used to close all nested structures (maps and lists), backing out to the returned nesting level.\r
 115 > - pop: is called to return to a given nesting level. All lists and maps with a deeper nesting level must be closed.\r
 116 > - number, string, bool: output one element of the specific type.\r
 117 > \r
 118 > All functions should use the state object to insert delimiters etc. automatically when appropriate.\r
 119 > \r
 120 > Example:\r
 121 > int top, one;\r
 122 > top = map(state);\r
 123 > map_key(state, "foo");\r
 124 > one = list(state);\r
 125 > number(state, 1);\r
 126 > number(state, 2);\r
 127 > number(state, 3);\r
 128 > pop(state, i);\r
 129 > map_key(state, "bar");\r
 130 > map(state);\r
 131 > map_key(state, "baaz");\r
 132 > string(state, "hello world");\r
 133 > pop(state, top);\r
 134 > \r
 135 > would output JSON as follows:\r
 136 > \r
 137 > {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}\r
 138 > ---\r
 139 >  notmuch-search.c | 491 ++++++++++++++++++++++++++++++++++++++++---------------\r
 140 >  1 file changed, 361 insertions(+), 130 deletions(-)\r
 141 > \r
 142 > diff --git a/notmuch-search.c b/notmuch-search.c\r
 143 > index 3be296d..4127777 100644\r
 144 > --- a/notmuch-search.c\r
 145 > +++ b/notmuch-search.c\r
 146 > @@ -28,6 +28,210 @@ typedef enum {\r
 147 >      OUTPUT_TAGS\r
 148 >  } output_t;\r
 149 >  \r
 150 > +/* structured formatting, useful for JSON, S-Expressions, ...\r
 151 > +\r
 152 > +- initial_state: is called to create a state object, that is passed to all invocations. This should be used to keep track of the output file and everything else necessary to correctly format output.\r
 153 > +- map: is called when a new map (associative array, dictionary) is started. map_key and the primitives (string, number, bool) are used alternatingly to add key/value pairs. pop is used to close the map (see there). This function must return a nesting level identifier that can be used to close all nested structures (maps and lists), backing out to the returned nesting level.\r
 154 > +- list: is called when a new list (array, vector) is started. the primitives (string, number, bool) are used consecutively to add values to the list. pop is used to close the list. This function must return a nesting level identifier that can be used to close all nested structures (maps and lists), backing out to the returned nesting level.\r
 155 > +- pop: is called to return to a given nesting level. All lists and maps with a deeper nesting level must be closed.\r
 156 > +- number, string, bool: output one element of the specific type.\r
 157 > +\r
 158 > +All functions should use state to insert delimiters etc. automatically when appropriate.\r
 159 > +\r
 160 > +Example:\r
 161 > +int top, one;\r
 162 > +top = map(state);\r
 163 > +map_key(state, "foo");\r
 164 > +one = list(state);\r
 165 > +number(state, 1);\r
 166 > +number(state, 2);\r
 167 > +number(state, 3);\r
 168 > +pop(state, i);\r
 169 > +map_key(state, "bar");\r
 170 > +map(state);\r
 171 > +map_key(state, "baaz");\r
 172 > +string(state, "hello world");\r
 173 > +pop(state, top);\r
 174 > +\r
 175 > +would output JSON as follows:\r
 176 > +\r
 177 > +{"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}\r
 178 > +\r
 179 > + */\r
 180 \r
 181 The above comment should follow the style of other notmuch comments:\r
 182 \r
 183 /* Put a star before every line, use full sentences starting with a\r
 184  * capital letter, and wrap the comment at 72 columns.\r
 185  */\r
 186 \r
 187 For the descriptions of the function pointers, it probably makes sense\r
 188 to comment them inline in the struct itself so the reader doesn't have\r
 189 to match things up.  E.g.,\r
 190 \r
 191 typedef struct structure_printer {\r
 192     /* (Description of map.) */\r
 193     int (*map) (void *state);\r
 194     ...\r
 195 } structure_printer_t;\r
 196 \r
 197 > +typedef struct structure_printer {\r
 198 > +    int (*map)(void *state);\r
 199 \r
 200 This is a rather less obvious application of notmuch code style, but\r
 201 there should be a space before the parameter list here, too:\r
 202     int (*map) (void *state);\r
 203 Likewise for the other fields, of course.\r
 204 \r
 205 > +    int (*list)(void *state);\r
 206 > +    void (*pop)(void *state, int level);\r
 207 > +    void (*map_key)(void *state, const char *key);\r
 208 > +    void (*number)(void *state, int val);\r
 209 > +    void (*string)(void *state, const char *val);\r
 210 > +    void (*bool)(void *state, notmuch_bool_t val);\r
 211 > +    void *(*initial_state)(const struct structure_printer *sp, FILE *output);\r
 212 > +} structure_printer_t;\r
 213 \r
 214 Is there a reason to separate the structure printer's vtable from its\r
 215 state?  It seems like this forces the caller to keep track of two\r
 216 things instead of just one.  For show this isn't too bad because the\r
 217 structure printer is the formatter, but I think it will be cumbersome\r
 218 for the search code.  Keeping them together would also be at least\r
 219 nominally more typesafe.\r
 220 \r
 221 > +\r
 222 > +/* JSON structure printer */\r
 223 > +\r
 224 > +/* single linked list implementation for keeping track of the array/map nesting state */\r
 225 \r
 226 Same comment formatting comment here.\r
 227 \r
 228 > +typedef struct json_list {\r
 229 > +    int type;\r
 230 > +    int first_already_seen;\r
 231 \r
 232 Maybe just "first_seen"?  The "already" doesn't add anything.  Also,\r
 233 this should probably be a notmuch_bool_t.\r
 234 \r
 235 > +    struct json_list *rest;\r
 236 > +} json_list_t;\r
 237 > +\r
 238 > +#define TYPE_JSON_MAP 1\r
 239 > +#define TYPE_JSON_ARRAY 2\r
 240 \r
 241 An enum would be preferable here.\r
 242 \r
 243 > +\r
 244 > +typedef struct json_state {\r
 245 > +    FILE *output;\r
 246 > +    json_list_t *stack;\r
 247 > +    int level;\r
 248 > +} json_state_t;\r
 249 > +\r
 250 > +int json_map(void *state);\r
 251 > +int json_list(void *state);\r
 252 > +void json_pop(void *state, int level);\r
 253 > +void json_map_key(void *state, const char *key);\r
 254 > +void json_number(void *state, int val);\r
 255 > +void json_string(void *state, const char *val);\r
 256 > +void json_bool(void *state, notmuch_bool_t val);\r
 257 > +void *json_initial_state(const struct structure_printer *sp, FILE *output);\r
 258 > +\r
 259 > +structure_printer_t json_structure_printer = {\r
 260 > +    &json_map,\r
 261 > +    &json_list,\r
 262 > +    &json_pop,\r
 263 > +    &json_map_key,\r
 264 > +    &json_number,\r
 265 > +    &json_string,\r
 266 > +    &json_bool,\r
 267 > +    &json_initial_state\r
 268 > +};\r
 269 > +\r
 270 > +int json_map(void *st) {\r
 271 > +    json_state_t *state = (json_state_t*)st;\r
 272 > +    FILE *output = state->output;\r
 273 > +    if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
 274 > +     fputs(",", output);\r
 275 > +     if(state->level == 1)\r
 276 > +         fputs("\n", output);\r
 277 > +     else\r
 278 > +         fputs(" ", output);\r
 279 > +    }\r
 280 > +    if(state->stack != NULL) {\r
 281 > +     state->stack->first_already_seen = TRUE;\r
 282 > +    }\r
 283 \r
 284 You repeat the above code (or something very similar) in several\r
 285 places.  It would make sense to put it in a separate function,\r
 286 possibly with an additional char argument to control whether to follow\r
 287 the comma with a newline or a space.\r
 288 \r
 289 > +    fputs("{", output);\r
 290 > +    void *ctx_json_map = talloc_new (0);\r
 291 > +    json_list_t *el = talloc(ctx_json_map, json_list_t);\r
 292 \r
 293 You're leaking ctx_json_map here.  It's also not necessary: you should\r
 294 use st as the talloc context for el.  It still makes sense to free\r
 295 these as you pop, but that way if the caller needs to abort it can\r
 296 free the whole state structure in one swoop.\r
 297 \r
 298 > +    el->type = TYPE_JSON_MAP;\r
 299 > +    el->first_already_seen = FALSE;\r
 300 > +    el->rest = state->stack;\r
 301 > +    state->stack = el;\r
 302 > +    return state->level++;\r
 303 > +}\r
 304 > +\r
 305 > +int json_list(void *st) {\r
 306 > +    json_state_t *state = (json_state_t*)st;\r
 307 > +    FILE *output = state->output;\r
 308 > +    if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
 309 > +     fputs(",", output);\r
 310 > +     if(state->level == 1)\r
 311 > +         fputs("\n", output);\r
 312 > +     else\r
 313 > +         fputs(" ", output);\r
 314 > +    }\r
 315 > +    if(state->stack != NULL) {\r
 316 > +     state->stack->first_already_seen = TRUE;\r
 317 > +    }\r
 318 > +    fputs("[", output);\r
 319 > +    void *ctx_json_map = talloc_new (0);\r
 320 > +    json_list_t *el = talloc(ctx_json_map, json_list_t);\r
 321 > +    el->type = TYPE_JSON_ARRAY;\r
 322 > +    el->first_already_seen = FALSE;\r
 323 > +    el->rest = state->stack;\r
 324 > +    state->stack = el;\r
 325 > +    return state->level++;\r
 326 > +}\r
 327 > +\r
 328 > +void json_pop(void *st, int level) {\r
 329 > +    int i;\r
 330 > +    json_state_t *state = (json_state_t*)st;\r
 331 > +    FILE *output = state->output;\r
 332 > +    for(i = state->level; i > level; i--) {\r
 333 \r
 334 Maybe "while (state->level > level)"?\r
 335 \r
 336 > +     json_list_t *tos = state->stack;\r
 337 > +     if(tos->type == TYPE_JSON_MAP) {\r
 338 > +         fputs("}", output);\r
 339 > +     }\r
 340 > +     if(tos->type == TYPE_JSON_ARRAY) {\r
 341 \r
 342 Maybe "else if"?\r
 343 \r
 344 > +         fputs("]", output);\r
 345 > +     }\r
 346 > +     state->stack = tos->rest;\r
 347 > +     state->level--;\r
 348 > +     talloc_free(tos);\r
 349 > +    }\r
 350 > +    if(state->level == 0)\r
 351 > +     fputs("\n", output);\r
 352 > +}\r
 353 > +\r
 354 > +void json_map_key(void *st, const char *key) {\r
 355 > +    json_state_t *state = (json_state_t*)st;\r
 356 > +    FILE *output = state->output;\r
 357 > +    if(state->stack != NULL && state->stack->first_already_seen) {\r
 358 > +     fputs(",\n", output);\r
 359 > +    }\r
 360 > +    fputs("\"", output);\r
 361 > +    fputs(key, output);\r
 362 \r
 363 The key needs to be escaped like any JSON string.\r
 364 \r
 365 > +    fputs("\": ", output);\r
 366 > +}\r
 367 > +\r
 368 > +void json_number(void *st, int val) {\r
 369 > +    json_state_t *state = (json_state_t*)st;\r
 370 > +    FILE *output = state->output;\r
 371 > +    if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
 372 > +     fputs(", ", output);\r
 373 > +    }\r
 374 > +    state->stack->first_already_seen = TRUE;\r
 375 > +    fprintf(output, "%i", val);\r
 376 \r
 377 "%d" is much more common (I had to look up %i!)\r
 378 \r
 379 > +}\r
 380 > +\r
 381 > +void json_string(void *st, const char *val) {\r
 382 > +    json_state_t *state = (json_state_t*)st;\r
 383 > +    FILE *output = state->output;\r
 384 > +    void *ctx = talloc_new(0);\r
 385 > +    if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
 386 > +     fputs(",", output);\r
 387 > +     if(state->level == 1)\r
 388 > +         fputs("\n", output);\r
 389 > +     else\r
 390 > +         fputs(" ", output);\r
 391 > +    }\r
 392 > +\r
 393 > +    state->stack->first_already_seen = TRUE;\r
 394 > +    fprintf(output, "%s", json_quote_str(ctx, val));\r
 395 \r
 396 Take a look at the string quoting function in\r
 397 id:"87d34hsdx8.fsf@awakening.csail.mit.edu".  It quotes directly to a\r
 398 FILE * without allocating an intermediate buffer.  It's also actually\r
 399 correct, unlike json_quote_str.\r
 400 \r
 401 > +    talloc_free(ctx);\r
 402 > +}\r
 403 > +\r
 404 > +void json_bool(void *st, notmuch_bool_t val) {\r
 405 > +    json_state_t *state = (json_state_t*)st;\r
 406 > +    FILE *output = state->output;\r
 407 \r
 408 This needs to insert a comma like the other output functions.\r
 409 \r
 410 > +    if(val)\r
 411 > +     fputs("true", output);\r
 412 > +    else\r
 413 > +     fputs("false", output);\r
 414 \r
 415 Maybe fputs(val ? "true" : "false", output)?\r
 416 \r
 417 > +}\r
 418 > +\r
 419 > +void *json_initial_state(const struct structure_printer *sp, FILE *output) {\r
 420 > +    (void)sp;\r
 421 > +    json_state_t *st = talloc(0, json_state_t);\r
 422 > +    st->level = 0;\r
 423 > +    st->stack = NULL;\r
 424 > +    st->output = output;\r
 425 > +    return st;\r
 426 > +}\r
 427 \r
 428 This is as far as I've gotten.  I'll switch to your newer version and\r
 429 review the rest when I get a chance.\r