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

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

[Mark Walters]

This is version 4 of this patch set (previous version at [1]) which
removes the --entire-thread constraint from json output in
notmuch-show, and then uses to implement the emacs elide
functionality.

Changes from the previous version: 

	split the cli patch 

	update devel/schemata 

	output "null" rather than "{}" for a null message and do so
	using a format string remove this feature

	from devel/TODO

The first three patches implement the client side changes: one to
allow null messages to be output in Json (without upsetting the
parsing/structure), one to do the command line parsing for
--entire-thread=false and one to update the schemata.

The final emacs patch is independent (and the first three make sense
without it) and uses this new functionality to implement the elide
option.

I think this addresses all review comments

Best wishes

Mark

[1] id:"1334999750-21811-1-git-send-email-markwalters1009@gmail.com"

Mark Walters (4):
  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.

 devel/TODO            |    2 --
 devel/schemata        |    2 +-
 emacs/notmuch-show.el |   18 ++++++++++--------
 notmuch-client.h      |    1 +
 notmuch-show.c        |   36 ++++++++++++++++++++++++++++++------
 5 files changed, 42 insertions(+), 17 deletions(-)

-- 
1.7.9.1

[PATCH v4 1/4] 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 da4a797..0d21f1a 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 v4 2/4] 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.

The one subtlety is that we initialise a notmuch_bool_t to -1 to
indicate that the option parsing has not set it. This allows the code
to distinguish between the option being omitted from the command line,
and the option being set to false on the command line.
---
 notmuch-show.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 0d21f1a..48551bb 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -996,7 +996,13 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     char *query_string;
     int opt_index, ret;
     const notmuch_show_format_t *format = &format_text;
-    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
+
+    /* We abuse the notmuch_bool_t variable params.entire-thread by
+     * setting it to -1 to denote that the command line parsing has
+     * not set it. We ensure it is set to TRUE or FALSE before passing
+     * it to any other function.*/
+    notmuch_show_params_t params = { .part = -1, .entire_thread = -1 };
+
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
     notmuch_bool_t verify = FALSE;
     int exclude = EXCLUDE_TRUE;
@@ -1036,7 +1042,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 (params.entire_thread == -1)
+	    params.entire_thread = TRUE;
 	break;
     case NOTMUCH_FORMAT_TEXT:
 	format = &format_text;
@@ -1058,6 +1066,10 @@ 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 (params.entire_thread == -1)
+	params.entire_thread = FALSE;
 
     if (params.decrypt || verify) {
 #ifdef GMIME_ATLEAST_26
-- 
1.7.9.1

[PATCH v4 3/4] 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 v4 4/4] 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 4ee4290..347f90c 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -976,9 +976,9 @@ current buffer, if possible."
   "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)
@@ -1059,16 +1059,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

Re: [PATCH v4 2/4] cli: make --entire-thread=false work for format=json.

[Peter Wang]

On Tue, 24 Apr 2012 10:11:13 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
> 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.
> 
> The one subtlety is that we initialise a notmuch_bool_t to -1 to
> indicate that the option parsing has not set it. This allows the code
> to distinguish between the option being omitted from the command line,
> and the option being set to false on the command line.
> ---
>  notmuch-show.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 0d21f1a..48551bb 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -996,7 +996,13 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>      char *query_string;
>      int opt_index, ret;
>      const notmuch_show_format_t *format = &format_text;
> -    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
> +
> +    /* We abuse the notmuch_bool_t variable params.entire-thread by
> +     * setting it to -1 to denote that the command line parsing has
> +     * not set it. We ensure it is set to TRUE or FALSE before passing
> +     * it to any other function.*/
> +    notmuch_show_params_t params = { .part = -1, .entire_thread = -1 };
> +
>      int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>      notmuch_bool_t verify = FALSE;
>      int exclude = EXCLUDE_TRUE;

Hi Mark,

As an alternative to the abuse, could you just treat it as with exclude,
using an enum with three values (TRUE|FALSE|DEFAULT)?
Then set params.entire_thread afterwards.

Peter

Re: [PATCH v4 2/4] cli: make --entire-thread=false work for format=json.

[Mark Walters]

On Sat, 26 May 2012, Peter Wang <novalazy@gmail.com> wrote:
> On Tue, 24 Apr 2012 10:11:13 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>> 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.
>> 
>> The one subtlety is that we initialise a notmuch_bool_t to -1 to
>> indicate that the option parsing has not set it. This allows the code
>> to distinguish between the option being omitted from the command line,
>> and the option being set to false on the command line.
>> ---
>>  notmuch-show.c |   16 ++++++++++++++--
>>  1 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/notmuch-show.c b/notmuch-show.c
>> index 0d21f1a..48551bb 100644
>> --- a/notmuch-show.c
>> +++ b/notmuch-show.c
>> @@ -996,7 +996,13 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>>      char *query_string;
>>      int opt_index, ret;
>>      const notmuch_show_format_t *format = &format_text;
>> -    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
>> +
>> +    /* We abuse the notmuch_bool_t variable params.entire-thread by
>> +     * setting it to -1 to denote that the command line parsing has
>> +     * not set it. We ensure it is set to TRUE or FALSE before passing
>> +     * it to any other function.*/
>> +    notmuch_show_params_t params = { .part = -1, .entire_thread = -1 };
>> +
>>      int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>>      notmuch_bool_t verify = FALSE;
>>      int exclude = EXCLUDE_TRUE;
>
> Hi Mark,
>
> As an alternative to the abuse, could you just treat it as with exclude,
> using an enum with three values (TRUE|FALSE|DEFAULT)?
> Then set params.entire_thread afterwards.

The reason I haven't done this is that the current command line parser
does not allow keyword options to take default values: in other words
--entire-thread without an "=<something>" would not be allowed.

It is easy to change the keyword parsing code to allow this: I include a
first draft of such a patch below. This would allow the solution you
suggest and thus avoid the hack/abuse.  What do people think?

Best wishes

Mark


---
 command-line-arguments.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 76b185f..d40c7e6 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -11,10 +11,16 @@
 */
 
 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 so return first option as default */
+	*((int *)arg_desc->output_var) = keywords->value;
+	return TRUE;
+    }
+
     while (keywords->name) {
 	if (strcmp (arg_str, keywords->name) == 0) {
 	    if (arg_desc->output_var) {
@@ -99,7 +105,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 +117,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 v4 2/4] cli: make --entire-thread=false work for format=json.

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> On Sat, 26 May 2012, Peter Wang <novalazy@gmail.com> wrote:
>> On Tue, 24 Apr 2012 10:11:13 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>
> It is easy to change the keyword parsing code to allow this: I include a
> first draft of such a patch below. This would allow the solution you
> suggest and thus avoid the hack/abuse.  What do people think?
>

It didn't carefully review it, but it looks sane enough at a quick
glance.

d

Re: [PATCH v4 0/4] Allow JSON to use non-entire thread, and use for elide

[Daniel Schoepe]

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

On Tue, 24.04.2012 11:11, Mark Walters wrote:
> The first three patches implement the client side changes: one to
> allow null messages to be output in Json (without upsetting the
> parsing/structure), one to do the command line parsing for
> --entire-thread=false and one to update the schemata.
>
> The final emacs patch is independent (and the first three make sense
> without it) and uses this new functionality to implement the elide
> option.
>
> I think this addresses all review comments

LGTM (all four patches). I think both version of the command line
parsing patch would be acceptable, but the second one seems cleaner to
me.

Cheers,
Daniel

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]