Proposed structure formatter API

Proposed structure formatter API

[Austin Clements]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

On IRC today, Peter renewed discussions of supporting an S-expression
format, which I think would be a fantastic thing to have.

A while back [1] I tried to explain an API I thought would form a good
foundation for this by abstracting away the differences between JSON and
S-expression output so both structured formats can use the same code,
but I didn't express the idea very clearly.  I built essentially this
API for another project recently and it worked well (and I learned a few
things), so I figured I would give it another shot.  It's simple enough
that I tossed together an example implementation, which is attached.

The idea is similar to the existing format structures, but instead of
dealing with complex, high-level concepts like threads or messages, this
API deals with syntax-level concepts like lists and strings.  All of the
structured output formats can then be supported by a single high-level
formatter backed by different syntax formatters.

There are a few ways something like this could be integrated with the
high-level formatters.  For show, the least invasive thing would be to
have two notmuch_show_format structures with different part functions.
These part functions would be thin wrappers that simply create the right
structure printer and then pass it to a common part function.

It would be even nicer if we could get rid of the
message_set_{start,sep,end} fields, since those duplicate functionality
from the structure printer and their use is haphazard and overly
complicated.  We could do this by changing notmuch_show_format to
something like

typedef struct notmuch_show_format {
    struct sprinter *(*new_sprinter) (const void *ctx);
    notmuch_status_t (*part) (const void *ctx, struct sprinter *sp,
                              struct mime_node *node,
                              const struct notmuch_show_params *params);
} notmuch_show_format_t;

For the JSON and S-expression show formats, new_sprinter would be
sprinter_json_new and sprinter_sexp_new, respectively, and they could
share a part function.  For the text, mbox, and raw formatters,
new_sprinter could simply be NULL, or we could provide a "NULL structure
printer" implementation that does nothing.

We could do something similar for reply.

search_format is more complicated, but might also benefit more.  Most of
those fields have to do with how to print lists and objects and could be
removed if the format simply provided a new_sprinter method like the
notmuch_show_format I suggested above.

[1] id:"20120121220407.GK16740@mit.edu"


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sprinter.c --]
[-- Type: text/x-csrc, Size: 4987 bytes --]

#include <stdbool.h>
#include <stdio.h>
#include <talloc.h>
#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))

/* Structure printer interface */
struct sprinter
{
    void (*begin_map) (struct sprinter *);
    void (*begin_list) (struct sprinter *);
    void (*end) (struct sprinter *);

    void (*string) (struct sprinter *, const char *);
    void (*integer) (struct sprinter *, int);
    void (*boolean) (struct sprinter *, bool);
    void (*null) (struct sprinter *);
    void (*map_key) (struct sprinter *, const char *);

    void (*frame) (struct sprinter *);
};

/* Create a new structure printer that emits JSON */
struct sprinter *
sprinter_json_new(const void *ctx, FILE *stream);

int
main(int argc, char **argv)
{
    struct sprinter *test = sprinter_json_new (NULL, stdout);
    test->begin_list (test);
    test->string (test, "test");
    test->integer (test, 42);
    test->boolean (test, true);
    test->null (test);
    test->frame (test);
    test->begin_map (test);
    test->map_key (test, "Hello");
    test->string (test, "world\n");
    test->end (test);
    test->frame (test);
    test->end (test);
    talloc_free (test);
}

/*
 * Every below here is private implementation.
 */

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;
};

struct json_state
{
    struct json_state *parent;
    /* True if nothing has been printed in this aggregate yet.
     * Suppresses the comma before a value. */
    bool 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)
	    fputs (", ", 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)
{
    const static 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, bool 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_frame(struct sprinter *sp)
{
    struct sprinter_json *spj = (struct sprinter_json*)sp;
    fputc ('\n', spj->stream);
}

struct sprinter *
sprinter_json_new(const void *ctx, FILE *stream)
{
    const static 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,
	    .frame = json_frame,
	}
    };
    struct sprinter_json *res;

    res = talloc (ctx, struct sprinter_json);
    if (!res)
	return NULL;

    *res = template;
    res->stream = stream;
    return &res->vtable;
}

Structured Formatters for JSON Output

[craven]

Currently there is no easy way to add support for different structured
formatters (like JSON). For example, adding support for S-Expressions
would result in code duplication.

This patch series amends the situation by introducing structured
formatters, which allow different implementations of structured output
for elements like lists, maps, strings and numbers.

The new code in sprinter.h and sprinter.c can be used
instead of the current ad-hoc output in all parts of notmuch, a patch
for notmuch-search.c is included.

In a later patch, all other parts of notmuch should be adapted to the
structured formatters, and the creation of formatters should be
centralised (to make adding new formatters easier).

[PATCH v4 1/3] Add support for structured output formatters.

[craven]

This patch adds a new type sprinter_t, which is used for structured
formatting, e.g. JSON or S-Expressions. The structure printer is the
code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).

The structure printer contains the following function pointers:

/* start a new map/dictionary structure.
 */
void (*begin_map) (struct sprinter *);

/* start a new list/array structure
 */
void (*begin_list) (struct sprinter *);

/* end the last opened list or map structure
 */
void (*end) (struct sprinter *);

/* print one string/integer/boolean/null element (possibly inside a
 * list or map
 */
void (*string) (struct sprinter *, const char *);
void (*integer) (struct sprinter *, int);
void (*boolean) (struct sprinter *, notmuch_bool_t);
void (*null) (struct sprinter *);

/* print the key of a map's key/value pair.
 */
void (*map_key) (struct sprinter *, const char *);

/* print a frame delimiter (such as a newline or extra whitespace)
 */
void (*frame) (struct sprinter *);

The printer can (and should) use internal state to insert delimiters and
syntax at the correct places.

Example:

format->begin_map(format);
format->map_key(format, "foo");
format->begin_list(format);
format->integer(format, 1);
format->integer(format, 2);
format->integer(format, 3);
format->end(format);
format->map_key(format, "bar");
format->begin_map(format);
format->map_key(format, "baaz");
format->string(format, "hello world");
format->end(format);
format->end(format);

would output JSON as follows:

{"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}
---
 sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 sprinter.h

diff --git a/sprinter.h b/sprinter.h
new file mode 100644
index 0000000..1dad9a0
--- /dev/null
+++ b/sprinter.h
@@ -0,0 +1,49 @@
+#ifndef NOTMUCH_SPRINTER_H
+#define NOTMUCH_SPRINTER_H
+
+/* for notmuch_bool_t */
+#include "notmuch-client.h"
+
+/* Structure printer interface */
+typedef struct sprinter
+{
+    /* start a new map/dictionary structure.
+     */
+    void (*begin_map) (struct sprinter *);
+
+    /* start a new list/array structure
+     */
+    void (*begin_list) (struct sprinter *);
+
+    /* end the last opened list or map structure
+     */
+    void (*end) (struct sprinter *);
+
+    /* print one string/integer/boolean/null element (possibly inside a
+     * list or map
+     */
+    void (*string) (struct sprinter *, const char *);
+    void (*integer) (struct sprinter *, int);
+    void (*boolean) (struct sprinter *, notmuch_bool_t);
+    void (*null) (struct sprinter *);
+
+    /* print the key of a map's key/value pair.
+     */
+    void (*map_key) (struct sprinter *, const char *);
+
+    /* print a frame delimiter (such as a newline or extra whitespace)
+     */
+    void (*frame) (struct sprinter *);
+} sprinter_t;
+
+/* Create a new structure printer that emits JSON */
+struct sprinter *
+sprinter_json_new(const void *ctx, FILE *stream);
+
+/* A dummy structure printer that signifies that standard text output is
+ * to be used instead of any structured format.
+ */
+struct sprinter *
+sprinter_text;
+
+#endif // NOTMUCH_SPRINTER_H
-- 
1.7.11.1

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Mark Walters]

Hi

this definitely looks nice. I have a couple of suggestions for the
comments:

On Thu, 12 Jul 2012, craven@gmx.net wrote:
> This patch adds a new type sprinter_t, which is used for structured
> formatting, e.g. JSON or S-Expressions. The structure printer is the
> code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).
>
> The structure printer contains the following function pointers:
>
> /* start a new map/dictionary structure.
>  */
> void (*begin_map) (struct sprinter *);
>
> /* start a new list/array structure
>  */
> void (*begin_list) (struct sprinter *);
>
> /* end the last opened list or map structure
>  */
> void (*end) (struct sprinter *);
>
> /* print one string/integer/boolean/null element (possibly inside a
>  * list or map
>  */
> void (*string) (struct sprinter *, const char *);
> void (*integer) (struct sprinter *, int);
> void (*boolean) (struct sprinter *, notmuch_bool_t);
> void (*null) (struct sprinter *);
>
> /* print the key of a map's key/value pair.
>  */
> void (*map_key) (struct sprinter *, const char *);
>
> /* print a frame delimiter (such as a newline or extra whitespace)
>  */
> void (*frame) (struct sprinter *);
>
> The printer can (and should) use internal state to insert delimiters and
> syntax at the correct places.
>
> Example:
>
> format->begin_map(format);
> format->map_key(format, "foo");
> format->begin_list(format);
> format->integer(format, 1);
> format->integer(format, 2);
> format->integer(format, 3);
> format->end(format);
> format->map_key(format, "bar");
> format->begin_map(format);
> format->map_key(format, "baaz");
> format->string(format, "hello world");
> format->end(format);
> format->end(format);
>
> would output JSON as follows:
>
> {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}
> ---
>  sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 sprinter.h
>
> diff --git a/sprinter.h b/sprinter.h
> new file mode 100644
> index 0000000..1dad9a0
> --- /dev/null
> +++ b/sprinter.h
> @@ -0,0 +1,49 @@
> +#ifndef NOTMUCH_SPRINTER_H
> +#define NOTMUCH_SPRINTER_H
> +
> +/* for notmuch_bool_t */
> +#include "notmuch-client.h"
> +
> +/* Structure printer interface */
> +typedef struct sprinter
> +{
> +    /* start a new map/dictionary structure.
> +     */
> +    void (*begin_map) (struct sprinter *);
> +
> +    /* start a new list/array structure
> +     */
> +    void (*begin_list) (struct sprinter *);
> +
> +    /* end the last opened list or map structure
> +     */
> +    void (*end) (struct sprinter *);
> +
> +    /* print one string/integer/boolean/null element (possibly inside a
> +     * list or map
> +     */

I think this should say that it also prints any separator necessary.

> +    void (*string) (struct sprinter *, const char *);
> +    void (*integer) (struct sprinter *, int);
> +    void (*boolean) (struct sprinter *, notmuch_bool_t);
> +    void (*null) (struct sprinter *);
> +
> +    /* print the key of a map's key/value pair.
> +     */
> +    void (*map_key) (struct sprinter *, const char *);
> +
> +    /* print a frame delimiter (such as a newline or extra whitespace)
> +     */
> +    void (*frame) (struct sprinter *);

I think this should say that the intention is that this only prints
non-syntactic stuff.

Best wishes

Mark


> +} sprinter_t;
> +
> +/* Create a new structure printer that emits JSON */
> +struct sprinter *
> +sprinter_json_new(const void *ctx, FILE *stream);
> +
> +/* A dummy structure printer that signifies that standard text output is
> + * to be used instead of any structured format.
> + */
> +struct sprinter *
> +sprinter_text;
> +
> +#endif // NOTMUCH_SPRINTER_H
> -- 
> 1.7.11.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Mark Walters]

A couple more things:

On Thu, 12 Jul 2012, craven@gmx.net wrote:
> This patch adds a new type sprinter_t, which is used for structured
> formatting, e.g. JSON or S-Expressions. The structure printer is the
> code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).
>
> The structure printer contains the following function pointers:
>
> /* start a new map/dictionary structure.
>  */
> void (*begin_map) (struct sprinter *);
>
> /* start a new list/array structure
>  */
> void (*begin_list) (struct sprinter *);
>
> /* end the last opened list or map structure
>  */
> void (*end) (struct sprinter *);

what is the advantage of having this as one function rather than end_map
and end_list? Indeed, my choice (but I think most other people would
disagree) would be to have both functions but still keep state as this
currently does and then throw an error if the code closes the wrong
thing.

A second question: do you have an implementation in this style for
s-expressions. I find it hard to tell whether the interface is right
with just a single example. Even a completely hacky not ready for review
example would be helpful.

Best wishes

Mark

> /* print one string/integer/boolean/null element (possibly inside a
>  * list or map
>  */
> void (*string) (struct sprinter *, const char *);
> void (*integer) (struct sprinter *, int);
> void (*boolean) (struct sprinter *, notmuch_bool_t);
> void (*null) (struct sprinter *);
>
> /* print the key of a map's key/value pair.
>  */
> void (*map_key) (struct sprinter *, const char *);
>
> /* print a frame delimiter (such as a newline or extra whitespace)
>  */
> void (*frame) (struct sprinter *);
>
> The printer can (and should) use internal state to insert delimiters and
> syntax at the correct places.
>
> Example:
>
> format->begin_map(format);
> format->map_key(format, "foo");
> format->begin_list(format);
> format->integer(format, 1);
> format->integer(format, 2);
> format->integer(format, 3);
> format->end(format);
> format->map_key(format, "bar");
> format->begin_map(format);
> format->map_key(format, "baaz");
> format->string(format, "hello world");
> format->end(format);
> format->end(format);
>
> would output JSON as follows:
>
> {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}
> ---
>  sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 sprinter.h
>
> diff --git a/sprinter.h b/sprinter.h
> new file mode 100644
> index 0000000..1dad9a0
> --- /dev/null
> +++ b/sprinter.h
> @@ -0,0 +1,49 @@
> +#ifndef NOTMUCH_SPRINTER_H
> +#define NOTMUCH_SPRINTER_H
> +
> +/* for notmuch_bool_t */
> +#include "notmuch-client.h"
> +
> +/* Structure printer interface */
> +typedef struct sprinter
> +{
> +    /* start a new map/dictionary structure.
> +     */
> +    void (*begin_map) (struct sprinter *);
> +
> +    /* start a new list/array structure
> +     */
> +    void (*begin_list) (struct sprinter *);
> +
> +    /* end the last opened list or map structure
> +     */
> +    void (*end) (struct sprinter *);
> +
> +    /* print one string/integer/boolean/null element (possibly inside a
> +     * list or map
> +     */
> +    void (*string) (struct sprinter *, const char *);
> +    void (*integer) (struct sprinter *, int);
> +    void (*boolean) (struct sprinter *, notmuch_bool_t);
> +    void (*null) (struct sprinter *);
> +
> +    /* print the key of a map's key/value pair.
> +     */
> +    void (*map_key) (struct sprinter *, const char *);
> +
> +    /* print a frame delimiter (such as a newline or extra whitespace)
> +     */
> +    void (*frame) (struct sprinter *);
> +} sprinter_t;
> +
> +/* Create a new structure printer that emits JSON */
> +struct sprinter *
> +sprinter_json_new(const void *ctx, FILE *stream);
> +
> +/* A dummy structure printer that signifies that standard text output is
> + * to be used instead of any structured format.
> + */
> +struct sprinter *
> +sprinter_text;
> +
> +#endif // NOTMUCH_SPRINTER_H
> -- 
> 1.7.11.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 1/3] Add support for structured output formatters.

[craven]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

> what is the advantage of having this as one function rather than end_map
> and end_list? Indeed, my choice (but I think most other people would
> disagree) would be to have both functions but still keep state as this
> currently does and then throw an error if the code closes the wrong
> thing.

There's probably no advantage, one way or the other is fine, I'd say.
I've thought about introducing checks into the formatter functions, to
raise errors for improper closing, map_key not inside a map and things
like that, I just wasn't sure that would be acceptable.

> A second question: do you have an implementation in this style for
> s-expressions. I find it hard to tell whether the interface is right
> with just a single example. Even a completely hacky not ready for review
> example would be helpful.

See the attached patch :)

Thanks for the suggestions!

Peter

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-an-S-Expression-output-format.patch --]
[-- Type: text/x-patch, Size: 6660 bytes --]

From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001
From: <craven@gmx.net>
Date: Thu, 12 Jul 2012 10:17:05 +0200
Subject: [PATCH] Add an S-Expression output format.

---
 notmuch-search.c |   7 ++-
 sprinter.c       | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sprinter.h       |   4 ++
 3 files changed, 180 insertions(+), 1 deletion(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index b853f5f..f8bea9b 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -77,6 +77,7 @@ do_search_threads (sprinter_t *format,
 
     if (format != sprinter_text) {
 	format->begin_list (format);
+	format->frame (format);
     }
 
     for (i = 0;
@@ -380,7 +381,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     int exclude = EXCLUDE_TRUE;
     unsigned int i;
 
-    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
+    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT, NOTMUCH_FORMAT_SEXP }
 	format_sel = NOTMUCH_FORMAT_TEXT;
 
     notmuch_opt_desc_t options[] = {
@@ -391,6 +392,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
 	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
 				  { "text", NOTMUCH_FORMAT_TEXT },
+				  { "sexp", NOTMUCH_FORMAT_SEXP },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
 	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
@@ -422,6 +424,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     case NOTMUCH_FORMAT_JSON:
 	format = sprinter_json_new (ctx, stdout);
 	break;
+    case NOTMUCH_FORMAT_SEXP:
+	format = sprinter_sexp_new (ctx, stdout);
+        break;
     }
 
     config = notmuch_config_open (ctx, NULL, NULL);
diff --git a/sprinter.c b/sprinter.c
index 649f79a..fce0f9b 100644
--- a/sprinter.c
+++ b/sprinter.c
@@ -170,3 +170,173 @@ sprinter_json_new(const void *ctx, FILE *stream)
     res->stream = stream;
     return &res->vtable;
 }
+
+/*
+ * Every below here is the implementation of the SEXP printer.
+ */
+
+typedef enum { MAP, LIST } aggregate_t;
+
+struct sprinter_sexp
+{
+    struct sprinter vtable;
+    FILE *stream;
+    /* Top of the state stack, or NULL if the printer is not currently
+     * inside any aggregate types. */
+    struct sexp_state *state;
+};
+
+struct sexp_state
+{
+    struct sexp_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. */
+    aggregate_t type;
+};
+
+static struct sprinter_sexp *
+sexp_begin_value(struct sprinter *sp)
+{
+    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
+    if (spsx->state) {
+	if (!spsx->state->first)
+	    fputc (' ', spsx->stream);
+	else
+	    spsx->state->first = false;
+    }
+    return spsx;
+}
+
+static void
+sexp_begin_aggregate(struct sprinter *sp, aggregate_t type)
+{
+    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
+    struct sexp_state *state = talloc (spsx, struct sexp_state);
+
+    fputc ('(', spsx->stream);
+    state->parent = spsx->state;
+    state->first = true;
+    spsx->state = state;
+    state->type = type;
+}
+
+static void
+sexp_begin_map(struct sprinter *sp)
+{
+    sexp_begin_aggregate (sp, MAP);
+}
+
+static void
+sexp_begin_list(struct sprinter *sp)
+{
+    sexp_begin_aggregate (sp, LIST);
+}
+
+static void
+sexp_end(struct sprinter *sp)
+{
+    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
+    struct sexp_state *state = spsx->state;
+
+    fputc (')', spsx->stream);
+    spsx->state = state->parent;
+    talloc_free (state);
+    if(spsx->state == NULL)
+	fputc ('\n', spsx->stream);
+}
+
+static void
+sexp_string(struct sprinter *sp, const char *val)
+{
+    static const char * const escapes[] = {
+	['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
+	['\f'] = "\\f",  ['\n'] = "\\n",  ['\t'] = "\\t"
+    };
+    struct sprinter_sexp *spsx = sexp_begin_value(sp);
+    fputc ('"', spsx->stream);
+    for (; *val; ++val) {
+	unsigned char ch = *val;
+	if (ch < ARRAY_SIZE(escapes) && escapes[ch])
+	    fputs (escapes[ch], spsx->stream);
+	else if (ch >= 32)
+	    fputc (ch, spsx->stream);
+	else
+	    fprintf (spsx->stream, "\\u%04x", ch);
+    }
+    fputc ('"', spsx->stream);
+    if (spsx->state != NULL &&  spsx->state->type == MAP)
+	fputc (')', spsx->stream);
+    spsx->state->first = false;
+}
+
+static void
+sexp_integer(struct sprinter *sp, int val)
+{
+    struct sprinter_sexp *spsx = sexp_begin_value(sp);
+    fprintf (spsx->stream, "%d", val);
+    if (spsx->state != NULL &&  spsx->state->type == MAP)
+	fputc (')', spsx->stream);
+}
+
+static void
+sexp_boolean(struct sprinter *sp, notmuch_bool_t val)
+{
+    struct sprinter_sexp *spsx = sexp_begin_value(sp);
+    fputs (val ? "#t" : "#f", spsx->stream);
+    if (spsx->state != NULL &&  spsx->state->type == MAP)
+	fputc (')', spsx->stream);
+}
+
+static void
+sexp_null(struct sprinter *sp)
+{
+    struct sprinter_sexp *spsx = sexp_begin_value(sp);
+    fputs ("'()", spsx->stream);
+    spsx->state->first = false;
+}
+
+static void
+sexp_map_key(struct sprinter *sp, const char *key)
+{
+    struct sprinter_sexp *spsx = sexp_begin_value(sp);
+    fputc ('(', spsx->stream);
+    fputs (key, spsx->stream);
+    fputs (" . ", spsx->stream);
+    spsx->state->first = true;
+}
+
+static void
+sexp_frame(struct sprinter *sp)
+{
+    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
+    fputc ('\n', spsx->stream);
+}
+
+struct sprinter *
+sprinter_sexp_new(const void *ctx, FILE *stream)
+{
+    static const struct sprinter_sexp template = {
+	.vtable = {
+	    .begin_map = sexp_begin_map,
+	    .begin_list = sexp_begin_list,
+	    .end = sexp_end,
+	    .string = sexp_string,
+	    .integer = sexp_integer,
+	    .boolean = sexp_boolean,
+	    .null = sexp_null,
+	    .map_key = sexp_map_key,
+	    .frame = sexp_frame,
+	}
+    };
+    struct sprinter_sexp *res;
+
+    res = talloc (ctx, struct sprinter_sexp);
+    if (!res)
+	return NULL;
+
+    *res = template;
+    res->stream = stream;
+    return &res->vtable;
+}
diff --git a/sprinter.h b/sprinter.h
index 1dad9a0..a89eaa5 100644
--- a/sprinter.h
+++ b/sprinter.h
@@ -40,6 +40,10 @@ typedef struct sprinter
 struct sprinter *
 sprinter_json_new(const void *ctx, FILE *stream);
 
+/* Create a new structure printer that emits S-Expressions */
+struct sprinter *
+sprinter_sexp_new(const void *ctx, FILE *stream);
+
 /* A dummy structure printer that signifies that standard text output is
  * to be used instead of any structured format.
  */
-- 
1.7.11.1

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Mark Walters]

On Thu, 12 Jul 2012, craven@gmx.net wrote:
>> what is the advantage of having this as one function rather than end_map
>> and end_list? Indeed, my choice (but I think most other people would
>> disagree) would be to have both functions but still keep state as this
>> currently does and then throw an error if the code closes the wrong
>> thing.
>
> There's probably no advantage, one way or the other is fine, I'd say.
> I've thought about introducing checks into the formatter functions, to
> raise errors for improper closing, map_key not inside a map and things
> like that, I just wasn't sure that would be acceptable.

I will leave others to comment.

>> A second question: do you have an implementation in this style for
>> s-expressions. I find it hard to tell whether the interface is right
>> with just a single example. Even a completely hacky not ready for review
>> example would be helpful.
>
> See the attached patch :)

This looks great. I found it much easier to review by splitting
sprinter.c into two files sprinter-json.c and sprinter-sexp.c and then
running meld on them. The similarity is then very clear. It might be
worth submitting them as two files, but I leave other people to comment.

(Doing so made some of the difference between json and s-exp clear: like
that keys in mapkeys are quoted in json but not in s-exp)

It could be that some of the code could be merged, but I am not sure
that there is much advantage. I would imagine that these two sprinter.c
files would basically never change so there is not much risk of them
diverging.

I wonder if it would be worth using aggregate_t for both rather than
using the closing symbol for this purpose in the json output.

In any case this patch answers my query: the new structure does
generalise very easily to s-expressions!

Best wishes

Mark



