--- /dev/null
+Return-Path: <m.walters@qmul.ac.uk>\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 13A20431E64\r
+ for <notmuch@notmuchmail.org>; Tue, 10 Jul 2012 05:45:31 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -1.098\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
+ tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
+ NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] 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 sV1no7SKsaoG for <notmuch@notmuchmail.org>;\r
+ Tue, 10 Jul 2012 05:45:28 -0700 (PDT)\r
+Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
+ (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 325E5431FBF\r
+ for <notmuch@notmuchmail.org>; Tue, 10 Jul 2012 05:45:28 -0700 (PDT)\r
+Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
+ by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
+ (envelope-from <m.walters@qmul.ac.uk>)\r
+ id 1SoZoh-0004fV-DE; Tue, 10 Jul 2012 13:45:24 +0100\r
+Received: from 94-192-233-223.zone6.bethere.co.uk ([94.192.233.223]\r
+ helo=localhost)\r
+ by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
+ (envelope-from <m.walters@qmul.ac.uk>)\r
+ id 1SoZog-0007mM-53; Tue, 10 Jul 2012 13:45:23 +0100\r
+From: Mark Walters <markwalters1009@gmail.com>\r
+To: craven@gmx.net, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH] FIXED: Added better support for multiple structured\r
+ output formats.\r
+In-Reply-To: <87fw8zvqmm.fsf@nexoid.at>\r
+References: <87liisue70.fsf@nexoid.at> <87fw8zvqmm.fsf@nexoid.at>\r
+User-Agent: Notmuch/0.13.2+61~gf708609 (http://notmuchmail.org) Emacs/23.4.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Tue, 10 Jul 2012 13:45:20 +0100\r
+Message-ID: <87liir944f.fsf@qmul.ac.uk>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+X-Sender-Host-Address: 94.192.233.223\r
+X-QM-SPAM-Info: Sender has good ham record. :)\r
+X-QM-Body-MD5: 88d3bcae25d4eb1ece97e842e0d7dc78 (of first 20000 bytes)\r
+X-SpamAssassin-Score: -1.8\r
+X-SpamAssassin-SpamBar: -\r
+X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
+ determine if it is\r
+ spam. We require at least 5.0 points to mark a message as spam.\r
+ This message scored -1.8 points.\r
+ Summary of the scoring: \r
+ * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
+ * medium trust\r
+ * [138.37.6.40 listed in list.dnswl.org]\r
+ * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
+ provider * (markwalters1009[at]gmail.com)\r
+ * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay\r
+ * domain\r
+ * 0.5 AWL AWL: From: address is in the auto white-list\r
+X-QM-Scan-Virus: ClamAV says the message is clean\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 12:45:31 -0000\r
+\r
+\r
+> notmuch-search.c | 453 +++++++++++++++++++++++++++++++++++++++----------------\r
+> 1 file changed, 326 insertions(+), 127 deletions(-)\r
+\r
+Hi for such a large patch this was surprisingly easy to follow (though I\r
+am not saying I have worked through all the details). However, there are\r
+some preliminary comments below which I think are worth fixing before a\r
+full review as they will make it easier for other reviewers. One thing\r
+that would help a lot is some comments.\r
+\r
+Best wishes\r
+\r
+Mark\r
+\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.\r
+\r
+If I understand it correctly, the output should be identical to before?\r
+If that is the case I think it's worth saying. \r
+\r
+> Structure printers have functions for starting a map, a list, closing\r
+> any number of these two, printing a map key, a number value, a string\r
+> value, and boolean values. By threading a state through the\r
+> invocations, arbitrary structured formatting can be achieved.\r
+\r
+I think I would also add something saying that the text format is just\r
+different (and that a significant chunk of the patch is just that).\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
+>\r
+> diff --git a/notmuch-search.c b/notmuch-search.c\r
+> index 3be296d..3413b79 100644\r
+> --- a/notmuch-search.c\r
+> +++ b/notmuch-search.c\r
+> @@ -28,6 +28,181 @@ typedef enum {\r
+> OUTPUT_TAGS\r
+> } output_t;\r
+> \r
+> +typedef void * structure_printer_state_t;\r
+> +\r
+> +typedef struct structure_printer {\r
+> + int (*map)(void *state);\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
+I think this needs some comments on what these functions do. number,\r
+string and boolean are relatively clear (but saying "output a number"\r
+etc is worthwhile). But what map and list do is much less clear and\r
+definitely deserves a comment. And it is definitely unclear what they\r
+are meant to return.\r
+\r
+I would also say something about the variable state: eg "the variable\r
+`state` can contain any state the structure_printer wishes to maintain".\r
+\r
+> +\r
+> +/* JSON structure printer */\r
+> +\r
+> +typedef struct json_list {\r
+> + int type;\r
+> + int first_already_seen;\r
+> + struct json_list *rest;\r
+> +} json_list_t;\r
+> +\r
+> +#define TYPE_JSON_MAP 1\r
+> +#define TYPE_JSON_ARRAY 2\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
+> +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
+> + 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_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
+> + 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
+> + 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
+> + 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
+> +\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
+> + 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
+> + if(val)\r
+> + fputs("true", output);\r
+> + else\r
+> + fputs("false", output);\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
+> +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
+Since you forward declare all these functions I think this would be more\r
+natural before the full declarations.\r
+\r
+> +structure_printer_t *text_structure_printer = NULL;\r
+\r
+I would rename this particularly given the next comment: it makes it\r
+seem like there is non-structured text output and structured text\r
+output. Maybe `unstructured_text_printer`? \r
+> +\r
+> +/* legacy, only needed for non-structured text output */\r
+> typedef struct search_format {\r
+> const char *results_start;\r
+> const char *item_start;\r
+> @@ -51,6 +226,7 @@ typedef struct search_format {\r
+> const char *results_null;\r
+> } search_format_t;\r
+> \r
+> +\r
+> static void\r
+> format_item_id_text (const void *ctx,\r
+> const char *item_type,\r
+\r
+just a whitespace change so omit.\r
+\r
+> @@ -64,6 +240,7 @@ format_thread_text (const void *ctx,\r
+> const int total,\r
+> const char *authors,\r
+> const char *subject);\r
+> +\r
+> static const search_format_t format_text = {\r
+> "",\r
+> "",\r
+\r
+just a whitespace change so omit.\r
+\r
+(there are also some trailing whitespaces in various of the lines that\r
+should be omitted).\r
+\r
+> @@ -78,35 +255,6 @@ static const search_format_t format_text = {\r
+> };\r
+> \r
+> static void\r
+> -format_item_id_json (const void *ctx,\r
+> - const char *item_type,\r
+> - const char *item_id);\r
+> -\r
+> -static void\r
+> -format_thread_json (const void *ctx,\r
+> - const char *thread_id,\r
+> - const time_t date,\r
+> - const int matched,\r
+> - const int total,\r
+> - const char *authors,\r
+> - const char *subject);\r
+> -\r
+> -/* Any changes to the JSON format should be reflected in the file\r
+> - * devel/schemata. */\r
+> -static const search_format_t format_json = {\r
+> - "[",\r
+> - "{",\r
+> - format_item_id_json,\r
+> - format_thread_json,\r
+> - "\"tags\": [",\r
+> - "\"%s\"", ", ",\r
+> - "]", ",\n",\r
+> - "}",\r
+> - "]\n",\r
+> - "]\n",\r
+> -};\r
+> -\r
+> -static void\r
+> format_item_id_text (unused (const void *ctx),\r
+> const char *item_type,\r
+> const char *item_id)\r
+> @@ -153,50 +301,9 @@ format_thread_text (const void *ctx,\r
+> talloc_free (ctx_quote);\r
+> }\r
+> \r
+> -static void\r
+> -format_item_id_json (const void *ctx,\r
+> - unused (const char *item_type),\r
+> - const char *item_id)\r
+> -{\r
+> - void *ctx_quote = talloc_new (ctx);\r
+> -\r
+> - printf ("%s", json_quote_str (ctx_quote, item_id));\r
+> -\r
+> - talloc_free (ctx_quote);\r
+> - \r
+> -}\r
+> -\r
+> -static void\r
+> -format_thread_json (const void *ctx,\r
+> - const char *thread_id,\r
+> - const time_t date,\r
+> - const int matched,\r
+> - const int total,\r
+> - const char *authors,\r
+> - const char *subject)\r
+> -{\r
+> - void *ctx_quote = talloc_new (ctx);\r
+> -\r
+> - printf ("\"thread\": %s,\n"\r
+> - "\"timestamp\": %ld,\n"\r
+> - "\"date_relative\": \"%s\",\n"\r
+> - "\"matched\": %d,\n"\r
+> - "\"total\": %d,\n"\r
+> - "\"authors\": %s,\n"\r
+> - "\"subject\": %s,\n",\r
+> - json_quote_str (ctx_quote, thread_id),\r
+> - date,\r
+> - notmuch_time_relative_date (ctx, date),\r
+> - matched,\r
+> - total,\r
+> - json_quote_str (ctx_quote, authors),\r
+> - json_quote_str (ctx_quote, subject));\r
+> -\r
+> - talloc_free (ctx_quote);\r
+> -}\r
+> -\r
+> static int\r
+> -do_search_threads (const search_format_t *format,\r
+> +do_search_threads (const structure_printer_t *format,\r
+> + void *state,\r
+> notmuch_query_t *query,\r
+> notmuch_sort_t sort,\r
+> output_t output,\r
+> @@ -209,7 +316,8 @@ do_search_threads (const search_format_t *format,\r
+> time_t date;\r
+> int first_thread = 1;\r
+> int i;\r
+> -\r
+> + int outermost_level = 0;\r
+> + int items_level = 0;\r
+> if (offset < 0) {\r
+> offset += notmuch_query_count_threads (query);\r
+> if (offset < 0)\r
+> @@ -220,7 +328,10 @@ do_search_threads (const search_format_t *format,\r
+> if (threads == NULL)\r
+> return 1;\r
+> \r
+> - fputs (format->results_start, stdout);\r
+> + if(format == text_structure_printer)\r
+> + fputs(format_text.results_start, stdout);\r
+> + else\r
+> + outermost_level = format->list(state);\r
+> \r
+> for (i = 0;\r
+> notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);\r
+> @@ -235,43 +346,93 @@ do_search_threads (const search_format_t *format,\r
+> continue;\r
+> }\r
+> \r
+> - if (! first_thread)\r
+> - fputs (format->item_sep, stdout);\r
+> + if (format == text_structure_printer && ! first_thread)\r
+> + fputs (format_text.item_sep, stdout);\r
+> \r
+> if (output == OUTPUT_THREADS) {\r
+> - format->item_id (thread, "thread:",\r
+> - notmuch_thread_get_thread_id (thread));\r
+> + if(format == text_structure_printer) {\r
+> + format_text.item_id (thread, "thread:",\r
+> + notmuch_thread_get_thread_id (thread));\r
+> + }\r
+> + else {\r
+> + char buffer[128];\r
+> + strncpy(buffer, "thread:", 1 + strlen("thread:"));\r
+> + strncat(buffer, notmuch_thread_get_thread_id (thread), 128 - strlen("thread:"));\r
+> + format->string(state, buffer);\r
+\r
+This seems rather ugly: wouldn't a snprintf or possibly a talloc_printf\r
+or something be nicer?\r
+\r
+> + }\r
+> + \r
+> } else { /* output == OUTPUT_SUMMARY */\r
+> - fputs (format->item_start, stdout);\r
+> + int tags_level = 0;\r
+> + void *ctx = talloc_new (0);\r
+> +\r
+> + if(format == text_structure_printer)\r
+> + fputs (format_text.item_start, stdout);\r
+> + else\r
+> + items_level = format->map(state);\r
+> \r
+> if (sort == NOTMUCH_SORT_OLDEST_FIRST)\r
+> date = notmuch_thread_get_oldest_date (thread);\r
+> else\r
+> date = notmuch_thread_get_newest_date (thread);\r
+> \r
+> - format->thread_summary (thread,\r
+> - notmuch_thread_get_thread_id (thread),\r
+> - date,\r
+> - notmuch_thread_get_matched_messages (thread),\r
+> - notmuch_thread_get_total_messages (thread),\r
+> - notmuch_thread_get_authors (thread),\r
+> - notmuch_thread_get_subject (thread));\r
+> + if(format == text_structure_printer) {\r
+> + format_text.thread_summary (thread,\r
+> + notmuch_thread_get_thread_id (thread),\r
+> + date,\r
+> + notmuch_thread_get_matched_messages (thread),\r
+> + notmuch_thread_get_total_messages (thread),\r
+> + notmuch_thread_get_authors (thread),\r
+> + notmuch_thread_get_subject (thread));\r
+> + } else {\r
+> + format->map_key(state, "thread");\r
+> + format->string(state, notmuch_thread_get_thread_id (thread));\r
+> + format->map_key(state, "timestamp");\r
+> + format->number(state, date);\r
+> + format->map_key(state, "date_relative");\r
+> + format->string(state, notmuch_time_relative_date(ctx, date));\r
+> + format->map_key(state, "matched");\r
+> + format->number(state, notmuch_thread_get_matched_messages(thread));\r
+> + format->map_key(state, "total");\r
+> + format->number(state, notmuch_thread_get_total_messages(thread));\r
+> + format->map_key(state, "authors");\r
+> + format->string(state, notmuch_thread_get_authors(thread));\r
+> + format->map_key(state, "subject");\r
+> + format->string(state, notmuch_thread_get_subject(thread));\r
+> + }\r
+> +\r
+> + if(format == text_structure_printer) {\r
+> + fputs (format_text.tag_start, stdout);\r
+> + } else {\r
+> + format->map_key(state, "tags");\r
+> +\r
+> + tags_level = format->list(state);\r
+> + }\r
+> \r
+> - fputs (format->tag_start, stdout);\r
+> \r
+> for (tags = notmuch_thread_get_tags (thread);\r
+> notmuch_tags_valid (tags);\r
+> notmuch_tags_move_to_next (tags))\r
+> {\r
+> - if (! first_tag)\r
+> - fputs (format->tag_sep, stdout);\r
+> - printf (format->tag, notmuch_tags_get (tags));\r
+> + if (format == text_structure_printer && ! first_tag) {\r
+> + fputs (format_text.tag_sep, stdout);\r
+> + }\r
+> + if(format == text_structure_printer) {\r
+> + printf (format_text.tag, notmuch_tags_get (tags));\r
+> + } else {\r
+> + format->string(state, notmuch_tags_get(tags));\r
+> + }\r
+> first_tag = 0;\r
+> }\r
+> \r
+> - fputs (format->tag_end, stdout);\r
+> + if(format == text_structure_printer)\r
+> + fputs (format_text.tag_end, stdout);\r
+> + else\r
+> + format->pop(state, tags_level);\r
+> \r
+> - fputs (format->item_end, stdout);\r
+> + if(format == text_structure_printer)\r
+> + fputs (format_text.item_end, stdout);\r
+> + else\r
+> + format->pop(state, items_level);\r
+> }\r
+> \r
+> first_thread = 0;\r
+> @@ -279,16 +440,21 @@ do_search_threads (const search_format_t *format,\r
+> notmuch_thread_destroy (thread);\r
+> }\r
+> \r
+> - if (first_thread)\r
+> - fputs (format->results_null, stdout);\r
+> - else\r
+> - fputs (format->results_end, stdout);\r
+> + if(format == text_structure_printer) {\r
+> + if (first_thread)\r
+> + fputs (format_text.results_null, stdout);\r
+> + else\r
+> + fputs (format_text.results_end, stdout);\r
+> + } else {\r
+> + format->pop(state, outermost_level);\r
+> + }\r
+> \r
+> return 0;\r
+> }\r
+> \r
+> static int\r
+> -do_search_messages (const search_format_t *format,\r
+> +do_search_messages (const structure_printer_t *format,\r
+> + void *state,\r
+> notmuch_query_t *query,\r
+> output_t output,\r
+> int offset,\r
+> @@ -299,6 +465,7 @@ do_search_messages (const search_format_t *format,\r
+> notmuch_filenames_t *filenames;\r
+> int first_message = 1;\r
+> int i;\r
+> + int outermost_level = 0;\r
+> \r
+> if (offset < 0) {\r
+> offset += notmuch_query_count_messages (query);\r
+> @@ -310,7 +477,10 @@ do_search_messages (const search_format_t *format,\r
+> if (messages == NULL)\r
+> return 1;\r
+> \r
+> - fputs (format->results_start, stdout);\r
+> + if(format == text_structure_printer)\r
+> + fputs (format_text.results_start, stdout);\r
+> + else\r
+> + outermost_level = format->list(state);\r
+> \r
+> for (i = 0;\r
+> notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);\r
+> @@ -328,23 +498,32 @@ do_search_messages (const search_format_t *format,\r
+> notmuch_filenames_valid (filenames);\r
+> notmuch_filenames_move_to_next (filenames))\r
+> {\r
+> - if (! first_message)\r
+> - fputs (format->item_sep, stdout);\r
+> -\r
+> - format->item_id (message, "",\r
+> - notmuch_filenames_get (filenames));\r
+> -\r
+> + if(format == text_structure_printer) {\r
+> + if (! first_message)\r
+> + fputs (format_text.item_sep, stdout);\r
+> +\r
+> + format_text.item_id (message, "",\r
+> + notmuch_filenames_get (filenames));\r
+> + } else {\r
+> + format->string(state, notmuch_filenames_get (filenames));\r
+> + }\r
+> + \r
+> first_message = 0;\r
+> }\r
+> \r
+> notmuch_filenames_destroy( filenames );\r
+> \r
+> } else { /* output == OUTPUT_MESSAGES */\r
+> - if (! first_message)\r
+> - fputs (format->item_sep, stdout);\r
+> + if(format == text_structure_printer) {\r
+> + if (! first_message)\r
+> + fputs (format_text.item_sep, stdout);\r
+> +\r
+> + format_text.item_id (message, "id:",\r
+> + notmuch_message_get_message_id (message));\r
+> + } else {\r
+> + format->string(state, notmuch_message_get_message_id (message));\r
+> + }\r
+> \r
+> - format->item_id (message, "id:",\r
+> - notmuch_message_get_message_id (message));\r
+> first_message = 0;\r
+> }\r
+> \r
+> @@ -353,23 +532,29 @@ do_search_messages (const search_format_t *format,\r
+> \r
+> notmuch_messages_destroy (messages);\r
+> \r
+> - if (first_message)\r
+> - fputs (format->results_null, stdout);\r
+> - else\r
+> - fputs (format->results_end, stdout);\r
+> + if(format == text_structure_printer) {\r
+> + if (first_message)\r
+> + fputs (format_text.results_null, stdout);\r
+> + else\r
+> + fputs (format_text.results_end, stdout);\r
+> + } else {\r
+> + format->pop(state, outermost_level);\r
+> + }\r
+> \r
+> return 0;\r
+> }\r
+> \r
+> static int\r
+> do_search_tags (notmuch_database_t *notmuch,\r
+> - const search_format_t *format,\r
+> + const structure_printer_t *format,\r
+> + void *state,\r
+> notmuch_query_t *query)\r
+> {\r
+> notmuch_messages_t *messages = NULL;\r
+> notmuch_tags_t *tags;\r
+> const char *tag;\r
+> int first_tag = 1;\r
+> + int outermost_level = 0;\r
+> \r
+> /* should the following only special case if no excluded terms\r
+> * specified? */\r
+> @@ -387,7 +572,10 @@ do_search_tags (notmuch_database_t *notmuch,\r
+> if (tags == NULL)\r
+> return 1;\r
+> \r
+> - fputs (format->results_start, stdout);\r
+> + if(format == text_structure_printer)\r
+> + fputs (format_text.results_start, stdout);\r
+> + else\r
+> + outermost_level = format->list(state);\r
+> \r
+> for (;\r
+> notmuch_tags_valid (tags);\r
+> @@ -395,10 +583,14 @@ do_search_tags (notmuch_database_t *notmuch,\r
+> {\r
+> tag = notmuch_tags_get (tags);\r
+> \r
+> - if (! first_tag)\r
+> - fputs (format->item_sep, stdout);\r
+> + if(format == text_structure_printer) {\r
+> + if (! first_tag)\r
+> + fputs (format_text.item_sep, stdout);\r
+> \r
+> - format->item_id (tags, "", tag);\r
+> + format_text.item_id (tags, "", tag);\r
+> + } else {\r
+> + format->string(state, tag);\r
+> + }\r
+> \r
+> first_tag = 0;\r
+> }\r
+> @@ -408,10 +600,14 @@ do_search_tags (notmuch_database_t *notmuch,\r
+> if (messages)\r
+> notmuch_messages_destroy (messages);\r
+> \r
+> - if (first_tag)\r
+> - fputs (format->results_null, stdout);\r
+> - else\r
+> - fputs (format->results_end, stdout);\r
+> + if(format == text_structure_printer) {\r
+> + if (first_tag)\r
+> + fputs (format_text.results_null, stdout);\r
+> + else\r
+> + fputs (format_text.results_end, stdout);\r
+> + } else {\r
+> + format->pop(state, outermost_level);\r
+> + }\r
+> \r
+> return 0;\r
+> }\r
+> @@ -430,7 +626,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
+> notmuch_query_t *query;\r
+> char *query_str;\r
+> notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST;\r
+> - const search_format_t *format = &format_text;\r
+> + const structure_printer_t *format = text_structure_printer;\r
+> + void *state = NULL;\r
+> int opt_index, ret;\r
+> output_t output = OUTPUT_SUMMARY;\r
+> int offset = 0;\r
+> @@ -475,10 +672,12 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
+> \r
+> switch (format_sel) {\r
+> case NOTMUCH_FORMAT_TEXT:\r
+> - format = &format_text;\r
+> + format = text_structure_printer;\r
+> + state = 0;\r
+> break;\r
+> case NOTMUCH_FORMAT_JSON:\r
+> - format = &format_json;\r
+> + format = &json_structure_printer;\r
+> + state = format->initial_state(format, stdout);\r
+> break;\r
+> }\r
+> \r
+> @@ -532,14 +731,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])\r
+> default:\r
+> case OUTPUT_SUMMARY:\r
+> case OUTPUT_THREADS:\r
+> - ret = do_search_threads (format, query, sort, output, offset, limit);\r
+> + ret = do_search_threads (format, state, query, sort, output, offset, limit);\r
+> break;\r
+> case OUTPUT_MESSAGES:\r
+> case OUTPUT_FILES:\r
+> - ret = do_search_messages (format, query, output, offset, limit);\r
+> + ret = do_search_messages (format, state, query, output, offset, limit);\r
+> break;\r
+> case OUTPUT_TAGS:\r
+> - ret = do_search_tags (notmuch, format, query);\r
+> + ret = do_search_tags (notmuch, format, state, query);\r
+> break;\r
+> }\r
+> \r
+\r
+One final comment: you have quite a lot of things of the form\r
+\r
+\r
+ if(format == text_structure_printer) {\r
+ do_something\r
+ } else {\r
+ do_something_else\r
+ }\r
+\r
+in some of the cases they are obviously analagous things (eg output a\r
+string) but in other cases they look like they might really just be\r
+different. If there are some of the latter (and I haven't worked through\r
+it carefully enough to be sure) then my preference would be to have them\r
+as\r
+\r
+ if(format == text_structure_printer) {\r
+ do_something\r
+ } \r
+\r
+ if (format != text_structure_printer) {\r
+ do_something_else\r
+ }\r
+\r
+but that is a personal preference and others (and you) may easily disagree.\r
+\r
+Best wishes \r
+\r
+Mark\r
+\r
projects / notmuch-archives.git / commitdiff