[PATCH v6 0/6] Allow JSON to use non-entire thread, and use for elide

[PATCH v6 0/6] Allow JSON to use non-entire thread, and use for elide

[Mark Walters]

This is version 6 of this patch series: version 5 is at
id:"1338047694-32548-1-git-send-email-markwalters1009@gmail.com".

The changes for this version:

As suggest by Peter in id:"20120527123253.GA3049@hili.localdomain" the
command line handling for keywords is changed so that if the option is
specified as --option (i.e., not --option=<something>) then an empty
string is passed to the parser. Thus the caller can deal with this
case rather than it being an automatic error.

This makes it easier to add options to an existing boolean option
without breaking the syntax.

Patch 3/6 has a one line change in response to the above change. The
previous version also had some whitespace errors that have been fixed.

Patch 6/6 is a trivial whitespace fix. It is right next to the main
changes so may be worth taking (since any patches touching this code
will need to be fixed up anyway) but is obviously independent of the
series.

Best wishes

Mark


Mark Walters (6):
  cli: command line parsing: allow default for keyword options
  cli: Let json output "null" messages for non --entire-thread
  cli: make --entire-thread=false work for format=json.
  Update devel/schemata for --entire-thread=false
  emacs: make elide messages use notmuch-show for omitting messages.
  cli: notmuch-show.c fix whitespace error

 command-line-arguments.c |   17 +++++++++++---
 devel/TODO               |    2 -
 devel/schemata           |    2 +-
 emacs/notmuch-show.el    |   18 ++++++++------
 notmuch-client.h         |    1 +
 notmuch-show.c           |   54 +++++++++++++++++++++++++++++++++++++--------
 6 files changed, 69 insertions(+), 25 deletions(-)

-- 
1.7.9.1

[PATCH v6 1/6] cli: command line parsing: allow default for keyword options

[Mark Walters]

This changes the parsing for "keyword" options so that if the option
is specified with no argument the argument is parsed as if it were
passed an empty string. This make it easier to add options to existing
boolean arguments (the existing --option can default to TRUE).
---
 command-line-arguments.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 76b185f..2fb8a1b 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -11,10 +11,15 @@
 */
 
 static notmuch_bool_t
-_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
+_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
     const notmuch_keyword_t *keywords = arg_desc->keywords;
 
+    if (next == 0) {
+	/* No keyword given */
+	arg_str="";
+    }
+
     while (keywords->name) {
 	if (strcmp (arg_str, keywords->name) == 0) {
 	    if (arg_desc->output_var) {
@@ -24,7 +29,10 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
 	}
 	keywords++;
     }
-    fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    if (next!=0)
+	fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    else
+	fprintf (stderr, "option %s needs a keyword\n", arg_desc->name);
     return FALSE;
 }
 