>
> Thanks for the suggestions!
>
> Peter
> From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001
> From: <craven@gmx.net>
> Date: Thu, 12 Jul 2012 10:17:05 +0200
> Subject: [PATCH] Add an S-Expression output format.
>
> ---
>  notmuch-search.c |   7 ++-
>  sprinter.c       | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sprinter.h       |   4 ++
>  3 files changed, 180 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-search.c b/notmuch-search.c
> index b853f5f..f8bea9b 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -77,6 +77,7 @@ do_search_threads (sprinter_t *format,
>  
>      if (format != sprinter_text) {
>  	format->begin_list (format);
> +	format->frame (format);
>      }
>  
>      for (i = 0;
> @@ -380,7 +381,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      int exclude = EXCLUDE_TRUE;
>      unsigned int i;
>  
> -    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
> +    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT, NOTMUCH_FORMAT_SEXP }
>  	format_sel = NOTMUCH_FORMAT_TEXT;
>  
>      notmuch_opt_desc_t options[] = {
> @@ -391,6 +392,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
>  	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
>  				  { "text", NOTMUCH_FORMAT_TEXT },
> +				  { "sexp", NOTMUCH_FORMAT_SEXP },
>  				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
>  	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
> @@ -422,6 +424,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      case NOTMUCH_FORMAT_JSON:
>  	format = sprinter_json_new (ctx, stdout);
>  	break;
> +    case NOTMUCH_FORMAT_SEXP:
> +	format = sprinter_sexp_new (ctx, stdout);
> +        break;
>      }
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
> diff --git a/sprinter.c b/sprinter.c
> index 649f79a..fce0f9b 100644
> --- a/sprinter.c
> +++ b/sprinter.c
> @@ -170,3 +170,173 @@ sprinter_json_new(const void *ctx, FILE *stream)
>      res->stream = stream;
>      return &res->vtable;
>  }
> +
> +/*
> + * Every below here is the implementation of the SEXP printer.
> + */
> +
> +typedef enum { MAP, LIST } aggregate_t;
> +
> +struct sprinter_sexp
> +{
> +    struct sprinter vtable;
> +    FILE *stream;
> +    /* Top of the state stack, or NULL if the printer is not currently
> +     * inside any aggregate types. */
> +    struct sexp_state *state;
> +};
> +
> +struct sexp_state
> +{
> +    struct sexp_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. */
> +    aggregate_t type;
> +};
> +
> +static struct sprinter_sexp *
> +sexp_begin_value(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    if (spsx->state) {
> +	if (!spsx->state->first)
> +	    fputc (' ', spsx->stream);
> +	else
> +	    spsx->state->first = false;
> +    }
> +    return spsx;
> +}
> +
> +static void
> +sexp_begin_aggregate(struct sprinter *sp, aggregate_t type)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    struct sexp_state *state = talloc (spsx, struct sexp_state);
> +
> +    fputc ('(', spsx->stream);
> +    state->parent = spsx->state;
> +    state->first = true;
> +    spsx->state = state;
> +    state->type = type;
> +}
> +
> +static void
> +sexp_begin_map(struct sprinter *sp)
> +{
> +    sexp_begin_aggregate (sp, MAP);
> +}
> +
> +static void
> +sexp_begin_list(struct sprinter *sp)
> +{
> +    sexp_begin_aggregate (sp, LIST);
> +}
> +
> +static void
> +sexp_end(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    struct sexp_state *state = spsx->state;
> +
> +    fputc (')', spsx->stream);
> +    spsx->state = state->parent;
> +    talloc_free (state);
> +    if(spsx->state == NULL)
> +	fputc ('\n', spsx->stream);
> +}
> +
> +static void
> +sexp_string(struct sprinter *sp, const char *val)
> +{
> +    static const char * const escapes[] = {
> +	['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
> +	['\f'] = "\\f",  ['\n'] = "\\n",  ['\t'] = "\\t"
> +    };
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputc ('"', spsx->stream);
> +    for (; *val; ++val) {
> +	unsigned char ch = *val;
> +	if (ch < ARRAY_SIZE(escapes) && escapes[ch])
> +	    fputs (escapes[ch], spsx->stream);
> +	else if (ch >= 32)
> +	    fputc (ch, spsx->stream);
> +	else
> +	    fprintf (spsx->stream, "\\u%04x", ch);
> +    }
> +    fputc ('"', spsx->stream);
> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
> +	fputc (')', spsx->stream);
> +    spsx->state->first = false;
> +}
> +
> +static void
> +sexp_integer(struct sprinter *sp, int val)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fprintf (spsx->stream, "%d", val);
> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
> +	fputc (')', spsx->stream);
> +}
> +
> +static void
> +sexp_boolean(struct sprinter *sp, notmuch_bool_t val)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputs (val ? "#t" : "#f", spsx->stream);
> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
> +	fputc (')', spsx->stream);
> +}
> +
> +static void
> +sexp_null(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputs ("'()", spsx->stream);
> +    spsx->state->first = false;
> +}
> +
> +static void
> +sexp_map_key(struct sprinter *sp, const char *key)
> +{
> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
> +    fputc ('(', spsx->stream);
> +    fputs (key, spsx->stream);
> +    fputs (" . ", spsx->stream);
> +    spsx->state->first = true;
> +}
> +
> +static void
> +sexp_frame(struct sprinter *sp)
> +{
> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
> +    fputc ('\n', spsx->stream);
> +}
> +
> +struct sprinter *
> +sprinter_sexp_new(const void *ctx, FILE *stream)
> +{
> +    static const struct sprinter_sexp template = {
> +	.vtable = {
> +	    .begin_map = sexp_begin_map,
> +	    .begin_list = sexp_begin_list,
> +	    .end = sexp_end,
> +	    .string = sexp_string,
> +	    .integer = sexp_integer,
> +	    .boolean = sexp_boolean,
> +	    .null = sexp_null,
> +	    .map_key = sexp_map_key,
> +	    .frame = sexp_frame,
> +	}
> +    };
> +    struct sprinter_sexp *res;
> +
> +    res = talloc (ctx, struct sprinter_sexp);
> +    if (!res)
> +	return NULL;
> +
> +    *res = template;
> +    res->stream = stream;
> +    return &res->vtable;
> +}
> diff --git a/sprinter.h b/sprinter.h
> index 1dad9a0..a89eaa5 100644
> --- a/sprinter.h
> +++ b/sprinter.h
> @@ -40,6 +40,10 @@ typedef struct sprinter
>  struct sprinter *
>  sprinter_json_new(const void *ctx, FILE *stream);
>  
> +/* Create a new structure printer that emits S-Expressions */
> +struct sprinter *
> +sprinter_sexp_new(const void *ctx, FILE *stream);
> +
>  /* A dummy structure printer that signifies that standard text output is
>   * to be used instead of any structured format.
>   */
> -- 
> 1.7.11.1

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Tomi Ollila]

On Thu, Jul 12 2012, Mark Walters <markwalters1009@gmail.com> wrote:

> On Thu, 12 Jul 2012, craven@gmx.net wrote:
>>> what is the advantage of having this as one function rather than end_map
>>> and end_list? Indeed, my choice (but I think most other people would
>>> disagree) would be to have both functions but still keep state as this
>>> currently does and then throw an error if the code closes the wrong
>>> thing.
>>
>> There's probably no advantage, one way or the other is fine, I'd say.
>> I've thought about introducing checks into the formatter functions, to
>> raise errors for improper closing, map_key not inside a map and things
>> like that, I just wasn't sure that would be acceptable.
>
> I will leave others to comment.

I like the current implementation -- record what has been opened and
have a common closing function which knows what it is closing. Less
checking in the code (i.e. less possible branches).
This makes the use of the interface a bit less self-documenting as
'end' is used instead of list/hash end (defining macros for this 
documentation purposes would be really dumb thing to do ;)

What needs to be checked is that software doesn't attempt to 'end'
too many contexts (i quess it's doing it already). Any other output
errors (like forgetting to 'end' some blocks should be taken care
by proper test cases).

>>> A second question: do you have an implementation in this style for
>>> s-expressions. I find it hard to tell whether the interface is right
>>> with just a single example. Even a completely hacky not ready for review
>>> example would be helpful.
>>
>> See the attached patch :)
>
> This looks great. I found it much easier to review by splitting
> sprinter.c into two files sprinter-json.c and sprinter-sexp.c and then
> running meld on them. The similarity is then very clear. It might be
> worth submitting them as two files, but I leave other people to comment.

I like that -- is there (currently) need for splinter.c for common code ?
(splinter.h is definitely needed).

> (Doing so made some of the difference between json and s-exp clear: like
> that keys in mapkeys are quoted in json but not in s-exp)
>
> It could be that some of the code could be merged, but I am not sure
> that there is much advantage. I would imagine that these two sprinter.c
> files would basically never change so there is not much risk of them
> diverging.

I was thinking the same when looking splinter code yesterday -- how to
have even more common code for json&sexp. Maybe there could be something
to do just for these 2 purposes but It requires more effort and might
add more complexity for humans to perceive. ATM I'd go with this interface
and see later if anyone wants to do/experiment more -- as you said the
risk of diverging is minimal -- and in case there are separate source
files for json & sexp diffing those will be easy.


> I wonder if it would be worth using aggregate_t for both rather than
> using the closing symbol for this purpose in the json output.
>
> In any case this patch answers my query: the new structure does
> generalise very easily to s-expressions!
>
> Best wishes
>
> Mark

Tomi


>
>
>
>>
>> Thanks for the suggestions!
>>
>> Peter
>> From cf2c5eeab814970736510ca2210b909643a8cf19 Mon Sep 17 00:00:00 2001
>> From: <craven@gmx.net>
>> Date: Thu, 12 Jul 2012 10:17:05 +0200
>> Subject: [PATCH] Add an S-Expression output format.
>>
>> ---
>>  notmuch-search.c |   7 ++-
>>  sprinter.c       | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  sprinter.h       |   4 ++
>>  3 files changed, 180 insertions(+), 1 deletion(-)
>>
>> diff --git a/notmuch-search.c b/notmuch-search.c
>> index b853f5f..f8bea9b 100644
>> --- a/notmuch-search.c
>> +++ b/notmuch-search.c
>> @@ -77,6 +77,7 @@ do_search_threads (sprinter_t *format,
>>  
>>      if (format != sprinter_text) {
>>  	format->begin_list (format);
>> +	format->frame (format);
>>      }
>>  
>>      for (i = 0;
>> @@ -380,7 +381,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>>      int exclude = EXCLUDE_TRUE;
>>      unsigned int i;
>>  
>> -    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
>> +    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT, NOTMUCH_FORMAT_SEXP }
>>  	format_sel = NOTMUCH_FORMAT_TEXT;
>>  
>>      notmuch_opt_desc_t options[] = {
>> @@ -391,6 +392,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>>  	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
>>  	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
>>  				  { "text", NOTMUCH_FORMAT_TEXT },
>> +				  { "sexp", NOTMUCH_FORMAT_SEXP },
>>  				  { 0, 0 } } },
>>  	{ NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
>>  	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
>> @@ -422,6 +424,9 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>>      case NOTMUCH_FORMAT_JSON:
>>  	format = sprinter_json_new (ctx, stdout);
>>  	break;
>> +    case NOTMUCH_FORMAT_SEXP:
>> +	format = sprinter_sexp_new (ctx, stdout);
>> +        break;
>>      }
>>  
>>      config = notmuch_config_open (ctx, NULL, NULL);
>> diff --git a/sprinter.c b/sprinter.c
>> index 649f79a..fce0f9b 100644
>> --- a/sprinter.c
>> +++ b/sprinter.c
>> @@ -170,3 +170,173 @@ sprinter_json_new(const void *ctx, FILE *stream)
>>      res->stream = stream;
>>      return &res->vtable;
>>  }
>> +
>> +/*
>> + * Every below here is the implementation of the SEXP printer.
>> + */
>> +
>> +typedef enum { MAP, LIST } aggregate_t;
>> +
>> +struct sprinter_sexp
>> +{
>> +    struct sprinter vtable;
>> +    FILE *stream;
>> +    /* Top of the state stack, or NULL if the printer is not currently
>> +     * inside any aggregate types. */
>> +    struct sexp_state *state;
>> +};
>> +
>> +struct sexp_state
>> +{
>> +    struct sexp_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. */
>> +    aggregate_t type;
>> +};
>> +
>> +static struct sprinter_sexp *
>> +sexp_begin_value(struct sprinter *sp)
>> +{
>> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> +    if (spsx->state) {
>> +	if (!spsx->state->first)
>> +	    fputc (' ', spsx->stream);
>> +	else
>> +	    spsx->state->first = false;
>> +    }
>> +    return spsx;
>> +}
>> +
>> +static void
>> +sexp_begin_aggregate(struct sprinter *sp, aggregate_t type)
>> +{
>> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> +    struct sexp_state *state = talloc (spsx, struct sexp_state);
>> +
>> +    fputc ('(', spsx->stream);
>> +    state->parent = spsx->state;
>> +    state->first = true;
>> +    spsx->state = state;
>> +    state->type = type;
>> +}
>> +
>> +static void
>> +sexp_begin_map(struct sprinter *sp)
>> +{
>> +    sexp_begin_aggregate (sp, MAP);
>> +}
>> +
>> +static void
>> +sexp_begin_list(struct sprinter *sp)
>> +{
>> +    sexp_begin_aggregate (sp, LIST);
>> +}
>> +
>> +static void
>> +sexp_end(struct sprinter *sp)
>> +{
>> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> +    struct sexp_state *state = spsx->state;
>> +
>> +    fputc (')', spsx->stream);
>> +    spsx->state = state->parent;
>> +    talloc_free (state);
>> +    if(spsx->state == NULL)
>> +	fputc ('\n', spsx->stream);
>> +}
>> +
>> +static void
>> +sexp_string(struct sprinter *sp, const char *val)
>> +{
>> +    static const char * const escapes[] = {
>> +	['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
>> +	['\f'] = "\\f",  ['\n'] = "\\n",  ['\t'] = "\\t"
>> +    };
>> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> +    fputc ('"', spsx->stream);
>> +    for (; *val; ++val) {
>> +	unsigned char ch = *val;
>> +	if (ch < ARRAY_SIZE(escapes) && escapes[ch])
>> +	    fputs (escapes[ch], spsx->stream);
>> +	else if (ch >= 32)
>> +	    fputc (ch, spsx->stream);
>> +	else
>> +	    fprintf (spsx->stream, "\\u%04x", ch);
>> +    }
>> +    fputc ('"', spsx->stream);
>> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
>> +	fputc (')', spsx->stream);
>> +    spsx->state->first = false;
>> +}
>> +
>> +static void
>> +sexp_integer(struct sprinter *sp, int val)
>> +{
>> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> +    fprintf (spsx->stream, "%d", val);
>> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
>> +	fputc (')', spsx->stream);
>> +}
>> +
>> +static void
>> +sexp_boolean(struct sprinter *sp, notmuch_bool_t val)
>> +{
>> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> +    fputs (val ? "#t" : "#f", spsx->stream);
>> +    if (spsx->state != NULL &&  spsx->state->type == MAP)
>> +	fputc (')', spsx->stream);
>> +}
>> +
>> +static void
>> +sexp_null(struct sprinter *sp)
>> +{
>> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> +    fputs ("'()", spsx->stream);
>> +    spsx->state->first = false;
>> +}
>> +
>> +static void
>> +sexp_map_key(struct sprinter *sp, const char *key)
>> +{
>> +    struct sprinter_sexp *spsx = sexp_begin_value(sp);
>> +    fputc ('(', spsx->stream);
>> +    fputs (key, spsx->stream);
>> +    fputs (" . ", spsx->stream);
>> +    spsx->state->first = true;
>> +}
>> +
>> +static void
>> +sexp_frame(struct sprinter *sp)
>> +{
>> +    struct sprinter_sexp *spsx = (struct sprinter_sexp*)sp;
>> +    fputc ('\n', spsx->stream);
>> +}
>> +
>> +struct sprinter *
>> +sprinter_sexp_new(const void *ctx, FILE *stream)
>> +{
>> +    static const struct sprinter_sexp template = {
>> +	.vtable = {
>> +	    .begin_map = sexp_begin_map,
>> +	    .begin_list = sexp_begin_list,
>> +	    .end = sexp_end,
>> +	    .string = sexp_string,
>> +	    .integer = sexp_integer,
>> +	    .boolean = sexp_boolean,
>> +	    .null = sexp_null,
>> +	    .map_key = sexp_map_key,
>> +	    .frame = sexp_frame,
>> +	}
>> +    };
>> +    struct sprinter_sexp *res;
>> +
>> +    res = talloc (ctx, struct sprinter_sexp);
>> +    if (!res)
>> +	return NULL;
>> +
>> +    *res = template;
>> +    res->stream = stream;
>> +    return &res->vtable;
>> +}
>> diff --git a/sprinter.h b/sprinter.h
>> index 1dad9a0..a89eaa5 100644
>> --- a/sprinter.h
>> +++ b/sprinter.h
>> @@ -40,6 +40,10 @@ typedef struct sprinter
>>  struct sprinter *
>>  sprinter_json_new(const void *ctx, FILE *stream);
>>  
>> +/* Create a new structure printer that emits S-Expressions */
>> +struct sprinter *
>> +sprinter_sexp_new(const void *ctx, FILE *stream);
>> +
>>  /* A dummy structure printer that signifies that standard text output is
>>   * to be used instead of any structured format.
>>   */
>> -- 
>> 1.7.11.1
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Tomi Ollila]

