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 BD3FF431FB6 for ; Fri, 13 Jul 2012 19:02:15 -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 WeD2gnFmBSmI for ; Fri, 13 Jul 2012 19:02:13 -0700 (PDT) Received: from dmz-mailsec-scanner-2.mit.edu (DMZ-MAILSEC-SCANNER-2.MIT.EDU [18.9.25.13]) by olra.theworths.org (Postfix) with ESMTP id 3D3DC431FAE for ; Fri, 13 Jul 2012 19:02:13 -0700 (PDT) X-AuditID: 1209190d-b7fd56d000000933-cd-5000d32426b7 Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) by dmz-mailsec-scanner-2.mit.edu (Symantec Messaging Gateway) with SMTP id 25.F7.02355.423D0005; Fri, 13 Jul 2012 22:02:12 -0400 (EDT) Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q6E22CSA007243; Fri, 13 Jul 2012 22:02:12 -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 q6E22Bcu009180 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); Fri, 13 Jul 2012 22:02:12 -0400 (EDT) Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) (envelope-from ) id 1SprgR-0000e7-CG; Fri, 13 Jul 2012 22:02:11 -0400 Date: Fri, 13 Jul 2012 22:02:11 -0400 From: Austin Clements To: Peter Feigl Subject: Re: [PATCH v5 2/3] Add structured output formatter for JSON and text. Message-ID: <20120714020211.GC31670@mit.edu> References: <20120710191331.GE7332@mit.edu> <1342167097-25012-1-git-send-email-craven@gmx.net> <1342167097-25012-3-git-send-email-craven@gmx.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1342167097-25012-3-git-send-email-craven@gmx.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmphleLIzCtJLcpLzFFi42IRYrdT0VW5zBBg0N5hYLG3oZ3R4vrNmcwO TB6LN+1n83i26hZzAFMUl01Kak5mWWqRvl0CV8aa+T+YCjbmVbz6u4K9gXFuSBcjJ4eEgInE xDmn2SFsMYkL99azdTFycQgJ7GOUuLlwKiuEs4FR4u70zSwQzkkmiWWzL0A5SxglTjVtBirj 4GARUJV4d7oMZBSbgIbEtv3LGUFsEQEFiWfrmsBsZgFpiW+/m5lAbGEBP4nVRw+wgti8AjoS J3fMhlo9lVFiycZzUAlBiZMzn7BANGtJ3Pj3kglkF8ig5f84QMKcAvYS7Ve6wWaKCqhITDm5 jW0Co9AsJN2zkHTPQuhewMi8ilE2JbdKNzcxM6c4NVm3ODkxLy+1SNdILzezRC81pXQTIyiw OSV5dzC+O6h0iFGAg1GJhzfVnyFAiDWxrLgy9xCjJAeTkijvrItAIb6k/JTKjMTijPii0pzU 4kOMEhzMSiK85m1AOd6UxMqq1KJ8mJQ0B4uSOO+VlJv+QgLpiSWp2ampBalFMFkZDg4lCd6w S0CNgkWp6akVaZk5JQhpJg5OkOE8QMP1QWp4iwsSc4sz0yHypxgVpcR5i0ESAiCJjNI8uF5Y 4nnFKA70ijCvM0gVDzBpwXW/AhrMBDR41s9//kCDSxIRUlINjNLnBN5Mfnp499K57OViUm2m CRa+h2wKPyh9temqti7aVnp+n5b6iouxZ8sCF/bMbJpSKbREW/GC3p1LkpV7lGYKuWT4z7F/ sjX82Gux5pZHa3QjpPcqMnkcfO/Zlry48L1+59TFpw9tOiumtvLKTNEzMx6qbzI1im9b45Of N/PWcv4VCddMmJRYijMSDbWYi4oTAdGJc5oXAwAA 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: Sat, 14 Jul 2012 02:02:15 -0000 Quoth Peter Feigl on Jul 13 at 10:11 am: > From: > > Using the new structured printer support in sprinter.h, implement > sprinter_json_create, which returns a new JSON structured output > formatter. The formatter prints output similar to the existing JSON, but > with differences in whitespace (mostly newlines, --output=summary prints > the entire message summary on one line, not split across multiple lines). > > Also implement a "structured" formatter that prints the current plain > text format. This passes all tests, but the exact formatting is probably > specific to notmuch-search and cannot easily (if at all) be adapted to > be used across all of notmuch-{search,reply,show,...}. > --- > Makefile.local | 2 + > sprinter-json.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++ > sprinter-text-search.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++++ > sprinter.h | 8 +++ > 4 files changed, 384 insertions(+) > create mode 100644 sprinter-json.c > create mode 100644 sprinter-text-search.c > > diff --git a/Makefile.local b/Makefile.local > index a890df2..b6c7e0c 100644 > --- a/Makefile.local > +++ b/Makefile.local > @@ -290,6 +290,8 @@ notmuch_client_srcs = \ > notmuch-show.c \ > notmuch-tag.c \ > notmuch-time.c \ > + sprinter-json.c \ > + sprinter-text-search.c \ > query-string.c \ > mime-node.c \ > crypto.c \ > diff --git a/sprinter-json.c b/sprinter-json.c > new file mode 100644 > index 0000000..215151d > --- /dev/null > +++ b/sprinter-json.c > @@ -0,0 +1,184 @@ > +#include > +#include > +#include > +#include "sprinter.h" > + > +struct sprinter_json { > + struct sprinter vtable; > + FILE *stream; > + /* Top of the state stack, or NULL if the printer is not currently > + * inside any aggregate types. */ > + struct json_state *state; > + > + /* A flag to signify that a separator should be inserted in the > + * output as soon as possible. > + */ > + notmuch_bool_t insert_separator; > +}; > + > +struct json_state { > + struct json_state *parent; > + /* True if nothing has been printed in this aggregate yet. > + * Suppresses the comma before a value. */ > + notmuch_bool_t first; > + /* The character that closes the current aggregate. */ > + char close; > +}; > + > +/* Helper function to set up the stream to print a value. If this > + * value follows another value, prints a comma. */ > +static struct sprinter_json * > +json_begin_value (struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json *) sp; > + > + if (spj->state) { > + if (! spj->state->first) { > + fputc (',', spj->stream); > + if (spj->insert_separator) { > + fputc ('\n', spj->stream); > + spj->insert_separator = FALSE; > + } else > + fputc (' ', spj->stream); > + } else > + spj->state->first = FALSE; > + } > + return spj; > +} > + > +/* Helper function to begin an aggregate type. Prints the open > + * character and pushes a new state frame. */ > +static void > +json_begin_aggregate (struct sprinter *sp, char open, char close) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + struct json_state *state = talloc (spj, struct json_state); > + > + fputc (open, spj->stream); > + state->parent = spj->state; > + state->first = TRUE; > + state->close = close; > + spj->state = state; > +} > + > +static void > +json_begin_map (struct sprinter *sp) > +{ > + json_begin_aggregate (sp, '{', '}'); > +} > + > +static void > +json_begin_list (struct sprinter *sp) > +{ > + json_begin_aggregate (sp, '[', ']'); > +} > + > +static void > +json_end (struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json *) sp; > + struct json_state *state = spj->state; > + > + fputc (spj->state->close, spj->stream); > + spj->state = state->parent; > + talloc_free (state); > + if (spj->state == NULL) > + fputc ('\n', spj->stream); > +} > + > +static void > +json_string (struct sprinter *sp, const char *val) > +{ > + static const char *const escapes[] = { > + ['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b", > + ['\f'] = "\\f", ['\n'] = "\\n", ['\t'] = "\\t" > + }; > + struct sprinter_json *spj = json_begin_value (sp); > + > + fputc ('"', spj->stream); > + for (; *val; ++val) { > + unsigned char ch = *val; > + if (ch < ARRAY_SIZE (escapes) && escapes[ch]) > + fputs (escapes[ch], spj->stream); > + else if (ch >= 32) > + fputc (ch, spj->stream); > + else > + fprintf (spj->stream, "\\u%04x", ch); > + } > + fputc ('"', spj->stream); > +} > + > +static void > +json_integer (struct sprinter *sp, int val) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + > + fprintf (spj->stream, "%d", val); > +} > + > +static void > +json_boolean (struct sprinter *sp, notmuch_bool_t val) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + > + fputs (val ? "true" : "false", spj->stream); > +} > + > +static void > +json_null (struct sprinter *sp) > +{ > + struct sprinter_json *spj = json_begin_value (sp); > + > + fputs ("null", spj->stream); > +} > + > +static void > +json_map_key (struct sprinter *sp, const char *key) > +{ > + struct sprinter_json *spj = (struct sprinter_json *) sp; > + > + json_string (sp, key); > + fputs (": ", spj->stream); > + spj->state->first = TRUE; > +} > + > +static void > +json_set_prefix (unused (struct sprinter *sp), unused (const char *name)) > +{ > +} > + > +static void > +json_separator (struct sprinter *sp) > +{ > + struct sprinter_json *spj = (struct sprinter_json *) sp; > + > + spj->insert_separator = TRUE; > +} > + > +struct sprinter * > +sprinter_json_create (const void *ctx, FILE *stream) > +{ > + static const struct sprinter_json template = { > + .vtable = { > + .begin_map = json_begin_map, > + .begin_list = json_begin_list, > + .end = json_end, > + .string = json_string, > + .integer = json_integer, > + .boolean = json_boolean, > + .null = json_null, > + .map_key = json_map_key, > + .separator = json_separator, > + .set_prefix = json_set_prefix, > + } > + }; > + struct sprinter_json *res; > + > + res = talloc (ctx, struct sprinter_json); > + if (! res) > + return NULL; > + > + *res = template; > + res->stream = stream; > + return &res->vtable; > +} > diff --git a/sprinter-text-search.c b/sprinter-text-search.c > new file mode 100644 > index 0000000..95ed9cb > --- /dev/null > +++ b/sprinter-text-search.c > @@ -0,0 +1,190 @@ > +#include > +#include > +#include > +#include "sprinter.h" > + > +/* "Structured printer" interface for unstructured text printing. > + * This is at best a misuse of the interface, but it simplifies the code > + * in notmuch-search.c considerably. > + */ > + > +struct sprinter_text_search { > + struct sprinter vtable; > + FILE *stream; > + > + /* The current name or prefix to be printed with string/integer/boolean > + * data. > + */ > + const char *current_name; "current_prefix"? Or maybe just "prefix"? This is the only place where you use the term "name" for this. > + > + /* A flag to indicate if this is the first tag. Used in list of tags > + * for summary. > + */ > + notmuch_bool_t first_tag; > +}; > + > +/* struct text_search_state { */ > +/* struct text_search_state *parent; */ > +/* }; */ Left over scratch code? > + > +static notmuch_bool_t > +current_context (struct sprinter_text_search *sptxt, const char *marker) > +{ > + return (sptxt->current_name != NULL > + && ! strncmp (marker, sptxt->current_name, strlen (marker))); > +} All of this context stuff seems way more complicated than having a few special cases in notmuch-search.c. This is, in effect, just moving this special casing from there to here, but since the text format is highly irregular, attempting to generalize it is only going to obfuscate it. I think the simplest and most readable thing to do is to make all of these functions no-ops (literally empty function bodies) except text_search_separator and text_search_set_prefix, which should be like you have them, and text_search_string: static void text_search_string (struct sprinter *sp, const char *val) { struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; if (sptxt->current_name) fprintf (sptxt->stream, "%s:", sptxt->current_name); print_sanitized_string (sptxt->stream, val); } For the summary output, you'll have to format the summary line in notmuch-search.c, much like you did in v4, which was a much simpler and more readable way to put together the text format lines. > + > +static void > +print_sanitized_string (FILE *stream, const char *str) > +{ > + if (NULL == str) > + return; > + > + for (; *str; str++) { > + if ((unsigned char) (*str) < 32) > + fputc ('?', stream); > + else > + fputc (*str, stream); > + } > +} > + > +static void > +text_search_begin_map (unused (struct sprinter *sp)) > +{ > +} > + > +static void > +text_search_begin_list (struct sprinter *sp) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + if (current_context (sptxt, "tags")) { > + fputs (" (", sptxt->stream); > + sptxt->first_tag = TRUE; > + } > +} > + > +static void > +text_search_end (struct sprinter *sp) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + if (current_context (sptxt, "tags")) { > + fputc (')', sptxt->stream); > + sptxt->current_name = NULL; > + } > +} > + > +static void > +text_search_string (struct sprinter *sp, const char *val) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + if (sptxt->current_name != NULL) { > + if (current_context (sptxt, "thread")) > + fprintf ( sptxt->stream, "thread:%s ", val); > + else if (current_context (sptxt, "date_relative")) > + fprintf ( sptxt->stream, "%12s ", val); > + else if (current_context (sptxt, "authors")) { > + print_sanitized_string (sptxt->stream, val); > + fputs ("; ", sptxt->stream); > + } else if (current_context (sptxt, "subject")) > + print_sanitized_string (sptxt->stream, val); > + else if (current_context (sptxt, "tags")) { > + if (! sptxt->first_tag) > + fputc (' ', sptxt->stream); > + else > + sptxt->first_tag = FALSE; > + > + fputs (val, sptxt->stream); > + } else { > + fputs (sptxt->current_name, sptxt->stream); > + fputc (':', sptxt->stream); > + fputs (val, sptxt->stream); > + } > + } else { > + fputs (val, sptxt->stream); > + } > +} > + > +static void > +text_search_integer (struct sprinter *sp, int val) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + if (sptxt->current_name != NULL) { > + if (current_context (sptxt, "matched")) > + fprintf ( sptxt->stream, "[%d/", val); > + else if (current_context (sptxt, "total")) > + fprintf ( sptxt->stream, "%d] ", val); > + } else > + fprintf (sptxt->stream, "%d", val); > +} > + > +static void > +text_search_boolean (struct sprinter *sp, notmuch_bool_t val) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + fputs (val ? "true" : "false", sptxt->stream); > +} > + > +static void > +text_search_null (struct sprinter *sp) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + fputs ("null", sptxt->stream); > +} > + > +static void > +text_search_map_key (struct sprinter *sp, const char *key) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + sptxt->current_name = key; > +} > + > +static void > +text_search_separator (struct sprinter *sp) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + fputc ('\n', sptxt->stream); > +} > + > +static void > +text_search_set_prefix (struct sprinter *sp, const char *name) > +{ > + struct sprinter_text_search *sptxt = (struct sprinter_text_search *) sp; > + > + sptxt->current_name = name; > +} > + > +struct sprinter * > +sprinter_text_search_create (const void *ctx, FILE *stream) > +{ > + static const struct sprinter_text_search template = { > + .vtable = { > + .begin_map = text_search_begin_map, > + .begin_list = text_search_begin_list, > + .end = text_search_end, > + .string = text_search_string, > + .integer = text_search_integer, > + .boolean = text_search_boolean, > + .null = text_search_null, > + .map_key = text_search_map_key, > + .separator = text_search_separator, > + .set_prefix = text_search_set_prefix, > + } > + }; > + struct sprinter_text_search *res; > + > + res = talloc (ctx, struct sprinter_text_search); > + if (! res) > + return NULL; > + > + *res = template; > + res->stream = stream; > + return &res->vtable; > +} > diff --git a/sprinter.h b/sprinter.h > index c9cd6a6..4241d65 100644 > --- a/sprinter.h > +++ b/sprinter.h > @@ -47,4 +47,12 @@ typedef struct sprinter { > void (*separator)(struct sprinter *); > } sprinter_t; > > +/* Create a new unstructured printer that emits the default Text format for search. */ Wrap at 70 columns. > +struct sprinter * > +sprinter_text_search_create (const void *ctx, FILE *stream); > + > +/* Create a new structure printer that emits JSON. */ > +struct sprinter * > +sprinter_json_create (const void *ctx, FILE *stream); > + > #endif // NOTMUCH_SPRINTER_H