@@ -99,7 +107,8 @@ parse_option (const char *arg,
 	     */
 	    if (next != '=' && next != ':' && next != 0) return FALSE;
 	    if (next == 0) {
-		if (try->opt_type != NOTMUCH_OPT_BOOLEAN)
+		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
+		    try->opt_type != NOTMUCH_OPT_KEYWORD)
 		    return FALSE;
 	    } else {
 		if (value[0] == 0) return FALSE;
@@ -110,7 +119,7 @@ parse_option (const char *arg,
 
 	    switch (try->opt_type) {
 	    case NOTMUCH_OPT_KEYWORD:
-		return _process_keyword_arg (try, value);
+		return _process_keyword_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_BOOLEAN:
 		return _process_boolean_arg (try, next, value);
-- 
1.7.9.1

Re: [PATCH v6 1/6] cli: command line parsing: allow default for keyword options

[Peter Wang]

On Sun, 27 May 2012 09:22:21 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
> This changes the parsing for "keyword" options so that if the option
> is specified with no argument the argument is parsed as if it were
> passed an empty string. This make it easier to add options to existing
> boolean arguments (the existing --option can default to TRUE).
> ---
>  command-line-arguments.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/command-line-arguments.c b/command-line-arguments.c
> index 76b185f..2fb8a1b 100644
> --- a/command-line-arguments.c
> +++ b/command-line-arguments.c
> @@ -11,10 +11,15 @@
>  */
>  
>  static notmuch_bool_t
> -_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
> +_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
>  
>      const notmuch_keyword_t *keywords = arg_desc->keywords;
>  
> +    if (next == 0) {
> +	/* No keyword given */
> +	arg_str="";
> +    }
> +

Whitespace.

>      while (keywords->name) {
>  	if (strcmp (arg_str, keywords->name) == 0) {
>  	    if (arg_desc->output_var) {
> @@ -24,7 +29,10 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
>  	}
>  	keywords++;
>      }
> -    fprintf (stderr, "unknown keyword: %s\n", arg_str);
> +    if (next!=0)

Whitespace.

> +	fprintf (stderr, "unknown keyword: %s\n", arg_str);
> +    else
> +	fprintf (stderr, "option %s needs a keyword\n", arg_desc->name);

I think "keyword argument" is clearer.

Peter

Re: [PATCH v6 1/6] cli: command line parsing: allow default for keyword options

[Mark Walters]

On Mon, 28 May 2012, Peter Wang <novalazy@gmail.com> wrote:
> On Sun, 27 May 2012 09:22:21 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>> This changes the parsing for "keyword" options so that if the option
>> is specified with no argument the argument is parsed as if it were
>> passed an empty string. This make it easier to add options to existing
>> boolean arguments (the existing --option can default to TRUE).
>> ---
>>  command-line-arguments.c |   17 +++++++++++++----
>>  1 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/command-line-arguments.c b/command-line-arguments.c
>> index 76b185f..2fb8a1b 100644
>> --- a/command-line-arguments.c
>> +++ b/command-line-arguments.c
>> @@ -11,10 +11,15 @@
>>  */
>>  
>>  static notmuch_bool_t
>> -_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
>> +_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
>>  
>>      const notmuch_keyword_t *keywords = arg_desc->keywords;
>>  
>> +    if (next == 0) {
>> +	/* No keyword given */
>> +	arg_str="";
>> +    }
>> +
>
> Whitespace.

Will fix

>>      while (keywords->name) {
>>  	if (strcmp (arg_str, keywords->name) == 0) {
>>  	    if (arg_desc->output_var) {
>> @@ -24,7 +29,10 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
>>  	}
>>  	keywords++;
>>      }
>> -    fprintf (stderr, "unknown keyword: %s\n", arg_str);
>> +    if (next!=0)
>
> Whitespace.

Will fix

>> +	fprintf (stderr, "unknown keyword: %s\n", arg_str);
>> +    else
>> +	fprintf (stderr, "option %s needs a keyword\n", arg_desc->name);
>
> I think "keyword argument" is clearer.

I think I agree: the reason I kept it is that the first case is the
existing error message. Any comments from anyone?

Best wishes

Mark

[PATCH v6 2/6] cli: Let json output "null" messages for non --entire-thread

[Mark Walters]

All formats except Json can output empty messages for non
entire-thread, but in Json format we output "null" to keep the other
elements (e.g. the replies to the omitted message) in the correct
place.
---
 notmuch-client.h |    1 +
 notmuch-show.c   |   20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 19b7f01..08540a7 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -72,6 +72,7 @@ typedef struct notmuch_show_format {
 			      const struct notmuch_show_params *params);
     const char *message_set_sep;
     const char *message_set_end;
+    const char *null_message;
 } notmuch_show_format_t;
 
 typedef struct notmuch_show_params {
diff --git a/notmuch-show.c b/notmuch-show.c
index 95427d4..97da5cc 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -37,7 +37,8 @@ static const notmuch_show_format_t format_json = {
     .message_set_start = "[",
     .part = format_part_json_entry,
     .message_set_sep = ", ",
-    .message_set_end = "]"
+    .message_set_end = "]",
+    .null_message = "null"
 };
 
 static notmuch_status_t
@@ -800,6 +801,15 @@ format_part_raw (unused (const void *ctx), mime_node_t *node,
 }
 
 static notmuch_status_t
+show_null_message (const notmuch_show_format_t *format)
+{
+    /* Output a null message. Currently empty for all formats except Json */
+    if (format->null_message)
+	printf ("%s", format->null_message);
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
 	      notmuch_message_t *message,
@@ -862,11 +872,13 @@ show_messages (void *ctx,
 	    if (status && !res)
 		res = status;
 	    next_indent = indent + 1;
-
-	    if (!status && format->message_set_sep)
-		fputs (format->message_set_sep, stdout);
+	} else {
+	    status = show_null_message (format);
 	}
 
+	if (!status && format->message_set_sep)
+	    fputs (format->message_set_sep, stdout);
+
 	status = show_messages (ctx,
 				format,
 				notmuch_message_get_replies (message),
-- 
1.7.9.1

[PATCH v6 3/6] cli: make --entire-thread=false work for format=json.

[Mark Walters]

The --entire-thread option in notmuch-show.c defaults to true when
format=json. Previously there was no way to turn this off. This patch
makes it respect --entire-thread=false.

To do this the patch moves the --entire-thread option to be a keyword
option using the new command line parsing to allow the existing
--entire-thread to keep working.
---
 notmuch-show.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 97da5cc..4e2965e 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -981,6 +981,12 @@ enum {
     NOTMUCH_FORMAT_RAW
 };
 
+enum {
+    ENTIRE_THREAD_DEFAULT,
+    ENTIRE_THREAD_TRUE,
+    ENTIRE_THREAD_FALSE,
+};
+
 /* The following is to allow future options to be added more easily */
 enum {
     EXCLUDE_TRUE,
@@ -1000,6 +1006,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
     notmuch_bool_t verify = FALSE;
     int exclude = EXCLUDE_TRUE;
+    int entire_thread = ENTIRE_THREAD_DEFAULT;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
@@ -1012,8 +1019,12 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
           (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
                                   { "false", EXCLUDE_FALSE },
                                   { 0, 0 } } },
+	{ NOTMUCH_OPT_KEYWORD, &entire_thread, "entire-thread", 't',
+	  (notmuch_keyword_t []){ { "true", ENTIRE_THREAD_TRUE },
+				  { "false", ENTIRE_THREAD_FALSE },
+				  { "", ENTIRE_THREAD_TRUE },
+				  { 0, 0 } } },
 	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
 	{ 0, 0, 0, 0, 0 }
@@ -1036,7 +1047,9 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     switch (format_sel) {
     case NOTMUCH_FORMAT_JSON:
 	format = &format_json;
-	params.entire_thread = TRUE;
+	/* JSON defaults to entire-thread TRUE */
+	if (entire_thread == ENTIRE_THREAD_DEFAULT)
+	    entire_thread = ENTIRE_THREAD_TRUE;
 	break;
     case NOTMUCH_FORMAT_TEXT:
 	format = &format_text;
@@ -1058,6 +1071,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	params.raw = TRUE;
 	break;
     }
+    /* Default is entire-thread = FALSE except for format=json which
+     * is dealt with above. */
+    if (entire_thread == ENTIRE_THREAD_DEFAULT)
+	entire_thread = ENTIRE_THREAD_FALSE;
+
+    if (entire_thread == ENTIRE_THREAD_TRUE)
+	params.entire_thread = TRUE;
+    else
+	params.entire_thread = FALSE;
 
     if (params.decrypt || verify) {
 #ifdef GMIME_ATLEAST_26
-- 
1.7.9.1

Re: [PATCH v6 3/6] cli: make --entire-thread=false work for format=json.

[Peter Wang]

On Sun, 27 May 2012 09:22:23 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
> @@ -1036,7 +1047,9 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>      switch (format_sel) {
>      case NOTMUCH_FORMAT_JSON:
>  	format = &format_json;
> -	params.entire_thread = TRUE;
> +	/* JSON defaults to entire-thread TRUE */
> +	if (entire_thread == ENTIRE_THREAD_DEFAULT)
> +	    entire_thread = ENTIRE_THREAD_TRUE;
>  	break;

Minor point, but you can defer this until later and keep the logic in
one place.

>      case NOTMUCH_FORMAT_TEXT:
>  	format = &format_text;
> @@ -1058,6 +1071,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	params.raw = TRUE;
>  	break;
>      }
> +    /* Default is entire-thread = FALSE except for format=json which
> +     * is dealt with above. */
> +    if (entire_thread == ENTIRE_THREAD_DEFAULT)
> +	entire_thread = ENTIRE_THREAD_FALSE;
> +
> +    if (entire_thread == ENTIRE_THREAD_TRUE)
> +	params.entire_thread = TRUE;
> +    else
> +	params.entire_thread = FALSE;

    /* Default is entire-thread = FALSE except for format=json. */
    if (entire_thread == ENTIRE_THREAD_DEFAULT) {
	if (format == &format_json)
	    entire_thread = ENTIRE_THREAD_TRUE;
	else
	    entire_thread = ENTIRE_THREAD_FALSE;
    }

Peter

Re: [PATCH v6 3/6] cli: make --entire-thread=false work for format=json.

[Mark Walters]

On Mon, 28 May 2012, Peter Wang <novalazy@gmail.com> wrote:
> On Sun, 27 May 2012 09:22:23 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>> @@ -1036,7 +1047,9 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>>      switch (format_sel) {
>>      case NOTMUCH_FORMAT_JSON:
>>  	format = &format_json;
>> -	params.entire_thread = TRUE;
>> +	/* JSON defaults to entire-thread TRUE */
>> +	if (entire_thread == ENTIRE_THREAD_DEFAULT)
>> +	    entire_thread = ENTIRE_THREAD_TRUE;
>>  	break;
>
> Minor point, but you can defer this until later and keep the logic in
> one place.
>
>>      case NOTMUCH_FORMAT_TEXT:
>>  	format = &format_text;
>> @@ -1058,6 +1071,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>>  	params.raw = TRUE;
>>  	break;
>>      }
>> +    /* Default is entire-thread = FALSE except for format=json which
>> +     * is dealt with above. */
>> +    if (entire_thread == ENTIRE_THREAD_DEFAULT)
>> +	entire_thread = ENTIRE_THREAD_FALSE;
>> +
>> +    if (entire_thread == ENTIRE_THREAD_TRUE)
>> +	params.entire_thread = TRUE;
>> +    else
>> +	params.entire_thread = FALSE;
>
>     /* Default is entire-thread = FALSE except for format=json. */
>     if (entire_thread == ENTIRE_THREAD_DEFAULT) {
> 	if (format == &format_json)
> 	    entire_thread = ENTIRE_THREAD_TRUE;
> 	else
> 	    entire_thread = ENTIRE_THREAD_FALSE;
>     }

Yes I agree that this is cleaner. I will put it in the next version.

Many thanks for the reviews!

Mark

[PATCH v6 4/6] Update devel/schemata for --entire-thread=false

[Mark Walters]

Also remove the Json --entire-thread item from devel/TODO.
---
 devel/TODO     |    2 --
 devel/schemata |    2 +-
 2 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/devel/TODO b/devel/TODO
index 7b750af..eb757af 100644
--- a/devel/TODO
+++ b/devel/TODO
@@ -92,8 +92,6 @@ and email address in the From: line. We could also then easily support
 "notmuch compose --from <something>" to support getting at alternate
 email addresses.
 
-Fix the --format=json option to not imply --entire-thread.
-
 Implement "notmuch search --exclude-threads=<search-terms>" to allow
 for excluding muted threads, (and any other negative, thread-based
 filtering that the user wants to do).
diff --git a/devel/schemata b/devel/schemata
index 977cea7..8fcab8e 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -32,7 +32,7 @@ thread = [thread_node*]
 
 # A message and its replies (show_messages)
 thread_node = [
-    message?,                 # present if --entire-thread or matched
+    message?,                 # null if not matched and not --entire-thread
     [thread_node*]            # children of message
 ]
 
-- 
1.7.9.1

[PATCH v6 5/6] emacs: make elide messages use notmuch-show for omitting messages.

[Mark Walters]

Previously the elide messages code got the entire-thread from
notmuch-show.c and then threw away all non-matching messages. This
version calls notmuch-show.c without the --entire-thread flag so
it never receives the non-matching messages in the first place.

This makes it substantially faster.
---
 emacs/notmuch-show.el |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d318430..9cc68be 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -958,9 +958,9 @@ message at DEPTH in the current thread."
   "Insert the message tree TREE at depth DEPTH in the current thread."
   (let ((msg (car tree))
 	(replies (cadr tree)))
-    (if (or (not notmuch-show-elide-non-matching-messages)
-	    (plist-get msg :match))
-	(notmuch-show-insert-msg msg depth))
+    ;; We test whether there is a message or just some replies.
+    (when msg
+      (notmuch-show-insert-msg msg depth))
     (notmuch-show-insert-thread replies (1+ depth))))
 
 (defun notmuch-show-insert-thread (thread depth)
@@ -1041,16 +1041,18 @@ function is used."
 	     (args (if notmuch-show-query-context
 		       (append (list "\'") basic-args
 			       (list "and (" notmuch-show-query-context ")\'"))
-		     (append (list "\'") basic-args (list "\'")))))
-	(notmuch-show-insert-forest (notmuch-query-get-threads
-				     (cons "--exclude=false" args)))
+		     (append (list "\'") basic-args (list "\'"))))
+	     (cli-args (cons "--exclude=false"
+			     (when notmuch-show-elide-non-matching-messages
+			       (list "--entire-thread=false")))))
+
+	(notmuch-show-insert-forest (notmuch-query-get-threads (append cli-args args)))
 	;; If the query context reduced the results to nothing, run
 	;; the basic query.
 	(when (and (eq (buffer-size) 0)
 		   notmuch-show-query-context)
 	  (notmuch-show-insert-forest
-	   (notmuch-query-get-threads
-	    (cons "--exclude=false" basic-args)))))
+	   (notmuch-query-get-threads (append cli-args basic-args)))))
 
       (jit-lock-register #'notmuch-show-buttonise-links)
 
-- 
1.7.9.1

[PATCH v6 6/6] cli: notmuch-show.c fix whitespace error

[Mark Walters]

Fix an existing whitespace error since it is right next to
the changes of this series.
---
 notmuch-show.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 4e2965e..e19ffda 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1015,10 +1015,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 				  { "mbox", NOTMUCH_FORMAT_MBOX },
 				  { "raw", NOTMUCH_FORMAT_RAW },
 				  { 0, 0 } } },
-        { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
-          (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
-                                  { "false", EXCLUDE_FALSE },
-                                  { 0, 0 } } },
+	{ NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
+	  (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
+				  { "false", EXCLUDE_FALSE },
+				  { 0, 0 } } },
 	{ NOTMUCH_OPT_KEYWORD, &entire_thread, "entire-thread", 't',
 	  (notmuch_keyword_t []){ { "true", ENTIRE_THREAD_TRUE },
 				  { "false", ENTIRE_THREAD_FALSE },
-- 
1.7.9.1