On Thu, Jul 12 2012, craven@gmx.net wrote:

> This patch adds a new type sprinter_t, which is used for structured
> formatting, e.g. JSON or S-Expressions. The structure printer is the
> code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).

...

> +
> +/* Create a new structure printer that emits JSON */
> +struct sprinter *
> +sprinter_json_new(const void *ctx, FILE *stream);
> +
> +/* A dummy structure printer that signifies that standard text output is
> + * to be used instead of any structured format.
> + */
> +struct sprinter *
> +sprinter_text;

Looks good... but this needs to be 'extern struct sprinter * sprinter_text;'

see id:"1340563714-3103-1-git-send-email-tomi.ollila@iki.fi"
for reference.


> +
> +#endif // NOTMUCH_SPRINTER_H
> -- 
> 1.7.11.1

Tomi

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Austin Clements]

Quoth craven@gmx.net on Jul 12 at  9:43 am:
> This patch adds a new type sprinter_t, which is used for structured
> formatting, e.g. JSON or S-Expressions. The structure printer is the
> code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).
> 
> The structure printer contains the following function pointers:
> 
> /* start a new map/dictionary structure.
>  */
> void (*begin_map) (struct sprinter *);
> 
> /* start a new list/array structure
>  */
> void (*begin_list) (struct sprinter *);
> 
> /* end the last opened list or map structure
>  */
> void (*end) (struct sprinter *);
> 
> /* print one string/integer/boolean/null element (possibly inside a
>  * list or map
>  */
> void (*string) (struct sprinter *, const char *);
> void (*integer) (struct sprinter *, int);
> void (*boolean) (struct sprinter *, notmuch_bool_t);
> void (*null) (struct sprinter *);
> 
> /* print the key of a map's key/value pair.
>  */
> void (*map_key) (struct sprinter *, const char *);
> 
> /* print a frame delimiter (such as a newline or extra whitespace)
>  */
> void (*frame) (struct sprinter *);
> 
> The printer can (and should) use internal state to insert delimiters and
> syntax at the correct places.
> 
> Example:
> 
> format->begin_map(format);
> format->map_key(format, "foo");
> format->begin_list(format);
> format->integer(format, 1);
> format->integer(format, 2);
> format->integer(format, 3);
> format->end(format);
> format->map_key(format, "bar");
> format->begin_map(format);
> format->map_key(format, "baaz");
> format->string(format, "hello world");
> format->end(format);
> format->end(format);
> 
> would output JSON as follows:
> 
> {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}
> ---
>  sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 sprinter.h
> 
> diff --git a/sprinter.h b/sprinter.h
> new file mode 100644
> index 0000000..1dad9a0
> --- /dev/null
> +++ b/sprinter.h
> @@ -0,0 +1,49 @@
> +#ifndef NOTMUCH_SPRINTER_H
> +#define NOTMUCH_SPRINTER_H
> +
> +/* for notmuch_bool_t */

Style/consistency nit: all of the comments should start with a capital
letter and anything that's a sentence should end with a period (some
of your comments do, some of them don't).

> +#include "notmuch-client.h"
> +
> +/* Structure printer interface */
> +typedef struct sprinter
> +{
> +    /* start a new map/dictionary structure.

This should probably mention how other functions should be called
within the map structure.  Perhaps,

/* Start a new map/dictionary structure.  This should be followed by a
 * sequence of alternating calls to map_key and one of the value
 * printing functions until the map is ended.
 */

(You had a comment to this effect in one of your earlier patches.)

> +     */
> +    void (*begin_map) (struct sprinter *);
> +
> +    /* start a new list/array structure
> +     */
> +    void (*begin_list) (struct sprinter *);
> +
> +    /* end the last opened list or map structure
> +     */
> +    void (*end) (struct sprinter *);
> +
> +    /* print one string/integer/boolean/null element (possibly inside a
> +     * list or map

Missing close paren.

> +     */
> +    void (*string) (struct sprinter *, const char *);
> +    void (*integer) (struct sprinter *, int);
> +    void (*boolean) (struct sprinter *, notmuch_bool_t);
> +    void (*null) (struct sprinter *);
> +
> +    /* print the key of a map's key/value pair.
> +     */
> +    void (*map_key) (struct sprinter *, const char *);
> +
> +    /* print a frame delimiter (such as a newline or extra whitespace)
> +     */
> +    void (*frame) (struct sprinter *);

I wonder if frame is still necessary.  At the time, I was thinking
that we would use embedded newline framing to do streaming JSON
parsing, but I wound up going with a general incremental JSON parser,
eliminating the need for framing.

It's probably still useful to have a function that inserts a newline
purely for readability/debuggability purposes.  In practice, the
implementation would be the same as frame (though if it's for
readability, maybe you'd want to print the comma before the newline
instead of after), but since the intent would be different, it would
need different documentation and maybe a different name.  "Frame"
isn't a bad name for either intent.  We can't use "break".
"Separator" (or just "sep")?  "Space"?  "Split"?  The comment could be
something like

/* Insert a separator for improved readability without affecting the
 * abstract syntax of the structure being printed.  For JSON, this is
 * simply a line break.
 */

> +} sprinter_t;
> +
> +/* Create a new structure printer that emits JSON */
> +struct sprinter *
> +sprinter_json_new(const void *ctx, FILE *stream);

This should probably be called sprinter_json_create to be consistent
with the naming of other constructors in notmuch.

Also, missing space between the function name and parameter list
(sorry, my fault).

> +
> +/* A dummy structure printer that signifies that standard text output is
> + * to be used instead of any structured format.
> + */
> +struct sprinter *
> +sprinter_text;

It looks like you never assign anything to this pointer, so all of
your tests against sprinter_text are equivalent to just checking for a
NULL pointer.  You could either just use NULL as the designated value
for the text format, or you could use this global variable, but make
it a struct sprinter instead of a struct sprinter *.

Also, this can probably be a static declaration in notmuch-search.c.

> +
> +#endif // NOTMUCH_SPRINTER_H

Re: [PATCH v4 1/3] Add support for structured output formatters.

[Austin Clements]

Quoth craven@gmx.net on Jul 12 at  9:43 am:
> This patch adds a new type sprinter_t, which is used for structured
> formatting, e.g. JSON or S-Expressions. The structure printer is the
> code from Austin Clements (id:87d34hsdx8.fsf@awakening.csail.mit.edu).
> 
> The structure printer contains the following function pointers:
> 
> /* start a new map/dictionary structure.
>  */
> void (*begin_map) (struct sprinter *);
> 
> /* start a new list/array structure
>  */
> void (*begin_list) (struct sprinter *);
> 
> /* end the last opened list or map structure
>  */
> void (*end) (struct sprinter *);
> 
> /* print one string/integer/boolean/null element (possibly inside a
>  * list or map
>  */
> void (*string) (struct sprinter *, const char *);
> void (*integer) (struct sprinter *, int);
> void (*boolean) (struct sprinter *, notmuch_bool_t);
> void (*null) (struct sprinter *);
> 
> /* print the key of a map's key/value pair.
>  */
> void (*map_key) (struct sprinter *, const char *);
> 
> /* print a frame delimiter (such as a newline or extra whitespace)
>  */
> void (*frame) (struct sprinter *);
> 
> The printer can (and should) use internal state to insert delimiters and
> syntax at the correct places.
> 
> Example:
> 
> format->begin_map(format);
> format->map_key(format, "foo");
> format->begin_list(format);
> format->integer(format, 1);
> format->integer(format, 2);
> format->integer(format, 3);
> format->end(format);
> format->map_key(format, "bar");
> format->begin_map(format);
> format->map_key(format, "baaz");
> format->string(format, "hello world");
> format->end(format);
> format->end(format);
> 
> would output JSON as follows:
> 
> {"foo": [1, 2, 3], "bar": { "baaz": "hello world"}}
> ---
>  sprinter.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 sprinter.h
> 
> diff --git a/sprinter.h b/sprinter.h
> new file mode 100644
> index 0000000..1dad9a0
> --- /dev/null
> +++ b/sprinter.h
> @@ -0,0 +1,49 @@
> +#ifndef NOTMUCH_SPRINTER_H
> +#define NOTMUCH_SPRINTER_H
> +
> +/* for notmuch_bool_t */
> +#include "notmuch-client.h"
> +
> +/* Structure printer interface */
> +typedef struct sprinter
> +{
> +    /* start a new map/dictionary structure.
> +     */
> +    void (*begin_map) (struct sprinter *);
> +
> +    /* start a new list/array structure
> +     */
> +    void (*begin_list) (struct sprinter *);
> +
> +    /* end the last opened list or map structure
> +     */
> +    void (*end) (struct sprinter *);
> +
> +    /* print one string/integer/boolean/null element (possibly inside a
> +     * list or map
> +     */
> +    void (*string) (struct sprinter *, const char *);

Oh, also, the documentation for string should mention the string
argument should be UTF-8 encoded.

> +    void (*integer) (struct sprinter *, int);
> +    void (*boolean) (struct sprinter *, notmuch_bool_t);
> +    void (*null) (struct sprinter *);
> +
> +    /* print the key of a map's key/value pair.
> +     */
> +    void (*map_key) (struct sprinter *, const char *);
> +
> +    /* print a frame delimiter (such as a newline or extra whitespace)
> +     */
> +    void (*frame) (struct sprinter *);
> +} sprinter_t;
> +
> +/* Create a new structure printer that emits JSON */
> +struct sprinter *
> +sprinter_json_new(const void *ctx, FILE *stream);
> +
> +/* A dummy structure printer that signifies that standard text output is
> + * to be used instead of any structured format.
> + */
> +struct sprinter *
> +sprinter_text;
> +
> +#endif // NOTMUCH_SPRINTER_H

[PATCH v4 2/3] Add structured output formatter for JSON.

[craven]

Using the new structured printer support in sprinter.h, implement
sprinter_json_new, which returns a new JSON structured output
formatter.

The formatter prints output similar to the existing JSON, but with
differences in whitespace (mostly newlines).
---
 Makefile.local |   1 +
 sprinter.c     | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+)
 create mode 100644 sprinter.c

diff --git a/Makefile.local b/Makefile.local
index a890df2..8baf0c2 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -290,6 +290,7 @@ notmuch_client_srcs =		\
 	notmuch-show.c		\
 	notmuch-tag.c		\
 	notmuch-time.c		\
+	sprinter.c		\
 	query-string.c		\
 	mime-node.c		\
 	crypto.c		\
