From: Austin Clements Date: Wed, 18 Jul 2012 19:56:02 +0000 (+2000) Subject: Re: [PATCH v6 3/3] Use the structured formatters in notmuch-search.c. X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=9f351bc6299c107fb009bffd16ac41f8910d5d65;p=notmuch-archives.git Re: [PATCH v6 3/3] Use the structured formatters in notmuch-search.c. --- diff --git a/e5/18dfef6d6acd9efaeb4228a63171f1fcabe993 b/e5/18dfef6d6acd9efaeb4228a63171f1fcabe993 new file mode 100644 index 000000000..8797a361b --- /dev/null +++ b/e5/18dfef6d6acd9efaeb4228a63171f1fcabe993 @@ -0,0 +1,612 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 14337431FAF + for ; Wed, 18 Jul 2012 12:56:06 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id X5JBicJO2Qfx for ; + Wed, 18 Jul 2012 12:56:04 -0700 (PDT) +Received: from dmz-mailsec-scanner-1.mit.edu (DMZ-MAILSEC-SCANNER-1.MIT.EDU + [18.9.25.12]) + by olra.theworths.org (Postfix) with ESMTP id 9D8DF431FAE + for ; Wed, 18 Jul 2012 12:56:04 -0700 (PDT) +X-AuditID: 1209190c-b7f806d000006b87-56-500714d4dc0b +Received: from mailhub-auth-4.mit.edu ( [18.7.62.39]) + by dmz-mailsec-scanner-1.mit.edu (Symantec Messaging Gateway) with SMTP + id 95.D3.27527.4D417005; Wed, 18 Jul 2012 15:56:04 -0400 (EDT) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id q6IJu4Lw024341; + Wed, 18 Jul 2012 15:56:04 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q6IJu2Ii011856 + (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); + Wed, 18 Jul 2012 15:56:03 -0400 (EDT) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) + (envelope-from ) + id 1SraLq-000165-GP; Wed, 18 Jul 2012 15:56:02 -0400 +Date: Wed, 18 Jul 2012 15:56:02 -0400 +From: Austin Clements +To: craven@gmx.net +Subject: Re: [PATCH v6 3/3] Use the structured formatters in notmuch-search.c. +Message-ID: <20120718195602.GQ31670@mit.edu> +References: <20120714020954.GD31670@mit.edu> + <1342427702-23316-1-git-send-email-craven@gmx.net> + <1342427702-23316-4-git-send-email-craven@gmx.net> +MIME-Version: 1.0 +Content-Type: text/plain; charset=iso-8859-1 +Content-Disposition: inline +Content-Transfer-Encoding: 8bit +In-Reply-To: <1342427702-23316-4-git-send-email-craven@gmx.net> +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFprDKsWRmVeSWpSXmKPExsUixG6nrntFhD3A4P8fHou9De2MFtdvzmR2 + YPJYvGk/m8ezVbeYA5iiuGxSUnMyy1KL9O0SuDK2n1rLXLC5hbGi4f95pgbG98ldjJwcEgIm + Ehf/rmCEsMUkLtxbzwZiCwnsY5T49Ly0i5ELyN7AKHGpaykzhHOSSWJH12x2CGcJo8TuW5PZ + QVpYBFQlHp7dAmazCWhIbNu/HGysiICQxKQvr1hAbGYBaYlvv5uZQGxhAT+JnqWHwdbxCuhI + rDm/FGroNEaJ78++s0IkBCVOznwC1awjsXPrHaAGDrBBy/9xQITlJZq3zmYGCXMK2Es83KED + EhYVUJGYcnIb2wRG4VlIBs1CMmgWwqBZSAYtYGRZxSibklulm5uYmVOcmqxbnJyYl5dapGuo + l5tZopeaUrqJERQJnJI8OxjfHFQ6xCjAwajEw/tgF2uAEGtiWXFl7iFGSQ4mJVHeT0LsAUJ8 + SfkplRmJxRnxRaU5qcWHGCU4mJVEeB8IAuV4UxIrq1KL8mFS0hwsSuK8l1Nu+gsJpCeWpGan + phakFsFkZTg4lCR4k4ERLyRYlJqeWpGWmVOCkGbi4AQZzgM0PAqkhre4IDG3ODMdIn+KUVFK + nLcYJCEAksgozYPrhSWqV4ziQK8I88qBVPEAkxxc9yugwUxAg7mL2UAGlyQipKQaGMvM16pl + L7BmvKvluKPw2XutOxs+2b26fmXCmqbVOQ39CafK//lMKZrc7xH4yb58Zfn2J/VnBE3uyz6f + v9wx683CZ8VywbZSb2fvilhx/cDyhwdlpaYrsDh9z2g6yRp1abb465q7XQ/nJPQxB/2eeErT + fuf2Rr/Tn0VnOOy3cbQROW5bljnvGb8SS3FGoqEWc1FxIgA3t36uLwMAAA== +Cc: notmuch@notmuchmail.org +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Wed, 18 Jul 2012 19:56:06 -0000 + +Just a few comments (don't forget to scroll all the way down). +Overall this is looking pretty good. + +Quoth craven@gmx.net on Jul 16 at 10:35 am: +> This patch switches from the current ad-hoc printer to the structured +> formatters in sprinter.h, sprinter-text-search.c and sprinter-json.c. +> +> The JSON tests are changed slightly in order to make them PASS for the +> new structured output formatter. +> +> The text tests pass without adaptation. +> --- +> notmuch-search.c | 300 ++++++++++++++++--------------------------------------- +> test/json | 18 +--- +> 2 files changed, 86 insertions(+), 232 deletions(-) + +That's a fantastic diffstat. + +> +> diff --git a/notmuch-search.c b/notmuch-search.c +> index 3be296d..cf927e6 100644 +> --- a/notmuch-search.c +> +++ b/notmuch-search.c +> @@ -19,6 +19,7 @@ +> */ +> +> #include "notmuch-client.h" +> +#include "sprinter.h" +> +> typedef enum { +> OUTPUT_SUMMARY, +> @@ -28,92 +29,6 @@ typedef enum { +> OUTPUT_TAGS +> } output_t; +> +> -typedef struct search_format { +> - const char *results_start; +> - const char *item_start; +> - void (*item_id) (const void *ctx, +> - const char *item_type, +> - const char *item_id); +> - void (*thread_summary) (const void *ctx, +> - const char *thread_id, +> - const time_t date, +> - const int matched, +> - const int total, +> - const char *authors, +> - const char *subject); +> - const char *tag_start; +> - const char *tag; +> - const char *tag_sep; +> - const char *tag_end; +> - const char *item_sep; +> - const char *item_end; +> - const char *results_end; +> - const char *results_null; +> -} search_format_t; +> - +> -static void +> -format_item_id_text (const void *ctx, +> - const char *item_type, +> - const char *item_id); +> - +> -static void +> -format_thread_text (const void *ctx, +> - const char *thread_id, +> - const time_t date, +> - const int matched, +> - const int total, +> - const char *authors, +> - const char *subject); +> -static const search_format_t format_text = { +> - "", +> - "", +> - format_item_id_text, +> - format_thread_text, +> - " (", +> - "%s", " ", +> - ")", "\n", +> - "", +> - "\n", +> - "", +> -}; +> - +> -static void +> -format_item_id_json (const void *ctx, +> - const char *item_type, +> - const char *item_id); +> - +> -static void +> -format_thread_json (const void *ctx, +> - const char *thread_id, +> - const time_t date, +> - const int matched, +> - const int total, +> - const char *authors, +> - const char *subject); +> - +> -/* Any changes to the JSON format should be reflected in the file +> - * devel/schemata. */ +> -static const search_format_t format_json = { +> - "[", +> - "{", +> - format_item_id_json, +> - format_thread_json, +> - "\"tags\": [", +> - "\"%s\"", ", ", +> - "]", ",\n", +> - "}", +> - "]\n", +> - "]\n", +> -}; +> - +> -static void +> -format_item_id_text (unused (const void *ctx), +> - const char *item_type, +> - const char *item_id) +> -{ +> - printf ("%s%s", item_type, item_id); +> -} +> - +> static char * +> sanitize_string (const void *ctx, const char *str) +> { +> @@ -131,72 +46,8 @@ sanitize_string (const void *ctx, const char *str) +> return out; +> } +> +> -static void +> -format_thread_text (const void *ctx, +> - const char *thread_id, +> - const time_t date, +> - const int matched, +> - const int total, +> - const char *authors, +> - const char *subject) +> -{ +> - void *ctx_quote = talloc_new (ctx); +> - +> - printf ("thread:%s %12s [%d/%d] %s; %s", +> - thread_id, +> - notmuch_time_relative_date (ctx, date), +> - matched, +> - total, +> - sanitize_string (ctx_quote, authors), +> - sanitize_string (ctx_quote, subject)); +> - +> - talloc_free (ctx_quote); +> -} +> - +> -static void +> -format_item_id_json (const void *ctx, +> - unused (const char *item_type), +> - const char *item_id) +> -{ +> - void *ctx_quote = talloc_new (ctx); +> - +> - printf ("%s", json_quote_str (ctx_quote, item_id)); +> - +> - talloc_free (ctx_quote); +> - +> -} +> - +> -static void +> -format_thread_json (const void *ctx, +> - const char *thread_id, +> - const time_t date, +> - const int matched, +> - const int total, +> - const char *authors, +> - const char *subject) +> -{ +> - void *ctx_quote = talloc_new (ctx); +> - +> - printf ("\"thread\": %s,\n" +> - "\"timestamp\": %ld,\n" +> - "\"date_relative\": \"%s\",\n" +> - "\"matched\": %d,\n" +> - "\"total\": %d,\n" +> - "\"authors\": %s,\n" +> - "\"subject\": %s,\n", +> - json_quote_str (ctx_quote, thread_id), +> - date, +> - notmuch_time_relative_date (ctx, date), +> - matched, +> - total, +> - json_quote_str (ctx_quote, authors), +> - json_quote_str (ctx_quote, subject)); +> - +> - talloc_free (ctx_quote); +> -} +> - +> static int +> -do_search_threads (const search_format_t *format, +> +do_search_threads (sprinter_t *format, +> notmuch_query_t *query, +> notmuch_sort_t sort, +> output_t output, +> @@ -207,7 +58,6 @@ do_search_threads (const search_format_t *format, +> notmuch_threads_t *threads; +> notmuch_tags_t *tags; +> time_t date; +> - int first_thread = 1; +> int i; +> +> if (offset < 0) { +> @@ -220,14 +70,12 @@ do_search_threads (const search_format_t *format, +> if (threads == NULL) +> return 1; +> +> - fputs (format->results_start, stdout); +> + format->begin_list (format); +> +> for (i = 0; +> notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit); +> notmuch_threads_move_to_next (threads), i++) +> { +> - int first_tag = 1; +> - +> thread = notmuch_threads_get (threads); +> +> if (i < offset) { +> @@ -235,60 +83,96 @@ do_search_threads (const search_format_t *format, +> continue; +> } +> +> - if (! first_thread) +> - fputs (format->item_sep, stdout); +> - +> if (output == OUTPUT_THREADS) { +> - format->item_id (thread, "thread:", +> - notmuch_thread_get_thread_id (thread)); +> + format->set_prefix (format, "thread"); +> + format->string (format, +> + notmuch_thread_get_thread_id (thread)); +> + format->separator (format); +> } else { /* output == OUTPUT_SUMMARY */ +> - fputs (format->item_start, stdout); +> + void *ctx_quote = talloc_new (thread); +> + const char *authors = notmuch_thread_get_authors (thread); +> + const char *subject = notmuch_thread_get_subject (thread); +> + const char *thread_id = notmuch_thread_get_thread_id (thread); +> + int matched = notmuch_thread_get_matched_messages (thread); +> + int total = notmuch_thread_get_total_messages (thread); +> + const char *relative_date = NULL; +> + notmuch_bool_t first_tag = TRUE; +> + +> + format->begin_map (format); +> +> if (sort == NOTMUCH_SORT_OLDEST_FIRST) +> date = notmuch_thread_get_oldest_date (thread); +> else +> date = notmuch_thread_get_newest_date (thread); +> +> - format->thread_summary (thread, +> - notmuch_thread_get_thread_id (thread), +> - date, +> - notmuch_thread_get_matched_messages (thread), +> - notmuch_thread_get_total_messages (thread), +> - notmuch_thread_get_authors (thread), +> - notmuch_thread_get_subject (thread)); +> + relative_date = notmuch_time_relative_date (ctx_quote, date); +> + +> + if (format->is_text_printer (format)) { +> + /* Special case for the text formatter */ +> + printf ("thread:%s %12s [%d/%d] %s; %s (", +> + thread_id, +> + relative_date, +> + matched, +> + total, +> + sanitize_string (ctx_quote, authors), +> + sanitize_string (ctx_quote, subject)); + +Great. This seems much simpler than trying to track the context in +the text printer. + +> + } else { /* Structured Output */ +> + format->map_key (format, "thread"); +> + format->string (format, thread_id); +> + format->map_key (format, "timestamp"); +> + format->integer (format, date); +> + format->map_key (format, "date_relative"); +> + format->string (format, relative_date); +> + format->map_key (format, "matched"); +> + format->integer (format, matched); +> + format->map_key (format, "total"); +> + format->integer (format, total); +> + format->map_key (format, "authors"); +> + format->string (format, authors); +> + format->map_key (format, "subject"); +> + format->string (format, subject); +> + } +> + +> + talloc_free (ctx_quote); +> +> - fputs (format->tag_start, stdout); +> + format->map_key (format, "tags"); +> + format->begin_list (format); +> +> for (tags = notmuch_thread_get_tags (thread); +> notmuch_tags_valid (tags); +> notmuch_tags_move_to_next (tags)) +> { +> - if (! first_tag) +> - fputs (format->tag_sep, stdout); +> - printf (format->tag, notmuch_tags_get (tags)); +> - first_tag = 0; +> + const char *tag = notmuch_tags_get (tags); +> + +> + if (format->is_text_printer (format)) { +> + /* Special case for the text formatter */ +> + if (first_tag) +> + first_tag = FALSE; +> + else +> + fputc (' ', stdout); +> + fputs (tag, stdout); +> + } else /* Structured Output */ + +Please put braces around the else part as well (since you need braces +around the if part). + +> + format->string (format, tag); +> } +> +> - fputs (format->tag_end, stdout); +> + if (format->is_text_printer (format)) +> + printf (")"); +> +> - fputs (format->item_end, stdout); +> + format->end (format); +> + format->end (format); +> + format->separator (format); +> } +> +> - first_thread = 0; +> - +> notmuch_thread_destroy (thread); +> } +> +> - if (first_thread) +> - fputs (format->results_null, stdout); +> - else +> - fputs (format->results_end, stdout); +> + format->end (format); +> +> return 0; +> } +> +> static int +> -do_search_messages (const search_format_t *format, +> +do_search_messages (sprinter_t *format, +> notmuch_query_t *query, +> output_t output, +> int offset, +> @@ -297,7 +181,6 @@ do_search_messages (const search_format_t *format, +> notmuch_message_t *message; +> notmuch_messages_t *messages; +> notmuch_filenames_t *filenames; +> - int first_message = 1; +> int i; +> +> if (offset < 0) { +> @@ -310,7 +193,7 @@ do_search_messages (const search_format_t *format, +> if (messages == NULL) +> return 1; +> +> - fputs (format->results_start, stdout); +> + format->begin_list (format); +> +> for (i = 0; +> notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit); +> @@ -328,24 +211,17 @@ do_search_messages (const search_format_t *format, +> notmuch_filenames_valid (filenames); +> notmuch_filenames_move_to_next (filenames)) +> { +> - if (! first_message) +> - fputs (format->item_sep, stdout); +> - +> - format->item_id (message, "", +> - notmuch_filenames_get (filenames)); +> - +> - first_message = 0; +> + format->string (format, notmuch_filenames_get (filenames)); +> + format->separator (format); +> } +> +> notmuch_filenames_destroy( filenames ); +> +> } else { /* output == OUTPUT_MESSAGES */ +> - if (! first_message) +> - fputs (format->item_sep, stdout); +> - +> - format->item_id (message, "id:", +> - notmuch_message_get_message_id (message)); +> - first_message = 0; +> + format->set_prefix (format, "id"); +> + format->string (format, +> + notmuch_message_get_message_id (message)); +> + format->separator (format); +> } +> +> notmuch_message_destroy (message); +> @@ -353,23 +229,19 @@ do_search_messages (const search_format_t *format, +> +> notmuch_messages_destroy (messages); +> +> - if (first_message) +> - fputs (format->results_null, stdout); +> - else +> - fputs (format->results_end, stdout); +> + format->end (format); +> +> return 0; +> } +> +> static int +> do_search_tags (notmuch_database_t *notmuch, +> - const search_format_t *format, +> + sprinter_t *format, +> notmuch_query_t *query) +> { +> notmuch_messages_t *messages = NULL; +> notmuch_tags_t *tags; +> const char *tag; +> - int first_tag = 1; +> +> /* should the following only special case if no excluded terms +> * specified? */ +> @@ -387,7 +259,7 @@ do_search_tags (notmuch_database_t *notmuch, +> if (tags == NULL) +> return 1; +> +> - fputs (format->results_start, stdout); +> + format->begin_list (format); +> +> for (; +> notmuch_tags_valid (tags); +> @@ -395,12 +267,9 @@ do_search_tags (notmuch_database_t *notmuch, +> { +> tag = notmuch_tags_get (tags); +> +> - if (! first_tag) +> - fputs (format->item_sep, stdout); +> + format->string (format, tag); +> + format->separator (format); +> +> - format->item_id (tags, "", tag); +> - +> - first_tag = 0; +> } +> +> notmuch_tags_destroy (tags); +> @@ -408,10 +277,7 @@ do_search_tags (notmuch_database_t *notmuch, +> if (messages) +> notmuch_messages_destroy (messages); +> +> - if (first_tag) +> - fputs (format->results_null, stdout); +> - else +> - fputs (format->results_end, stdout); +> + format->end (format); +> +> return 0; +> } +> @@ -430,7 +296,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) +> notmuch_query_t *query; +> char *query_str; +> notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST; +> - const search_format_t *format = &format_text; +> + sprinter_t *format = NULL; +> int opt_index, ret; +> output_t output = OUTPUT_SUMMARY; +> int offset = 0; +> @@ -475,10 +341,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) +> +> switch (format_sel) { +> case NOTMUCH_FORMAT_TEXT: +> - format = &format_text; +> + format = sprinter_text_search_create (ctx, stdout); +> break; +> case NOTMUCH_FORMAT_JSON: +> - format = &format_json; +> + format = sprinter_json_create (ctx, stdout); +> break; +> } +> +> @@ -546,5 +412,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) +> notmuch_query_destroy (query); +> notmuch_database_destroy (notmuch); +> +> + talloc_free (format); +> + +> return ret; +> } +> diff --git a/test/json b/test/json +> index 6439788..f0ebf08 100755 +> --- a/test/json +> +++ b/test/json +> @@ -10,14 +10,7 @@ test_expect_equal "$output" "[[[{\"id\": \"${gen_msg_id}\", \"match\": true, \"e +> test_begin_subtest "Search message: json" +> add_message "[subject]=\"json-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"json-search-message\"" +> output=$(notmuch search --format=json "json-search-message" | notmuch_search_sanitize) + +What's your reason for not piping this through +notmuch_json_show_sanitize? + +> -test_expect_equal "$output" "[{\"thread\": \"XXX\", +> -\"timestamp\": 946728000, +> -\"date_relative\": \"2000-01-01\", +> -\"matched\": 1, +> -\"total\": 1, +> -\"authors\": \"Notmuch Test Suite\", +> -\"subject\": \"json-search-subject\", +> -\"tags\": [\"inbox\", \"unread\"]}]" +> +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-subject\", \"tags\": [\"inbox\", \"unread\"]}]" +> +> test_begin_subtest "Show message: json, utf-8" +> add_message "[subject]=\"json-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\"" +> @@ -40,13 +33,6 @@ test_expect_equal "$output" "[[[{\"id\": \"$id\", \"match\": true, \"excluded\": +> test_begin_subtest "Search message: json, utf-8" +> add_message "[subject]=\"json-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\"" +> output=$(notmuch search --format=json "jsön-search-méssage" | notmuch_search_sanitize) +> -test_expect_equal "$output" "[{\"thread\": \"XXX\", +> -\"timestamp\": 946728000, +> -\"date_relative\": \"2000-01-01\", +> -\"matched\": 1, +> -\"total\": 1, +> -\"authors\": \"Notmuch Test Suite\", +> -\"subject\": \"json-search-utf8-body-sübjéct\", +> -\"tags\": [\"inbox\", \"unread\"]}]" +> +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-utf8-body-sübjéct\", \"tags\": [\"inbox\", \"unread\"]}]" +> +> test_done