unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH 1/3] test: add functions to count how much times notmuch was called
@ 2011-11-26  1:44 Dmitry Kurochkin
  2011-11-26  1:44 ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
                   ` (5 more replies)
  0 siblings, 6 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-26  1:44 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts
  2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
@ 2011-11-26  1:44 ` Dmitry Kurochkin
  2011-11-28  2:46   ` Austin Clements
  2011-11-26  1:44 ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-26  1:44 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
  2011-11-26  1:44 ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
@ 2011-11-26  1:44 ` Dmitry Kurochkin
  2011-11-28  2:54   ` Austin Clements
  2011-11-28  2:44 ` [PATCH 1/3] test: add functions to count how much times notmuch was called Austin Clements
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-26  1:44 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
  2011-11-26  1:44 ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
  2011-11-26  1:44 ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
@ 2011-11-28  2:44 ` Austin Clements
  2011-11-28  3:09   ` Dmitry Kurochkin
  2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Austin Clements @ 2011-11-28  2:44 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts
  2011-11-26  1:44 ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
@ 2011-11-28  2:46   ` Austin Clements
  0 siblings, 0 replies; 30+ messages in thread
From: Austin Clements @ 2011-11-28  2:46 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-26  1:44 ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
@ 2011-11-28  2:54   ` Austin Clements
  0 siblings, 0 replies; 30+ messages in thread
From: Austin Clements @ 2011-11-28  2:54 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-28  2:44 ` [PATCH 1/3] test: add functions to count how much times notmuch was called Austin Clements
@ 2011-11-28  3:09   ` Dmitry Kurochkin
  0 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28  3:09 UTC (permalink / raw)
  To: Austin Clements, notmuch

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
> > 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
                   ` (2 preceding siblings ...)
  2011-11-28  2:44 ` [PATCH 1/3] test: add functions to count how much times notmuch was called Austin Clements