diff --git a/sprinter.c b/sprinter.c
new file mode 100644
index 0000000..649f79a
--- /dev/null
+++ b/sprinter.c
@@ -0,0 +1,172 @@
+#include <stdbool.h>
+#include <stdio.h>
+#include <talloc.h>
+#include "sprinter.h"
+
+#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
+
+struct sprinter *
+sprinter_text = NULL;
+
+/*
+ * Every below here is the implementation of the JSON printer.
+ */
+
+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;
+};
+
+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)
+	    fputs (", ", 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_frame(struct sprinter *sp)
+{
+    struct sprinter_json *spj = (struct sprinter_json*)sp;
+    fputc ('\n', spj->stream);
+}
+
+struct sprinter *
+sprinter_json_new(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,
+	    .frame = json_frame,
+	}
+    };
+    struct sprinter_json *res;
+
+    res = talloc (ctx, struct sprinter_json);
+    if (!res)
+	return NULL;
+
+    *res = template;
+    res->stream = stream;
+    return &res->vtable;
+}
-- 
1.7.11.1

Re: [PATCH v4 2/3] Add structured output formatter for JSON.

[Tomi Ollila]

On Thu, Jul 12 2012, craven@gmx.net wrote:

> Using the new structured printer support in sprinter.h, implement
> sprinter_json_new, which returns a new JSON structured output
> formatter.
>
> The formatter prints output similar to the existing JSON, but with
> differences in whitespace (mostly newlines).
> ---
>  Makefile.local |   1 +
>  sprinter.c     | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 173 insertions(+)
>  create mode 100644 sprinter.c
>
> diff --git a/Makefile.local b/Makefile.local
> index a890df2..8baf0c2 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -290,6 +290,7 @@ notmuch_client_srcs =		\
>  	notmuch-show.c		\
>  	notmuch-tag.c		\
>  	notmuch-time.c		\
> +	sprinter.c		\
>  	query-string.c		\
>  	mime-node.c		\
>  	crypto.c		\
> diff --git a/sprinter.c b/sprinter.c
> new file mode 100644
> index 0000000..649f79a
> --- /dev/null
> +++ b/sprinter.c
> @@ -0,0 +1,172 @@
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <talloc.h>
> +#include "sprinter.h"
> +
> +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))

You're including sprinter.h which includes notmuch-client.h which
defines ARRAY_SIZE (Interesting that you did not get error(/warning?)
about this)

Rest looks good -- except the whitespace -- as I looked through Austin's
code yesterday you're just replicated the same lines :D. Easiest
to fix is probably just running 
uncrustify -c devel/uncrustify.cfg --replace sprinter.c 

Tomi

> +
> +struct sprinter *
> +sprinter_text = NULL;
> +
> +/*
> + * Every below here is the implementation of the JSON printer.
> + */
> +
> +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;
> +};
> +
> +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)
> +	    fputs (", ", 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_frame(struct sprinter *sp)
> +{
> +    struct sprinter_json *spj = (struct sprinter_json*)sp;
> +    fputc ('\n', spj->stream);
> +}
> +
> +struct sprinter *
> +sprinter_json_new(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,
> +	    .frame = json_frame,
> +	}
> +    };
> +    struct sprinter_json *res;
> +
> +    res = talloc (ctx, struct sprinter_json);
> +    if (!res)
> +	return NULL;
> +
> +    *res = template;
> +    res->stream = stream;
> +    return &res->vtable;
> +}
> -- 
> 1.7.11.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 2/3] Add structured output formatter for JSON.

[Austin Clements]

Quoth Tomi Ollila on Jul 12 at  1:10 pm:
> On Thu, Jul 12 2012, craven@gmx.net wrote:
> 
> > Using the new structured printer support in sprinter.h, implement
> > sprinter_json_new, which returns a new JSON structured output
> > formatter.
> >
> > The formatter prints output similar to the existing JSON, but with
> > differences in whitespace (mostly newlines).
> > ---
> >  Makefile.local |   1 +
> >  sprinter.c     | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 173 insertions(+)
> >  create mode 100644 sprinter.c
> >
> > diff --git a/Makefile.local b/Makefile.local
> > index a890df2..8baf0c2 100644
> > --- a/Makefile.local
> > +++ b/Makefile.local
> > @@ -290,6 +290,7 @@ notmuch_client_srcs =		\
> >  	notmuch-show.c		\
> >  	notmuch-tag.c		\
> >  	notmuch-time.c		\
> > +	sprinter.c		\
> >  	query-string.c		\
> >  	mime-node.c		\
> >  	crypto.c		\
> > diff --git a/sprinter.c b/sprinter.c
> > new file mode 100644
> > index 0000000..649f79a
> > --- /dev/null
> > +++ b/sprinter.c
> > @@ -0,0 +1,172 @@
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <talloc.h>
> > +#include "sprinter.h"
> > +
> > +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
> 
> You're including sprinter.h which includes notmuch-client.h which
> defines ARRAY_SIZE (Interesting that you did not get error(/warning?)
> about this)
> 
> Rest looks good -- except the whitespace -- as I looked through Austin's
> code yesterday you're just replicated the same lines :D. Easiest
> to fix is probably just running 
> uncrustify -c devel/uncrustify.cfg --replace sprinter.c 

Oops.  Shame on me.  Is anything wrong besides the missing space
before the argument list of all of the function declarations?

Re: [PATCH v4 2/3] Add structured output formatter for JSON.

[Austin Clements]

Quoth craven@gmx.net on Jul 12 at  9:43 am:
> Using the new structured printer support in sprinter.h, implement
> sprinter_json_new, which returns a new JSON structured output
> formatter.
> 
> The formatter prints output similar to the existing JSON, but with
> differences in whitespace (mostly newlines).
> ---
>  Makefile.local |   1 +
>  sprinter.c     | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 173 insertions(+)
>  create mode 100644 sprinter.c
> 
> diff --git a/Makefile.local b/Makefile.local
> index a890df2..8baf0c2 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -290,6 +290,7 @@ notmuch_client_srcs =		\
>  	notmuch-show.c		\
>  	notmuch-tag.c		\
>  	notmuch-time.c		\
> +	sprinter.c		\
>  	query-string.c		\
>  	mime-node.c		\
>  	crypto.c		\
> diff --git a/sprinter.c b/sprinter.c
> new file mode 100644
> index 0000000..649f79a
> --- /dev/null
> +++ b/sprinter.c
> @@ -0,0 +1,172 @@
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <talloc.h>
> +#include "sprinter.h"
> +
> +#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
> +
> +struct sprinter *
> +sprinter_text = NULL;
> +
> +/*
> + * Every below here is the implementation of the JSON printer.

s/Every/Everything/

Or this can be shortened to just

/*
 * JSON printer
 */

If we wind up with separate files for the JSON and S-exp printers, I
don't think this comment is necessary at all.

> + */
> +
> +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;
> +};
> +
> +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)

As Tomi pointed out, there should be spaces before the parameter
lists (sorry).

> +{
> +    struct sprinter_json *spj = (struct sprinter_json*)sp;
> +    if (spj->state) {
> +	if (!spj->state->first)
> +	    fputs (", ", 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)

Ah, another missing space.

> +	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_frame(struct sprinter *sp)
> +{
> +    struct sprinter_json *spj = (struct sprinter_json*)sp;
> +    fputc ('\n', spj->stream);
> +}
> +
> +struct sprinter *
> +sprinter_json_new(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,
> +	    .frame = json_frame,
> +	}
> +    };
> +    struct sprinter_json *res;
> +
> +    res = talloc (ctx, struct sprinter_json);
> +    if (!res)
> +	return NULL;
> +
> +    *res = template;
> +    res->stream = stream;
> +    return &res->vtable;
> +}

[PATCH v4 3/3] Use the structured format printer for JSON in notmuch search.

[craven]

This patch switches from the current ad-hoc printer to the structured
output formatter in sprinter.h.

It removes search_format_t, replaces it by sprinter_t and inlines the
text printer where necessary.

The tests are changed (only whitespaces and regular expressions) in
order to make them PASS for the new structured output formatter.
---
 notmuch-search.c   | 349 ++++++++++++++++++++++-------------------------------
 test/json          |  20 +--
 test/search-output | 270 +++++++++++++++++++++--------------------
 3 files changed, 288 insertions(+), 351 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 3be296d..b853f5f 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,91 +29,9 @@ 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 const char * text_item_sep = "\n";
+static const char * text_results_null = "";
+static const char * text_results_end = "\n";
 
 static char *
 sanitize_string (const void *ctx, const char *str)
@@ -131,72 +50,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,
@@ -220,7 +75,9 @@ do_search_threads (const search_format_t *format,
     if (threads == NULL)
 	return 1;
 
-    fputs (format->results_start, stdout);
+    if (format != sprinter_text) {
+	format->begin_list (format);
+    }
 
     for (i = 0;
 	 notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);
@@ -235,43 +92,96 @@ do_search_threads (const search_format_t *format,
 	    continue;
 	}
 
-	if (! first_thread)
-	    fputs (format->item_sep, stdout);
+	if (format == sprinter_text && ! first_thread)
+	    fputs (text_item_sep, stdout);
 
 	if (output == OUTPUT_THREADS) {
-	    format->item_id (thread, "thread:",
-			     notmuch_thread_get_thread_id (thread));
+	    const char *thread_id = notmuch_thread_get_thread_id (thread);
+	    if (format == sprinter_text)
+		printf ("thread:%s", thread_id);
+	    else {
+		format->string (format, thread_id);
+		format->frame (format);
+	    }
+
 	} else { /* output == OUTPUT_SUMMARY */
-	    fputs (format->item_start, stdout);
+	    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;
+
+	    if (format != sprinter_text)
+		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));
+	    void *ctx_quote = talloc_new (thread);
+	    relative_date =
+		notmuch_time_relative_date (ctx_quote, date);
+
+	    if (format == sprinter_text) {
+		printf ("thread:%s %12s [%d/%d] %s; %s",
+			thread_id,
+		        relative_date,
+			matched,
+			total,
+			sanitize_string (ctx_quote, authors),
+			sanitize_string (ctx_quote, subject));
+	    } else {
+		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);
+	    if (format == sprinter_text) {
+		fputs (" (", stdout);
+	    } else {
+		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));
+		const char *tag = notmuch_tags_get (tags);
+		if (format == sprinter_text) {
+		    if (! first_tag)
+			fputs (" ", stdout);
+		    fputs (tag, stdout);
+		} else {
+		    format->string (format, tag);
+		}
+
 		first_tag = 0;
 	    }
 
-	    fputs (format->tag_end, stdout);
-
-	    fputs (format->item_end, stdout);
+	    if (format == sprinter_text) {
+		fputs (")", stdout);
+	    } else {
+		format->end (format);
+		format->end (format);
+		format->frame (format);
+	    }
 	}
 
 	first_thread = 0;
@@ -279,16 +189,20 @@ do_search_threads (const search_format_t *format,
 	notmuch_thread_destroy (thread);
     }
 
-    if (first_thread)
-	fputs (format->results_null, stdout);
-    else
-	fputs (format->results_end, stdout);
+    if (format == sprinter_text)
+	if (first_thread)
+	    fputs (text_results_null, stdout);
+	else
+	    fputs (text_results_end, stdout);
+    else {
+	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,
@@ -310,7 +224,8 @@ do_search_messages (const search_format_t *format,
     if (messages == NULL)
 	return 1;
 
-    fputs (format->results_start, stdout);
+    if (format != sprinter_text)
+	format->begin_list (format);
 
     for (i = 0;
 	 notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);
@@ -328,23 +243,36 @@ 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);
+		const char *filenames_str = notmuch_filenames_get (filenames);
+
+		if (format == sprinter_text && ! first_message)
+		    fputs (text_item_sep, stdout);
 
-		format->item_id (message, "",
-				 notmuch_filenames_get (filenames));
+		if (format == sprinter_text)
+		    fputs (filenames_str, stdout);
+		else {
+		    format->string (format, filenames_str);
+		    format->frame (format);
+		}
 
 		first_message = 0;
 	    }
-	    
-	    notmuch_filenames_destroy( filenames );
+
+	    notmuch_filenames_destroy (filenames);
 
 	} else { /* output == OUTPUT_MESSAGES */
-	    if (! first_message)
-		fputs (format->item_sep, stdout);
+	    const char *message_id = notmuch_message_get_message_id (message);
+
+	    if (format == sprinter_text && ! first_message)
+		fputs (text_item_sep, stdout);
+
+	    if (format == sprinter_text)
+		printf ("id:%s", message_id);
+	    else {
+		format->string (format, message_id);
+		format->frame (format);
+	    }
 
-	    format->item_id (message, "id:",
-			     notmuch_message_get_message_id (message));
 	    first_message = 0;
 	}
 
