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

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

[Mark Walters]

These two patches supersede the series [1] and the first patch
replaces [2].

The first patch allows --entire-thread=false for notmuch-show.c when
the output format is JSON. In the previous version [2] Austin
suggested that we should output an empty message (i.e., {}) for
non-matching messages rather than just omitting them. This version
does that.

Note the first patch is entirely functional without the second.

The second patch uses the first to implement the "elide" functionality
in the emacs interface on the cli-side rather than on the emacs
side. This is substantially faster in some cases. In current emacs
show view it is a relatively small speed-up which is only noticable
with large threads. However, it will be used by notmuch-pick [3] and
there the speed up will be important. (I have a current version of
notmuch-pick which I will submit in the near future.)

Best wishes

Mark


[1] id:"1330033294-21980-1-git-send-email-markwalters1009@gmail.com"
[2] id:"1331377533-30262-3-git-send-email-markwalters1009@gmail.com"
[3] id:"1329072579-27340-1-git-send-email-markwalters1009@gmail.com"

Mark Walters (2):
  cli: make --entire-thread=false work for format=json.
  emacs: make elide messages use notmuch-show for omitting messages.

 emacs/notmuch-show.el |   17 +++++++++--------
 notmuch-show.c        |   33 ++++++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 13 deletions(-)

-- 
1.7.9.1

[PATCH 1/2] 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.

Finally, all formats except Json can output empty messages for non
entire-thread, but in Json format we need to output {} to keep the
other elements (e.g. the replies to this message) in the correct
place.
---
 notmuch-show.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 7af8e64..5d58bfd 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -809,6 +809,16 @@ format_part_raw (unused (const void *ctx), mime_node_t *node,
 }
 
 static notmuch_status_t
