--- /dev/null
+Return-Path: <amdragon@mit.edu>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 12369431FBF\r
+ for <notmuch@notmuchmail.org>; Tue, 10 Jul 2012 12:13:37 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+ tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id ho7jLnyld7BR for <notmuch@notmuchmail.org>;\r
+ Tue, 10 Jul 2012 12:13:35 -0700 (PDT)\r
+Received: from dmz-mailsec-scanner-2.mit.edu (DMZ-MAILSEC-SCANNER-2.MIT.EDU\r
+ [18.9.25.13])\r
+ by olra.theworths.org (Postfix) with ESMTP id AA571431FAE\r
+ for <notmuch@notmuchmail.org>; Tue, 10 Jul 2012 12:13:35 -0700 (PDT)\r
+X-AuditID: 1209190d-b7fd56d000000933-0d-4ffc7edec63f\r
+Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
+ by dmz-mailsec-scanner-2.mit.edu (Symantec Messaging Gateway) with SMTP\r
+ id 65.B0.02355.EDE7CFF4; Tue, 10 Jul 2012 15:13:34 -0400 (EDT)\r
+Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
+ by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id q6AJDXmL009316; \r
+ Tue, 10 Jul 2012 15:13:34 -0400\r
+Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
+ (authenticated bits=0)\r
+ (User authenticated as amdragon@ATHENA.MIT.EDU)\r
+ by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q6AJDWY5012664\r
+ (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
+ Tue, 10 Jul 2012 15:13:33 -0400 (EDT)\r
+Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
+ (envelope-from <amdragon@mit.edu>)\r
+ id 1SofsK-0005gp-2V; Tue, 10 Jul 2012 15:13:32 -0400\r
+Date: Tue, 10 Jul 2012 15:13:31 -0400\r
+From: Austin Clements <amdragon@MIT.EDU>\r
+To: craven@gmx.net\r
+Subject: Re: [PATCH] v2: Added better support for multiple structured output\r
+ formats.\r
+Message-ID: <20120710191331.GE7332@mit.edu>\r
+References: <87liisue70.fsf@nexoid.at> <87fw8zvqmm.fsf@nexoid.at>\r
+ <87liir944f.fsf@qmul.ac.uk> <87a9z7vj3p.fsf@nexoid.at>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+Content-Disposition: inline\r
+In-Reply-To: <87a9z7vj3p.fsf@nexoid.at>\r
+User-Agent: Mutt/1.5.21 (2010-09-15)\r
+X-Brightmail-Tracker:\r
+ H4sIAAAAAAAAA+NgFprAKsWRmVeSWpSXmKPExsUixCmqrXuv7o+/wa79whZ7G9oZLa7fnMns\r
+ wOSxeNN+No9nq24xBzBFcdmkpOZklqUW6dslcGW82/KBraA3veLx0yOMDYwtgV2MnBwSAiYS\r
+ j47MZ4ewxSQu3FvP1sXIxSEksI9R4ufTNiYIZwOjxKddD9khnJNMEk+utEA5Sxglds6/zgzS\r
+ zyKgKnH201ewWWwCGhLb9i9nBLFFBIQkJn15xQJiMwtIS3z73cwEYgsLhEv8bYXo5RXQljj9\r
+ 6hFYXEigTmLB7iuMEHFBiZMzn0D1aknc+PcSqIYDbM7yfxwgYU4BdYmP+16AjREVUJGYcnIb\r
+ 2wRGoVlIumch6Z6F0L2AkXkVo2xKbpVubmJmTnFqsm5xcmJeXmqRrpFebmaJXmpK6SZGcGBL\r
+ 8u5gfHdQ6RCjAAejEg/vjLQ//kKsiWXFlbmHGCU5mJREeadUAoX4kvJTKjMSizPii0pzUosP\r
+ MUpwMCuJ8D50BsrxpiRWVqUW5cOkpDlYlMR5r6Tc9BcSSE8sSc1OTS1ILYLJynBwKEnwRgAj\r
+ WEiwKDU9tSItM6cEIc3EwQkynAdouB5IDW9xQWJucWY6RP4Uoy7HujdHbjAKseTl56VKifNa\r
+ gRQJgBRllObBzYElpFeM4kBvCfNag1TxAJMZ3KRXQEuYgJa09/wCWVKSiJCSamBcNeu7nHTK\r
+ wWknWFRDjk1LWSBitcmrhjn0VhbPghqNZ5br3LW2neKfp8W38cl9c8GPvRLZhrxLL38Qm56V\r
+ amD3vZGnRfDvbhHVxjTP7xlcE573+67WTTQ6piv7/mPYhyz1/e8CN2l8Otb+MNbEIMNvTd3n\r
+ vLhoywCFAI+VaROr5p5vN0mVn6fEUpyRaKjFXFScCAC+NB5/IwMAAA==\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Tue, 10 Jul 2012 19:13:37 -0000\r
+\r
+Since it would be great to use the structure printer for show as well,\r
+it would make sense to put it in its own source file, where it can\r
+easily be shared between commands.\r
+\r
+There are a few systematic code formatting problems in your patch. To\r
+be consistent with other notmuch code, there should be a space between\r
+function names and parameter lists (in both declarations and calls)\r
+and there should be a space between a keyword and a paren (e.g., "if\r
+(" and "for ("). In a function definition, the function name should\r
+start on a new line (this makes it easy to grep for a function's\r
+definition). Also, in general, try to keep lines under 80 characters\r
+wide (we're not very consistent about this one, but it would be nice\r
+to not become even less consistent about it).\r
+\r
+More detailed comments below.\r
+\r
+Quoth craven@gmx.net on Jul 10 at 3:30 pm:\r
+> As discussed in <id:20120121220407.GK16740@mit.edu>, this patch adds\r
+> support for new structured output formats (like s-expressions) by using\r
+> stateful structure_printers. An implementation of the JSON structure\r
+> printer that passes all tests is included. The output for JSON (and\r
+> text) is identical to the current output. S-Expressions will be added in\r
+> a later patch.\r
+> \r
+> A large part of this patch just implements the differentiation between\r
+> structured and non-structured output (all the code within \r
+> "if(format == unstructured_text_printer)").\r
+> \r
+> In a second patch, the structured output code should be isolated in a\r
+> separate file, and also used in all other parts of notmuch.\r
+> \r
+> The interface is a structure structure_printer, which contains the following methods:\r
+> \r
+> - 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
+> - 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
+> - 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
+> - pop: is called to return to a given nesting level. All lists and maps with a deeper nesting level must be closed.\r
+> - number, string, bool: output one element of the specific type.\r
+> \r
+> All functions should use the state object to insert delimiters etc. automatically when appropriate.\r
+> \r
+> Example:\r
+> int top, one;\r
+> top = map(state);\r
+> map_key(state, "foo");\r
+> one = list(state);\r
+> number(state, 1);\r
+> number(state, 2);\r
+> number(state, 3);\r
+> pop(state, i);\r
+> map_key(state, "bar");\r
+> map(state);\r
+> map_key(state, "baaz");\r
+> string(state, "hello world");\r
+> pop(state, top);\r
+> \r
+> would output JSON as follows:\r
+> \r
+> {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}\r
+> ---\r
+> notmuch-search.c | 491 ++++++++++++++++++++++++++++++++++++++++---------------\r
+> 1 file changed, 361 insertions(+), 130 deletions(-)\r
+> \r
+> diff --git a/notmuch-search.c b/notmuch-search.c\r
+> index 3be296d..4127777 100644\r
+> --- a/notmuch-search.c\r
+> +++ b/notmuch-search.c\r
+> @@ -28,6 +28,210 @@ typedef enum {\r
+> OUTPUT_TAGS\r
+> } output_t;\r
+> \r
+> +/* structured formatting, useful for JSON, S-Expressions, ...\r
+> +\r
+> +- 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
+> +- 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
+> +- 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
+> +- pop: is called to return to a given nesting level. All lists and maps with a deeper nesting level must be closed.\r
+> +- number, string, bool: output one element of the specific type.\r
+> +\r
+> +All functions should use state to insert delimiters etc. automatically when appropriate.\r
+> +\r
+> +Example:\r
+> +int top, one;\r
+> +top = map(state);\r
+> +map_key(state, "foo");\r
+> +one = list(state);\r
+> +number(state, 1);\r
+> +number(state, 2);\r
+> +number(state, 3);\r
+> +pop(state, i);\r
+> +map_key(state, "bar");\r
+> +map(state);\r
+> +map_key(state, "baaz");\r
+> +string(state, "hello world");\r
+> +pop(state, top);\r
+> +\r
+> +would output JSON as follows:\r
+> +\r
+> +{"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}\r
+> +\r
+> + */\r
+\r
+The above comment should follow the style of other notmuch comments:\r
+\r
+/* Put a star before every line, use full sentences starting with a\r
+ * capital letter, and wrap the comment at 72 columns.\r
+ */\r
+\r
+For the descriptions of the function pointers, it probably makes sense\r
+to comment them inline in the struct itself so the reader doesn't have\r
+to match things up. E.g.,\r
+\r
+typedef struct structure_printer {\r
+ /* (Description of map.) */\r
+ int (*map) (void *state);\r
+ ...\r
+} structure_printer_t;\r
+\r
+> +typedef struct structure_printer {\r
+> + int (*map)(void *state);\r
+\r
+This is a rather less obvious application of notmuch code style, but\r
+there should be a space before the parameter list here, too:\r
+ int (*map) (void *state);\r
+Likewise for the other fields, of course.\r
+\r
+> + int (*list)(void *state);\r
+> + void (*pop)(void *state, int level);\r
+> + void (*map_key)(void *state, const char *key);\r
+> + void (*number)(void *state, int val);\r
+> + void (*string)(void *state, const char *val);\r
+> + void (*bool)(void *state, notmuch_bool_t val);\r
+> + void *(*initial_state)(const struct structure_printer *sp, FILE *output);\r
+> +} structure_printer_t;\r
+\r
+Is there a reason to separate the structure printer's vtable from its\r
+state? It seems like this forces the caller to keep track of two\r
+things instead of just one. For show this isn't too bad because the\r
+structure printer is the formatter, but I think it will be cumbersome\r
+for the search code. Keeping them together would also be at least\r
+nominally more typesafe.\r
+\r
+> +\r
+> +/* JSON structure printer */\r
+> +\r
+> +/* single linked list implementation for keeping track of the array/map nesting state */\r
+\r
+Same comment formatting comment here.\r
+\r
+> +typedef struct json_list {\r
+> + int type;\r
+> + int first_already_seen;\r
+\r
+Maybe just "first_seen"? The "already" doesn't add anything. Also,\r
+this should probably be a notmuch_bool_t.\r
+\r
+> + struct json_list *rest;\r
+> +} json_list_t;\r
+> +\r
+> +#define TYPE_JSON_MAP 1\r
+> +#define TYPE_JSON_ARRAY 2\r
+\r
+An enum would be preferable here.\r
+\r
+> +\r
+> +typedef struct json_state {\r
+> + FILE *output;\r
+> + json_list_t *stack;\r
+> + int level;\r
+> +} json_state_t;\r
+> +\r
+> +int json_map(void *state);\r
+> +int json_list(void *state);\r
+> +void json_pop(void *state, int level);\r
+> +void json_map_key(void *state, const char *key);\r
+> +void json_number(void *state, int val);\r
+> +void json_string(void *state, const char *val);\r
+> +void json_bool(void *state, notmuch_bool_t val);\r
+> +void *json_initial_state(const struct structure_printer *sp, FILE *output);\r
+> +\r
+> +structure_printer_t json_structure_printer = {\r
+> + &json_map,\r
+> + &json_list,\r
+> + &json_pop,\r
+> + &json_map_key,\r
+> + &json_number,\r
+> + &json_string,\r
+> + &json_bool,\r
+> + &json_initial_state\r
+> +};\r
+> +\r
+> +int json_map(void *st) {\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+> + if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
+> + fputs(",", output);\r
+> + if(state->level == 1)\r
+> + fputs("\n", output);\r
+> + else\r
+> + fputs(" ", output);\r
+> + }\r
+> + if(state->stack != NULL) {\r
+> + state->stack->first_already_seen = TRUE;\r
+> + }\r
+\r
+You repeat the above code (or something very similar) in several\r
+places. It would make sense to put it in a separate function,\r
+possibly with an additional char argument to control whether to follow\r
+the comma with a newline or a space.\r
+\r
+> + fputs("{", output);\r
+> + void *ctx_json_map = talloc_new (0);\r
+> + json_list_t *el = talloc(ctx_json_map, json_list_t);\r
+\r
+You're leaking ctx_json_map here. It's also not necessary: you should\r
+use st as the talloc context for el. It still makes sense to free\r
+these as you pop, but that way if the caller needs to abort it can\r
+free the whole state structure in one swoop.\r
+\r
+> + el->type = TYPE_JSON_MAP;\r
+> + el->first_already_seen = FALSE;\r
+> + el->rest = state->stack;\r
+> + state->stack = el;\r
+> + return state->level++;\r
+> +}\r
+> +\r
+> +int json_list(void *st) {\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+> + if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
+> + fputs(",", output);\r
+> + if(state->level == 1)\r
+> + fputs("\n", output);\r
+> + else\r
+> + fputs(" ", output);\r
+> + }\r
+> + if(state->stack != NULL) {\r
+> + state->stack->first_already_seen = TRUE;\r
+> + }\r
+> + fputs("[", output);\r
+> + void *ctx_json_map = talloc_new (0);\r
+> + json_list_t *el = talloc(ctx_json_map, json_list_t);\r
+> + el->type = TYPE_JSON_ARRAY;\r
+> + el->first_already_seen = FALSE;\r
+> + el->rest = state->stack;\r
+> + state->stack = el;\r
+> + return state->level++;\r
+> +}\r
+> +\r
+> +void json_pop(void *st, int level) {\r
+> + int i;\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+> + for(i = state->level; i > level; i--) {\r
+\r
+Maybe "while (state->level > level)"?\r
+\r
+> + json_list_t *tos = state->stack;\r
+> + if(tos->type == TYPE_JSON_MAP) {\r
+> + fputs("}", output);\r
+> + }\r
+> + if(tos->type == TYPE_JSON_ARRAY) {\r
+\r
+Maybe "else if"?\r
+\r
+> + fputs("]", output);\r
+> + }\r
+> + state->stack = tos->rest;\r
+> + state->level--;\r
+> + talloc_free(tos);\r
+> + }\r
+> + if(state->level == 0)\r
+> + fputs("\n", output);\r
+> +}\r
+> +\r
+> +void json_map_key(void *st, const char *key) {\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+> + if(state->stack != NULL && state->stack->first_already_seen) {\r
+> + fputs(",\n", output);\r
+> + }\r
+> + fputs("\"", output);\r
+> + fputs(key, output);\r
+\r
+The key needs to be escaped like any JSON string.\r
+\r
+> + fputs("\": ", output);\r
+> +}\r
+> +\r
+> +void json_number(void *st, int val) {\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+> + if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
+> + fputs(", ", output);\r
+> + }\r
+> + state->stack->first_already_seen = TRUE;\r
+> + fprintf(output, "%i", val);\r
+\r
+"%d" is much more common (I had to look up %i!)\r
+\r
+> +}\r
+> +\r
+> +void json_string(void *st, const char *val) {\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+> + void *ctx = talloc_new(0);\r
+> + if(state->stack != NULL && state->stack->type == TYPE_JSON_ARRAY && state->stack->first_already_seen) {\r
+> + fputs(",", output);\r
+> + if(state->level == 1)\r
+> + fputs("\n", output);\r
+> + else\r
+> + fputs(" ", output);\r
+> + }\r
+> +\r
+> + state->stack->first_already_seen = TRUE;\r
+> + fprintf(output, "%s", json_quote_str(ctx, val));\r
+\r
+Take a look at the string quoting function in\r
+id:"87d34hsdx8.fsf@awakening.csail.mit.edu". It quotes directly to a\r
+FILE * without allocating an intermediate buffer. It's also actually\r
+correct, unlike json_quote_str.\r
+\r
+> + talloc_free(ctx);\r
+> +}\r
+> +\r
+> +void json_bool(void *st, notmuch_bool_t val) {\r
+> + json_state_t *state = (json_state_t*)st;\r
+> + FILE *output = state->output;\r
+\r
+This needs to insert a comma like the other output functions.\r
+\r
+> + if(val)\r
+> + fputs("true", output);\r
+> + else\r
+> + fputs("false", output);\r
+\r
+Maybe fputs(val ? "true" : "false", output)?\r
+\r
+> +}\r
+> +\r
+> +void *json_initial_state(const struct structure_printer *sp, FILE *output) {\r
+> + (void)sp;\r
+> + json_state_t *st = talloc(0, json_state_t);\r
+> + st->level = 0;\r
+> + st->stack = NULL;\r
+> + st->output = output;\r
+> + return st;\r
+> +}\r
+\r
+This is as far as I've gotten. I'll switch to your newer version and\r
+review the rest when I get a chance.\r
projects / notmuch-archives.git / commitdiff