@@ -353,17 +281,21 @@ 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);
+    if (format == sprinter_text)
+	if (first_message)
+	    fputs (text_results_null, stdout);
+	else
+	    fputs (text_results_end, stdout);
+    else {
+	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;
@@ -387,7 +319,8 @@ do_search_tags (notmuch_database_t *notmuch,
     if (tags == NULL)
 	return 1;
 
-    fputs (format->results_start, stdout);
+    if (format != sprinter_text)
+	format->begin_list (format);
 
     for (;
 	 notmuch_tags_valid (tags);
@@ -395,10 +328,15 @@ do_search_tags (notmuch_database_t *notmuch,
     {
 	tag = notmuch_tags_get (tags);
 
-	if (! first_tag)
-	    fputs (format->item_sep, stdout);
+	if (format == sprinter_text && ! first_tag)
+	    fputs (text_item_sep, stdout);
 
-	format->item_id (tags, "", tag);
+	if (format == sprinter_text)
+	    fputs (tag, stdout);
+	else {
+	    format->string (format, tag);
+	    format->frame (format);
+	}
 
 	first_tag = 0;
     }
@@ -408,10 +346,14 @@ 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);
+    if (format == sprinter_text)
+	if (first_tag)
+	    fputs (text_results_null, stdout);
+	else
+	    fputs (text_results_end, stdout);
+    else {
+	format->end (format);
+    }
 
     return 0;
 }
@@ -430,7 +372,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; /* the default output is text */
     int opt_index, ret;
     output_t output = OUTPUT_SUMMARY;
     int offset = 0;
@@ -475,10 +417,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 
     switch (format_sel) {
     case NOTMUCH_FORMAT_TEXT:
-	format = &format_text;
+	format = NULL;
 	break;
     case NOTMUCH_FORMAT_JSON:
-	format = &format_json;
+	format = sprinter_json_new (ctx, stdout);
 	break;
     }
 
@@ -546,5 +488,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     notmuch_query_destroy (query);
     notmuch_database_destroy (notmuch);
 
+    if (format != sprinter_text)
+	talloc_free(format);
+
     return ret;
 }
diff --git a/test/json b/test/json
index 6439788..88b8a6d 100755
--- a/test/json
+++ b/test/json
@@ -10,14 +10,8 @@ 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)
-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 +34,7 @@ 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
diff --git a/test/search-output b/test/search-output
index 8b57a43..f2650f7 100755
--- a/test/search-output
+++ b/test/search-output
@@ -37,30 +37,31 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=threads --format=json"
 notmuch search --format=json --output=threads '*' | sed -e s/\".*\"/\"THREADID\"/ >OUTPUT
 cat <<EOF >EXPECTED
-["THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID",
-"THREADID"]
+["THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+, "THREADID"
+]
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
@@ -125,58 +126,59 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=messages --format=json"
 notmuch search --format=json --output=messages '*' >OUTPUT
 cat <<EOF >EXPECTED
-["4EFC743A.3060609@april.org",
-"877h1wv7mg.fsf@inf-8657.int-evry.fr",
-"1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk",
-"877htoqdbo.fsf@yoom.home.cworth.org",
-"878we4qdqf.fsf@yoom.home.cworth.org",
-"87aaykqe24.fsf@yoom.home.cworth.org",
-"87bpj0qeng.fsf@yoom.home.cworth.org",
-"87fx8cqf8v.fsf@yoom.home.cworth.org",
-"87hbssqfix.fsf@yoom.home.cworth.org",
-"87iqd8qgiz.fsf@yoom.home.cworth.org",
-"87k4xoqgnl.fsf@yoom.home.cworth.org",
-"87ocn0qh6d.fsf@yoom.home.cworth.org",
-"87pr7gqidx.fsf@yoom.home.cworth.org",
-"867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
-"1258532999-9316-1-git-send-email-keithp@keithp.com",
-"86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
-"86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
-"ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com",
-"86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
-"736613.51770.qm@web113505.mail.gq1.yahoo.com",
-"1258520223-15328-1-git-send-email-jan@ryngle.com",
-"ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com",
-"1258510940-7018-1-git-send-email-stewart@flamingspork.com",
-"yunzl6kd1w0.fsf@aiko.keithp.com",
-"yun1vjwegii.fsf@aiko.keithp.com",
-"yun3a4cegoa.fsf@aiko.keithp.com",
-"1258509400-32511-1-git-send-email-stewart@flamingspork.com",
-"1258506353-20352-1-git-send-email-stewart@flamingspork.com",
-"20091118010116.GC25380@dottiness.seas.harvard.edu",
-"20091118005829.GB25380@dottiness.seas.harvard.edu",
-"20091118005040.GA25380@dottiness.seas.harvard.edu",
-"cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com",
-"1258500222-32066-1-git-send-email-ingmar@exherbo.org",
-"20091117232137.GA7669@griffis1.net",
-"20091118002059.067214ed@hikari",
-"1258498485-sup-142@elly",
-"f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com",
-"f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com",
-"1258496327-12086-1-git-send-email-jan@ryngle.com",
-"1258493565-13508-1-git-send-email-keithp@keithp.com",
-"yunaayketfm.fsf@aiko.keithp.com",
-"yunbpj0etua.fsf@aiko.keithp.com",
-"1258491078-29658-1-git-send-email-dottedmag@dottedmag.net",
-"87fx8can9z.fsf@vertex.dottedmag",
-"20091117203301.GV3165@dottiness.seas.harvard.edu",
-"87lji4lx9v.fsf@yoom.home.cworth.org",
-"cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com",
-"87iqd9rn3l.fsf@vertex.dottedmag",
-"20091117190054.GU3165@dottiness.seas.harvard.edu",
-"87lji5cbwo.fsf@yoom.home.cworth.org",
-"1258471718-6781-2-git-send-email-dottedmag@dottedmag.net",
-"1258471718-6781-1-git-send-email-dottedmag@dottedmag.net"]
+["4EFC743A.3060609@april.org"
+, "877h1wv7mg.fsf@inf-8657.int-evry.fr"
+, "1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk"
+, "877htoqdbo.fsf@yoom.home.cworth.org"
+, "878we4qdqf.fsf@yoom.home.cworth.org"
+, "87aaykqe24.fsf@yoom.home.cworth.org"
+, "87bpj0qeng.fsf@yoom.home.cworth.org"
+, "87fx8cqf8v.fsf@yoom.home.cworth.org"
+, "87hbssqfix.fsf@yoom.home.cworth.org"
+, "87iqd8qgiz.fsf@yoom.home.cworth.org"
+, "87k4xoqgnl.fsf@yoom.home.cworth.org"
+, "87ocn0qh6d.fsf@yoom.home.cworth.org"
+, "87pr7gqidx.fsf@yoom.home.cworth.org"
+, "867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
+, "1258532999-9316-1-git-send-email-keithp@keithp.com"
+, "86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
+, "86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
+, "ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com"
+, "86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
+, "736613.51770.qm@web113505.mail.gq1.yahoo.com"
+, "1258520223-15328-1-git-send-email-jan@ryngle.com"
+, "ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com"
+, "1258510940-7018-1-git-send-email-stewart@flamingspork.com"
+, "yunzl6kd1w0.fsf@aiko.keithp.com"
+, "yun1vjwegii.fsf@aiko.keithp.com"
+, "yun3a4cegoa.fsf@aiko.keithp.com"
+, "1258509400-32511-1-git-send-email-stewart@flamingspork.com"
+, "1258506353-20352-1-git-send-email-stewart@flamingspork.com"
+, "20091118010116.GC25380@dottiness.seas.harvard.edu"
+, "20091118005829.GB25380@dottiness.seas.harvard.edu"
+, "20091118005040.GA25380@dottiness.seas.harvard.edu"
+, "cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com"
+, "1258500222-32066-1-git-send-email-ingmar@exherbo.org"
+, "20091117232137.GA7669@griffis1.net"
+, "20091118002059.067214ed@hikari"
+, "1258498485-sup-142@elly"
+, "f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com"
+, "f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com"
+, "1258496327-12086-1-git-send-email-jan@ryngle.com"
+, "1258493565-13508-1-git-send-email-keithp@keithp.com"
+, "yunaayketfm.fsf@aiko.keithp.com"
+, "yunbpj0etua.fsf@aiko.keithp.com"
+, "1258491078-29658-1-git-send-email-dottedmag@dottedmag.net"
+, "87fx8can9z.fsf@vertex.dottedmag"
+, "20091117203301.GV3165@dottiness.seas.harvard.edu"
+, "87lji4lx9v.fsf@yoom.home.cworth.org"
+, "cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com"
+, "87iqd9rn3l.fsf@vertex.dottedmag"
+, "20091117190054.GU3165@dottiness.seas.harvard.edu"
+, "87lji5cbwo.fsf@yoom.home.cworth.org"
+, "1258471718-6781-2-git-send-email-dottedmag@dottedmag.net"
+, "1258471718-6781-1-git-send-email-dottedmag@dottedmag.net"
+]
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
@@ -242,59 +244,60 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=files --format=json"
 notmuch search --format=json --output=files '*' | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT
 cat <<EOF >EXPECTED
-["MAIL_DIR/cur/52:2,",
-"MAIL_DIR/cur/53:2,",
-"MAIL_DIR/cur/50:2,",
-"MAIL_DIR/cur/49:2,",
-"MAIL_DIR/cur/48:2,",
-"MAIL_DIR/cur/47:2,",
-"MAIL_DIR/cur/46:2,",
-"MAIL_DIR/cur/45:2,",
-"MAIL_DIR/cur/44:2,",
-"MAIL_DIR/cur/43:2,",
-"MAIL_DIR/cur/42:2,",
-"MAIL_DIR/cur/41:2,",
-"MAIL_DIR/cur/40:2,",
-"MAIL_DIR/cur/39:2,",
-"MAIL_DIR/cur/38:2,",
-"MAIL_DIR/cur/37:2,",
-"MAIL_DIR/cur/36:2,",
-"MAIL_DIR/cur/35:2,",
-"MAIL_DIR/cur/34:2,",
-"MAIL_DIR/cur/33:2,",
-"MAIL_DIR/cur/32:2,",
-"MAIL_DIR/cur/31:2,",
-"MAIL_DIR/cur/30:2,",
-"MAIL_DIR/cur/29:2,",
-"MAIL_DIR/cur/28:2,",
-"MAIL_DIR/cur/27:2,",
-"MAIL_DIR/cur/26:2,",
-"MAIL_DIR/cur/25:2,",
-"MAIL_DIR/cur/24:2,",
-"MAIL_DIR/cur/23:2,",
-"MAIL_DIR/cur/22:2,",
-"MAIL_DIR/cur/21:2,",
-"MAIL_DIR/cur/19:2,",
-"MAIL_DIR/cur/18:2,",
-"MAIL_DIR/cur/51:2,",
-"MAIL_DIR/cur/20:2,",
-"MAIL_DIR/cur/17:2,",
-"MAIL_DIR/cur/16:2,",
-"MAIL_DIR/cur/15:2,",
-"MAIL_DIR/cur/14:2,",
-"MAIL_DIR/cur/13:2,",
-"MAIL_DIR/cur/12:2,",
-"MAIL_DIR/cur/11:2,",
-"MAIL_DIR/cur/10:2,",
-"MAIL_DIR/cur/09:2,",
-"MAIL_DIR/cur/08:2,",
-"MAIL_DIR/cur/06:2,",
-"MAIL_DIR/cur/05:2,",
-"MAIL_DIR/cur/04:2,",
-"MAIL_DIR/cur/03:2,",
-"MAIL_DIR/cur/07:2,",
-"MAIL_DIR/cur/02:2,",
-"MAIL_DIR/cur/01:2,"]
+["MAIL_DIR/cur/52:2,"
+, "MAIL_DIR/cur/53:2,"
+, "MAIL_DIR/cur/50:2,"
+, "MAIL_DIR/cur/49:2,"
+, "MAIL_DIR/cur/48:2,"
+, "MAIL_DIR/cur/47:2,"
+, "MAIL_DIR/cur/46:2,"
+, "MAIL_DIR/cur/45:2,"
+, "MAIL_DIR/cur/44:2,"
+, "MAIL_DIR/cur/43:2,"
+, "MAIL_DIR/cur/42:2,"
+, "MAIL_DIR/cur/41:2,"
+, "MAIL_DIR/cur/40:2,"
+, "MAIL_DIR/cur/39:2,"
+, "MAIL_DIR/cur/38:2,"
+, "MAIL_DIR/cur/37:2,"
+, "MAIL_DIR/cur/36:2,"
+, "MAIL_DIR/cur/35:2,"
+, "MAIL_DIR/cur/34:2,"
+, "MAIL_DIR/cur/33:2,"
+, "MAIL_DIR/cur/32:2,"
+, "MAIL_DIR/cur/31:2,"
+, "MAIL_DIR/cur/30:2,"
+, "MAIL_DIR/cur/29:2,"
+, "MAIL_DIR/cur/28:2,"
+, "MAIL_DIR/cur/27:2,"
+, "MAIL_DIR/cur/26:2,"
+, "MAIL_DIR/cur/25:2,"
+, "MAIL_DIR/cur/24:2,"
+, "MAIL_DIR/cur/23:2,"
+, "MAIL_DIR/cur/22:2,"
+, "MAIL_DIR/cur/21:2,"
+, "MAIL_DIR/cur/19:2,"
+, "MAIL_DIR/cur/18:2,"
+, "MAIL_DIR/cur/51:2,"
+, "MAIL_DIR/cur/20:2,"
+, "MAIL_DIR/cur/17:2,"
+, "MAIL_DIR/cur/16:2,"
+, "MAIL_DIR/cur/15:2,"
+, "MAIL_DIR/cur/14:2,"
+, "MAIL_DIR/cur/13:2,"
+, "MAIL_DIR/cur/12:2,"
+, "MAIL_DIR/cur/11:2,"
+, "MAIL_DIR/cur/10:2,"
+, "MAIL_DIR/cur/09:2,"
+, "MAIL_DIR/cur/08:2,"
+, "MAIL_DIR/cur/06:2,"
+, "MAIL_DIR/cur/05:2,"
+, "MAIL_DIR/cur/04:2,"
+, "MAIL_DIR/cur/03:2,"
+, "MAIL_DIR/cur/07:2,"
+, "MAIL_DIR/cur/02:2,"
+, "MAIL_DIR/cur/01:2,"
+]
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
@@ -311,10 +314,11 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "--output=tags --format=json"
 notmuch search --format=json --output=tags '*' >OUTPUT
 cat <<EOF >EXPECTED
