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