[PATCH 1/3] test: add functions to count how much times notmuch was called

[PATCH 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

The patch adds two auxiliary functions and a variable:

  notmuch_counter_reset
  $notmuch_counter
  notmuch_counter

They allow to count how many times notmuch binary is called.
notmuch_counter_reset() function generates a script that counts how
many times it is called and resets the counter to zero.  The function
sets $notmuch_counter variable to the path to the generated script
that should be called instead of notmuch to do the counting.  The
notmuch_counter() function returns the current counter value.
---
 test/README      |   16 ++++++++++++++--
 test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/test/README b/test/README
index 2481f16..1570f7c 100644
--- a/test/README
+++ b/test/README
@@ -187,8 +187,8 @@ library for your script to use.
    is to summarize successes and failures in the test script and
    exit with an appropriate error code.
 
-There are also a number of mail-specific functions which are useful in
-writing tests:
+There are also a number of notmuch-specific auxiliary functions and
+variables which are useful in writing tests:
 
   generate_message
 
@@ -212,3 +212,15 @@ writing tests:
     will initialize the mail database to a known state of 50 sample
     messages, (culled from the early history of the notmuch mailing
     list).
+
+  notmuch_counter_reset
+  $notmuch_counter
+  notmuch_counter
+
+    These allow to count how many times notmuch binary is called.
+    notmuch_counter_reset() function generates a script that counts
+    how many times it is called and resets the counter to zero.  The
+    function sets $notmuch_counter variable to the path to the
+    generated script that should be called instead of notmuch to do
+    the counting.  The notmuch_counter() function returns the current
+    counter value.
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 93867b0..e3b85d0 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -864,6 +864,38 @@ test_emacs () {
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
+# Creates a script that counts how much time it is executed and calls
+# notmuch.  $notmuch_counter is set to the path to the generated
+# script.  Use notmuch_counter() function to get the current counter
+# value.
+notmuch_counter_reset () {
+	notmuch_counter="$TMP_DIRECTORY/notmuch_counter"
+	if [ ! -x "$notmuch_counter" ]; then
+		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
+		cat <<EOF >"$notmuch_counter"
+#!/bin/sh
+
+count=\$(cat "$notmuch_counter_state_path")
+echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
+
+exec notmuch "\$@"
+EOF
+		chmod +x "$notmuch_counter" || return
+	fi
+
+	echo -n 0 > "$notmuch_counter_state_path" || return
+}
+
+# Returns the current notmuch counter value.
+notmuch_counter () {
+	if [ -r "$notmuch_counter_state_path" ]; then
+		count=$(cat "$notmuch_counter_state_path")
+	else
+		count=0
+	fi
+	echo -n $count
+}
+
 
 find_notmuch_path ()
 {
-- 
1.7.7.3

[PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts

[Dmitry Kurochkin]

The patch adds two new test cases:

* Do not call notmuch for non-inlinable application/mpeg parts
* Do not call notmuch for non-inlinable audio/mpeg parts

The application/mpeg test passes thanks to a workaround for
application/* Content-Types.  The audio/mpeg is currently broken.
---
 test/emacs |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 3bf2e29..38efe26 100755
--- a/test/emacs
+++ b/test/emacs
@@ -451,4 +451,35 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
 	    (test-visible-output)'
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Do not call notmuch for non-inlinable application/mpeg parts"
+id='message-with-application/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with application/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"application/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter) 1
+
+test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
+test_subtest_known_broken
+id='message-with-audio/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with audio/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"audio/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter) 1
+
 test_done
-- 
1.7.7.3

Re: [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts

[Austin Clements]

LGTM (modulo any changes that propagate from the first patch)

On Sat, 26 Nov 2011 05:44:37 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> The patch adds two new test cases:
> 
> * Do not call notmuch for non-inlinable application/mpeg parts
> * Do not call notmuch for non-inlinable audio/mpeg parts
> 
> The application/mpeg test passes thanks to a workaround for
> application/* Content-Types.  The audio/mpeg is currently broken.

[PATCH 3/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Before the change, there was a workaround to avoid notmuch show calls
for parts with application/* Content-Type.  But non-inlinable parts
are not limited to this Content-Type (e.g. mp3 files have audio/mpeg
Content-Type and are not inlinable).  For such parts
`notmuch-show-insert-part-*/*' handler is called which unconditionally
fetches contents for all parts.

The patch moves content fetching from `notmuch-show-insert-part-*/*'
to `notmuch-show-mm-display-part-inline' function after MIME inlinable
checks are done to avoid useless notmuch show calls.  The
application/* hack is no longer needed and removed.
---
 emacs/notmuch-show.el |   17 +++++------------
 test/emacs            |    1 -
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 7be88f8..2b0820e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -310,17 +310,17 @@ message at DEPTH in the current thread."
 	;; ange-ftp, which is reasonable to use here.
 	(mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t)))))
 
-(defun notmuch-show-mm-display-part-inline (msg part content-type content)
+(defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (insert content)
       (let ((handle (mm-make-handle (current-buffer) (list content-type))))
-	(set-buffer display-buffer)
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
-	    (progn
+	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
+	      (insert content)
+	      (set-buffer display-buffer)
 	      (mm-display-part handle)
 	      t)
 	  nil)))))
@@ -578,17 +578,10 @@ current buffer, if possible."
 		nil))
 	  nil))))
 
-(defun notmuch-show-insert-part-application/* (msg part content-type nth depth declared-type
-)
-  ;; do not render random "application" parts
-  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename)))
-
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-  (let ((content (notmuch-show-get-bodypart-content msg part nth)))
-    (if content
-	(notmuch-show-mm-display-part-inline msg part content-type content)))
+  (notmuch-show-mm-display-part-inline msg part nth content-type)
   t)
 
 ;; Functions for determining how to handle MIME parts.
diff --git a/test/emacs b/test/emacs
index 38efe26..e2b438b 100755
--- a/test/emacs
+++ b/test/emacs
@@ -467,7 +467,6 @@ test_emacs "(let ((notmuch-command \"$notmuch_counter\"))
 test_expect_equal $(notmuch_counter) 1
 
 test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
-test_subtest_known_broken
 id='message-with-audio/mpeg-attachment@notmuchmail.org'
 emacs_deliver_message \
     'Message with audio/mpeg attachment' \
-- 
1.7.7.3

Re: [PATCH 3/3] emacs: do not call notmuch show for non-inlinable parts

[Austin Clements]

LGTM.  We should probably be paying attention to the content
disposition, but this is a definite improvement on what we do now.

As an interesting side-note, this lets us inline a handful of types that
we previously wouldn't, like application/emacs-lisp.

On Sat, 26 Nov 2011 05:44:38 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Before the change, there was a workaround to avoid notmuch show calls
> for parts with application/* Content-Type.  But non-inlinable parts
> are not limited to this Content-Type (e.g. mp3 files have audio/mpeg
> Content-Type and are not inlinable).  For such parts
> `notmuch-show-insert-part-*/*' handler is called which unconditionally
> fetches contents for all parts.
> 
> The patch moves content fetching from `notmuch-show-insert-part-*/*'
> to `notmuch-show-mm-display-part-inline' function after MIME inlinable
> checks are done to avoid useless notmuch show calls.  The
> application/* hack is no longer needed and removed.
> ---
>  emacs/notmuch-show.el |   17 +++++------------
>  test/emacs            |    1 -
>  2 files changed, 5 insertions(+), 13 deletions(-)

Yay!

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Austin Clements]

On Sat, 26 Nov 2011 05:44:36 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> The patch adds two auxiliary functions and a variable:
> 
>   notmuch_counter_reset
>   $notmuch_counter
>   notmuch_counter
> 
> They allow to count how many times notmuch binary is called.
> notmuch_counter_reset() function generates a script that counts how
> many times it is called and resets the counter to zero.  The function
> sets $notmuch_counter variable to the path to the generated script
> that should be called instead of notmuch to do the counting.  The
> notmuch_counter() function returns the current counter value.

Great idea!

> ---
>  test/README      |   16 ++++++++++++++--
>  test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/test/README b/test/README
> index 2481f16..1570f7c 100644
> --- a/test/README
> +++ b/test/README
> @@ -187,8 +187,8 @@ library for your script to use.
>     is to summarize successes and failures in the test script and
>     exit with an appropriate error code.
>  
> -There are also a number of mail-specific functions which are useful in
> -writing tests:
> +There are also a number of notmuch-specific auxiliary functions and
> +variables which are useful in writing tests:
>  
>    generate_message
>  
> @@ -212,3 +212,15 @@ writing tests:
>      will initialize the mail database to a known state of 50 sample
>      messages, (culled from the early history of the notmuch mailing
>      list).
> +
> +  notmuch_counter_reset
> +  $notmuch_counter
> +  notmuch_counter

From the name, I would expect $notmuch_counter to be the counter value
(ignoring the difficulty of implementing that).  Perhaps
$notmuch_counter_command, since it's meant to be bound to
notmuch-command?

notmuch_counter is okay, but maybe notmuch_counter_value?

> +
> +    These allow to count how many times notmuch binary is called.
> +    notmuch_counter_reset() function generates a script that counts
> +    how many times it is called and resets the counter to zero.  The
> +    function sets $notmuch_counter variable to the path to the
> +    generated script that should be called instead of notmuch to do
> +    the counting.  The notmuch_counter() function returns the current
> +    counter value.

It doesn't return the counter value, it prints it.

> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 93867b0..e3b85d0 100755
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -864,6 +864,38 @@ test_emacs () {
>  	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>  }
>  
> +# Creates a script that counts how much time it is executed and calls
> +# notmuch.  $notmuch_counter is set to the path to the generated
> +# script.  Use notmuch_counter() function to get the current counter
> +# value.
> +notmuch_counter_reset () {
> +	notmuch_counter="$TMP_DIRECTORY/notmuch_counter"
> +	if [ ! -x "$notmuch_counter" ]; then
> +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> +		cat <<EOF >"$notmuch_counter"

This is totally a stylistic pet peeve, but consider
  cat >"$notmuch_counter" <<EOF

> +#!/bin/sh
> +
> +count=\$(cat "$notmuch_counter_state_path")
> +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> +
> +exec notmuch "\$@"
> +EOF
> +		chmod +x "$notmuch_counter" || return
> +	fi
> +
> +	echo -n 0 > "$notmuch_counter_state_path" || return

What are the || return's for?

> +}
> +
> +# Returns the current notmuch counter value.
> +notmuch_counter () {
> +	if [ -r "$notmuch_counter_state_path" ]; then
> +		count=$(cat "$notmuch_counter_state_path")
> +	else
> +		count=0
> +	fi
> +	echo -n $count
> +}
> +
>  
>  find_notmuch_path ()
>  {
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
> 

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

On Sun, 27 Nov 2011 21:44:23 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> On Sat, 26 Nov 2011 05:44:36 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > The patch adds two auxiliary functions and a variable:
> > 
> >   notmuch_counter_reset
> >   $notmuch_counter
> >   notmuch_counter
> > 
> > They allow to count how many times notmuch binary is called.
> > notmuch_counter_reset() function generates a script that counts how
> > many times it is called and resets the counter to zero.  The function
> > sets $notmuch_counter variable to the path to the generated script
> > that should be called instead of notmuch to do the counting.  The
> > notmuch_counter() function returns the current counter value.
> 
> Great idea!
> 
> > ---
> >  test/README      |   16 ++++++++++++++--
> >  test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+), 2 deletions(-)
> > 
> > diff --git a/test/README b/test/README
> > index 2481f16..1570f7c 100644
> > --- a/test/README
> > +++ b/test/README
> > @@ -187,8 +187,8 @@ library for your script to use.
> >     is to summarize successes and failures in the test script and
> >     exit with an appropriate error code.
> >  
> > -There are also a number of mail-specific functions which are useful in
> > -writing tests:
> > +There are also a number of notmuch-specific auxiliary functions and
> > +variables which are useful in writing tests:
> >  
> >    generate_message
> >  
> > @@ -212,3 +212,15 @@ writing tests:
> >      will initialize the mail database to a known state of 50 sample
> >      messages, (culled from the early history of the notmuch mailing
> >      list).
> > +
> > +  notmuch_counter_reset
> > +  $notmuch_counter
> > +  notmuch_counter
> 
> From the name, I would expect $notmuch_counter to be the counter value
> (ignoring the difficulty of implementing that).  Perhaps
> $notmuch_counter_command, since it's meant to be bound to
> notmuch-command?
> 

renamed

> notmuch_counter is okay, but maybe notmuch_counter_value?
> 

renamed

> > +
> > +    These allow to count how many times notmuch binary is called.
> > +    notmuch_counter_reset() function generates a script that counts
> > +    how many times it is called and resets the counter to zero.  The
> > +    function sets $notmuch_counter variable to the path to the
> > +    generated script that should be called instead of notmuch to do
> > +    the counting.  The notmuch_counter() function returns the current
> > +    counter value.
> 
> It doesn't return the counter value, it prints it.
> 

indeed, fixed

> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index 93867b0..e3b85d0 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -864,6 +864,38 @@ test_emacs () {
> >  	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
> >  }
> >  
> > +# Creates a script that counts how much time it is executed and calls
> > +# notmuch.  $notmuch_counter is set to the path to the generated
> > +# script.  Use notmuch_counter() function to get the current counter
> > +# value.
> > +notmuch_counter_reset () {
> > +	notmuch_counter="$TMP_DIRECTORY/notmuch_counter"
> > +	if [ ! -x "$notmuch_counter" ]; then
> > +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> > +		cat <<EOF >"$notmuch_counter"
> 
> This is totally a stylistic pet peeve, but consider
>   cat >"$notmuch_counter" <<EOF
> 

fixed

> > +#!/bin/sh
> > +
> > +count=\$(cat "$notmuch_counter_state_path")
> > +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> > +
> > +exec notmuch "\$@"
> > +EOF
> > +		chmod +x "$notmuch_counter" || return
> > +	fi
> > +
> > +	echo -n 0 > "$notmuch_counter_state_path" || return
> 
> What are the || return's for?
> 

If echo failed by some reason, that is an error.

Thank you for the reviews.  I will send amended series soon.

Regards,
  Dmitry

> > +}
> > +
> > +# Returns the current notmuch counter value.
> > +notmuch_counter () {
> > +	if [ -r "$notmuch_counter_state_path" ]; then
> > +		count=$(cat "$notmuch_counter_state_path")
> > +	else
> > +		count=0
> > +	fi
> > +	echo -n $count
> > +}
> > +
> >  
> >  find_notmuch_path ()
> >  {
> > -- 
> > 1.7.7.3
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> > 

[PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

This patch series has all comments from Austin's review [1] fixed.

Changes in v2 since v1:

* Rename $notmuch_counter variable to $notmuch_counter_command.
* Rename notmuch_counter() function to notmuch_counter_value().
* Fix documentation for notmuch_counter_value().
* Removed unneeded and add missing "|| return" in notmuch_counter_reset().
* Coding style fixes.

[1] id:"87ipm52cfs.fsf@awakening.csail.mit.edu"

[PATCH 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

The patch adds two auxiliary functions and a variable:

  notmuch_counter_reset
  $notmuch_counter_command
  notmuch_counter_value

They allow to count how many times notmuch binary is called.
notmuch_counter_reset() function generates a script that counts how
many times it is called and resets the counter to zero.  The function
sets $notmuch_counter_command variable to the path to the generated
script that should be called instead of notmuch to do the counting.
The notmuch_counter_value() function returns the current counter
value.
---
 test/README      |   16 ++++++++++++++--
 test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/test/README b/test/README
index 2481f16..5dc0638 100644
--- a/test/README
+++ b/test/README
@@ -187,8 +187,8 @@ library for your script to use.
    is to summarize successes and failures in the test script and
    exit with an appropriate error code.
 
-There are also a number of mail-specific functions which are useful in
-writing tests:
+There are also a number of notmuch-specific auxiliary functions and
+variables which are useful in writing tests:
 
   generate_message
 
@@ -212,3 +212,15 @@ writing tests:
     will initialize the mail database to a known state of 50 sample
     messages, (culled from the early history of the notmuch mailing
     list).
+
+  notmuch_counter_reset
+  $notmuch_counter_command
+  notmuch_counter_value
+
+    These allow to count how many times notmuch binary is called.
+    notmuch_counter_reset() function generates a script that counts
+    how many times it is called and resets the counter to zero.  The
+    function sets $notmuch_counter_command variable to the path to the
+    generated script that should be called instead of notmuch to do
+    the counting.  The notmuch_counter_value() function prints the
+    current counter value.
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 076f929..880bed9 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -868,6 +868,38 @@ test_emacs () {
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
+# Creates a script that counts how much time it is executed and calls
+# notmuch.  $notmuch_counter_command is set to the path to the
+# generated script.  Use notmuch_counter_value() function to get the
+# current counter value.
+notmuch_counter_reset () {
+	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
+	if [ ! -x "$notmuch_counter_command" ]; then
+		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
+		cat >"$notmuch_counter_command" <<EOF || return
+#!/bin/sh
+
+count=\$(cat "$notmuch_counter_state_path")
+echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
+
+exec notmuch "\$@"
+EOF
+		chmod +x "$notmuch_counter_command" || return
+	fi
+
+	echo -n 0 > "$notmuch_counter_state_path"
+}
+
+# Returns the current notmuch counter value.
+notmuch_counter_value () {
+	if [ -r "$notmuch_counter_state_path" ]; then
+		count=$(cat "$notmuch_counter_state_path")
+	else
+		count=0
+	fi
+	echo -n $count
+}
+
 test_reset_state_ () {
 	test_subtest_known_broken_=
 }
-- 
1.7.7.3

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Tomi Ollila]

On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:

[...]
> +
> +    These allow to count how many times notmuch binary is called.
> +    notmuch_counter_reset() function generates a script that counts
> +    how many times it is called and resets the counter to zero.  The
> +    function sets $notmuch_counter_command variable to the path to the
> +    generated script that should be called instead of notmuch to do
> +    the counting.  The notmuch_counter_value() function prints the
> +    current counter value.
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 076f929..880bed9 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -868,6 +868,38 @@ test_emacs () {
>  	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>  }
>  
> +# Creates a script that counts how much time it is executed and calls
> +# notmuch.  $notmuch_counter_command is set to the path to the
> +# generated script.  Use notmuch_counter_value() function to get the
> +# current counter value.
> +notmuch_counter_reset () {
> +	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
> +	if [ ! -x "$notmuch_counter_command" ]; then
> +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> +		cat >"$notmuch_counter_command" <<EOF || return
> +#!/bin/sh
> +
> +count=\$(cat "$notmuch_counter_state_path")
> +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> +
> +exec notmuch "\$@"
> +EOF
> +		chmod +x "$notmuch_counter_command" || return
> +	fi
> +
> +	echo -n 0 > "$notmuch_counter_state_path"
> +}
> +
> +# Returns the current notmuch counter value.
> +notmuch_counter_value () {
> +	if [ -r "$notmuch_counter_state_path" ]; then
> +		count=$(cat "$notmuch_counter_state_path")
> +	else
> +		count=0
> +	fi
> +	echo -n $count
> +}
> +

Good work! It would be nice if the state file contained newline after
count number. Also some optimizations could be done:

		cat >"$notmuch_counter_command" <<EOF || return
#!/bin/sh

read count < "$notmuch_counter_state_path"
echo \$((count + 1)) > "$notmuch_counter_state_path"

exec notmuch "\$@"
EOF
		chmod +x "$notmuch_counter_command" || return
	fi

	echo 0 > "$notmuch_counter_state_path"
}

# Returns the current notmuch counter value.
notmuch_counter_value () {
	if [ -r "$notmuch_counter_state_path" ]; then
		read count < "$notmuch_counter_state_path"
	else
		count=0
	fi
	echo -n $count
}


Tomi

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

Hi Tomi.

On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> 
> [...]
> > +
> > +    These allow to count how many times notmuch binary is called.
> > +    notmuch_counter_reset() function generates a script that counts
> > +    how many times it is called and resets the counter to zero.  The
> > +    function sets $notmuch_counter_command variable to the path to the
> > +    generated script that should be called instead of notmuch to do
> > +    the counting.  The notmuch_counter_value() function prints the
> > +    current counter value.
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index 076f929..880bed9 100644
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -868,6 +868,38 @@ test_emacs () {
> >  	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
> >  }
> >  
> > +# Creates a script that counts how much time it is executed and calls
> > +# notmuch.  $notmuch_counter_command is set to the path to the
> > +# generated script.  Use notmuch_counter_value() function to get the
> > +# current counter value.
> > +notmuch_counter_reset () {
> > +	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
> > +	if [ ! -x "$notmuch_counter_command" ]; then
> > +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> > +		cat >"$notmuch_counter_command" <<EOF || return
> > +#!/bin/sh
> > +
> > +count=\$(cat "$notmuch_counter_state_path")
> > +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> > +
> > +exec notmuch "\$@"
> > +EOF
> > +		chmod +x "$notmuch_counter_command" || return
> > +	fi
> > +
> > +	echo -n 0 > "$notmuch_counter_state_path"
> > +}
> > +
> > +# Returns the current notmuch counter value.
> > +notmuch_counter_value () {
> > +	if [ -r "$notmuch_counter_state_path" ]; then
> > +		count=$(cat "$notmuch_counter_state_path")
> > +	else
> > +		count=0
> > +	fi
> > +	echo -n $count
> > +}
> > +
> 
> Good work! It would be nice if the state file contained newline after
> count number.

I wonder why it is actually nice :)  I do not have strong preference
here.  So a newline is added in v3.  Also a newline is added to
notmuch_counter_value() output for consistency.

> Also some optimizations could be done:
> 

(Would be nice if you send a diff, or a human-friendly description of
the changes.)

> 		cat >"$notmuch_counter_command" <<EOF || return
> #!/bin/sh
> 
> read count < "$notmuch_counter_state_path"

Nice.  Fixed in the new patch version.

> echo \$((count + 1)) > "$notmuch_counter_state_path"
> 

I do not think this is really an optimization.  And I find expr more
clear than using $(()).  I always have troubles remembering "random
special char syntax" (yeah, not a Perl fan :)), prefer human friendly
words.

> exec notmuch "\$@"
> EOF
> 		chmod +x "$notmuch_counter_command" || return
> 	fi
> 
> 	echo 0 > "$notmuch_counter_state_path"
> }
> 
> # Returns the current notmuch counter value.
> notmuch_counter_value () {
> 	if [ -r "$notmuch_counter_state_path" ]; then
> 		read count < "$notmuch_counter_state_path"

Also changes in v3.

Regards,
  Dmitry

> 	else
> 		count=0
> 	fi
> 	echo -n $count
> }
> 
> 
> Tomi

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Tomi Ollila]

Hi Dmitry.

On Tue, 29 Nov 2011 01:26:39 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Tomi.
> 
> On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
[ ... ]
> > > +# Creates a script that counts how much time it is executed and calls
> > > +# notmuch.  $notmuch_counter_command is set to the path to the
> > > +# generated script.  Use notmuch_counter_value() function to get the
> > > +# current counter value.
> > > +notmuch_counter_reset () {
> > > +	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
> > > +	if [ ! -x "$notmuch_counter_command" ]; then
> > > +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> > > +		cat >"$notmuch_counter_command" <<EOF || return
> > > +#!/bin/sh
> > > +
> > > +count=\$(cat "$notmuch_counter_state_path")
> > > +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> > > +
> > > +exec notmuch "\$@"
> > > +EOF
> > > +		chmod +x "$notmuch_counter_command" || return
> > > +	fi
> > > +
> > > +	echo -n 0 > "$notmuch_counter_state_path"
> > > +}
> > > +
> > > +# Returns the current notmuch counter value.
> > > +notmuch_counter_value () {
> > > +	if [ -r "$notmuch_counter_state_path" ]; then
> > > +		count=$(cat "$notmuch_counter_state_path")
> > > +	else
> > > +		count=0
> > > +	fi
> > > +	echo -n $count
> > > +}
> > > +
> > 
> > Good work! It would be nice if the state file contained newline after
> > count number.
> 
> I wonder why it is actually nice :)  I do not have strong preference
> here.  So a newline is added in v3.  Also a newline is added to
> notmuch_counter_value() output for consistency.

It is nice when I enter cat /path/to/notmuch_counter from command
line and shell prompt is not appended at the end of the file contents
(but on next line :)

> 
> > Also some optimizations could be done:
> > 
> 
> (Would be nice if you send a diff, or a human-friendly description of
> the changes.)

Ok, I'll try to do this according to your wishes next time.

> > 		cat >"$notmuch_counter_command" <<EOF || return
> > #!/bin/sh
> > 
> > read count < "$notmuch_counter_state_path"
> 
> Nice.  Fixed in the new patch version.
> 
> > echo \$((count + 1)) > "$notmuch_counter_state_path"
> > 
> 
> I do not think this is really an optimization.  And I find expr more
> clear than using $(()).  I always have troubles remembering "random
> special char syntax" (yeah, not a Perl fan :)), prefer human friendly
> words.

The (posix) shell command language defines 'Arithmetic Expansion' in

http://pubs.opengroup.org/onlinepubs/007908799/xcu/chap2.html#tag_001_006_004

I.e. using format $(( expression )) makes shell doing the arithetic itself
instead of forking a process (or two!) to do so.

Normally in this case it is not so big deal (and still it isn't, but...)
In this  particular case the shell wrapper counting notmuch launches and
exec'ing it the wrapper could do this without fork(2)ing a single time
(i.e. keep the process count unchanged compared to execing notmuch
directly)

Anyway, many opinions; as far as it works I'm fine with it :) 

Now that you feel relaxed, check the results of some further
experimentation ;) :

excerpt from man strace:

       -ff         If the -o filename option is in effect,  each  processes
                   trace  is  written  to  filename.pid  where  pid  is the
                   numeric process id of each process.

Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(( 5 + 5 ))' 

will output '10' and create just one 'forked.<pid>' file

Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(expr 5 + 5)' 

output 10 as expected, but there is now *3* forked.<pid> files !

bash does not optmize; it forks subshell to execute $(...) and then
there just works as usual (forks subshell to execute builtin expr))

Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(exec expr 5 + 5)' 

(the added 'exec' takes off one fork -- just 2 forked.<pid> files appear).

I did the same tests using dash, ksh & zsh on linux system, and every one
of these managed to optimize one fork out in the above 3 fork case.


Tomi

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

Hi Tomi.

On Tue, 29 Nov 2011 14:58:00 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Hi Dmitry.
> 
> On Tue, 29 Nov 2011 01:26:39 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Hi Tomi.
> > 
> > On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > > On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> [ ... ]
> > > > +# Creates a script that counts how much time it is executed and calls
> > > > +# notmuch.  $notmuch_counter_command is set to the path to the
> > > > +# generated script.  Use notmuch_counter_value() function to get the
> > > > +# current counter value.
> > > > +notmuch_counter_reset () {
> > > > +	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
> > > > +	if [ ! -x "$notmuch_counter_command" ]; then
> > > > +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> > > > +		cat >"$notmuch_counter_command" <<EOF || return
> > > > +#!/bin/sh
> > > > +
> > > > +count=\$(cat "$notmuch_counter_state_path")
> > > > +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> > > > +
> > > > +exec notmuch "\$@"
> > > > +EOF
> > > > +		chmod +x "$notmuch_counter_command" || return
> > > > +	fi
> > > > +
> > > > +	echo -n 0 > "$notmuch_counter_state_path"
> > > > +}
> > > > +
> > > > +# Returns the current notmuch counter value.
> > > > +notmuch_counter_value () {
> > > > +	if [ -r "$notmuch_counter_state_path" ]; then
> > > > +		count=$(cat "$notmuch_counter_state_path")
> > > > +	else
> > > > +		count=0
> > > > +	fi
> > > > +	echo -n $count
> > > > +}
> > > > +
> > > 
> > > Good work! It would be nice if the state file contained newline after
> > > count number.
> > 
> > I wonder why it is actually nice :)  I do not have strong preference
> > here.  So a newline is added in v3.  Also a newline is added to
> > notmuch_counter_value() output for consistency.
> 
> It is nice when I enter cat /path/to/notmuch_counter from command
> line and shell prompt is not appended at the end of the file contents
> (but on next line :)
> 
> > 
> > > Also some optimizations could be done:
> > > 
> > 
> > (Would be nice if you send a diff, or a human-friendly description of
> > the changes.)
> 
> Ok, I'll try to do this according to your wishes next time.
> 
> > > 		cat >"$notmuch_counter_command" <<EOF || return
> > > #!/bin/sh
> > > 
> > > read count < "$notmuch_counter_state_path"
> > 
> > Nice.  Fixed in the new patch version.
> > 
> > > echo \$((count + 1)) > "$notmuch_counter_state_path"
> > > 
> > 
> > I do not think this is really an optimization.  And I find expr more
> > clear than using $(()).  I always have troubles remembering "random
> > special char syntax" (yeah, not a Perl fan :)), prefer human friendly
> > words.
> 
> The (posix) shell command language defines 'Arithmetic Expansion' in
> 
> http://pubs.opengroup.org/onlinepubs/007908799/xcu/chap2.html#tag_001_006_004
> 
> I.e. using format $(( expression )) makes shell doing the arithetic itself
> instead of forking a process (or two!) to do so.
> 

I though expr was a builtin.  Now I agree that it is better to replace
it with $(()), even though I still prefer the expr syntax.

> Normally in this case it is not so big deal (and still it isn't, but...)
> In this  particular case the shell wrapper counting notmuch launches and
> exec'ing it the wrapper could do this without fork(2)ing a single time
> (i.e. keep the process count unchanged compared to execing notmuch
> directly)
> 
> Anyway, many opinions; as far as it works I'm fine with it :) 
> 
> Now that you feel relaxed, check the results of some further
> experimentation ;) :
> 
> excerpt from man strace:
> 
>        -ff         If the -o filename option is in effect,  each  processes
>                    trace  is  written  to  filename.pid  where  pid  is the
>                    numeric process id of each process.
> 
> Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(( 5 + 5 ))' 
> 
> will output '10' and create just one 'forked.<pid>' file
> 
> Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(expr 5 + 5)' 
> 
> output 10 as expected, but there is now *3* forked.<pid> files !
> 
> bash does not optmize; it forks subshell to execute $(...) and then
> there just works as usual (forks subshell to execute builtin expr))
> 
> Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(exec expr 5 + 5)' 
> 
> (the added 'exec' takes off one fork -- just 2 forked.<pid> files appear).
> 
> I did the same tests using dash, ksh & zsh on linux system, and every one
> of these managed to optimize one fork out in the above 3 fork case.
> 

Thanks for details.

Regards,
  Dmitry

> 
> Tomi

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Tomi Ollila]

On Wed, 30 Nov 2011 01:03:27 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Tomi.
> 
> On Tue, 29 Nov 2011 14:58:00 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > Hi Dmitry.

[ ... ]

> > 
> > The (posix) shell command language defines 'Arithmetic Expansion' in
> > 
> > http://pubs.opengroup.org/onlinepubs/007908799/xcu/chap2.html#tag_001_006_004
> > 
> > I.e. using format $(( expression )) makes shell doing the arithetic itself
> > instead of forking a process (or two!) to do so.
> > 
> 
> I though expr was a builtin.  Now I agree that it is better to replace
> it with $(()), even though I still prefer the expr syntax.

Actually, I thought also that expr was a builtin. That makes the resolution
'forks subshell to execute builtin expr' below wrong. If it were a builtin
then bash would also fork only once (to get details right). I re-tested
with zsh using 'builtin pwd' and '/bin/pwd' instead of 'expr' -- only one fork 
in each case. So, those who examined my tests with deep interest also note
this correction.


> > Normally in this case it is not so big deal (and still it isn't, but...)
> > In this  particular case the shell wrapper counting notmuch launches and
> > exec'ing it the wrapper could do this without fork(2)ing a single time
> > (i.e. keep the process count unchanged compared to execing notmuch
> > directly)
> > 
> > Anyway, many opinions; as far as it works I'm fine with it :) 
> > 
> > Now that you feel relaxed, check the results of some further
> > experimentation ;) :
> > 
> > excerpt from man strace:
> > 
> >        -ff         If the -o filename option is in effect,  each  processes
> >                    trace  is  written  to  filename.pid  where  pid  is the
> >                    numeric process id of each process.
> > 
> > Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(( 5 + 5 ))' 
> > 
> > will output '10' and create just one 'forked.<pid>' file
> > 
> > Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(expr 5 + 5)' 
> > 
> > output 10 as expected, but there is now *3* forked.<pid> files !
> > 
> > bash does not optmize; it forks subshell to execute $(...) and then
> > there just works as usual (forks subshell to execute builtin expr))
> > 
> > Executing  rm -f forked.*; strace -ff -o forked bash -c 'echo $(exec expr 5 + 5)' 
> > 
> > (the added 'exec' takes off one fork -- just 2 forked.<pid> files appear).
> > 
> > I did the same tests using dash, ksh & zsh on linux system, and every one
> > of these managed to optimize one fork out in the above 3 fork case.
> > 
> 
> Thanks for details.
> 
> Regards,
>   Dmitry
> 
> > 
> > Tomi

Tomi

Re: [PATCH 1/3] test: add functions to count how much times notmuch was called

[Jameson Graef Rollins]

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

On Tue, 29 Nov 2011 01:26:39 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > echo \$((count + 1)) > "$notmuch_counter_state_path"
> 
> I do not think this is really an optimization.  And I find expr more
> clear than using $(()).  I always have troubles remembering "random
> special char syntax" (yeah, not a Perl fan :)), prefer human friendly
> words.

This allows the shell to do the math itself, without calling a
subprocess.  I agree with Tomi that $(()) is a little cleaner.

jamie.

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

[PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts

[Dmitry Kurochkin]

The patch adds two new test cases:

* Do not call notmuch for non-inlinable application/mpeg parts
* Do not call notmuch for non-inlinable audio/mpeg parts

The application/mpeg test passes thanks to a workaround for
application/* Content-Types.  The audio/mpeg is currently broken.
---
 test/emacs |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 198c27b..20f8449 100755
--- a/test/emacs
+++ b/test/emacs
@@ -472,4 +472,35 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
 	    (test-visible-output)'
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Do not call notmuch for non-inlinable application/mpeg parts"
+id='message-with-application/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with application/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"application/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter_value) 1
+
+test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
+test_subtest_known_broken
+id='message-with-audio/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with audio/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"audio/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter_value) 1
+
 test_done
-- 
1.7.7.3

[PATCH 3/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Before the change, there was a workaround to avoid notmuch show calls
for parts with application/* Content-Type.  But non-inlinable parts
are not limited to this Content-Type (e.g. mp3 files have audio/mpeg
Content-Type and are not inlinable).  For such parts
`notmuch-show-insert-part-*/*' handler is called which unconditionally
fetches contents for all parts.

The patch moves content fetching from `notmuch-show-insert-part-*/*'
to `notmuch-show-mm-display-part-inline' function after MIME inlinable
checks are done to avoid useless notmuch show calls.  The
application/* hack is no longer needed and removed.
---
 emacs/notmuch-show.el |   17 +++++------------
 test/emacs            |    1 -
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d7fbbca..d2d4968 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -320,17 +320,17 @@ message at DEPTH in the current thread."
 	;; ange-ftp, which is reasonable to use here.
 	(mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t)))))
 
-(defun notmuch-show-mm-display-part-inline (msg part content-type content)
+(defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (insert content)
       (let ((handle (mm-make-handle (current-buffer) (list content-type))))
-	(set-buffer display-buffer)
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
-	    (progn
+	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
+	      (insert content)
+	      (set-buffer display-buffer)
 	      (mm-display-part handle)
 	      t)
 	  nil)))))
@@ -588,17 +588,10 @@ current buffer, if possible."
 		nil))
 	  nil))))
 
-(defun notmuch-show-insert-part-application/* (msg part content-type nth depth declared-type
-)
-  ;; do not render random "application" parts
-  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename)))
-
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-  (let ((content (notmuch-show-get-bodypart-content msg part nth)))
-    (if content
-	(notmuch-show-mm-display-part-inline msg part content-type content)))
+  (notmuch-show-mm-display-part-inline msg part nth content-type)
   t)
 
 ;; Functions for determining how to handle MIME parts.
diff --git a/test/emacs b/test/emacs
index 20f8449..56ca87b 100755
--- a/test/emacs
+++ b/test/emacs
@@ -488,7 +488,6 @@ test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
 test_expect_equal $(notmuch_counter_value) 1
 
 test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
-test_subtest_known_broken
 id='message-with-audio/mpeg-attachment@notmuchmail.org'
 emacs_deliver_message \
     'Message with audio/mpeg attachment' \
-- 
1.7.7.3

Re: [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts

[Austin Clements]

LGTM

Re: [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts

[Jameson Graef Rollins]

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

On Sun, 27 Nov 2011 22:39:53 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> LGTM

Agreed.  Thanks for the nice patches, Dmitry.

jamie.

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

[PATCH v3 0/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Make some changes suggested by Tomi Ollila [1].

Changes:

v3 since v2:

* Use read function instead of cat(1) to read counter value.
* Add a newline after the count value in state file and
  notmuch_counter_value() output.

v2 since v1:

* Rename $notmuch_counter variable to $notmuch_counter_command.
* Rename notmuch_counter() function to notmuch_counter_value().
* Fix documentation for notmuch_counter_value().
* Removed unneeded and add missing "|| return" in notmuch_counter_reset().
* Coding style fixes.

[1] id:"yf6aa7gj7w5.fsf@taco2.nixu.fi"

[PATCH v3 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

The patch adds two auxiliary functions and a variable:

  notmuch_counter_reset
  $notmuch_counter_command
  notmuch_counter_value

They allow to count how many times notmuch binary is called.
notmuch_counter_reset() function generates a script that counts how
many times it is called and resets the counter to zero.  The function
sets $notmuch_counter_command variable to the path to the generated
script that should be called instead of notmuch to do the counting.
The notmuch_counter_value() function returns the current counter
value.
---
 test/README      |   16 ++++++++++++++--
 test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/test/README b/test/README
index 2481f16..5dc0638 100644
--- a/test/README
+++ b/test/README
@@ -187,8 +187,8 @@ library for your script to use.
    is to summarize successes and failures in the test script and
    exit with an appropriate error code.
 
-There are also a number of mail-specific functions which are useful in
-writing tests:
+There are also a number of notmuch-specific auxiliary functions and
+variables which are useful in writing tests:
 
   generate_message
 
@@ -212,3 +212,15 @@ writing tests:
     will initialize the mail database to a known state of 50 sample
     messages, (culled from the early history of the notmuch mailing
     list).
+
+  notmuch_counter_reset
+  $notmuch_counter_command
+  notmuch_counter_value
+
+    These allow to count how many times notmuch binary is called.
+    notmuch_counter_reset() function generates a script that counts
+    how many times it is called and resets the counter to zero.  The
+    function sets $notmuch_counter_command variable to the path to the
+    generated script that should be called instead of notmuch to do
+    the counting.  The notmuch_counter_value() function prints the
+    current counter value.
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 11e6646..7798e21 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -919,6 +919,38 @@ test_emacs () {
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
+# Creates a script that counts how much time it is executed and calls
+# notmuch.  $notmuch_counter_command is set to the path to the
+# generated script.  Use notmuch_counter_value() function to get the
+# current counter value.
+notmuch_counter_reset () {
+	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
+	if [ ! -x "$notmuch_counter_command" ]; then
+		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
+		cat >"$notmuch_counter_command" <<EOF || return
+#!/bin/sh
+
+read count < "$notmuch_counter_state_path"
+echo \$(expr \$count + 1) > "$notmuch_counter_state_path"
+
+exec notmuch "\$@"
+EOF
+		chmod +x "$notmuch_counter_command" || return
+	fi
+
+	echo 0 > "$notmuch_counter_state_path"
+}
+
+# Returns the current notmuch counter value.
+notmuch_counter_value () {
+	if [ -r "$notmuch_counter_state_path" ]; then
+		read count < "$notmuch_counter_state_path"
+	else
+		count=0
+	fi
+	echo $count
+}
+
 test_reset_state_ () {
 	test -z "$test_init_done_" && test_init_
 
-- 
1.7.7.3

[PATCH v3 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts

[Dmitry Kurochkin]

The patch adds two new test cases:

* Do not call notmuch for non-inlinable application/mpeg parts
* Do not call notmuch for non-inlinable audio/mpeg parts

The application/mpeg test passes thanks to a workaround for
application/* Content-Types.  The audio/mpeg is currently broken.
---
 test/emacs |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 3f8c72d..bdac84f 100755
--- a/test/emacs
+++ b/test/emacs
@@ -472,4 +472,35 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
 	    (test-visible-output)'
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Do not call notmuch for non-inlinable application/mpeg parts"
+id='message-with-application/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with application/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"application/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter_value) 1
+
+test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
+test_subtest_known_broken
+id='message-with-audio/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with audio/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"audio/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter_value) 1
+
 test_done
-- 
1.7.7.3

[PATCH v3 3/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Before the change, there was a workaround to avoid notmuch show calls
for parts with application/* Content-Type.  But non-inlinable parts
are not limited to this Content-Type (e.g. mp3 files have audio/mpeg
Content-Type and are not inlinable).  For such parts
`notmuch-show-insert-part-*/*' handler is called which unconditionally
fetches contents for all parts.

The patch moves content fetching from `notmuch-show-insert-part-*/*'
to `notmuch-show-mm-display-part-inline' function after MIME inlinable
checks are done to avoid useless notmuch show calls.  The
application/* hack is no longer needed and removed.
---
 emacs/notmuch-show.el |   17 +++++------------
 test/emacs            |    1 -
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d7fbbca..d2d4968 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -320,17 +320,17 @@ message at DEPTH in the current thread."
 	;; ange-ftp, which is reasonable to use here.
 	(mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t)))))
 
-(defun notmuch-show-mm-display-part-inline (msg part content-type content)
+(defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (insert content)
       (let ((handle (mm-make-handle (current-buffer) (list content-type))))
-	(set-buffer display-buffer)
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
-	    (progn
+	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
+	      (insert content)
+	      (set-buffer display-buffer)
 	      (mm-display-part handle)
 	      t)
 	  nil)))))
@@ -588,17 +588,10 @@ current buffer, if possible."
 		nil))
 	  nil))))
 
-(defun notmuch-show-insert-part-application/* (msg part content-type nth depth declared-type
-)
-  ;; do not render random "application" parts
-  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename)))
-
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-  (let ((content (notmuch-show-get-bodypart-content msg part nth)))
-    (if content
-	(notmuch-show-mm-display-part-inline msg part content-type content)))
+  (notmuch-show-mm-display-part-inline msg part nth content-type)
   t)
 
 ;; Functions for determining how to handle MIME parts.
diff --git a/test/emacs b/test/emacs
index bdac84f..42e5d61 100755
--- a/test/emacs
+++ b/test/emacs
@@ -488,7 +488,6 @@ test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
 test_expect_equal $(notmuch_counter_value) 1
 
 test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
-test_subtest_known_broken
 id='message-with-audio/mpeg-attachment@notmuchmail.org'
 emacs_deliver_message \
     'Message with audio/mpeg attachment' \
-- 
1.7.7.3

[PATCH v4 0/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Changes:

v4 since v3:

* Use $(()) instead of expr(1) for math.

v3 since v2:

* Use read function instead of cat(1) to read counter value.
* Add a newline after the count value in state file and
  notmuch_counter_value() output.

v2 since v1:

* Rename $notmuch_counter variable to $notmuch_counter_command.
* Rename notmuch_counter() function to notmuch_counter_value().
* Fix documentation for notmuch_counter_value().
* Removed unneeded and add missing "|| return" in notmuch_counter_reset().
* Coding style fixes.

[PATCH v4 1/3] test: add functions to count how much times notmuch was called

[Dmitry Kurochkin]

The patch adds two auxiliary functions and a variable:

  notmuch_counter_reset
  $notmuch_counter_command
  notmuch_counter_value

They allow to count how many times notmuch binary is called.
notmuch_counter_reset() function generates a script that counts how
many times it is called and resets the counter to zero.  The function
sets $notmuch_counter_command variable to the path to the generated
script that should be called instead of notmuch to do the counting.
The notmuch_counter_value() function returns the current counter
value.
---
 test/README      |   16 ++++++++++++++--
 test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/test/README b/test/README
index 2481f16..5dc0638 100644
--- a/test/README
+++ b/test/README
@@ -187,8 +187,8 @@ library for your script to use.
    is to summarize successes and failures in the test script and
    exit with an appropriate error code.
 
-There are also a number of mail-specific functions which are useful in
-writing tests:
+There are also a number of notmuch-specific auxiliary functions and
+variables which are useful in writing tests:
 
   generate_message
 
@@ -212,3 +212,15 @@ writing tests:
     will initialize the mail database to a known state of 50 sample
     messages, (culled from the early history of the notmuch mailing
     list).
+
+  notmuch_counter_reset
+  $notmuch_counter_command
+  notmuch_counter_value
+
+    These allow to count how many times notmuch binary is called.
+    notmuch_counter_reset() function generates a script that counts
+    how many times it is called and resets the counter to zero.  The
+    function sets $notmuch_counter_command variable to the path to the
+    generated script that should be called instead of notmuch to do
+    the counting.  The notmuch_counter_value() function prints the
+    current counter value.
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 11e6646..5f5e67e 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -919,6 +919,38 @@ test_emacs () {
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
+# Creates a script that counts how much time it is executed and calls
+# notmuch.  $notmuch_counter_command is set to the path to the
+# generated script.  Use notmuch_counter_value() function to get the
+# current counter value.
+notmuch_counter_reset () {
+	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
+	if [ ! -x "$notmuch_counter_command" ]; then
+		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
+		cat >"$notmuch_counter_command" <<EOF || return
+#!/bin/sh
+
+read count < "$notmuch_counter_state_path"
+echo \$((count + 1)) > "$notmuch_counter_state_path"
+
+exec notmuch "\$@"
+EOF
+		chmod +x "$notmuch_counter_command" || return
+	fi
+
+	echo 0 > "$notmuch_counter_state_path"
+}
+
+# Returns the current notmuch counter value.
+notmuch_counter_value () {
+	if [ -r "$notmuch_counter_state_path" ]; then
+		read count < "$notmuch_counter_state_path"
+	else
+		count=0
+	fi
+	echo $count
+}
+
 test_reset_state_ () {
 	test -z "$test_init_done_" && test_init_
 
-- 
1.7.7.3

Re: [PATCH v4 1/3] test: add functions to count how much times notmuch was called

[Tomi Ollila]

On Wed, 30 Nov 2011 01:19:52 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> The patch adds two auxiliary functions and a variable:
> 
>   notmuch_counter_reset
>   $notmuch_counter_command
>   notmuch_counter_value
> 
> They allow to count how many times notmuch binary is called.
> notmuch_counter_reset() function generates a script that counts how
> many times it is called and resets the counter to zero.  The function
> sets $notmuch_counter_command variable to the path to the generated
> script that should be called instead of notmuch to do the counting.
> The notmuch_counter_value() function returns the current counter
> value.

LGTM

Tomi

> ---
>  test/README      |   16 ++++++++++++++--
>  test/test-lib.sh |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/test/README b/test/README
> index 2481f16..5dc0638 100644
> --- a/test/README
> +++ b/test/README
> @@ -187,8 +187,8 @@ library for your script to use.
>     is to summarize successes and failures in the test script and
>     exit with an appropriate error code.
>  
> -There are also a number of mail-specific functions which are useful in
> -writing tests:
> +There are also a number of notmuch-specific auxiliary functions and
> +variables which are useful in writing tests:
>  
>    generate_message
>  
> @@ -212,3 +212,15 @@ writing tests:
>      will initialize the mail database to a known state of 50 sample
>      messages, (culled from the early history of the notmuch mailing
>      list).
> +
> +  notmuch_counter_reset
> +  $notmuch_counter_command
> +  notmuch_counter_value
> +
> +    These allow to count how many times notmuch binary is called.
> +    notmuch_counter_reset() function generates a script that counts
> +    how many times it is called and resets the counter to zero.  The
> +    function sets $notmuch_counter_command variable to the path to the
> +    generated script that should be called instead of notmuch to do
> +    the counting.  The notmuch_counter_value() function prints the
> +    current counter value.
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 11e6646..5f5e67e 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -919,6 +919,38 @@ test_emacs () {
>  	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>  }
>  
> +# Creates a script that counts how much time it is executed and calls
> +# notmuch.  $notmuch_counter_command is set to the path to the
> +# generated script.  Use notmuch_counter_value() function to get the
> +# current counter value.
> +notmuch_counter_reset () {
> +	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
> +	if [ ! -x "$notmuch_counter_command" ]; then
> +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> +		cat >"$notmuch_counter_command" <<EOF || return
> +#!/bin/sh
> +
> +read count < "$notmuch_counter_state_path"
> +echo \$((count + 1)) > "$notmuch_counter_state_path"
> +
> +exec notmuch "\$@"
> +EOF
> +		chmod +x "$notmuch_counter_command" || return
> +	fi
> +
> +	echo 0 > "$notmuch_counter_state_path"
> +}
> +
> +# Returns the current notmuch counter value.
> +notmuch_counter_value () {
> +	if [ -r "$notmuch_counter_state_path" ]; then
> +		read count < "$notmuch_counter_state_path"
> +	else
> +		count=0
> +	fi
> +	echo $count
> +}
> +
>  test_reset_state_ () {
>  	test -z "$test_init_done_" && test_init_
>  
> -- 
> 1.7.7.3

[PATCH v4 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts

[Dmitry Kurochkin]

The patch adds two new test cases:

* Do not call notmuch for non-inlinable application/mpeg parts
* Do not call notmuch for non-inlinable audio/mpeg parts

The application/mpeg test passes thanks to a workaround for
application/* Content-Types.  The audio/mpeg is currently broken.
---
 test/emacs |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index 3f8c72d..bdac84f 100755
--- a/test/emacs
+++ b/test/emacs
@@ -472,4 +472,35 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
 	    (test-visible-output)'
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "Do not call notmuch for non-inlinable application/mpeg parts"
+id='message-with-application/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with application/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"application/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter_value) 1
+
+test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
+test_subtest_known_broken
+id='message-with-audio/mpeg-attachment@notmuchmail.org'
+emacs_deliver_message \
+    'Message with audio/mpeg attachment' \
+    '' \
+    "(message-goto-eoh)
+     (insert \"Message-ID: <$id>\n\")
+     (message-goto-body)
+     (mml-insert-part \"audio/mpeg\")
+     (insert \"a fake mp3 file\")"
+notmuch_counter_reset
+test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
+	      (notmuch-show \"id:$id\"))"
+test_expect_equal $(notmuch_counter_value) 1
+
 test_done
-- 
1.7.7.3

[PATCH v4 3/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Before the change, there was a workaround to avoid notmuch show calls
for parts with application/* Content-Type.  But non-inlinable parts
are not limited to this Content-Type (e.g. mp3 files have audio/mpeg
Content-Type and are not inlinable).  For such parts
`notmuch-show-insert-part-*/*' handler is called which unconditionally
fetches contents for all parts.

The patch moves content fetching from `notmuch-show-insert-part-*/*'
to `notmuch-show-mm-display-part-inline' function after MIME inlinable
checks are done to avoid useless notmuch show calls.  The
application/* hack is no longer needed and removed.
---
 emacs/notmuch-show.el |   17 +++++------------
 test/emacs            |    1 -
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d7fbbca..d2d4968 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -320,17 +320,17 @@ message at DEPTH in the current thread."
 	;; ange-ftp, which is reasonable to use here.
 	(mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t)))))
 
-(defun notmuch-show-mm-display-part-inline (msg part content-type content)
+(defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (insert content)
       (let ((handle (mm-make-handle (current-buffer) (list content-type))))
-	(set-buffer display-buffer)
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
-	    (progn
+	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
+	      (insert content)
+	      (set-buffer display-buffer)
 	      (mm-display-part handle)
 	      t)
 	  nil)))))
@@ -588,17 +588,10 @@ current buffer, if possible."
 		nil))
 	  nil))))
 
-(defun notmuch-show-insert-part-application/* (msg part content-type nth depth declared-type
-)
-  ;; do not render random "application" parts
-  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename)))
-
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-  (let ((content (notmuch-show-get-bodypart-content msg part nth)))
-    (if content
-	(notmuch-show-mm-display-part-inline msg part content-type content)))
+  (notmuch-show-mm-display-part-inline msg part nth content-type)
   t)
 
 ;; Functions for determining how to handle MIME parts.
diff --git a/test/emacs b/test/emacs
index bdac84f..42e5d61 100755
--- a/test/emacs
+++ b/test/emacs
@@ -488,7 +488,6 @@ test_emacs "(let ((notmuch-command \"$notmuch_counter_command\"))
 test_expect_equal $(notmuch_counter_value) 1
 
 test_begin_subtest "Do not call notmuch for non-inlinable audio/mpeg parts"
-test_subtest_known_broken
 id='message-with-audio/mpeg-attachment@notmuchmail.org'
 emacs_deliver_message \
     'Message with audio/mpeg attachment' \
-- 
1.7.7.3

Re: [PATCH v4 0/3] emacs: do not call notmuch show for non-inlinable parts

[Dmitry Kurochkin]

Hi David.

How about pushing this series?  Just a humble ping :)

Regards,
  Dmitry

Re: [PATCH v4 0/3] emacs: do not call notmuch show for non-inlinable parts

[David Bremner]

On Wed, 30 Nov 2011 01:19:51 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Changes:
> 
> v4 since v3:
> 
> * Use $(()) instead of expr(1) for math.
> 

pushed 

d