-["attachment",
-"inbox",
-"signed",
-"unread"]
+["attachment"
+, "inbox"
+, "signed"
+, "unread"
+]
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.11.1

Re: [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search.

[Austin Clements]

This is fantastic.  It simplifies the code a lot, and I think it opens
up opportunities to simplify it even further.

Detailed comments are below, but first one general comment.  For the
text format, I wonder if most of the special case code would go away
with a stub sprinter that did nothing for most operations and for
string printed the string followed by a newline (or maybe the newline
would be printed by the "frame" method or whatever you end up calling
it).  I believe this would unify all of the code in do_search_tags and
do_search_files between the two formats.  For do_search_threads,
you'll still need some special-casing to format the summary line, but
I think it would unify all of the framing code.  (If this does work
out, some of my comments below will be irrelevant.)

Quoth craven@gmx.net on Jul 12 at  9:43 am:
> This patch switches from the current ad-hoc printer to the structured
> output formatter in sprinter.h.
> 
> It removes search_format_t, replaces it by sprinter_t and inlines the
> text printer where necessary.
> 
> The tests are changed (only whitespaces and regular expressions) in
> order to make them PASS for the new structured output formatter.
> ---
>  notmuch-search.c   | 349 ++++++++++++++++++++++-------------------------------
>  test/json          |  20 +--
>  test/search-output | 270 +++++++++++++++++++++--------------------
>  3 files changed, 288 insertions(+), 351 deletions(-)
> 
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 3be296d..b853f5f 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,91 +29,9 @@ 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 const char * text_item_sep = "\n";
> +static const char * text_results_null = "";
> +static const char * text_results_end = "\n";

Given that you're special-casing the text format anyway, I think it's
actually more readable if you hard-code these in the appropriate
places below.

>  
>  static char *
>  sanitize_string (const void *ctx, const char *str)
> @@ -131,72 +50,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,
> @@ -220,7 +75,9 @@ do_search_threads (const search_format_t *format,
>      if (threads == NULL)
>  	return 1;
>  
> -    fputs (format->results_start, stdout);
> +    if (format != sprinter_text) {
> +	format->begin_list (format);
> +    }
>  
>      for (i = 0;
>  	 notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);
> @@ -235,43 +92,96 @@ do_search_threads (const search_format_t *format,
>  	    continue;
>  	}
>  
> -	if (! first_thread)
> -	    fputs (format->item_sep, stdout);
> +	if (format == sprinter_text && ! first_thread)
> +	    fputs (text_item_sep, stdout);

I think you can drop this (and the first_thread variable) if you print
")\n" instead of ")" down below.  This roundabout first_thread stuff
was necessary to support the JSON format because it uses separators
between results; however, the text format is much simpler because it
just has the terminating newline after each result.  Hence, you can
just bake printing the newline in with printing the result and you
never have to worry about whether it's the first result of if you
didn't print any results.

>  
>  	if (output == OUTPUT_THREADS) {
> -	    format->item_id (thread, "thread:",
> -			     notmuch_thread_get_thread_id (thread));
> +	    const char *thread_id = notmuch_thread_get_thread_id (thread);
> +	    if (format == sprinter_text)
> +		printf ("thread:%s", thread_id);
> +	    else {
> +		format->string (format, thread_id);
> +		format->frame (format);
> +	    }
> +
>  	} else { /* output == OUTPUT_SUMMARY */
> -	    fputs (format->item_start, stdout);
> +	    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;
> +
> +	    if (format != sprinter_text)
> +		format->begin_map (format);
> +

Extra blank line.

>  
>  	    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));
> +	    void *ctx_quote = talloc_new (thread);
> +	    relative_date =
> +		notmuch_time_relative_date (ctx_quote, date);
> +
> +	    if (format == sprinter_text) {
> +		printf ("thread:%s %12s [%d/%d] %s; %s",
> +			thread_id,
> +		        relative_date,
> +			matched,
> +			total,
> +			sanitize_string (ctx_quote, authors),
> +			sanitize_string (ctx_quote, subject));
> +	    } else {
> +		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);
> +	    if (format == sprinter_text) {
> +		fputs (" (", stdout);
> +	    } else {
> +		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));
> +		const char *tag = notmuch_tags_get (tags);
> +		if (format == sprinter_text) {
> +		    if (! first_tag)
> +			fputs (" ", stdout);
> +		    fputs (tag, stdout);
> +		} else {
> +		    format->string (format, tag);
> +		}
> +
>  		first_tag = 0;
>  	    }
>  
> -	    fputs (format->tag_end, stdout);
> -
> -	    fputs (format->item_end, stdout);
> +	    if (format == sprinter_text) {
> +		fputs (")", stdout);
> +	    } else {
> +		format->end (format);
> +		format->end (format);
> +		format->frame (format);
> +	    }
>  	}
>  
>  	first_thread = 0;
> @@ -279,16 +189,20 @@ do_search_threads (const search_format_t *format,
>  	notmuch_thread_destroy (thread);
>      }
>  
> -    if (first_thread)
> -	fputs (format->results_null, stdout);
> -    else
> -	fputs (format->results_end, stdout);
> +    if (format == sprinter_text)
> +	if (first_thread)
> +	    fputs (text_results_null, stdout);
> +	else
> +	    fputs (text_results_end, stdout);

You can drop this, too, if you print ")\n" instead of ")" above.

> +    else {
> +	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,
> @@ -310,7 +224,8 @@ do_search_messages (const search_format_t *format,
>      if (messages == NULL)
>  	return 1;
>  
> -    fputs (format->results_start, stdout);
> +    if (format != sprinter_text)
> +	format->begin_list (format);
>  
>      for (i = 0;
>  	 notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);
> @@ -328,23 +243,36 @@ 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);
> +		const char *filenames_str = notmuch_filenames_get (filenames);
> +
> +		if (format == sprinter_text && ! first_message)
> +		    fputs (text_item_sep, stdout);

Likewise, you can remove this, the first_message logic for
OUTPUT_MESSAGES, and the first_message logic at the end of this
function if you print a newline after filenames_str and message_id.

>  
> -		format->item_id (message, "",
> -				 notmuch_filenames_get (filenames));
> +		if (format == sprinter_text)
> +		    fputs (filenames_str, stdout);
> +		else {
> +		    format->string (format, filenames_str);
> +		    format->frame (format);
> +		}
>  
>  		first_message = 0;
>  	    }
> -	    
> -	    notmuch_filenames_destroy( filenames );
> +
> +	    notmuch_filenames_destroy (filenames);

No need to reformat this.

>  
>  	} else { /* output == OUTPUT_MESSAGES */
> -	    if (! first_message)
> -		fputs (format->item_sep, stdout);
> +	    const char *message_id = notmuch_message_get_message_id (message);
> +
> +	    if (format == sprinter_text && ! first_message)
> +		fputs (text_item_sep, stdout);
> +
> +	    if (format == sprinter_text)
> +		printf ("id:%s", message_id);
> +	    else {
> +		format->string (format, message_id);
> +		format->frame (format);
> +	    }
>  
> -	    format->item_id (message, "id:",
> -			     notmuch_message_get_message_id (message));
>  	    first_message = 0;
>  	}
>  
> @@ -353,17 +281,21 @@ 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);
> +    if (format == sprinter_text)
> +	if (first_message)
> +	    fputs (text_results_null, stdout);
> +	else
> +	    fputs (text_results_end, stdout);
> +    else {
> +	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;
> @@ -387,7 +319,8 @@ do_search_tags (notmuch_database_t *notmuch,
>      if (tags == NULL)
>  	return 1;
>  
> -    fputs (format->results_start, stdout);
> +    if (format != sprinter_text)
> +	format->begin_list (format);
>  
>      for (;
>  	 notmuch_tags_valid (tags);
> @@ -395,10 +328,15 @@ do_search_tags (notmuch_database_t *notmuch,
>      {
>  	tag = notmuch_tags_get (tags);
>  
> -	if (! first_tag)
> -	    fputs (format->item_sep, stdout);
> +	if (format == sprinter_text && ! first_tag)
> +	    fputs (text_item_sep, stdout);

Same thing about dropping the first_tag logic in this function if you
print a newline after tag below.

>  
> -	format->item_id (tags, "", tag);
> +	if (format == sprinter_text)
> +	    fputs (tag, stdout);
> +	else {
> +	    format->string (format, tag);
> +	    format->frame (format);
> +	}
>  
>  	first_tag = 0;
>      }
> @@ -408,10 +346,14 @@ 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);
> +    if (format == sprinter_text)
> +	if (first_tag)
> +	    fputs (text_results_null, stdout);
> +	else
> +	    fputs (text_results_end, stdout);
> +    else {
> +	format->end (format);
> +    }
>  
>      return 0;
>  }
> @@ -430,7 +372,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; /* the default output is text */
>      int opt_index, ret;
>      output_t output = OUTPUT_SUMMARY;
>      int offset = 0;
> @@ -475,10 +417,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  
>      switch (format_sel) {
>      case NOTMUCH_FORMAT_TEXT:
> -	format = &format_text;
> +	format = NULL;
>  	break;
>      case NOTMUCH_FORMAT_JSON:
> -	format = &format_json;
> +	format = sprinter_json_new (ctx, stdout);
>  	break;
>      }
>  
> @@ -546,5 +488,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      notmuch_query_destroy (query);
>      notmuch_database_destroy (notmuch);
>  
> +    if (format != sprinter_text)
> +	talloc_free(format);

Missing space before argument list.

> +
>      return ret;
>  }
> diff --git a/test/json b/test/json
> index 6439788..88b8a6d 100755
> --- a/test/json
> +++ b/test/json
> @@ -10,14 +10,8 @@ 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)

If you additionally pipe this through notmuch_json_show_sanitize (or
maybe define a similar function for search), then the output will be
much closer to the output currently in the test.

> -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 +34,7 @@ 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)

Same here.

> -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
> diff --git a/test/search-output b/test/search-output
> index 8b57a43..f2650f7 100755
> --- a/test/search-output
> +++ b/test/search-output
> @@ -37,30 +37,31 @@ test_expect_equal_file OUTPUT EXPECTED
>  test_begin_subtest "--output=threads --format=json"
>  notmuch search --format=json --output=threads '*' | sed -e s/\".*\"/\"THREADID\"/ >OUTPUT
>  cat <<EOF >EXPECTED
> -["THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID",
> -"THREADID"]
> +["THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +, "THREADID"
> +]

Hmm.  It would be nice if json_frame printed this in the way people
usually format JSON and the test currently expects.  Doing this isn't
entirely straightforward because the caller could call frame and then
immediately call end, so you can't just print ",\n"...

You could replace json_state.first with
  /* The separator to print before the next value. */
  const char *sep;
json_begin_value would fputs sep and set it to ", ",
json_begin_aggregate would set it to "", and json_map_key would set it
to ": " and not fputs(": ", spj->stream) itself.  json_frame could
then be something like

static void
json_frame(struct sprinter *sp)
{
    struct json_state *state = ((struct sprinter_json*)sp)->state;
    if (!state)
      return;
    if (state->sep[0] == ',')
      state->sep = ",\n";
    else if (state->sep[0] == ':')
      state->sep = ":\n";
}

I think that would work.