+show_null_message (const notmuch_show_format_t *format)
+{
+    /* For all formats except json an empty message output is valid;
+     * for json we need the braces.*/
+    if (format == &format_json)
+	printf ("{}");
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
 	      notmuch_message_t *message,
@@ -895,10 +905,11 @@ show_messages (void *ctx,
 	    if (status && !res)
 		res = status;
 	    next_indent = indent + 1;
+	} else
+	    status = show_null_message (format);
 
-	    if (!status)
-		fputs (format->message_set_sep, stdout);
-	}
+	if (!status)
+	    fputs (format->message_set_sep, stdout);
 
 	status = show_messages (ctx,
 				format,
@@ -1013,7 +1024,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;
@@ -1053,7 +1070,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;
@@ -1075,6 +1094,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

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

[Adam Wolfe Gordon]

Hi Mark,

This looks good to me, but I haven't tested it. It's probably worth
adding a test for the new functionality.

One style issue below, which is a matter of taste and I'll defer to
others if they disagree:

On Tue, Apr 10, 2012 at 11:04, Mark Walters <markwalters1009@gmail.com> wrote:
> @@ -895,10 +905,11 @@ show_messages (void *ctx,
>            if (status && !res)
>                res = status;
>            next_indent = indent + 1;
> +       } else
> +           status = show_null_message (format);

I accept, but don't particularly like, the notmuch style of omitting
braces where they aren't required. However, an else with a brace on
only one side just looks weird. If they're like this everywhere else
then I guess it's best to be consistent, but to me it's a lot more
readable as } else {.

As I said above, I'll defer to others' judgement here, just thought it
was worth pointing out.

-- Adam

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

[Mark Walters]

On Tue, 10 Apr 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Hi Mark,
>
> This looks good to me, but I haven't tested it. It's probably worth
> adding a test for the new functionality.

Yes I agree. I could add a test to the json tests, or I could update the
pair of test patches at
id:"1332171061-27983-1-git-send-email-markwalters1009@gmail.com" as
there do not seem to be any tests for --entire-thread or not for the
other formats either. What do you think?

> One style issue below, which is a matter of taste and I'll defer to
> others if they disagree:
>
> On Tue, Apr 10, 2012 at 11:04, Mark Walters <markwalters1009@gmail.com> wrote:
>> @@ -895,10 +905,11 @@ show_messages (void *ctx,
>>            if (status && !res)
>>                res = status;
>>            next_indent = indent + 1;
>> +       } else
>> +           status = show_null_message (format);
>
> I accept, but don't particularly like, the notmuch style of omitting
> braces where they aren't required. However, an else with a brace on
> only one side just looks weird. If they're like this everywhere else
> then I guess it's best to be consistent, but to me it's a lot more
> readable as } else {.
>
> As I said above, I'll defer to others' judgement here, just thought it
> was worth pointing out.

Yes I will fix this in the next version (I recall Jani agreed with you
so I will take that as preferred unless someone says otherwise.)

thanks

Mark

[PATCH 2/2] 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 |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 30b26d1..31e6937 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,17 @@ 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 (when notmuch-show-elide-non-matching-messages
+			 (list "--entire-thread=false" "--exclude=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] 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.
---

This replaces the patch [1] as that mistakenly did not 
set --exclude=false in the case when messages are not elided.

 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 30b26d1..820bb41 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 0/2] Allow JSON to use non-entire thread, and use for elide

[Jameson Graef Rollins]

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

On Tue, Apr 10 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> The first patch allows --entire-thread=false for notmuch-show.c when
> the output format is JSON. In the previous version [2] Austin
> suggested that we should output an empty message (i.e., {}) for
> non-matching messages rather than just omitting them. This version
> does that.

Can this be applied to notmuch-reply as well, so that we can regain the
ability to reply to threads?

jamie.

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

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

[Jameson Graef Rollins]

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

On Sat, Apr 14 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> Can this be applied to notmuch-reply as well, so that we can regain the
> ability to reply to threads?

Sorry, nevermind.  I was misinterpreting the point of this patch.

jamie.

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

[PATCH 0/2] Make elide messages use cli side

[Mark Walters]

Currently the elide messages option in notmuch-show.el fetches all
messages in the thread (via notmuch-show.c) parses the JSON and throws 
away all the non-matching messages.

This patch removes the entire-thread assumption for format=json from 
notmuch-show.c. This allows the emacs code to call notmuch-show with 
out the --entire-thread flag and just receive the matching messages 
(but with the full thread structure) in return. 

This makes notmuch-show.c much faster (but in practice this is small
anyway) but more importantly substantially improves the speed of the
JSON parsing. Rough wall clock timing on my system shows (best case) a
4 second time for showing a large thread (170 messages) reduced to an
almost instant time for a single matching message from that thread.

For the existing show view the gain is nice but not critical. However,
notmuch-pick shows a single message at a time and the speed increase
will be needed there.

Best wishes

Mark



Mark Walters (2):
  emacs/cli: remove entire-thread default from show: JSON
  emacs: make elide messages use notmuch-show for omitting messages.

 emacs/notmuch-show.el |   15 +++++++++------
 notmuch-show.c        |    1 -
 2 files changed, 9 insertions(+), 7 deletions(-)

-- 
1.7.2.3

[PATCH 2/2] 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 |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index dcef3d5..e368363 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -981,9 +981,10 @@ 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.
+    (if (eq (length tree) 2)
+	(notmuch-show-insert-msg msg depth)
+      (setq replies msg))
     (notmuch-show-insert-thread replies (1+ depth))))
 
 (defun notmuch-show-insert-thread (thread depth)
@@ -1064,7 +1065,8 @@ function is used."
 		       (append (list "\'") basic-args
 			       (list "and (" notmuch-show-query-context ")\'"))
 		     (append (list "\'") basic-args (list "\'"))))
-	     (cli-args (list "--entire-thread")))
+	     (cli-args (unless notmuch-show-elide-non-matching-messages
+			 (list "--entire-thread"))))
 	(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.
-- 
1.7.2.3