@ 2011-11-28  3:28 ` Dmitry Kurochkin
  2011-11-28  3:28   ` [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
                     ` (3 more replies)
  2011-11-28 21:13 ` [PATCH v3 " Dmitry Kurochkin
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
  5 siblings, 4 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28  3:28 UTC (permalink / raw)
  To: notmuch

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"

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
@ 2011-11-28  3:28   ` Dmitry Kurochkin
  2011-11-28 20:42     ` Tomi Ollila
  2011-11-28  3:28   ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28  3:28 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts
  2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
  2011-11-28  3:28   ` [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
@ 2011-11-28  3:28   ` Dmitry Kurochkin
  2011-11-28  3:28   ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
  2011-11-28  3:39   ` [PATCH 0/3] " Austin Clements
  3 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28  3:28 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
  2011-11-28  3:28   ` [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
  2011-11-28  3:28   ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
@ 2011-11-28  3:28   ` Dmitry Kurochkin
  2011-11-28  3:39   ` [PATCH 0/3] " Austin Clements
  3 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28  3:28 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
                     ` (2 preceding siblings ...)
  2011-11-28  3:28   ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
@ 2011-11-28  3:39   ` Austin Clements
  2011-11-28 14:24     ` Jameson Graef Rollins
  3 siblings, 1 reply; 30+ messages in thread
From: Austin Clements @ 2011-11-28  3:39 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

LGTM

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-28  3:39   ` [PATCH 0/3] " Austin Clements
@ 2011-11-28 14:24     ` Jameson Graef Rollins
  0 siblings, 0 replies; 30+ messages in thread
From: Jameson Graef Rollins @ 2011-11-28 14:24 UTC (permalink / raw)
  To: Austin Clements, Dmitry Kurochkin, notmuch

[-- 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 --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-28  3:28   ` [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
@ 2011-11-28 20:42     ` Tomi Ollila
  2011-11-28 21:26       ` Dmitry Kurochkin
  0 siblings, 1 reply; 30+ messages in thread
From: Tomi Ollila @ 2011-11-28 20:42 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
                   ` (3 preceding siblings ...)
  2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
@ 2011-11-28 21:13 ` Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
                     ` (2 more replies)
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
  5 siblings, 3 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28 21:13 UTC (permalink / raw)
  To: notmuch

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"

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 1/3] test: add functions to count how much times notmuch was called
  2011-11-28 21:13 ` [PATCH v3 " Dmitry Kurochkin
@ 2011-11-28 21:13   ` Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
  2 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28 21:13 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v3 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts
  2011-11-28 21:13 ` [PATCH v3 " Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
@ 2011-11-28 21:13   ` Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
  2 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28 21:13 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v3 3/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-28 21:13 ` [PATCH v3 " Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
  2011-11-28 21:13   ` [PATCH v3 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
@ 2011-11-28 21:13   ` Dmitry Kurochkin
  2 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28 21:13 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-28 20:42     ` Tomi Ollila
@ 2011-11-28 21:26       ` Dmitry Kurochkin
  2011-11-29 12:58         ` Tomi Ollila
  2011-11-29 14:40         ` Jameson Graef Rollins
  0 siblings, 2 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-28 21:26 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-28 21:26       ` Dmitry Kurochkin
@ 2011-11-29 12:58         ` Tomi Ollila
  2011-11-29 21:03           ` Dmitry Kurochkin
  2011-11-29 14:40         ` Jameson Graef Rollins
  1 sibling, 1 reply; 30+ messages in thread
From: Tomi Ollila @ 2011-11-29 12:58 UTC (permalink / raw)
  To: Dmitry Kurochkin, Tomi Ollila, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-28 21:26       ` Dmitry Kurochkin
  2011-11-29 12:58         ` Tomi Ollila
@ 2011-11-29 14:40         ` Jameson Graef Rollins
  1 sibling, 0 replies; 30+ messages in thread
From: Jameson Graef Rollins @ 2011-11-29 14:40 UTC (permalink / raw)
  To: Dmitry Kurochkin, Tomi Ollila, notmuch

[-- 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 --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-29 12:58         ` Tomi Ollila
@ 2011-11-29 21:03           ` Dmitry Kurochkin
  2011-11-30 11:53             ` Tomi Ollila
  0 siblings, 1 reply; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-29 21:03 UTC (permalink / raw)
  To: Tomi Ollila, Tomi Ollila, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v4 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
                   ` (4 preceding siblings ...)
  2011-11-28 21:13 ` [PATCH v3 " Dmitry Kurochkin
@ 2011-11-29 21:19 ` Dmitry Kurochkin
  2011-11-29 21:19   ` [PATCH v4 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
                     ` (4 more replies)
  5 siblings, 5 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-29 21:19 UTC (permalink / raw)
  To: notmuch

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v4 1/3] test: add functions to count how much times notmuch was called
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
@ 2011-11-29 21:19   ` Dmitry Kurochkin
  2011-11-30 11:57     ` Tomi Ollila
  2011-11-29 21:19   ` [PATCH v4 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-29 21:19 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v4 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
  2011-11-29 21:19   ` [PATCH v4 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
@ 2011-11-29 21:19   ` Dmitry Kurochkin
  2011-11-29 21:19   ` [PATCH v4 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-29 21:19 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v4 3/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
  2011-11-29 21:19   ` [PATCH v4 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
  2011-11-29 21:19   ` [PATCH v4 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
@ 2011-11-29 21:19   ` Dmitry Kurochkin
  2011-12-07 15:11   ` [PATCH v4 0/3] " Dmitry Kurochkin
  2011-12-08  1:00   ` David Bremner
  4 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-11-29 21:19 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] test: add functions to count how much times notmuch was called
  2011-11-29 21:03           ` Dmitry Kurochkin
@ 2011-11-30 11:53             ` Tomi Ollila
  0 siblings, 0 replies; 30+ messages in thread
From: Tomi Ollila @ 2011-11-30 11:53 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 1/3] test: add functions to count how much times notmuch was called
  2011-11-29 21:19   ` [PATCH v4 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
@ 2011-11-30 11:57     ` Tomi Ollila
  0 siblings, 0 replies; 30+ messages in thread
From: Tomi Ollila @ 2011-11-30 11:57 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
                     ` (2 preceding siblings ...)
  2011-11-29 21:19   ` [PATCH v4 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
@ 2011-12-07 15:11   ` Dmitry Kurochkin
  2011-12-08  1:00   ` David Bremner
  4 siblings, 0 replies; 30+ messages in thread
From: Dmitry Kurochkin @ 2011-12-07 15:11 UTC (permalink / raw)
  To: David Bremner; +Cc: notmuch

Hi David.

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

Regards,
  Dmitry

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v4 0/3] emacs: do not call notmuch show for non-inlinable parts
  2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
                     ` (3 preceding siblings ...)
  2011-12-07 15:11   ` [PATCH v4 0/3] " Dmitry Kurochkin
@ 2011-12-08  1:00   ` David Bremner
  4 siblings, 0 replies; 30+ messages in thread
From: David Bremner @ 2011-12-08  1:00 UTC (permalink / raw)
  To: Dmitry Kurochkin, notmuch

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2011-12-08  1:00 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-26  1:44 [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
2011-11-26  1:44 ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
2011-11-28  2:46   ` Austin Clements
2011-11-26  1:44 ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
2011-11-28  2:54   ` Austin Clements
2011-11-28  2:44 ` [PATCH 1/3] test: add functions to count how much times notmuch was called Austin Clements
2011-11-28  3:09   ` Dmitry Kurochkin
2011-11-28  3:28 ` [PATCH 0/3] emacs: do not call notmuch show for non-inlinable parts Dmitry Kurochkin
2011-11-28  3:28   ` [PATCH 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
2011-11-28 20:42     ` Tomi Ollila
2011-11-28 21:26       ` Dmitry Kurochkin
2011-11-29 12:58         ` Tomi Ollila
2011-11-29 21:03           ` Dmitry Kurochkin
2011-11-30 11:53             ` Tomi Ollila
2011-11-29 14:40         ` Jameson Graef Rollins
2011-11-28  3:28   ` [PATCH 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
2011-11-28  3:28   ` [PATCH 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
2011-11-28  3:39   ` [PATCH 0/3] " Austin Clements
2011-11-28 14:24     ` Jameson Graef Rollins
2011-11-28 21:13 ` [PATCH v3 " Dmitry Kurochkin
2011-11-28 21:13   ` [PATCH v3 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
2011-11-28 21:13   ` [PATCH v3 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
2011-11-28 21:13   ` [PATCH v3 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
2011-11-29 21:19 ` [PATCH v4 0/3] " Dmitry Kurochkin
2011-11-29 21:19   ` [PATCH v4 1/3] test: add functions to count how much times notmuch was called Dmitry Kurochkin
2011-11-30 11:57     ` Tomi Ollila
2011-11-29 21:19   ` [PATCH v4 2/3] test: check that Emacs UI does not call notmuch for non-inlinable parts Dmitry Kurochkin
2011-11-29 21:19   ` [PATCH v4 3/3] emacs: do not call notmuch show " Dmitry Kurochkin
2011-12-07 15:11   ` [PATCH v4 0/3] " Dmitry Kurochkin
2011-12-08  1:00   ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).