>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> @@ -125,58 +126,59 @@ test_expect_equal_file OUTPUT EXPECTED
>  test_begin_subtest "--output=messages --format=json"
>  notmuch search --format=json --output=messages '*' >OUTPUT
>  cat <<EOF >EXPECTED
> -["4EFC743A.3060609@april.org",
> -"877h1wv7mg.fsf@inf-8657.int-evry.fr",
> -"1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk",
> -"877htoqdbo.fsf@yoom.home.cworth.org",
> -"878we4qdqf.fsf@yoom.home.cworth.org",
> -"87aaykqe24.fsf@yoom.home.cworth.org",
> -"87bpj0qeng.fsf@yoom.home.cworth.org",
> -"87fx8cqf8v.fsf@yoom.home.cworth.org",
> -"87hbssqfix.fsf@yoom.home.cworth.org",
> -"87iqd8qgiz.fsf@yoom.home.cworth.org",
> -"87k4xoqgnl.fsf@yoom.home.cworth.org",
> -"87ocn0qh6d.fsf@yoom.home.cworth.org",
> -"87pr7gqidx.fsf@yoom.home.cworth.org",
> -"867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
> -"1258532999-9316-1-git-send-email-keithp@keithp.com",
> -"86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
> -"86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
> -"ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com",
> -"86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me",
> -"736613.51770.qm@web113505.mail.gq1.yahoo.com",
> -"1258520223-15328-1-git-send-email-jan@ryngle.com",
> -"ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com",
> -"1258510940-7018-1-git-send-email-stewart@flamingspork.com",
> -"yunzl6kd1w0.fsf@aiko.keithp.com",
> -"yun1vjwegii.fsf@aiko.keithp.com",
> -"yun3a4cegoa.fsf@aiko.keithp.com",
> -"1258509400-32511-1-git-send-email-stewart@flamingspork.com",
> -"1258506353-20352-1-git-send-email-stewart@flamingspork.com",
> -"20091118010116.GC25380@dottiness.seas.harvard.edu",
> -"20091118005829.GB25380@dottiness.seas.harvard.edu",
> -"20091118005040.GA25380@dottiness.seas.harvard.edu",
> -"cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com",
> -"1258500222-32066-1-git-send-email-ingmar@exherbo.org",
> -"20091117232137.GA7669@griffis1.net",
> -"20091118002059.067214ed@hikari",
> -"1258498485-sup-142@elly",
> -"f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com",
> -"f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com",
> -"1258496327-12086-1-git-send-email-jan@ryngle.com",
> -"1258493565-13508-1-git-send-email-keithp@keithp.com",
> -"yunaayketfm.fsf@aiko.keithp.com",
> -"yunbpj0etua.fsf@aiko.keithp.com",
> -"1258491078-29658-1-git-send-email-dottedmag@dottedmag.net",
> -"87fx8can9z.fsf@vertex.dottedmag",
> -"20091117203301.GV3165@dottiness.seas.harvard.edu",
> -"87lji4lx9v.fsf@yoom.home.cworth.org",
> -"cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com",
> -"87iqd9rn3l.fsf@vertex.dottedmag",
> -"20091117190054.GU3165@dottiness.seas.harvard.edu",
> -"87lji5cbwo.fsf@yoom.home.cworth.org",
> -"1258471718-6781-2-git-send-email-dottedmag@dottedmag.net",
> -"1258471718-6781-1-git-send-email-dottedmag@dottedmag.net"]
> +["4EFC743A.3060609@april.org"
> +, "877h1wv7mg.fsf@inf-8657.int-evry.fr"
> +, "1258544095-16616-1-git-send-email-chris@chris-wilson.co.uk"
> +, "877htoqdbo.fsf@yoom.home.cworth.org"
> +, "878we4qdqf.fsf@yoom.home.cworth.org"
> +, "87aaykqe24.fsf@yoom.home.cworth.org"
> +, "87bpj0qeng.fsf@yoom.home.cworth.org"
> +, "87fx8cqf8v.fsf@yoom.home.cworth.org"
> +, "87hbssqfix.fsf@yoom.home.cworth.org"
> +, "87iqd8qgiz.fsf@yoom.home.cworth.org"
> +, "87k4xoqgnl.fsf@yoom.home.cworth.org"
> +, "87ocn0qh6d.fsf@yoom.home.cworth.org"
> +, "87pr7gqidx.fsf@yoom.home.cworth.org"
> +, "867hto2p0t.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
> +, "1258532999-9316-1-git-send-email-keithp@keithp.com"
> +, "86aayk2rbj.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
> +, "86d43g2w3y.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
> +, "ddd65cda0911172214t60d22b63hcfeb5a19ab54a39b@mail.gmail.com"
> +, "86einw2xof.fsf@fortitudo.i-did-not-set--mail-host-address--so-tickle-me"
> +, "736613.51770.qm@web113505.mail.gq1.yahoo.com"
> +, "1258520223-15328-1-git-send-email-jan@ryngle.com"
> +, "ddd65cda0911171950o4eea4389v86de9525e46052d3@mail.gmail.com"
> +, "1258510940-7018-1-git-send-email-stewart@flamingspork.com"
> +, "yunzl6kd1w0.fsf@aiko.keithp.com"
> +, "yun1vjwegii.fsf@aiko.keithp.com"
> +, "yun3a4cegoa.fsf@aiko.keithp.com"
> +, "1258509400-32511-1-git-send-email-stewart@flamingspork.com"
> +, "1258506353-20352-1-git-send-email-stewart@flamingspork.com"
> +, "20091118010116.GC25380@dottiness.seas.harvard.edu"
> +, "20091118005829.GB25380@dottiness.seas.harvard.edu"
> +, "20091118005040.GA25380@dottiness.seas.harvard.edu"
> +, "cf0c4d610911171623q3e27a0adx802e47039b57604b@mail.gmail.com"
> +, "1258500222-32066-1-git-send-email-ingmar@exherbo.org"
> +, "20091117232137.GA7669@griffis1.net"
> +, "20091118002059.067214ed@hikari"
> +, "1258498485-sup-142@elly"
> +, "f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com"
> +, "f35dbb950911171435ieecd458o853c873e35f4be95@mail.gmail.com"
> +, "1258496327-12086-1-git-send-email-jan@ryngle.com"
> +, "1258493565-13508-1-git-send-email-keithp@keithp.com"
> +, "yunaayketfm.fsf@aiko.keithp.com"
> +, "yunbpj0etua.fsf@aiko.keithp.com"
> +, "1258491078-29658-1-git-send-email-dottedmag@dottedmag.net"
> +, "87fx8can9z.fsf@vertex.dottedmag"
> +, "20091117203301.GV3165@dottiness.seas.harvard.edu"
> +, "87lji4lx9v.fsf@yoom.home.cworth.org"
> +, "cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com"
> +, "87iqd9rn3l.fsf@vertex.dottedmag"
> +, "20091117190054.GU3165@dottiness.seas.harvard.edu"
> +, "87lji5cbwo.fsf@yoom.home.cworth.org"
> +, "1258471718-6781-2-git-send-email-dottedmag@dottedmag.net"
> +, "1258471718-6781-1-git-send-email-dottedmag@dottedmag.net"
> +]
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> @@ -242,59 +244,60 @@ test_expect_equal_file OUTPUT EXPECTED
>  test_begin_subtest "--output=files --format=json"
>  notmuch search --format=json --output=files '*' | sed -e "s,$MAIL_DIR,MAIL_DIR," >OUTPUT
>  cat <<EOF >EXPECTED
> -["MAIL_DIR/cur/52:2,",
> -"MAIL_DIR/cur/53:2,",
> -"MAIL_DIR/cur/50:2,",
> -"MAIL_DIR/cur/49:2,",
> -"MAIL_DIR/cur/48:2,",
> -"MAIL_DIR/cur/47:2,",
> -"MAIL_DIR/cur/46:2,",
> -"MAIL_DIR/cur/45:2,",
> -"MAIL_DIR/cur/44:2,",
> -"MAIL_DIR/cur/43:2,",
> -"MAIL_DIR/cur/42:2,",
> -"MAIL_DIR/cur/41:2,",
> -"MAIL_DIR/cur/40:2,",
> -"MAIL_DIR/cur/39:2,",
> -"MAIL_DIR/cur/38:2,",
> -"MAIL_DIR/cur/37:2,",
> -"MAIL_DIR/cur/36:2,",
> -"MAIL_DIR/cur/35:2,",
> -"MAIL_DIR/cur/34:2,",
> -"MAIL_DIR/cur/33:2,",
> -"MAIL_DIR/cur/32:2,",
> -"MAIL_DIR/cur/31:2,",
> -"MAIL_DIR/cur/30:2,",
> -"MAIL_DIR/cur/29:2,",
> -"MAIL_DIR/cur/28:2,",
> -"MAIL_DIR/cur/27:2,",
> -"MAIL_DIR/cur/26:2,",
> -"MAIL_DIR/cur/25:2,",
> -"MAIL_DIR/cur/24:2,",
> -"MAIL_DIR/cur/23:2,",
> -"MAIL_DIR/cur/22:2,",
> -"MAIL_DIR/cur/21:2,",
> -"MAIL_DIR/cur/19:2,",
> -"MAIL_DIR/cur/18:2,",
> -"MAIL_DIR/cur/51:2,",
> -"MAIL_DIR/cur/20:2,",
> -"MAIL_DIR/cur/17:2,",
> -"MAIL_DIR/cur/16:2,",
> -"MAIL_DIR/cur/15:2,",
> -"MAIL_DIR/cur/14:2,",
> -"MAIL_DIR/cur/13:2,",
> -"MAIL_DIR/cur/12:2,",
> -"MAIL_DIR/cur/11:2,",
> -"MAIL_DIR/cur/10:2,",
> -"MAIL_DIR/cur/09:2,",
> -"MAIL_DIR/cur/08:2,",
> -"MAIL_DIR/cur/06:2,",
> -"MAIL_DIR/cur/05:2,",
> -"MAIL_DIR/cur/04:2,",
> -"MAIL_DIR/cur/03:2,",
> -"MAIL_DIR/cur/07:2,",
> -"MAIL_DIR/cur/02:2,",
> -"MAIL_DIR/cur/01:2,"]
> +["MAIL_DIR/cur/52:2,"
> +, "MAIL_DIR/cur/53:2,"
> +, "MAIL_DIR/cur/50:2,"
> +, "MAIL_DIR/cur/49:2,"
> +, "MAIL_DIR/cur/48:2,"
> +, "MAIL_DIR/cur/47:2,"
> +, "MAIL_DIR/cur/46:2,"
> +, "MAIL_DIR/cur/45:2,"
> +, "MAIL_DIR/cur/44:2,"
> +, "MAIL_DIR/cur/43:2,"
> +, "MAIL_DIR/cur/42:2,"
> +, "MAIL_DIR/cur/41:2,"
> +, "MAIL_DIR/cur/40:2,"
> +, "MAIL_DIR/cur/39:2,"
> +, "MAIL_DIR/cur/38:2,"
> +, "MAIL_DIR/cur/37:2,"
> +, "MAIL_DIR/cur/36:2,"
> +, "MAIL_DIR/cur/35:2,"
> +, "MAIL_DIR/cur/34:2,"
> +, "MAIL_DIR/cur/33:2,"
> +, "MAIL_DIR/cur/32:2,"
> +, "MAIL_DIR/cur/31:2,"
> +, "MAIL_DIR/cur/30:2,"
> +, "MAIL_DIR/cur/29:2,"
> +, "MAIL_DIR/cur/28:2,"
> +, "MAIL_DIR/cur/27:2,"
> +, "MAIL_DIR/cur/26:2,"
> +, "MAIL_DIR/cur/25:2,"
> +, "MAIL_DIR/cur/24:2,"
> +, "MAIL_DIR/cur/23:2,"
> +, "MAIL_DIR/cur/22:2,"
> +, "MAIL_DIR/cur/21:2,"
> +, "MAIL_DIR/cur/19:2,"
> +, "MAIL_DIR/cur/18:2,"
> +, "MAIL_DIR/cur/51:2,"
> +, "MAIL_DIR/cur/20:2,"
> +, "MAIL_DIR/cur/17:2,"
> +, "MAIL_DIR/cur/16:2,"
> +, "MAIL_DIR/cur/15:2,"
> +, "MAIL_DIR/cur/14:2,"
> +, "MAIL_DIR/cur/13:2,"
> +, "MAIL_DIR/cur/12:2,"
> +, "MAIL_DIR/cur/11:2,"
> +, "MAIL_DIR/cur/10:2,"
> +, "MAIL_DIR/cur/09:2,"
> +, "MAIL_DIR/cur/08:2,"
> +, "MAIL_DIR/cur/06:2,"
> +, "MAIL_DIR/cur/05:2,"
> +, "MAIL_DIR/cur/04:2,"
> +, "MAIL_DIR/cur/03:2,"
> +, "MAIL_DIR/cur/07:2,"
> +, "MAIL_DIR/cur/02:2,"
> +, "MAIL_DIR/cur/01:2,"
> +]
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> @@ -311,10 +314,11 @@ test_expect_equal_file OUTPUT EXPECTED
>  test_begin_subtest "--output=tags --format=json"
>  notmuch search --format=json --output=tags '*' >OUTPUT
>  cat <<EOF >EXPECTED
> -["attachment",
> -"inbox",
> -"signed",
> -"unread"]
> +["attachment"
> +, "inbox"
> +, "signed"
> +, "unread"
> +]
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  

Re: [PATCH v4 3/3] Use the structured format printer for JSON in notmuch search.

[Austin Clements]

Quoth myself on Jul 12 at  8:02 pm:
> This is fantastic.  It simplifies the code a lot, and I think it opens
> up opportunities to simplify it even further.
> 
> Detailed comments are below, but first one general comment.  For the
> text format, I wonder if most of the special case code would go away
> with a stub sprinter that did nothing for most operations and for
> string printed the string followed by a newline (or maybe the newline
> would be printed by the "frame" method or whatever you end up calling
> it).  I believe this would unify all of the code in do_search_tags and
> do_search_files between the two formats.  For do_search_threads,
> you'll still need some special-casing to format the summary line, but
> I think it would unify all of the framing code.  (If this does work
> out, some of my comments below will be irrelevant.)

Oh, wait, the text format adds prefixes...

I think something along the lines of what I described above would
still simplify things.  I can think of two ways to do it.  You could
have a completely stub sprinter where everything is a no-op; that
would at least save the predication of calls like
format->begin_list ().  Or, you could have an additional function for
the text formatter that registers a prefix, and have the string method
first print that prefix and then the argument string.  This additional
function doesn't have to be a method of sprinter; it could just be a
regular function that stashes the prefix away somewhere (a global
variable's probably fine; if you want to be fancy you store it in an
sprinter_text extension of sprinter_t like the one JSON uses).  For
the JSON format, this would be a no-op, so you could call it
regardless of what format you've selected.