Re: [PATCH v6 3/3] Use the structured formatters in notmuch-search.c.

Subject: Re: [PATCH v6 3/3] Use the structured formatters in notmuch-search.c.

Date: Wed, 18 Jul 2012 15:56:02 -0400

To: craven@gmx.net

Cc: notmuch@notmuchmail.org

From: Austin Clements


Just a few comments (don't forget to scroll all the way down).
Overall this is looking pretty good.

Quoth craven@gmx.net on Jul 16 at 10:35 am:
> This patch switches from the current ad-hoc printer to the structured
> formatters in sprinter.h, sprinter-text-search.c and sprinter-json.c.
> 
> The JSON tests are changed slightly in order to make them PASS for the
> new structured output formatter.
> 
> The text tests pass without adaptation.
> ---
>  notmuch-search.c | 300 ++++++++++++++++---------------------------------------
>  test/json        |  18 +---
>  2 files changed, 86 insertions(+), 232 deletions(-)

That's a fantastic diffstat.

> 
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 3be296d..cf927e6 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "notmuch-client.h"
> +#include "sprinter.h"
>  
>  typedef enum {
>      OUTPUT_SUMMARY,
> @@ -28,92 +29,6 @@ typedef enum {
>      OUTPUT_TAGS
>  } output_t;
>  
> -typedef struct search_format {
> -    const char *results_start;
> -    const char *item_start;
> -    void (*item_id) (const void *ctx,
> -		     const char *item_type,
> -		     const char *item_id);
> -    void (*thread_summary) (const void *ctx,
> -			    const char *thread_id,
> -			    const time_t date,
> -			    const int matched,
> -			    const int total,
> -			    const char *authors,
> -			    const char *subject);
> -    const char *tag_start;
> -    const char *tag;
> -    const char *tag_sep;
> -    const char *tag_end;
> -    const char *item_sep;
> -    const char *item_end;
> -    const char *results_end;
> -    const char *results_null;
> -} search_format_t;
> -
> -static void
> -format_item_id_text (const void *ctx,
> -		     const char *item_type,
> -		     const char *item_id);
> -
> -static void
> -format_thread_text (const void *ctx,
> -		    const char *thread_id,
> -		    const time_t date,
> -		    const int matched,
> -		    const int total,
> -		    const char *authors,
> -		    const char *subject);
> -static const search_format_t format_text = {
> -    "",
> -	"",
> -	    format_item_id_text,
> -	    format_thread_text,
> -	    " (",
> -		"%s", " ",
> -	    ")", "\n",
> -	"",
> -    "\n",
> -    "",
> -};
> -
> -static void
> -format_item_id_json (const void *ctx,
> -		     const char *item_type,
> -		     const char *item_id);
> -
> -static void
> -format_thread_json (const void *ctx,
> -		    const char *thread_id,
> -		    const time_t date,
> -		    const int matched,
> -		    const int total,
> -		    const char *authors,
> -		    const char *subject);
> -
> -/* Any changes to the JSON format should be reflected in the file
> - * devel/schemata. */
> -static const search_format_t format_json = {
> -    "[",
> -	"{",
> -	    format_item_id_json,
> -	    format_thread_json,
> -	    "\"tags\": [",
> -		"\"%s\"", ", ",
> -	    "]", ",\n",
> -	"}",
> -    "]\n",
> -    "]\n",
> -};
> -
> -static void
> -format_item_id_text (unused (const void *ctx),
> -		     const char *item_type,
> -		     const char *item_id)
> -{
> -    printf ("%s%s", item_type, item_id);
> -}
> -
>  static char *
>  sanitize_string (const void *ctx, const char *str)
>  {
> @@ -131,72 +46,8 @@ sanitize_string (const void *ctx, const char *str)
>      return out;
>  }
>  
> -static void
> -format_thread_text (const void *ctx,
> -		    const char *thread_id,
> -		    const time_t date,
> -		    const int matched,
> -		    const int total,
> -		    const char *authors,
> -		    const char *subject)
> -{
> -    void *ctx_quote = talloc_new (ctx);
> -
> -    printf ("thread:%s %12s [%d/%d] %s; %s",
> -	    thread_id,
> -	    notmuch_time_relative_date (ctx, date),
> -	    matched,
> -	    total,
> -	    sanitize_string (ctx_quote, authors),
> -	    sanitize_string (ctx_quote, subject));
> -
> -    talloc_free (ctx_quote);
> -}
> -
> -static void
> -format_item_id_json (const void *ctx,
> -		     unused (const char *item_type),
> -		     const char *item_id)
> -{
> -    void *ctx_quote = talloc_new (ctx);
> -
> -    printf ("%s", json_quote_str (ctx_quote, item_id));
> -
> -    talloc_free (ctx_quote);
> -    
> -}
> -
> -static void
> -format_thread_json (const void *ctx,
> -		    const char *thread_id,
> -		    const time_t date,
> -		    const int matched,
> -		    const int total,
> -		    const char *authors,
> -		    const char *subject)
> -{
> -    void *ctx_quote = talloc_new (ctx);
> -
> -    printf ("\"thread\": %s,\n"
> -	    "\"timestamp\": %ld,\n"
> -	    "\"date_relative\": \"%s\",\n"
> -	    "\"matched\": %d,\n"
> -	    "\"total\": %d,\n"
> -	    "\"authors\": %s,\n"
> -	    "\"subject\": %s,\n",
> -	    json_quote_str (ctx_quote, thread_id),
> -	    date,
> -	    notmuch_time_relative_date (ctx, date),
> -	    matched,
> -	    total,
> -	    json_quote_str (ctx_quote, authors),
> -	    json_quote_str (ctx_quote, subject));
> -
> -    talloc_free (ctx_quote);
> -}
> -
>  static int
> -do_search_threads (const search_format_t *format,
> +do_search_threads (sprinter_t *format,
>  		   notmuch_query_t *query,
>  		   notmuch_sort_t sort,
>  		   output_t output,
> @@ -207,7 +58,6 @@ do_search_threads (const search_format_t *format,
>      notmuch_threads_t *threads;
>      notmuch_tags_t *tags;
>      time_t date;
> -    int first_thread = 1;
>      int i;
>  
>      if (offset < 0) {
> @@ -220,14 +70,12 @@ do_search_threads (const search_format_t *format,
>      if (threads == NULL)
>  	return 1;
>  
> -    fputs (format->results_start, stdout);
> +    format->begin_list (format);
>  
>      for (i = 0;
>  	 notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit);
>  	 notmuch_threads_move_to_next (threads), i++)
>      {
> -	int first_tag = 1;
> -
>  	thread = notmuch_threads_get (threads);
>  
>  	if (i < offset) {
> @@ -235,60 +83,96 @@ do_search_threads (const search_format_t *format,
>  	    continue;
>  	}
>  
> -	if (! first_thread)
> -	    fputs (format->item_sep, stdout);
> -
>  	if (output == OUTPUT_THREADS) {
> -	    format->item_id (thread, "thread:",
> -			     notmuch_thread_get_thread_id (thread));
> +	    format->set_prefix (format, "thread");
> +	    format->string (format,
> +			    notmuch_thread_get_thread_id (thread));
> +	    format->separator (format);
>  	} else { /* output == OUTPUT_SUMMARY */
> -	    fputs (format->item_start, stdout);
> +	    void *ctx_quote = talloc_new (thread);
> +	    const char *authors = notmuch_thread_get_authors (thread);
> +	    const char *subject = notmuch_thread_get_subject (thread);
> +	    const char *thread_id = notmuch_thread_get_thread_id (thread);
> +	    int matched = notmuch_thread_get_matched_messages (thread);
> +	    int total = notmuch_thread_get_total_messages (thread);
> +	    const char *relative_date = NULL;
> +	    notmuch_bool_t first_tag = TRUE;
> +
> +	    format->begin_map (format);
>  
>  	    if (sort == NOTMUCH_SORT_OLDEST_FIRST)
>  		date = notmuch_thread_get_oldest_date (thread);
>  	    else
>  		date = notmuch_thread_get_newest_date (thread);
>  
> -	    format->thread_summary (thread,
> -				    notmuch_thread_get_thread_id (thread),
> -				    date,
> -				    notmuch_thread_get_matched_messages (thread),
> -				    notmuch_thread_get_total_messages (thread),
> -				    notmuch_thread_get_authors (thread),
> -				    notmuch_thread_get_subject (thread));
> +	    relative_date = notmuch_time_relative_date (ctx_quote, date);
> +
> +	    if (format->is_text_printer (format)) {
> +                /* Special case for the text formatter */
> +		printf ("thread:%s %12s [%d/%d] %s; %s (",
> +			thread_id,
> +			relative_date,
> +			matched,
> +			total,
> +			sanitize_string (ctx_quote, authors),
> +			sanitize_string (ctx_quote, subject));

Great.  This seems much simpler than trying to track the context in
the text printer.

> +	    } else { /* Structured Output */
> +		format->map_key (format, "thread");
> +		format->string (format, thread_id);
> +		format->map_key (format, "timestamp");
> +		format->integer (format, date);
> +		format->map_key (format, "date_relative");
> +		format->string (format, relative_date);
> +		format->map_key (format, "matched");
> +		format->integer (format, matched);
> +		format->map_key (format, "total");
> +		format->integer (format, total);
> +		format->map_key (format, "authors");
> +		format->string (format, authors);
> +		format->map_key (format, "subject");
> +		format->string (format, subject);
> +	    }
> +
> +	    talloc_free (ctx_quote);
>  
> -	    fputs (format->tag_start, stdout);
> +	    format->map_key (format, "tags");
> +	    format->begin_list (format);
>  
>  	    for (tags = notmuch_thread_get_tags (thread);
>  		 notmuch_tags_valid (tags);
>  		 notmuch_tags_move_to_next (tags))
>  	    {
> -		if (! first_tag)
> -		    fputs (format->tag_sep, stdout);
> -		printf (format->tag, notmuch_tags_get (tags));
> -		first_tag = 0;
> +		const char *tag = notmuch_tags_get (tags);
> +
> +		if (format->is_text_printer (format)) {
> +                  /* Special case for the text formatter */
> +		    if (first_tag)
> +			first_tag = FALSE;
> +		    else
> +			fputc (' ', stdout);
> +		    fputs (tag, stdout);
> +		} else /* Structured Output */

Please put braces around the else part as well (since you need braces
around the if part).

> +		    format->string (format, tag);
>  	    }
>  
> -	    fputs (format->tag_end, stdout);
> +	    if (format->is_text_printer (format))
> +		printf (")");
>  
> -	    fputs (format->item_end, stdout);
> +	    format->end (format);
> +	    format->end (format);
> +	    format->separator (format);
>  	}
>  
> -	first_thread = 0;
> -
>  	notmuch_thread_destroy (thread);
>      }
>  
> -    if (first_thread)
> -	fputs (format->results_null, stdout);
> -    else
> -	fputs (format->results_end, stdout);
> +    format->end (format);
>  
>      return 0;
>  }
>  
>  static int
> -do_search_messages (const search_format_t *format,
> +do_search_messages (sprinter_t *format,
>  		    notmuch_query_t *query,
>  		    output_t output,
>  		    int offset,
> @@ -297,7 +181,6 @@ do_search_messages (const search_format_t *format,
>      notmuch_message_t *message;
>      notmuch_messages_t *messages;
>      notmuch_filenames_t *filenames;
> -    int first_message = 1;
>      int i;
>  
>      if (offset < 0) {
> @@ -310,7 +193,7 @@ do_search_messages (const search_format_t *format,
>      if (messages == NULL)
>  	return 1;
>  
> -    fputs (format->results_start, stdout);
> +    format->begin_list (format);
>  
>      for (i = 0;
>  	 notmuch_messages_valid (messages) && (limit < 0 || i < offset + limit);
> @@ -328,24 +211,17 @@ do_search_messages (const search_format_t *format,
>  		 notmuch_filenames_valid (filenames);
>  		 notmuch_filenames_move_to_next (filenames))
>  	    {
> -		if (! first_message)
> -		    fputs (format->item_sep, stdout);
> -
> -		format->item_id (message, "",
> -				 notmuch_filenames_get (filenames));
> -
> -		first_message = 0;
> +		format->string (format, notmuch_filenames_get (filenames));
> +		format->separator (format);
>  	    }
>  	    
>  	    notmuch_filenames_destroy( filenames );
>  
>  	} else { /* output == OUTPUT_MESSAGES */
> -	    if (! first_message)
> -		fputs (format->item_sep, stdout);
> -
> -	    format->item_id (message, "id:",
> -			     notmuch_message_get_message_id (message));
> -	    first_message = 0;
> +	    format->set_prefix (format, "id");
> +	    format->string (format,
> +			    notmuch_message_get_message_id (message));
> +	    format->separator (format);
>  	}
>  
>  	notmuch_message_destroy (message);
> @@ -353,23 +229,19 @@ do_search_messages (const search_format_t *format,
>  
>      notmuch_messages_destroy (messages);
>  
> -    if (first_message)
> -	fputs (format->results_null, stdout);
> -    else
> -	fputs (format->results_end, stdout);
> +    format->end (format);
>  
>      return 0;
>  }
>  
>  static int
>  do_search_tags (notmuch_database_t *notmuch,
> -		const search_format_t *format,
> +		sprinter_t *format,
>  		notmuch_query_t *query)
>  {
>      notmuch_messages_t *messages = NULL;
>      notmuch_tags_t *tags;
>      const char *tag;
> -    int first_tag = 1;
>  
>      /* should the following only special case if no excluded terms
>       * specified? */
> @@ -387,7 +259,7 @@ do_search_tags (notmuch_database_t *notmuch,
>      if (tags == NULL)
>  	return 1;
>  
> -    fputs (format->results_start, stdout);
> +    format->begin_list (format);
>  
>      for (;
>  	 notmuch_tags_valid (tags);
> @@ -395,12 +267,9 @@ do_search_tags (notmuch_database_t *notmuch,
>      {
>  	tag = notmuch_tags_get (tags);
>  
> -	if (! first_tag)
> -	    fputs (format->item_sep, stdout);
> +	format->string (format, tag);
> +	format->separator (format);
>  
> -	format->item_id (tags, "", tag);
> -
> -	first_tag = 0;
>      }
>  
>      notmuch_tags_destroy (tags);
> @@ -408,10 +277,7 @@ do_search_tags (notmuch_database_t *notmuch,
>      if (messages)
>  	notmuch_messages_destroy (messages);
>  
> -    if (first_tag)
> -	fputs (format->results_null, stdout);
> -    else
> -	fputs (format->results_end, stdout);
> +    format->end (format);
>  
>      return 0;
>  }
> @@ -430,7 +296,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      notmuch_query_t *query;
>      char *query_str;
>      notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST;
> -    const search_format_t *format = &format_text;
> +    sprinter_t *format = NULL;
>      int opt_index, ret;
>      output_t output = OUTPUT_SUMMARY;
>      int offset = 0;
> @@ -475,10 +341,10 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  
>      switch (format_sel) {
>      case NOTMUCH_FORMAT_TEXT:
> -	format = &format_text;
> +	format = sprinter_text_search_create (ctx, stdout);
>  	break;
>      case NOTMUCH_FORMAT_JSON:
> -	format = &format_json;
> +	format = sprinter_json_create (ctx, stdout);
>  	break;
>      }
>  
> @@ -546,5 +412,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      notmuch_query_destroy (query);
>      notmuch_database_destroy (notmuch);
>  
> +    talloc_free (format);
> +
>      return ret;
>  }
> diff --git a/test/json b/test/json
> index 6439788..f0ebf08 100755
> --- a/test/json
> +++ b/test/json
> @@ -10,14 +10,7 @@ test_expect_equal "$output" "[[[{\"id\": \"${gen_msg_id}\", \"match\": true, \"e
>  test_begin_subtest "Search message: json"
>  add_message "[subject]=\"json-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"json-search-message\""
>  output=$(notmuch search --format=json "json-search-message" | notmuch_search_sanitize)

What's your reason for not piping this through
notmuch_json_show_sanitize?

> -test_expect_equal "$output" "[{\"thread\": \"XXX\",
> -\"timestamp\": 946728000,
> -\"date_relative\": \"2000-01-01\",
> -\"matched\": 1,
> -\"total\": 1,
> -\"authors\": \"Notmuch Test Suite\",
> -\"subject\": \"json-search-subject\",
> -\"tags\": [\"inbox\", \"unread\"]}]"
> +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-subject\", \"tags\": [\"inbox\", \"unread\"]}]"
>  
>  test_begin_subtest "Show message: json, utf-8"
>  add_message "[subject]=\"json-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\""
> @@ -40,13 +33,6 @@ test_expect_equal "$output" "[[[{\"id\": \"$id\", \"match\": true, \"excluded\":
>  test_begin_subtest "Search message: json, utf-8"
>  add_message "[subject]=\"json-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\""
>  output=$(notmuch search --format=json "jsön-search-méssage" | notmuch_search_sanitize)
> -test_expect_equal "$output" "[{\"thread\": \"XXX\",
> -\"timestamp\": 946728000,
> -\"date_relative\": \"2000-01-01\",
> -\"matched\": 1,
> -\"total\": 1,
> -\"authors\": \"Notmuch Test Suite\",
> -\"subject\": \"json-search-utf8-body-sübjéct\",
> -\"tags\": [\"inbox\", \"unread\"]}]"
> +test_expect_equal "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, \"date_relative\": \"2000-01-01\", \"matched\": 1, \"total\": 1, \"authors\": \"Notmuch Test Suite\", \"subject\": \"json-search-utf8-body-sübjéct\", \"tags\": [\"inbox\", \"unread\"]}]"
>  
>  test_done

Thread: