[PATCH] test: optionally print subtest number

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 15:56:47 -0400, David Bremner  wrote:
> From: David Bremner 
> 
> The idea is that $test_count could be used in tests to label
> intermediate files. The output enabled by this patch (and --debug)
> helps figure out which OUTPUT.nn file belongs to which test in case
> several subtests write to OUTPUT.$test_count
> ---
> 
> Is there something that depends on the test format? I find it pretty
> handy to have the subtest numbers, but I don't want to break some
> other tools. I followed the existing style of conditionally defining
> functions, but maybe someone with more bash-fu can improve that.
> 

Looks good to me.  Except for tabs taking too much space.  Perhaps the
following would be better?

printf "%-4s" "[$(($test_count - 1))]"

Regards,
  Dmitry

>  test/test-lib.sh |   12 
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 6be93fe..f5c 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -116,6 +116,16 @@ do
>   esac
>  done
>  
> +if test -n "$debug"; then
> +print_subtest () {
> + printf "\t[%d]\t" $(($test_count - 1))
> +}
> +else
> +print_subtest () {
> + true
> +}
> +fi
> +
>  if test -n "$color"; then
>   say_color () {
>   (
> @@ -132,6 +142,7 @@ if test -n "$color"; then
>   printf " "
>  printf "$@"
>   tput sgr0
> + print_subtest
>   )
>   }
>  else
> @@ -140,6 +151,7 @@ else
>   shift
>   printf " "
>  printf "$@"
> + print_subtest
>   }
>  fi
>  
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] notmuch: unref charset_filter (once more) to fix one memory leak

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 20:18:48 +0200, tomi.ollila at iki.fi wrote:
> From: Tomi Ollila 
> 
> In my test case added g_object_unref(charset_filter) reduces memory
> consumption over 90% when 'notmuch show --format=text "*"' is
> executed (~11000 messages, RES ~330M -> ~25M).
> ---
> Thanks Dmitry. I did not realize unref unreferences, does not deallocate
> memory/data structures. g_mime_stream_filter_add() takes an additional
> reference to the charset_filter object.

Looks good to me.

Regards,
  Dmitry

>  notmuch-show.c |5 -
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 873a7c4..8da3295 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -471,9 +471,12 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>   /* This result can be NULL for things like "unknown-8bit".
>* Don't set a NULL filter as that makes GMime print
>* annoying assertion-failure messages on stderr. */
> - if (charset_filter)
> + if (charset_filter) {
>   g_mime_stream_filter_add (GMIME_STREAM_FILTER (stream_filter),
> charset_filter);
> + g_object_unref (charset_filter);
> + }
> +
>  }
>  
>  wrapper = g_mime_part_get_content_object (GMIME_PART (part));
> -- 
> 1.7.6.1
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v3 4/4] emacs: add notmuch-hello-hook

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 18:32:12 +0100, Thomas Jost  
wrote:
> This hook is called every time the notmuch-hello buffer is updated.
> ---
>  emacs/notmuch-hello.el |9 -
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 0fe9c1d..112b40b 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -131,6 +131,11 @@ So:
> (integer :tag "Number of characters")
> (float :tag "Fraction of window")))
>  
> +(defcustom notmuch-hello-hook nil
> +  "Functions called after populating a `notmuch-hello' buffer."
> +  :type 'hook
> +  :group 'notmuch)
> +
>  (defcustom notmuch-thousands-separator ","
>"The string used as a thousands separator.
>  
> @@ -579,7 +584,9 @@ Complete list of currently available key bindings:
> (widget-forward 1)))
>  
>(unless (widget-at)
> - (notmuch-hello-goto-search)
> + (notmuch-hello-goto-search))
> +
> +  (run-hooks 'notmuch-hello-hook
>  

I spent some time finding out why run-hooks are not on the top level.
Turns out it is inside two let statements.  Can we move to the top level
of `notmuch-hello' to make it clear that it is always run (i.e. there
are no ifs or whens)?

BTW this would replace one of the oldest custom patches which I have in
my master branch :)

Regards,
  Dmitry

>  (defun notmuch-folder ()
>"Deprecated function for invoking notmuch---calling `notmuch' is preferred 
> now."
> -- 
> 1.7.8
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello

[Dmitry Kurochkin]

Hi Daniel.

I have finished reviewing this patch at last.  Sorry, it is a bit messy.
Overall, I like the patch.  It is a very nice improvement.

I am sure I have missed some important points, but I guess this is the
best I can do right now.  Perhaps I will find more comments for the next
version of the patch :)

As we already discussed on IRC, there are some trailing whitespaces to
cleanup.

Here is the review:

+(defvar notmuch-custom-section-options

s/notmuch-custom-section-options/notmuch-hello-custom-section-options/ for 
consistency?

+(:filter-count (string :tag "Different filter message counts"))

It was not clear to me what this option is for from the docstring.
Perhaps something like: "Count query filter, if different from :filter"?

+(:initially-hidden (const :tag "Hide this on startup?" t))

"This" refers to section, right?  If yes, let's state it explicitly:
"Hide this section on startup".  Also, we should probably remove the
question mark, or add it to other options for consistency.

Should the default be to show all sections?

+(:hide-if-empty (const :tag "Hide if empty" t)))

As I understand, this controls whether the whole sections is visible.
It is not clear what "if empty" means.  Does it mean that all queries
are empty?  Or all queries are empty and :show-empty-sections is
false?  Consider changing to something like: "Hide this section if all
queries are empty [and hidden]".

+  `(list :tag ""
+(const :tag "" notmuch-hello-insert-query-list)

Do we need to explicitly specify empty tags?  Aren't they empty by
default?

+  :tag "Customized tag-list (see docstring for details)"
+  :tag "Customized queries section (see docstring for details)"

Perhaps it would be more useful to add reference to
`notmuch-hello-sections'?  I.e. "see `notmuch-hello-sections' for
details.

Please s/Customized tag-list/Customized tag-list section/ everywhere for
consistency (or remove section from "Customized queries section").

+Each entry of this list should be a function of no arguments that
+should return if `notmuch-hello-target' is produced as part of its
+output and nil otherwise.

Something is missing between "return if".  IMO it is really hard to
understand what the function should actually do and what it should
return.  Are this functions expected to add section content to current
position?  As I understand, the return value indicates whether cursor
should be positioned somewhere inside this section.  It is a minor
detail, but it is described in the first (and complex sentence) as if
it was the most important part.  Consider moving the return and "no
arguments" to the 3rd paragraph which describes details about the
functions.  I would also swap 2nd and 3rd paragraph.  Smth like:

  The list contains functions which are used to construct sections in
  notmuch-hello buffer.  When notmuch-hello buffer is constructed,
  these functions are run in the order they appear in this list.  Each
  function produces a section simply by adding content to the current
  buffer.  A section should not end with an empty line, because a
  newline will be inserted after each section by `notmuch-hello'.

  Each function should take no arguments.  If the produced section
  includes `notmuch-hello-target' (i.e. cursor should be positioned
  inside this section), the function should return [something].
  Otherwise, it should return nil.

  For convenience an element can also be a list of the form (FUNC ARG1
  ARG2 .. ARGN) in which case FUNC will be applied to the rest of the
  list.

  [ details about customized tag-list and queries sections ]

This is just a draft.  Feel free to use it or ignore it.

+ For convenience an element can also be

Remove space the leading space and do `fill-paragraph'.

+   (function :tag "Custom function"

Perhaps "Custom section" would be more accurate?

+  "Button at position of point before rebuilding the notmuch-buffer

Missing dot at the end.

s/Button/Button text/?

+This variable contains the string of the button, if any, the

s/the string/text/ or label?

+rebuilt. This is never actually set globally and defined as a

s/is never actually set/should never be set/?

+(defvar notmuch-hello-hidden-sections nil
+  "List of query section titles whose contents are hidden")

Is this really for query sections only?

Does this duplicate :initially-hidden option from
notmuch-custom-section-options?

How about adding a global alist variable notmuch-hello-state to store
the state needed for section functions?  Currently, it would contain
two values: :first-run and :target.  This would allow us to add more
state variables in the future without polluting the global namespace.
Also, it would make clear what variables are section function are
supposed to use and perhaps even change (docstring should explain that
of course).

`notmuch-hello-filtered-query':

+  (and result (concat query " and (" result ")"

How about using the result as query instead of filter?  I.e.

[PATCH] notmuch: unref charset_filter to fix one memory leak

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 14:03:33 +0200, Tomi Ollila  wrote:
> In my use case g_object_unref(charset_filter) reduces memory
> consumption over 90% when 'notmuch show --format=text "*"' is
> executed (~11000 messages, RES ~330M -> ~25M).
> ---
>  notmuch-show.c |4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 873a7c4..23d7368 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -450,6 +450,7 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>  {
>  GMimeContentType *content_type = g_mime_object_get_content_type 
> (GMIME_OBJECT (part));
>  GMimeStream *stream_filter = NULL;
> +GMimeFilter *charset_filter = NULL;
>  GMimeDataWrapper *wrapper;
>  const char *charset;
>  
> @@ -466,7 +467,6 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>  
>  charset = g_mime_object_get_content_type_parameter (part, "charset");
>  if (charset) {
> - GMimeFilter *charset_filter;
>   charset_filter = g_mime_filter_charset_new (charset, "UTF-8");
>   /* This result can be NULL for things like "unknown-8bit".
>* Don't set a NULL filter as that makes GMime print
> @@ -479,6 +479,8 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>  wrapper = g_mime_part_get_content_object (GMIME_PART (part));
>  if (wrapper && stream_filter)
>   g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
> +if (charset_filter)
> + g_object_unref (charset_filter);

Why can't we do this inside the if (charset) block?

Regards,
  Dmitry

>  if (stream_filter)
>   g_object_unref(stream_filter);
>  }
> -- 
> 1.7.6.1
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] test: optionally print subtest number

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 15:35:53 -0800, Jameson Graef Rollins 
 wrote:
> On Wed, 14 Dec 2011 03:24:23 +0400, Dmitry Kurochkin 
>  wrote:
> > BTW I have some plans to introduce optional explicit test ids that can
> > be used for inter-test dependencies.  E.g.:
> > 
> >   test_begin_subtest test-id-1 "A subtest"
> >   
> >   ;; in another test requre that test-id-1 passed
> >   test_require_subtest test-id-1
> 
> Would the required test need to be listed twice, both on the
> begin_subtest line *and* in the require_subtest line?
> 
> And again, why would the test id have to be any different that the
> existing test names?  The tests already have names, so I don't
> understand why we would want to introduce some other kind of
> identification.  Seems like it's just going to add extra confusion.
> 

What you listed in the other email are test scripts, each with many
subtests.  I was talking about dependencies between subtests, not test
scripts.

> And speaking of which, I sometimes worry that the test infrastructure
> itself is getting too complicated.  Pretty soon we're going to need
> tests for the tests.

We already have them :)  Though, pretty limited.

>  I don't necessarily see the need to all of these
> extra features in the test suite, so I worry that it's just making
> everything harder to debug.
> 

I hope we can keep balance here.

Without inter-subtest dependencies, we have unhealthy situation where
some tests may be skipped because of missing prerequisites, but test
that depend on them are failing.  The only alternative I see is to
rewrite these tests to remove the dependencies.  But that would
complicate test cases itself, so I believe inter-subtest dependencies is
a better option.

Regards,
  Dmitry

> jamie.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] test: optionally print subtest number

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 19:18:16 -0400, David Bremner  wrote:
> On Tue, 13 Dec 2011 14:22:21 -0800, Jameson Graef Rollins 
>  wrote:
> 
> > I've only been vaguely following this "test count" stuff, but I'm not
> > sure I understand what's the point of giving tests a number that is
> > ultimately mutable.  Why not just label things by the test name, instead
> > of the count?  That wouldn't require keeping track of number/name
> > mapping, which will change over time.
> 
> We don't actually have test names, at least not ones directly suitable
> for file names.  I guess we could encode them or something, is that what
> you mean?
> 

BTW I have some plans to introduce optional explicit test ids that can
be used for inter-test dependencies.  E.g.:

  test_begin_subtest test-id-1 "A subtest"
  
  ;; in another test requre that test-id-1 passed
  test_require_subtest test-id-1

Regards,
  Dmitry

> d
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] test: optionally print subtest number

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 22:55:18 +0200, Tomi Ollila  wrote:
> On Wed, 14 Dec 2011 00:15:43 +0400, Dmitry Kurochkin 
>  wrote:
> > On Tue, 13 Dec 2011 15:56:47 -0400, David Bremner  wrote:
> > > From: David Bremner 
> > > 
> > > The idea is that $test_count could be used in tests to label
> > > intermediate files. The output enabled by this patch (and --debug)
> > > helps figure out which OUTPUT.nn file belongs to which test in case
> > > several subtests write to OUTPUT.$test_count
> > > ---
> > > 
> > > Is there something that depends on the test format? I find it pretty
> > > handy to have the subtest numbers, but I don't want to break some
> > > other tools. I followed the existing style of conditionally defining
> > > functions, but maybe someone with more bash-fu can improve that.
> > > 
> > 
> > Looks good to me.  Except for tabs taking too much space.  Perhaps the
> > following would be better?
> > 
> > printf "%-4s" "[$(($test_count - 1))]"
> 
> I attempted the same size reduction. Therefore I know that
> this should to be either
> 
>   printf " %-4s" "[$(($test_count - 1))]"
> or
>   printf "\t%-4s" "[$(($test_count - 1))]"
> 
> (or something similar) so that there is space betweem BROKEN and [num]
> 

Oh, apparently, not enough testing on my side.

I vote for the first version with a space.

Regards,
  Dmitry

> This takes 4 bytes out from width (and drops tab as field separator)
> (and only few lines goes over 80 char width (some goes even with this
> reduction). So ...
> 
> 
> > Regards,
> >   Dmitry
> 
> Tomi
> 
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] test: optionally print subtest number

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 15:56:47 -0400, David Bremner  wrote:
> From: David Bremner 
> 
> The idea is that $test_count could be used in tests to label
> intermediate files. The output enabled by this patch (and --debug)
> helps figure out which OUTPUT.nn file belongs to which test in case
> several subtests write to OUTPUT.$test_count
> ---
> 
> Is there something that depends on the test format? I find it pretty
> handy to have the subtest numbers, but I don't want to break some
> other tools. I followed the existing style of conditionally defining
> functions, but maybe someone with more bash-fu can improve that.
> 

Looks good to me.  Except for tabs taking too much space.  Perhaps the
following would be better?

printf "%-4s" "[$(($test_count - 1))]"

Regards,
  Dmitry

>  test/test-lib.sh |   12 
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 6be93fe..f5c 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -116,6 +116,16 @@ do
>   esac
>  done
>  
> +if test -n "$debug"; then
> +print_subtest () {
> + printf "\t[%d]\t" $(($test_count - 1))
> +}
> +else
> +print_subtest () {
> + true
> +}
> +fi
> +
>  if test -n "$color"; then
>   say_color () {
>   (
> @@ -132,6 +142,7 @@ if test -n "$color"; then
>   printf " "
>  printf "$@"
>   tput sgr0
> + print_subtest
>   )
>   }
>  else
> @@ -140,6 +151,7 @@ else
>   shift
>   printf " "
>  printf "$@"
> + print_subtest
>   }
>  fi
>  
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] notmuch: unref charset_filter (once more) to fix one memory leak

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 20:18:48 +0200, tomi.oll...@iki.fi wrote:
> From: Tomi Ollila 
> 
> In my test case added g_object_unref(charset_filter) reduces memory
> consumption over 90% when 'notmuch show --format=text "*"' is
> executed (~11000 messages, RES ~330M -> ~25M).
> ---
> Thanks Dmitry. I did not realize unref unreferences, does not deallocate
> memory/data structures. g_mime_stream_filter_add() takes an additional
> reference to the charset_filter object.

Looks good to me.

Regards,
  Dmitry

>  notmuch-show.c |5 -
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 873a7c4..8da3295 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -471,9 +471,12 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>   /* This result can be NULL for things like "unknown-8bit".
>* Don't set a NULL filter as that makes GMime print
>* annoying assertion-failure messages on stderr. */
> - if (charset_filter)
> + if (charset_filter) {
>   g_mime_stream_filter_add (GMIME_STREAM_FILTER (stream_filter),
> charset_filter);
> + g_object_unref (charset_filter);
> + }
> +
>  }
>  
>  wrapper = g_mime_part_get_content_object (GMIME_PART (part));
> -- 
> 1.7.6.1
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 4/4] emacs: add notmuch-hello-hook

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 18:32:12 +0100, Thomas Jost  wrote:
> This hook is called every time the notmuch-hello buffer is updated.
> ---
>  emacs/notmuch-hello.el |9 -
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 0fe9c1d..112b40b 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -131,6 +131,11 @@ So:
> (integer :tag "Number of characters")
> (float :tag "Fraction of window")))
>  
> +(defcustom notmuch-hello-hook nil
> +  "Functions called after populating a `notmuch-hello' buffer."
> +  :type 'hook
> +  :group 'notmuch)
> +
>  (defcustom notmuch-thousands-separator ","
>"The string used as a thousands separator.
>  
> @@ -579,7 +584,9 @@ Complete list of currently available key bindings:
> (widget-forward 1)))
>  
>(unless (widget-at)
> - (notmuch-hello-goto-search)
> + (notmuch-hello-goto-search))
> +
> +  (run-hooks 'notmuch-hello-hook
>  

I spent some time finding out why run-hooks are not on the top level.
Turns out it is inside two let statements.  Can we move to the top level
of `notmuch-hello' to make it clear that it is always run (i.e. there
are no ifs or whens)?

BTW this would replace one of the oldest custom patches which I have in
my master branch :)

Regards,
  Dmitry

>  (defun notmuch-folder ()
>"Deprecated function for invoking notmuch---calling `notmuch' is preferred 
> now."
> -- 
> 1.7.8
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] notmuch: unref charset_filter to fix one memory leak

[Dmitry Kurochkin]

On Tue, 13 Dec 2011 14:03:33 +0200, Tomi Ollila  wrote:
> In my use case g_object_unref(charset_filter) reduces memory
> consumption over 90% when 'notmuch show --format=text "*"' is
> executed (~11000 messages, RES ~330M -> ~25M).
> ---
>  notmuch-show.c |4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 873a7c4..23d7368 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -450,6 +450,7 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>  {
>  GMimeContentType *content_type = g_mime_object_get_content_type 
> (GMIME_OBJECT (part));
>  GMimeStream *stream_filter = NULL;
> +GMimeFilter *charset_filter = NULL;
>  GMimeDataWrapper *wrapper;
>  const char *charset;
>  
> @@ -466,7 +467,6 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>  
>  charset = g_mime_object_get_content_type_parameter (part, "charset");
>  if (charset) {
> - GMimeFilter *charset_filter;
>   charset_filter = g_mime_filter_charset_new (charset, "UTF-8");
>   /* This result can be NULL for things like "unknown-8bit".
>* Don't set a NULL filter as that makes GMime print
> @@ -479,6 +479,8 @@ show_text_part_content (GMimeObject *part, GMimeStream 
> *stream_out)
>  wrapper = g_mime_part_get_content_object (GMIME_PART (part));
>  if (wrapper && stream_filter)
>   g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
> +if (charset_filter)
> + g_object_unref (charset_filter);

Why can't we do this inside the if (charset) block?

Regards,
  Dmitry

>  if (stream_filter)
>   g_object_unref(stream_filter);
>  }
> -- 
> 1.7.6.1
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 16:24:51 -0500, Austin Clements  wrote:
> Quoth Jani Nikula on Dec 12 at 11:13 pm:
> > On Tue, 13 Dec 2011 00:53:05 +0400, Dmitry Kurochkin  > gmail.com> wrote:
> > > On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula  
> > > wrote:
> > > > +If set to nil (the default), new mail is processed by invoking
> > > > +\"notmuch new\". Otherwise, this should be set to a string that
> > > > +gives the name of an external script that processes new mail. If
> > > > +set to the empty string, no command will be run.
> > > 
> > > I think this should be "an empty string".  But I may be mistaking.
> > 
> > Shameless copy paste from a native speaker, who am I to argue? :)
> > Austin?
> 
> Either way is grammatically correct.  Really, this is a philosophical
> question.  Can two empty strings have different identities?  Or is
> there only one empty string in the universe?
> 
> (eq "" "") => t
> (eq "" (string)) => t
> (eq "" (make-string 0 ?a)) => t
> (eq "" (substring "a" 1)) => t
> 
> It would appear Elisp is squarely in the "there is one empty string"
> camp, so "the empty string" would be more correct.

Fine with me then :)

Regards,
  Dmitry

[PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula  wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula 
> 
> ---
> 
> v3: only documentation changes suggested by Austin and Dmitry.
> ---
>  emacs/notmuch.el |   35 +--
>  1 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..675a110 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,43 @@ same relative position within the new buffer."
>  (notmuch-search query oldest-first target-thread target-line 
> continuation)
>  (goto-char (point-min
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>"An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If set to nil (the default), new mail is processed by invoking
> +\"notmuch new\". Otherwise, this should be set to a string that
> +gives the name of an external script that processes new mail. If
> +set to the empty string, no command will be run.

I think this should be "an empty string".  But I may be mistaking.

Regards,
  Dmitry

> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the recommended way of achieving the same is using
> +\"notmuch new\" hooks."
> +  :type '(choice (const :tag "notmuch new" nil)
> +  (const :tag "Disabled" "")
> +  (string :tag "Custom script"))
>:group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>(interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -  (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +  (if (not (string= notmuch-poll-script ""))
> +   (call-process notmuch-poll-script nil nil))
> +(call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>"Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v2] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

Hi Jani.

On Mon, 12 Dec 2011 21:57:28 +0200, Jani Nikula  wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula 

Looks good to me.  Few comments regarding documentation below.

> ---
>  emacs/notmuch.el |   34 --
>  1 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..5a8ab9d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,42 @@ same relative position within the new buffer."
>  (notmuch-search query oldest-first target-thread target-line 
> continuation)
>  (goto-char (point-min
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>"An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If this variable is non empty, then it should name an external

Please s/non empty/non-empty string/ to make it more clear.

> +script to be run. If set to an empty string, no action is
> +invoked. Finally, if set to nil (the default), \"notmuch new\" is
> +run using the command specified by `notmuch-command'.
> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the same can be achieved through \"notmuch new\" hooks."

s/though/using/?

Would be nice to mention that using hooks is the recommended way for
this.

> +  :type '(choice (const :tag "Notmuch new" nil)

Perhaps s/Notmuch/notmuch/ because we are talking about a command here?

Regards,
  Dmitry

> +  (const :tag "Disabled" "")
> +  (string :tag "Custom script"))
>:group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>(interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -  (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +  (if (not (string= notmuch-poll-script ""))
> +   (call-process notmuch-poll-script nil nil))
> +(call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>"Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 12:15:44 +0200, Tomi Ollila  wrote:
> On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements  
> wrote:
> > 
> > So here's another idea, prefaced with a rant.
> > 
> > It's bothered me for a long time that notmuch-emacs didn't just know
> > by default how to check for new mail.  What MUA doesn't know how to
> > check for new mail?  Why does a new user of notmuch have to tell it
> > how to check for new mail?  Of course, this *had* to be configured
> > before because everyone had their own way of checking for new mail.
> > Hooks eliminate this unnecessary flexibility and make "notmuch new"
> > the one true way to check for new mail---as it ought to be---and in
> > turn make the notmuch-poll-script variable obsolete.
> > 
> > So, what about changing the default "" setting of notmuch-poll-script
> > from meaning "do nothing and be useless" to meaning "run notmuch new
> > (using notmuch-command)"?  It will then automatically do the right
> > thing for new users, while still being backward-compatible and
> > allowing an escape hatch for bizarre situations.
> 
> +1
> 
> So, it could work like this:
> 
> (defun notmuch-poll ()
>   "FIX DOCSTRING"
>   (interactive)
>   (if (stringp notmuch notmuch-poll-script)
>   (if (string= notmuch-poll-script "")
> (call-process notmuch-command nil nil nil "new")
> (call-process notmuch-poll-script
> 
> I.e. in case notmuch-poll-script == nil, (or not string)
> do nothing. In case notmuch-poll-script == "" execute notmuch new
> and if notmuch-poll-script is string with content execute that.
> 

I think the following scheme would be slightly better and more
consistent:

* nil - run "notmuch new", the new default
* "" - do nothing
* "script" - run script

Regards,
  Dmitry

> users who want other functionality can reimplement notmuch-poll function
> after notmuch has been loaded (and manage things themselves when internal
> implementation changes ;().
> 
> 
> Tomi
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 16:24:51 -0500, Austin Clements  wrote:
> Quoth Jani Nikula on Dec 12 at 11:13 pm:
> > On Tue, 13 Dec 2011 00:53:05 +0400, Dmitry Kurochkin 
> >  wrote:
> > > On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula  wrote:
> > > > +If set to nil (the default), new mail is processed by invoking
> > > > +\"notmuch new\". Otherwise, this should be set to a string that
> > > > +gives the name of an external script that processes new mail. If
> > > > +set to the empty string, no command will be run.
> > > 
> > > I think this should be "an empty string".  But I may be mistaking.
> > 
> > Shameless copy paste from a native speaker, who am I to argue? :)
> > Austin?
> 
> Either way is grammatically correct.  Really, this is a philosophical
> question.  Can two empty strings have different identities?  Or is
> there only one empty string in the universe?
> 
> (eq "" "") => t
> (eq "" (string)) => t
> (eq "" (make-string 0 ?a)) => t
> (eq "" (substring "a" 1)) => t
> 
> It would appear Elisp is squarely in the "there is one empty string"
> camp, so "the empty string" would be more correct.

Fine with me then :)

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 22:50:04 +0200, Jani Nikula  wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula 
> 
> ---
> 
> v3: only documentation changes suggested by Austin and Dmitry.
> ---
>  emacs/notmuch.el |   35 +--
>  1 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..675a110 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,43 @@ same relative position within the new buffer."
>  (notmuch-search query oldest-first target-thread target-line 
> continuation)
>  (goto-char (point-min
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>"An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If set to nil (the default), new mail is processed by invoking
> +\"notmuch new\". Otherwise, this should be set to a string that
> +gives the name of an external script that processes new mail. If
> +set to the empty string, no command will be run.

I think this should be "an empty string".  But I may be mistaking.

Regards,
  Dmitry

> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the recommended way of achieving the same is using
> +\"notmuch new\" hooks."
> +  :type '(choice (const :tag "notmuch new" nil)
> +  (const :tag "Disabled" "")
> +  (string :tag "Custom script"))
>:group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>(interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -  (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +  (if (not (string= notmuch-poll-script ""))
> +   (call-process notmuch-poll-script nil nil))
> +(call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>"Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

Hi Jani.

On Mon, 12 Dec 2011 21:57:28 +0200, Jani Nikula  wrote:
> Support nil value for notmuch-poll-script to run "notmuch new" instead of
> an external script, and make this the new default. "notmuch new" is run
> using the configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 
> Signed-off-by: Jani Nikula 

Looks good to me.  Few comments regarding documentation below.

> ---
>  emacs/notmuch.el |   34 --
>  1 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..5a8ab9d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,42 @@ same relative position within the new buffer."
>  (notmuch-search query oldest-first target-thread target-line 
> continuation)
>  (goto-char (point-min
>  
> -(defcustom notmuch-poll-script ""
> +(defcustom notmuch-poll-script nil
>"An external script to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> +This variable controls the action invoked by
> +`notmuch-search-poll-and-refresh-view' and
>  `notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +of 'G') to incorporate new mail into the notmuch database.
> +
> +If this variable is non empty, then it should name an external

Please s/non empty/non-empty string/ to make it more clear.

> +script to be run. If set to an empty string, no action is
> +invoked. Finally, if set to nil (the default), \"notmuch new\" is
> +run using the command specified by `notmuch-command'.
> +
> +The external script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +Note that the same can be achieved through \"notmuch new\" hooks."

s/though/using/?

Would be nice to mention that using hooks is the recommended way for
this.

> +  :type '(choice (const :tag "Notmuch new" nil)

Perhaps s/Notmuch/notmuch/ because we are talking about a command here?

Regards,
  Dmitry

> +  (const :tag "Disabled" "")
> +  (string :tag "Custom script"))
>:group 'notmuch)
>  
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run \"notmuch new\" or an external script to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', \"notmuch new\", or does nothing
> +depending on the value of `notmuch-poll-script'."
>(interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -  (call-process notmuch-poll-script nil nil)))
> +  (if (stringp notmuch-poll-script)
> +  (if (not (string= notmuch-poll-script ""))
> +   (call-process notmuch-poll-script nil nil))
> +(call-process notmuch-command nil nil nil "new")))
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>"Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements  wrote:
> Quoth Jani Nikula on Dec 12 at  1:10 am:
> >On Dec 12, 2011 12:56 AM, "Austin Clements" <[1]amdragon at mit.edu> 
> > wrote:
> >>
> >> Quoth Dmitry Kurochkin on Dec 12 at ?2:00 am:
> >> > Hi Jani.
> >> >
> >> > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <[2]jani at 
> > nikula.org>
> >wrote:
> >> > > Let notmuch-poll-script be a function as well as a string. Make
> >default
> >> > > value nil instead of an empty string, but allow "" for backwards
> >> > > compatibility. Add a notmuch poll function to call "notmuch new"
> >using the
> >> > > configured notmuch-command.
> >> > >
> >> > > This allows taking better advantage of the "notmuch new" hooks from
> >emacs
> >> > > without intermediate scripts.
> >> > >
> >> >
> >> > I was just thinking about working on this myself :)
> >> >
> >> > I think a better solution would be to allow running a command with
> >> > arguments. ?Creating a elisp function just to run a command with some
> >> > parameters feels wrong. ?This way we would have to add another
> >function
> >> > each time we want to add another argument.
> >>
> >> This seems a little awkward to me, too, though perhaps it's the best
> >> way. ?Other approaches to consider include accepting a list for
> >> notmuch-poll-script (e.g., ("notmuch" "new")) or leaving it as a
> >> string but treating it as a shell command so "notmuch new" would Just
> >> Work. ?Personally, I think the latter is the most intuitive, but it
> >> would be worth looking at how other customizable external commands are
> >> done in Emacs.
> >>
> >> A function seems powerful, but also like overkill. ?Can you give a use
> >> case for a function that wouldn't be more easily solved by one of the
> >> above approaches?
> > 
> >The only reason I had for using a function was running notmuch using
> >notmuch-command. Any ideas how to do that with the Just Works approach?
> 
> Oh, I see.  I'd missed that.
> 
> So here's another idea, prefaced with a rant.
> 
> It's bothered me for a long time that notmuch-emacs didn't just know
> by default how to check for new mail.  What MUA doesn't know how to
> check for new mail?  Why does a new user of notmuch have to tell it
> how to check for new mail?  Of course, this *had* to be configured
> before because everyone had their own way of checking for new mail.
> Hooks eliminate this unnecessary flexibility and make "notmuch new"
> the one true way to check for new mail---as it ought to be---and in
> turn make the notmuch-poll-script variable obsolete.
> 
> So, what about changing the default "" setting of notmuch-poll-script
> from meaning "do nothing and be useless" to meaning "run notmuch new
> (using notmuch-command)"?  It will then automatically do the right
> thing for new users, while still being backward-compatible and
> allowing an escape hatch for bizarre situations.

Fine with me.  AFAIK no one has asked for using custom functions for
notmuch-poll-script, so adding a sane default may be the simplest and
the best option.

Regards,
  Dmitry

[PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 00:19:36 +0200, Jani Nikula  wrote:
> 
> Hi Dmitry -
> 
> On Mon, 12 Dec 2011 02:00:45 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula  wrote:
> > > Let notmuch-poll-script be a function as well as a string. Make default
> > > value nil instead of an empty string, but allow "" for backwards
> > > compatibility. Add a notmuch poll function to call "notmuch new" using the
> > > configured notmuch-command.
> > > 
> > > This allows taking better advantage of the "notmuch new" hooks from emacs
> > > without intermediate scripts.
> > > 
> > 
> > I was just thinking about working on this myself :)
> 
> :)
> 
> > I think a better solution would be to allow running a command with
> > arguments.  Creating a elisp function just to run a command with some
> > parameters feels wrong.  This way we would have to add another function
> > each time we want to add another argument.
> 
> One thing to take into account is running "notmuch new" using
> notmuch-command, and make that happen with one click in custom.
> 

Indeed.  One click in custom should be easy, but using notmuch-command
may not be so.  So now I like your solution :) Though others may think
of something better.

The only comment I have:

+(function :tag "Custom function"
+  :value notmuch-poll-script-notmuch-new)

Should we set :value to notmuch-poll-script-notmuch-new here?  There is
another option for "notmuch new", so perhaps there should be no value
for custom function as for custom script?

Regards,
  Dmitry

> > Function support for notmuch-poll-script seems like a useful feature on
> > it's own.
> 
> I just did this quickly to scratch my own itches, so to speak. My elisp
> is not that great, so I really wouldn't mind if you wanted to continue
> from here and make it your own. It'll be a while before I have the time
> to (figure out how to) do this properly anyway. But up to you, really.
> 
> BR,
> Jani.
> 
> 
> > 
> > Regards,
> >   Dmitry
> > 
> > > Signed-off-by: Jani Nikula 
> > > ---
> > >  emacs/notmuch.el |   44 
> > >  1 files changed, 32 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > > index 8936149..6c7e800 100644
> > > --- a/emacs/notmuch.el
> > > +++ b/emacs/notmuch.el
> > > @@ -965,28 +965,48 @@ same relative position within the new buffer."
> > >  (notmuch-search query oldest-first target-thread target-line 
> > > continuation)
> > >  (goto-char (point-min
> > >  
> > > -(defcustom notmuch-poll-script ""
> > > -  "An external script to incorporate new mail into the notmuch database.
> > > +(defcustom notmuch-poll-script nil
> > > +  "A script or a function to incorporate new mail into the notmuch 
> > > database.
> > >  
> > > -If this variable is non empty, then it should name a script to be
> > > -invoked by `notmuch-search-poll-and-refresh-view' and
> > > -`notmuch-hello-poll-and-update' (each have a default keybinding
> > > -of 'G'). The script could do any of the following depending on
> > > +This variable can be set to a function or the name of an external
> > > +script to be invoked by `notmuch-search-poll-and-refresh-view'
> > > +and `notmuch-hello-poll-and-update' (each have a default
> > > +keybinding of 'G'). Set to nil to do nothing.
> > > +
> > > +The function or script could do any of the following depending on
> > >  the user's needs:
> > >  
> > >  1. Invoke a program to transfer mail to the local mail store
> > >  2. Invoke \"notmuch new\" to incorporate the new mail
> > > -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> > > -  :type 'string
> > > +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> > > +
> > > +You can also choose to use \"notmuch new\" through the provided
> > > +`notmuch-poll-script-notmuch-new' function, and have the
> > > +\"notmuch new\" hooks perform the actions above."
> > > +  :type '(choice (const :tag "Not set" nil)
> > > +  (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> > > +  

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 12:15:44 +0200, Tomi Ollila  wrote:
> On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements  wrote:
> > 
> > So here's another idea, prefaced with a rant.
> > 
> > It's bothered me for a long time that notmuch-emacs didn't just know
> > by default how to check for new mail.  What MUA doesn't know how to
> > check for new mail?  Why does a new user of notmuch have to tell it
> > how to check for new mail?  Of course, this *had* to be configured
> > before because everyone had their own way of checking for new mail.
> > Hooks eliminate this unnecessary flexibility and make "notmuch new"
> > the one true way to check for new mail---as it ought to be---and in
> > turn make the notmuch-poll-script variable obsolete.
> > 
> > So, what about changing the default "" setting of notmuch-poll-script
> > from meaning "do nothing and be useless" to meaning "run notmuch new
> > (using notmuch-command)"?  It will then automatically do the right
> > thing for new users, while still being backward-compatible and
> > allowing an escape hatch for bizarre situations.
> 
> +1
> 
> So, it could work like this:
> 
> (defun notmuch-poll ()
>   "FIX DOCSTRING"
>   (interactive)
>   (if (stringp notmuch notmuch-poll-script)
>   (if (string= notmuch-poll-script "")
> (call-process notmuch-command nil nil nil "new")
> (call-process notmuch-poll-script
> 
> I.e. in case notmuch-poll-script == nil, (or not string)
> do nothing. In case notmuch-poll-script == "" execute notmuch new
> and if notmuch-poll-script is string with content execute that.
> 

I think the following scheme would be slightly better and more
consistent:

* nil - run "notmuch new", the new default
* "" - do nothing
* "script" - run script

Regards,
  Dmitry

> users who want other functionality can reimplement notmuch-poll function
> after notmuch has been loaded (and manage things themselves when internal
> implementation changes ;().
> 
> 
> Tomi
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

Hi Jani.

On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula  wrote:
> Let notmuch-poll-script be a function as well as a string. Make default
> value nil instead of an empty string, but allow "" for backwards
> compatibility. Add a notmuch poll function to call "notmuch new" using the
> configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 

I was just thinking about working on this myself :)

I think a better solution would be to allow running a command with
arguments.  Creating a elisp function just to run a command with some
parameters feels wrong.  This way we would have to add another function
each time we want to add another argument.

Function support for notmuch-poll-script seems like a useful feature on
it's own.

Regards,
  Dmitry

> Signed-off-by: Jani Nikula 
> ---
>  emacs/notmuch.el |   44 
>  1 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..6c7e800 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,48 @@ same relative position within the new buffer."
>  (notmuch-search query oldest-first target-thread target-line 
> continuation)
>  (goto-char (point-min
>  
> -(defcustom notmuch-poll-script ""
> -  "An external script to incorporate new mail into the notmuch database.
> +(defcustom notmuch-poll-script nil
> +  "A script or a function to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> -`notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +This variable can be set to a function or the name of an external
> +script to be invoked by `notmuch-search-poll-and-refresh-view'
> +and `notmuch-hello-poll-and-update' (each have a default
> +keybinding of 'G'). Set to nil to do nothing.
> +
> +The function or script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +You can also choose to use \"notmuch new\" through the provided
> +`notmuch-poll-script-notmuch-new' function, and have the
> +\"notmuch new\" hooks perform the actions above."
> +  :type '(choice (const :tag "Not set" nil)
> +  (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> +  (function :tag "Custom function"
> +:value notmuch-poll-script-notmuch-new)
> +  (string :tag "Custom script"))
>:group 'notmuch)
>  
> +(defun notmuch-poll-script-notmuch-new ()
> +  "Run \"notmuch new\"."
> +  (call-process notmuch-command nil nil nil "new"))
> +
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run external script or call a function to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', which can be a function or the
> +name of an external script. Does nothing if `notmuch-poll-script'
> +is nil or an empty string."
>(interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -  (call-process notmuch-poll-script nil nil)))
> +  (cond
> +   ((stringp notmuch-poll-script)
> +(if (not (string= notmuch-poll-script "")) ;; for backwards compatibility
> + (call-process notmuch-poll-script nil nil)))
> +   ((functionp notmuch-poll-script)
> +(funcall notmuch-poll-script
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>"Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] util/hex-escape.[ch]: encoding/decoding strings into restricted character set

[Dmitry Kurochkin]

On Sun, 11 Dec 2011 12:19:44 -0400, David Bremner  wrote:
> From: David Bremner 
> 
> The character set is chosen to be suitable for pathnames, and the same
> as that used by contrib/nmbug. The new encoded/decoded strings are
> allocated using talloc.
> ---
> This isn't urgent, but it is useful for a couple projects I have
> brewing (nmbug compatible dump/restore and tag logging), so I thought
> I would get some feedback on it.
> 

I have some free time to spend on notmuch reviews today.  So here it is
comes :)

> 
>  util/Makefile.local |4 +-
>  util/hex-escape.c   |  110 
> +++
>  util/hex-escape.h   |   10 +
>  3 files changed, 122 insertions(+), 2 deletions(-)
>  create mode 100644 util/hex-escape.c
>  create mode 100644 util/hex-escape.h
> 
> diff --git a/util/Makefile.local b/util/Makefile.local
> index 0340899..2e63932 100644
> --- a/util/Makefile.local
> +++ b/util/Makefile.local
> @@ -3,11 +3,11 @@
>  dir := util
>  extra_cflags += -I$(srcdir)/$(dir)
>  
> -libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c
> +libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c
>  
>  libutil_modules := $(libutil_c_srcs:.c=.o)
>  
>  $(dir)/libutil.a: $(libutil_modules)
>   $(call quiet,AR) rcs $@ $^
>  
> -CLEAN := $(CLEAN) $(dir)/xutil.o $(dir)/error_util.o $(dir)/libutil.a
> +CLEAN := $(CLEAN) $(libutil_modules) $(dir)/libutil.a

IMO this should be pushed as a separate patch (that does not need a
review :)).

> diff --git a/util/hex-escape.c b/util/hex-escape.c
> new file mode 100644
> index 000..c294bb5
> --- /dev/null
> +++ b/util/hex-escape.c
> @@ -0,0 +1,110 @@
> +/* hex-escape.c -  Manage encoding and decoding of byte strings into
> + *  a restricted character set.
> + *
> + * Copyright (c) 2011 David Bremner
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner 
> + */
> +
> +#include 
> +#include 
> +#include "error_util.h"
> +#include "hex-escape.h"
> +
> +static int
> +escapes_needed (const char *str){

The name suggests that the function returns a boolean (needed or not
needed).  Consider renaming to escapes_count, count_escapes or similar.

Also there is a space missing before the {.

> +int escapes = 0;
> +
> +while (*str) {
> + if (index (HEX_NO_ESCAPE, *str) == NULL)

Consider adding a function (bool escape_needed(const char c)) (similar
to isalpha() and friends) which would call index() and use it here and
in hex_encode.  (This comment would not be actual if you decide to
introduce a general char counting function.)

> + escapes++;
> + str++;
> +}
> +
> +   return escapes;

Can we replace this function with a two-line for loop similar to the one
in hex_decode?

I think we should either use inline loops for counting chars in both
hex_encode and hex_decode, or introduce a more general function that
counts the number of given characters and use it in both hex_encode and
hex_decode.

> +}
> +
> +char *
> +hex_encode (void *ctx, const char *str) {
> +char *newstr = talloc_size (ctx, strlen (str)+3*escapes_needed (str)+1);

Do we need only strlen(str) + 2*escaped_count + 1 here?

IMO it is very weird that we have spaces after a function name, but do
not have spaces around +...

> +
> +char *out = newstr;
> +

Why do we need both out and newstr variables?

> +while (*str) {
> + if (index (HEX_NO_ESCAPE, *str)) {
> + *out++ = *str++;
> + } else {
> + sprintf (out, "%%%02x", *str);
> + str++;

Use *str++ as sprintf() parameter instead of doing it on a separate
line?

> + out += 3;
> + }
> +}
> +*out = 0;

I would prefer '\0' here.

> +return newstr;
> +}
> +
> +inline static int
> +_digit (char c) {

Perhaps rename to hex_digit?  Other static function names do not start
with underscore.  Let's be consistent.

> +if ('0' <= c && c <= '9')
> + return c - '0';
> +
> +if ('A' <= c && c <= 'F')
> + return c - 'A';
> +
> +if ('a' <= c && c <= 'f')
> + return c - 'a';
> +

Does this really work as expected?  'B' - 'A' would be 1, while it seems
that we expect 10.  Perhaps we should use sscanf(3) instead?  That may
make the code simpler and allow to convert both chars at once.

> +INTERNAL_ERROR ("Illegal hex digit %c", c);
> +/*NOTREAC

[PATCH] cli: factor out config handling code to get/set lists.

[Dmitry Kurochkin]

Hi David.

On Sun, 11 Dec 2011 12:07:51 -0400, David Bremner  wrote:
> From: David Bremner 
> 
> Two new internal routines are created _config_get_list and
> _config_set_list; the notmuch_config_get_* functions that deal with
> lists are simply wrappers for these functions.

Looks good to me.  Some comments below.

> ---
>  notmuch-config.c |  130 +++--
>  1 files changed, 66 insertions(+), 64 deletions(-)
> 
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 1a7ed58..e98b6a3 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -520,92 +520,94 @@ notmuch_config_set_user_primary_email (notmuch_config_t 
> *config,
>  config->user_primary_email = NULL;
>  }
>  
> -const char **
> -notmuch_config_get_user_other_email (notmuch_config_t *config,
> -  size_t *length)
> +static const char **
> +_config_get_list (notmuch_config_t *config,
> +   const char *section, const char *key,
> +   const char ***outlist, size_t *list_length, size_t 
> *ret_length)
>  {

It seems weird that the function both takes list as an output parameter
and returns it.  Having two length parameters which are set to the same
value is confusing as well.  I understand that it is done to make getter
functions smaller.  So perhaps it is ok.

> -char **emails;
> -size_t emails_length;
> +char **inlist;
>  unsigned int i;
>  

Please move variable declarations inside the if (!*outlist) and if
(inlist) blocks.  (I saw other code in notmuch that does it, so it must
be ok.)

> -if (config->user_other_email == NULL) {
> - emails = g_key_file_get_string_list (config->key_file,
> -  "user", "other_email",
> -  &emails_length, NULL);
> - if (emails) {
> - config->user_other_email = talloc_size (config,
> - sizeof (char *) *
> - (emails_length + 1));
> - for (i = 0; i < emails_length; i++)
> - config->user_other_email[i] = talloc_strdup 
> (config->user_other_email,
> -  emails[i]);
> - config->user_other_email[i] = NULL;
> -
> - g_strfreev (emails);
> -
> - config->user_other_email_length = emails_length;
> +if (*outlist == NULL) {
> + inlist = g_key_file_get_string_list (config->key_file,
> +  section, key,
> +  list_length, NULL);
> + if (inlist) {
> + *outlist = talloc_size (config, sizeof (char *) *
> + (*list_length + 1));
> + for (i = 0; i < *list_length; i++)
> + (*outlist)[i] = talloc_strdup (*outlist, inlist[i]);
> + (*outlist)[i] = NULL;
> +
> + g_strfreev (inlist);
>   }
>  }
>  
> -*length = config->user_other_email_length;
> -return config->user_other_email;
> +if (ret_length) *ret_length = *list_length;

I would prefer to have this on separate lines.

> +return *outlist;
>  }
>  
> -void
> -notmuch_config_set_user_other_email (notmuch_config_t *config,
> -  const char *other_email[],
> -  size_t length)
> +const char **
> +notmuch_config_get_user_other_email (notmuch_config_t *config,   size_t 
> *length)
>  {
> -g_key_file_set_string_list (config->key_file,
> - "user", "other_email",
> - other_email, length);
> +return _config_get_list (config, "user", "other_email",
> +  &(config->user_other_email),
> +  &(config->user_other_email_length), length);
> +}
>  
> -talloc_free (config->user_other_email);
> -config->user_other_email = NULL;
> +const char **
> +notmuch_config_get_new_tags (notmuch_config_t *config,   size_t *length)
> +{
> +return _config_get_list (config, "new", "tags",
> +  &(config->new_tags),
> +  &(config->new_tags_length), length);
>  }
>  
>  const char **
> -notmuch_config_get_new_tags (notmuch_config_t *config,
> -  size_t *length)
> +notmuch_config_get_log_subscribers (notmuch_config_t *config,   size_t 
> *length)
>  {
> -char **tags;
> -size_t tags_length;
> -unsigned int i;
> +return _config_get_list (config, "log", "subscribers",
> +  &(config->new_tags),
> +  &(config->new_tags_length), length);
> +}
>  

This should be part of a separate patch which adds the logging feature,
I guess.

Regards,
  Dmitry

> -if (config->new_tags == NULL) {
> - tags = g_key_file_get_string_list (config->key_file,
> -"new", "tags",
> -  

[PATCH 1/2] test: add a function to run Python tests

[Dmitry Kurochkin]

Hi Thomas.

On Wed,  7 Dec 2011 10:46:17 +0100, Thomas Jost  
wrote:
> The new test_python() function makes writing Python tests a little easier:
> - it sets the environment variables as needed
> - it redirects stdout to the OUTPUT file (like test_emacs()).
> 
> This commit also declares python as an external prereq.
> 
> The stdout redirection is required to avoid trouble when running commands like
> "python 'script' | sort > OUTPUT": in such a case, any error due to a missing
> external prereq would be "swallowed" by sort, resulting to a failed test 
> instead
> of a skipped one.
> ---
>  test/python  |6 ++
>  test/test-lib.sh |9 +
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/test/python b/test/python
> index f737749..c3aa726 100755
> --- a/test/python
> +++ b/test/python
> @@ -5,9 +5,7 @@ test_description="python bindings"
>  add_email_corpus
>  
>  test_begin_subtest "compare thread ids"
> -LD_LIBRARY_PATH=$TEST_DIRECTORY/../lib \
> -PYTHONPATH=$TEST_DIRECTORY/../bindings/python \
> -python < OUTPUT
> +test_python <  import notmuch
>  db = notmuch.Database(mode=notmuch.Database.MODE.READ_WRITE)
>  q_new = notmuch.Query(db, 'tag:inbox')
> @@ -15,5 +13,5 @@ for t in q_new.search_threads():
>  print t.get_thread_id()
>  EOF
>  notmuch search --output=threads tag:inbox | sed s/^thread:// | sort > 
> EXPECTED
> -test_expect_equal_file OUTPUT EXPECTED
> +test_expect_equal_file <(sort OUTPUT) EXPECTED
>  test_done
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index a975957..519bd84 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -919,6 +919,14 @@ test_emacs () {
>   emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>  }
>  
> +test_python() {
> + export LD_LIBRARY_PATH=$TEST_DIRECTORY/../lib
> + export PYTHONPATH=$TEST_DIRECTORY/../bindings/python
> +
> + (echo "import sys; _orig_stdout=sys.stdout; sys.stdout=open('OUTPUT', 
> 'w')"; cat) \
> + | python -

Perhaps we should have a test-lib.py for test-specific stuff like this
(similar to test-lib.el)?  I think it would be cleaner and makes it easy
to add more Python test auxiliary functions later.

Regards,
  Dmitry

> +}
> +
>  test_reset_state_ () {
>   test -z "$test_init_done_" && test_init_
>  
> @@ -1148,3 +1156,4 @@ test_declare_external_prereq emacs
>  test_declare_external_prereq emacsclient
>  test_declare_external_prereq gdb
>  test_declare_external_prereq gpg
> +test_declare_external_prereq python
> -- 
> 1.7.8
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Sun, 11 Dec 2011 19:31:03 -0500, Austin Clements  wrote:
> Quoth Jani Nikula on Dec 12 at  1:10 am:
> >On Dec 12, 2011 12:56 AM, "Austin Clements" <[1]amdra...@mit.edu> wrote:
> >>
> >> Quoth Dmitry Kurochkin on Dec 12 at  2:00 am:
> >> > Hi Jani.
> >> >
> >> > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula <[2]j...@nikula.org>
> >wrote:
> >> > > Let notmuch-poll-script be a function as well as a string. Make
> >default
> >> > > value nil instead of an empty string, but allow "" for backwards
> >> > > compatibility. Add a notmuch poll function to call "notmuch new"
> >using the
> >> > > configured notmuch-command.
> >> > >
> >> > > This allows taking better advantage of the "notmuch new" hooks from
> >emacs
> >> > > without intermediate scripts.
> >> > >
> >> >
> >> > I was just thinking about working on this myself :)
> >> >
> >> > I think a better solution would be to allow running a command with
> >> > arguments.  Creating a elisp function just to run a command with some
> >> > parameters feels wrong.  This way we would have to add another
> >function
> >> > each time we want to add another argument.
> >>
> >> This seems a little awkward to me, too, though perhaps it's the best
> >> way.  Other approaches to consider include accepting a list for
> >> notmuch-poll-script (e.g., ("notmuch" "new")) or leaving it as a
> >> string but treating it as a shell command so "notmuch new" would Just
> >> Work.  Personally, I think the latter is the most intuitive, but it
> >> would be worth looking at how other customizable external commands are
> >> done in Emacs.
> >>
> >> A function seems powerful, but also like overkill.  Can you give a use
> >> case for a function that wouldn't be more easily solved by one of the
> >> above approaches?
> > 
> >The only reason I had for using a function was running notmuch using
> >notmuch-command. Any ideas how to do that with the Just Works approach?
> 
> Oh, I see.  I'd missed that.
> 
> So here's another idea, prefaced with a rant.
> 
> It's bothered me for a long time that notmuch-emacs didn't just know
> by default how to check for new mail.  What MUA doesn't know how to
> check for new mail?  Why does a new user of notmuch have to tell it
> how to check for new mail?  Of course, this *had* to be configured
> before because everyone had their own way of checking for new mail.
> Hooks eliminate this unnecessary flexibility and make "notmuch new"
> the one true way to check for new mail---as it ought to be---and in
> turn make the notmuch-poll-script variable obsolete.
> 
> So, what about changing the default "" setting of notmuch-poll-script
> from meaning "do nothing and be useless" to meaning "run notmuch new
> (using notmuch-command)"?  It will then automatically do the right
> thing for new users, while still being backward-compatible and
> allowing an escape hatch for bizarre situations.

Fine with me.  AFAIK no one has asked for using custom functions for
notmuch-poll-script, so adding a sane default may be the simplest and
the best option.

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v3 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

Austin, good job! :) Thanks for this work.  (and continue, please :))

Most comments in my review are minor and/or concern the old code
(i.e. the new code does not make it worse).  Please feel free to ignore
them.

I vote for pushing this series as soon as Austin finds it appropriate.
If Austin makes a new version of this patch series with some of my
comments fixed, it gets my auto-approve without another round of review :)

Regards,
  Dmitry

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

On Mon, 12 Dec 2011 00:19:36 +0200, Jani Nikula  wrote:
> 
> Hi Dmitry -
> 
> On Mon, 12 Dec 2011 02:00:45 +0400, Dmitry Kurochkin 
>  wrote:
> > On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula  wrote:
> > > Let notmuch-poll-script be a function as well as a string. Make default
> > > value nil instead of an empty string, but allow "" for backwards
> > > compatibility. Add a notmuch poll function to call "notmuch new" using the
> > > configured notmuch-command.
> > > 
> > > This allows taking better advantage of the "notmuch new" hooks from emacs
> > > without intermediate scripts.
> > > 
> > 
> > I was just thinking about working on this myself :)
> 
> :)
> 
> > I think a better solution would be to allow running a command with
> > arguments.  Creating a elisp function just to run a command with some
> > parameters feels wrong.  This way we would have to add another function
> > each time we want to add another argument.
> 
> One thing to take into account is running "notmuch new" using
> notmuch-command, and make that happen with one click in custom.
> 

Indeed.  One click in custom should be easy, but using notmuch-command
may not be so.  So now I like your solution :) Though others may think
of something better.

The only comment I have:

+(function :tag "Custom function"
+  :value notmuch-poll-script-notmuch-new)

Should we set :value to notmuch-poll-script-notmuch-new here?  There is
another option for "notmuch new", so perhaps there should be no value
for custom function as for custom script?

Regards,
  Dmitry

> > Function support for notmuch-poll-script seems like a useful feature on
> > it's own.
> 
> I just did this quickly to scratch my own itches, so to speak. My elisp
> is not that great, so I really wouldn't mind if you wanted to continue
> from here and make it your own. It'll be a while before I have the time
> to (figure out how to) do this properly anyway. But up to you, really.
> 
> BR,
> Jani.
> 
> 
> > 
> > Regards,
> >   Dmitry
> > 
> > > Signed-off-by: Jani Nikula 
> > > ---
> > >  emacs/notmuch.el |   44 
> > >  1 files changed, 32 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > > index 8936149..6c7e800 100644
> > > --- a/emacs/notmuch.el
> > > +++ b/emacs/notmuch.el
> > > @@ -965,28 +965,48 @@ same relative position within the new buffer."
> > >  (notmuch-search query oldest-first target-thread target-line 
> > > continuation)
> > >  (goto-char (point-min
> > >  
> > > -(defcustom notmuch-poll-script ""
> > > -  "An external script to incorporate new mail into the notmuch database.
> > > +(defcustom notmuch-poll-script nil
> > > +  "A script or a function to incorporate new mail into the notmuch 
> > > database.
> > >  
> > > -If this variable is non empty, then it should name a script to be
> > > -invoked by `notmuch-search-poll-and-refresh-view' and
> > > -`notmuch-hello-poll-and-update' (each have a default keybinding
> > > -of 'G'). The script could do any of the following depending on
> > > +This variable can be set to a function or the name of an external
> > > +script to be invoked by `notmuch-search-poll-and-refresh-view'
> > > +and `notmuch-hello-poll-and-update' (each have a default
> > > +keybinding of 'G'). Set to nil to do nothing.
> > > +
> > > +The function or script could do any of the following depending on
> > >  the user's needs:
> > >  
> > >  1. Invoke a program to transfer mail to the local mail store
> > >  2. Invoke \"notmuch new\" to incorporate the new mail
> > > -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> > > -  :type 'string
> > > +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> > > +
> > > +You can also choose to use \"notmuch new\" through the provided
> > > +`notmuch-poll-script-notmuch-new' function, and have the
> > > +\"notmuch new\" hooks perform the actions above."
> > > +  :type '(choice (const :tag "Not set" nil)
> > > +  (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> > > +  (functio

[PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Dmitry Kurochkin]

Hi Austin.

I enjoyed reviewing this patch.  It is a pleasure to see how complex and
confusing code becomes much smaller and cleaner.

I still have some questions with the new code.  It seems confusing to me
that part_content is called first and then go envelope headers.  But I
this is just the first step of the rewrite, right? :)

The only comment I have:

+format->part_content (part);

For all other format members that are function pointers, we have a check
for NULL.  Perhaps we should add it here as well?

Regards,
  Dmitry

Re: [PATCH] emacs: support "notmuch new" as a notmuch-poll-script

[Dmitry Kurochkin]

Hi Jani.

On Sun, 11 Dec 2011 23:48:20 +0200, Jani Nikula  wrote:
> Let notmuch-poll-script be a function as well as a string. Make default
> value nil instead of an empty string, but allow "" for backwards
> compatibility. Add a notmuch poll function to call "notmuch new" using the
> configured notmuch-command.
> 
> This allows taking better advantage of the "notmuch new" hooks from emacs
> without intermediate scripts.
> 

I was just thinking about working on this myself :)

I think a better solution would be to allow running a command with
arguments.  Creating a elisp function just to run a command with some
parameters feels wrong.  This way we would have to add another function
each time we want to add another argument.

Function support for notmuch-poll-script seems like a useful feature on
it's own.

Regards,
  Dmitry

> Signed-off-by: Jani Nikula 
> ---
>  emacs/notmuch.el |   44 
>  1 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8936149..6c7e800 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -965,28 +965,48 @@ same relative position within the new buffer."
>  (notmuch-search query oldest-first target-thread target-line 
> continuation)
>  (goto-char (point-min
>  
> -(defcustom notmuch-poll-script ""
> -  "An external script to incorporate new mail into the notmuch database.
> +(defcustom notmuch-poll-script nil
> +  "A script or a function to incorporate new mail into the notmuch database.
>  
> -If this variable is non empty, then it should name a script to be
> -invoked by `notmuch-search-poll-and-refresh-view' and
> -`notmuch-hello-poll-and-update' (each have a default keybinding
> -of 'G'). The script could do any of the following depending on
> +This variable can be set to a function or the name of an external
> +script to be invoked by `notmuch-search-poll-and-refresh-view'
> +and `notmuch-hello-poll-and-update' (each have a default
> +keybinding of 'G'). Set to nil to do nothing.
> +
> +The function or script could do any of the following depending on
>  the user's needs:
>  
>  1. Invoke a program to transfer mail to the local mail store
>  2. Invoke \"notmuch new\" to incorporate the new mail
> -3. Invoke one or more \"notmuch tag\" commands to classify the mail"
> -  :type 'string
> +3. Invoke one or more \"notmuch tag\" commands to classify the mail
> +
> +You can also choose to use \"notmuch new\" through the provided
> +`notmuch-poll-script-notmuch-new' function, and have the
> +\"notmuch new\" hooks perform the actions above."
> +  :type '(choice (const :tag "Not set" nil)
> +  (const :tag "Notmuch new" notmuch-poll-script-notmuch-new)
> +  (function :tag "Custom function"
> +:value notmuch-poll-script-notmuch-new)
> +  (string :tag "Custom script"))
>:group 'notmuch)
>  
> +(defun notmuch-poll-script-notmuch-new ()
> +  "Run \"notmuch new\"."
> +  (call-process notmuch-command nil nil nil "new"))
> +
>  (defun notmuch-poll ()
> -  "Run external script to import mail.
> +  "Run external script or call a function to import mail.
>  
> -Invokes `notmuch-poll-script' if it is not set to an empty string."
> +Invokes `notmuch-poll-script', which can be a function or the
> +name of an external script. Does nothing if `notmuch-poll-script'
> +is nil or an empty string."
>(interactive)
> -  (if (not (string= notmuch-poll-script ""))
> -  (call-process notmuch-poll-script nil nil)))
> +  (cond
> +   ((stringp notmuch-poll-script)
> +(if (not (string= notmuch-poll-script "")) ;; for backwards compatibility
> + (call-process notmuch-poll-script nil nil)))
> +   ((functionp notmuch-poll-script)
> +(funcall notmuch-poll-script
>  
>  (defun notmuch-search-poll-and-refresh-view ()
>"Invoke `notmuch-poll' to import mail, then refresh the current view."
> -- 
> 1.7.5.4
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] util/hex-escape.[ch]: encoding/decoding strings into restricted character set

[Dmitry Kurochkin]

On Sun, 11 Dec 2011 12:19:44 -0400, David Bremner  wrote:
> From: David Bremner 
> 
> The character set is chosen to be suitable for pathnames, and the same
> as that used by contrib/nmbug. The new encoded/decoded strings are
> allocated using talloc.
> ---
> This isn't urgent, but it is useful for a couple projects I have
> brewing (nmbug compatible dump/restore and tag logging), so I thought
> I would get some feedback on it.
> 

I have some free time to spend on notmuch reviews today.  So here it is
comes :)

> 
>  util/Makefile.local |4 +-
>  util/hex-escape.c   |  110 
> +++
>  util/hex-escape.h   |   10 +
>  3 files changed, 122 insertions(+), 2 deletions(-)
>  create mode 100644 util/hex-escape.c
>  create mode 100644 util/hex-escape.h
> 
> diff --git a/util/Makefile.local b/util/Makefile.local
> index 0340899..2e63932 100644
> --- a/util/Makefile.local
> +++ b/util/Makefile.local
> @@ -3,11 +3,11 @@
>  dir := util
>  extra_cflags += -I$(srcdir)/$(dir)
>  
> -libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c
> +libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c
>  
>  libutil_modules := $(libutil_c_srcs:.c=.o)
>  
>  $(dir)/libutil.a: $(libutil_modules)
>   $(call quiet,AR) rcs $@ $^
>  
> -CLEAN := $(CLEAN) $(dir)/xutil.o $(dir)/error_util.o $(dir)/libutil.a
> +CLEAN := $(CLEAN) $(libutil_modules) $(dir)/libutil.a

IMO this should be pushed as a separate patch (that does not need a
review :)).

> diff --git a/util/hex-escape.c b/util/hex-escape.c
> new file mode 100644
> index 000..c294bb5
> --- /dev/null
> +++ b/util/hex-escape.c
> @@ -0,0 +1,110 @@
> +/* hex-escape.c -  Manage encoding and decoding of byte strings into
> + *  a restricted character set.
> + *
> + * Copyright (c) 2011 David Bremner
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner 
> + */
> +
> +#include 
> +#include 
> +#include "error_util.h"
> +#include "hex-escape.h"
> +
> +static int
> +escapes_needed (const char *str){

The name suggests that the function returns a boolean (needed or not
needed).  Consider renaming to escapes_count, count_escapes or similar.

Also there is a space missing before the {.

> +int escapes = 0;
> +
> +while (*str) {
> + if (index (HEX_NO_ESCAPE, *str) == NULL)

Consider adding a function (bool escape_needed(const char c)) (similar
to isalpha() and friends) which would call index() and use it here and
in hex_encode.  (This comment would not be actual if you decide to
introduce a general char counting function.)

> + escapes++;
> + str++;
> +}
> +
> +   return escapes;

Can we replace this function with a two-line for loop similar to the one
in hex_decode?

I think we should either use inline loops for counting chars in both
hex_encode and hex_decode, or introduce a more general function that
counts the number of given characters and use it in both hex_encode and
hex_decode.

> +}
> +
> +char *
> +hex_encode (void *ctx, const char *str) {
> +char *newstr = talloc_size (ctx, strlen (str)+3*escapes_needed (str)+1);

Do we need only strlen(str) + 2*escaped_count + 1 here?

IMO it is very weird that we have spaces after a function name, but do
not have spaces around +...

> +
> +char *out = newstr;
> +

Why do we need both out and newstr variables?

> +while (*str) {
> + if (index (HEX_NO_ESCAPE, *str)) {
> + *out++ = *str++;
> + } else {
> + sprintf (out, "%%%02x", *str);
> + str++;

Use *str++ as sprintf() parameter instead of doing it on a separate
line?

> + out += 3;
> + }
> +}
> +*out = 0;

I would prefer '\0' here.

> +return newstr;
> +}
> +
> +inline static int
> +_digit (char c) {

Perhaps rename to hex_digit?  Other static function names do not start
with underscore.  Let's be consistent.

> +if ('0' <= c && c <= '9')
> + return c - '0';
> +
> +if ('A' <= c && c <= 'F')
> + return c - 'A';
> +
> +if ('a' <= c && c <= 'f')
> + return c - 'a';
> +

Does this really work as expected?  'B' - 'A' would be 1, while it seems
that we expect 10.  Perhaps we should use sscanf(3) instead?  That may
make the code simpler and allow to convert both chars at once.

> +INTERNAL_ERROR ("Illegal hex digit %c", c);
> +/*NOTREAC

Re: [PATCH] cli: factor out config handling code to get/set lists.

[Dmitry Kurochkin]

Hi David.

On Sun, 11 Dec 2011 12:07:51 -0400, David Bremner  wrote:
> From: David Bremner 
> 
> Two new internal routines are created _config_get_list and
> _config_set_list; the notmuch_config_get_* functions that deal with
> lists are simply wrappers for these functions.

Looks good to me.  Some comments below.

> ---
>  notmuch-config.c |  130 +++--
>  1 files changed, 66 insertions(+), 64 deletions(-)
> 
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 1a7ed58..e98b6a3 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -520,92 +520,94 @@ notmuch_config_set_user_primary_email (notmuch_config_t 
> *config,
>  config->user_primary_email = NULL;
>  }
>  
> -const char **
> -notmuch_config_get_user_other_email (notmuch_config_t *config,
> -  size_t *length)
> +static const char **
> +_config_get_list (notmuch_config_t *config,
> +   const char *section, const char *key,
> +   const char ***outlist, size_t *list_length, size_t 
> *ret_length)
>  {

It seems weird that the function both takes list as an output parameter
and returns it.  Having two length parameters which are set to the same
value is confusing as well.  I understand that it is done to make getter
functions smaller.  So perhaps it is ok.

> -char **emails;
> -size_t emails_length;
> +char **inlist;
>  unsigned int i;
>  

Please move variable declarations inside the if (!*outlist) and if
(inlist) blocks.  (I saw other code in notmuch that does it, so it must
be ok.)

> -if (config->user_other_email == NULL) {
> - emails = g_key_file_get_string_list (config->key_file,
> -  "user", "other_email",
> -  &emails_length, NULL);
> - if (emails) {
> - config->user_other_email = talloc_size (config,
> - sizeof (char *) *
> - (emails_length + 1));
> - for (i = 0; i < emails_length; i++)
> - config->user_other_email[i] = talloc_strdup 
> (config->user_other_email,
> -  emails[i]);
> - config->user_other_email[i] = NULL;
> -
> - g_strfreev (emails);
> -
> - config->user_other_email_length = emails_length;
> +if (*outlist == NULL) {
> + inlist = g_key_file_get_string_list (config->key_file,
> +  section, key,
> +  list_length, NULL);
> + if (inlist) {
> + *outlist = talloc_size (config, sizeof (char *) *
> + (*list_length + 1));
> + for (i = 0; i < *list_length; i++)
> + (*outlist)[i] = talloc_strdup (*outlist, inlist[i]);
> + (*outlist)[i] = NULL;
> +
> + g_strfreev (inlist);
>   }
>  }
>  
> -*length = config->user_other_email_length;
> -return config->user_other_email;
> +if (ret_length) *ret_length = *list_length;

I would prefer to have this on separate lines.

> +return *outlist;
>  }
>  
> -void
> -notmuch_config_set_user_other_email (notmuch_config_t *config,
> -  const char *other_email[],
> -  size_t length)
> +const char **
> +notmuch_config_get_user_other_email (notmuch_config_t *config,   size_t 
> *length)
>  {
> -g_key_file_set_string_list (config->key_file,
> - "user", "other_email",
> - other_email, length);
> +return _config_get_list (config, "user", "other_email",
> +  &(config->user_other_email),
> +  &(config->user_other_email_length), length);
> +}
>  
> -talloc_free (config->user_other_email);
> -config->user_other_email = NULL;
> +const char **
> +notmuch_config_get_new_tags (notmuch_config_t *config,   size_t *length)
> +{
> +return _config_get_list (config, "new", "tags",
> +  &(config->new_tags),
> +  &(config->new_tags_length), length);
>  }
>  
>  const char **
> -notmuch_config_get_new_tags (notmuch_config_t *config,
> -  size_t *length)
> +notmuch_config_get_log_subscribers (notmuch_config_t *config,   size_t 
> *length)
>  {
> -char **tags;
> -size_t tags_length;
> -unsigned int i;
> +return _config_get_list (config, "log", "subscribers",
> +  &(config->new_tags),
> +  &(config->new_tags_length), length);
> +}
>  

This should be part of a separate patch which adds the logging feature,
I guess.

Regards,
  Dmitry

> -if (config->new_tags == NULL) {
> - tags = g_key_file_get_string_list (config->key_file,
> -"new", "tags",
> -  

Re: [PATCH 1/2] test: add a function to run Python tests

[Dmitry Kurochkin]

Hi Thomas.

On Wed,  7 Dec 2011 10:46:17 +0100, Thomas Jost  wrote:
> The new test_python() function makes writing Python tests a little easier:
> - it sets the environment variables as needed
> - it redirects stdout to the OUTPUT file (like test_emacs()).
> 
> This commit also declares python as an external prereq.
> 
> The stdout redirection is required to avoid trouble when running commands like
> "python 'script' | sort > OUTPUT": in such a case, any error due to a missing
> external prereq would be "swallowed" by sort, resulting to a failed test 
> instead
> of a skipped one.
> ---
>  test/python  |6 ++
>  test/test-lib.sh |9 +
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/test/python b/test/python
> index f737749..c3aa726 100755
> --- a/test/python
> +++ b/test/python
> @@ -5,9 +5,7 @@ test_description="python bindings"
>  add_email_corpus
>  
>  test_begin_subtest "compare thread ids"
> -LD_LIBRARY_PATH=$TEST_DIRECTORY/../lib \
> -PYTHONPATH=$TEST_DIRECTORY/../bindings/python \
> -python < OUTPUT
> +test_python <  import notmuch
>  db = notmuch.Database(mode=notmuch.Database.MODE.READ_WRITE)
>  q_new = notmuch.Query(db, 'tag:inbox')
> @@ -15,5 +13,5 @@ for t in q_new.search_threads():
>  print t.get_thread_id()
>  EOF
>  notmuch search --output=threads tag:inbox | sed s/^thread:// | sort > 
> EXPECTED
> -test_expect_equal_file OUTPUT EXPECTED
> +test_expect_equal_file <(sort OUTPUT) EXPECTED
>  test_done
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index a975957..519bd84 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -919,6 +919,14 @@ test_emacs () {
>   emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>  }
>  
> +test_python() {
> + export LD_LIBRARY_PATH=$TEST_DIRECTORY/../lib
> + export PYTHONPATH=$TEST_DIRECTORY/../bindings/python
> +
> + (echo "import sys; _orig_stdout=sys.stdout; sys.stdout=open('OUTPUT', 
> 'w')"; cat) \
> + | python -

Perhaps we should have a test-lib.py for test-specific stuff like this
(similar to test-lib.el)?  I think it would be cleaner and makes it easy
to add more Python test auxiliary functions later.

Regards,
  Dmitry

> +}
> +
>  test_reset_state_ () {
>   test -z "$test_init_done_" && test_init_
>  
> @@ -1148,3 +1156,4 @@ test_declare_external_prereq emacs
>  test_declare_external_prereq emacsclient
>  test_declare_external_prereq gdb
>  test_declare_external_prereq gpg
> +test_declare_external_prereq python
> -- 
> 1.7.8
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

Austin, good job! :) Thanks for this work.  (and continue, please :))

Most comments in my review are minor and/or concern the old code
(i.e. the new code does not make it worse).  Please feel free to ignore
them.

I vote for pushing this series as soon as Austin finds it appropriate.
If Austin makes a new version of this patch series with some of my
comments fixed, it gets my auto-approve without another round of review :)

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/4] show: Rewrite show_message_body to use the MIME tree interface.

[Dmitry Kurochkin]

Hi Austin.

I enjoyed reviewing this patch.  It is a pleasure to see how complex and
confusing code becomes much smaller and cleaner.

I still have some questions with the new code.  It seems confusing to me
that part_content is called first and then go envelope headers.  But I
this is just the first step of the rewrite, right? :)

The only comment I have:

+format->part_content (part);

For all other format members that are function pointers, we have a check
for NULL.  Perhaps we should add it here as well?

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Dmitry Kurochkin]

On Fri,  9 Dec 2011 14:54:27 -0500, Austin Clements  wrote:
> This function matches how we number parts for the --part argument to
> show.  It will allow us to jump directly to the desired part, rather
> than traversing the entire tree and carefully tracking whether or not
> we're "in the zone".
> ---
>  mime-node.c  |   25 +
>  notmuch-client.h |5 +
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index a8e4a59..207818e 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
>   g_type_name (G_OBJECT_TYPE (parent->part)));
>  }
>  }
> +
> +static mime_node_t *
> +_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
> +{
> +mime_node_t *ret = NULL;
> +int i;
> +

Can we move declarations below the if (which does not need them)?  I
always have troubles remembering if (recent enough) C standard allows
that or it is a GCC extension.  FWIW in the previous patch there are
declarations in the middle of a block, e.g.:

} else {
out->is_signed = TRUE;
...
GMimeSignatureValidity *sig_validity = 
g_mime_multipart_signed_verify
(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);

So either we can move these declarations to where they are needed, or we
should fix it in _mime_node_create().

> +if (*n <= 0)

Comment for mime_node_seek_dfs() says that the function returns the node
itself for n = 0, but does not say anything about n < 0.  I would expect
the function to return NULL for n < 0.  In any case, the comment below
should probably mention what happens for n < 0;

> + return node;
> +
> +*n = *n - 1;

Perhaps *n -= 1?  Or even --(*n)?

> +for (i = 0; i < node->children && !ret; i++) {

Consider s/i++/++i/.

Regards,
  Dmitry

> + mime_node_t *child = mime_node_child (node, i);
> + ret = _mime_node_seek_dfs_walk (child, n);
> + if (!ret)
> + talloc_free (child);
> +}
> +return ret;
> +}
> +
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n)
> +{
> +return _mime_node_seek_dfs_walk (node, &n);
> +}
> diff --git a/notmuch-client.h b/notmuch-client.h
> index fce1187..f215d4b 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -318,5 +318,10 @@ mime_node_open (const void *ctx, notmuch_message_t 
> *message,
>  mime_node_t *
>  mime_node_child (const mime_node_t *parent, int child);
>  
> +/* Return the nth child of node in a depth-first traversal.  If n is
> + * 0, returns node itself.  Returns NULL if there is no such part. */
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n);
> +
>  #include "command-line-arguments.h"
>  #endif
> -- 
> 1.7.7.3
> 
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/4] Utility function to seek in MIME trees in depth-first order.

[Dmitry Kurochkin]

On Fri,  9 Dec 2011 14:54:27 -0500, Austin Clements  wrote:
> This function matches how we number parts for the --part argument to
> show.  It will allow us to jump directly to the desired part, rather
> than traversing the entire tree and carefully tracking whether or not
> we're "in the zone".
> ---
>  mime-node.c  |   25 +
>  notmuch-client.h |5 +
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/mime-node.c b/mime-node.c
> index a8e4a59..207818e 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
>   g_type_name (G_OBJECT_TYPE (parent->part)));
>  }
>  }
> +
> +static mime_node_t *
> +_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
> +{
> +mime_node_t *ret = NULL;
> +int i;
> +

Can we move declarations below the if (which does not need them)?  I
always have troubles remembering if (recent enough) C standard allows
that or it is a GCC extension.  FWIW in the previous patch there are
declarations in the middle of a block, e.g.:

} else {
out->is_signed = TRUE;
...
GMimeSignatureValidity *sig_validity = 
g_mime_multipart_signed_verify
(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);

So either we can move these declarations to where they are needed, or we
should fix it in _mime_node_create().

> +if (*n <= 0)

Comment for mime_node_seek_dfs() says that the function returns the node
itself for n = 0, but does not say anything about n < 0.  I would expect
the function to return NULL for n < 0.  In any case, the comment below
should probably mention what happens for n < 0;

> + return node;
> +
> +*n = *n - 1;

Perhaps *n -= 1?  Or even --(*n)?

> +for (i = 0; i < node->children && !ret; i++) {

Consider s/i++/++i/.

Regards,
  Dmitry

> + mime_node_t *child = mime_node_child (node, i);
> + ret = _mime_node_seek_dfs_walk (child, n);
> + if (!ret)
> + talloc_free (child);
> +}
> +return ret;
> +}
> +
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n)
> +{
> +return _mime_node_seek_dfs_walk (node, &n);
> +}
> diff --git a/notmuch-client.h b/notmuch-client.h
> index fce1187..f215d4b 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -318,5 +318,10 @@ mime_node_open (const void *ctx, notmuch_message_t 
> *message,
>  mime_node_t *
>  mime_node_child (const mime_node_t *parent, int child);
>  
> +/* Return the nth child of node in a depth-first traversal.  If n is
> + * 0, returns node itself.  Returns NULL if there is no such part. */
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n);
> +
>  #include "command-line-arguments.h"
>  #endif
> -- 
> 1.7.7.3
> 
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Dmitry Kurochkin]

Hi Austin.

+/* The number of children of this part. */
+int children;

Consider renaming to children_count or similar to make it clear that it
is a counter and not the actual children.

+notmuch_bool_t decrypt_success;

Perhaps s/decrypt_success/is_decrypted/ for consistency with
is_encrypted?

+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif

#include should go above declarations.

+mime_node_t *out = talloc_zero (parent, mime_node_t);

Perhaps s/out/node/?

+   /* this violates RFC 3156 section 4, so we won't bother with it. */
+   fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+"message (should be exactly 2)\n",
+out->children);

Perhaps s/should be exactly/must be exactly/?  That is what the
RFC says.  Same for signature.

+   out->is_encrypted = TRUE;
+   out->is_signed = TRUE;

These are set only if we do decryption/verification.  But their
names and comments imply that they should reflect properties of
the part and be set independently from whether we are actually
doing decryption or verification.

+   /* For some reason the GMimeSignatureValidity returned
+* here is not a const (inconsistent with that
+* returned by
+* g_mime_multipart_encrypted_get_signature_validity,
+* and therefore needs to be properly disposed of.
+* Hopefully the API will become more consistent. */

Ouch.  In gmime 2.6 this API has changed, but looks like the
issue is still there.  Is there a bug for it?  If yes, we should
add a reference to the comment.  Otherwise, we should file the
bug and then add a reference to the comment :)

Both decryption and verification use cryptoctx.  But decryption
is done iff decrypt is true (without checking if cryptoctx is
set) and verification is done iff cryptoctx is set (without any
special flag).  Why is it asymmetric?  Do we need to check if
cryptoctx is set for decryption?

In mime_node_child():

+   GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+   if (child == 1 && parent->decrypted_child)
+   return _mime_node_create (parent, parent->decrypted_child);

Multipart is not needed here, please move it's declaration below
the condition.

+   GMimeObject *child = g_mime_message_get_mime_part (message);

The child variable eclipses the (int child) parameter.  Consider
using a different name for the variable (or parameter).

+   return _mime_node_create (parent, parent->decrypted_child);
+   return _mime_node_create (parent, g_mime_multipart_get_part (multipart, 
child));
...
+   return _mime_node_create (parent, child);

All returns are similar.  Consider declaring a local variable for
storing the part and using a single return, i.e.:

  GMimeObject *part;
  if (...)
part = ...;
  else ...
part = ...;

  return _mime_node_create (parent, part);

Regards,
  Dmitry

[PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Dmitry Kurochkin]

On Fri, 9 Dec 2011 14:54:26 -0500, Austin Clements  wrote:
> Quoth Dmitry Kurochkin on Dec 09 at 11:05 pm:
> > On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements  
> > wrote:
> > >  }
> > >  
> > >  notmuch_status_t
> > > -show_message_body (const char *filename,
> > > +show_message_body (notmuch_message_t *message,
> > >  const notmuch_show_format_t *format,
> > >  notmuch_show_params_t *params)
> > 
> > Is show_message_body() (or functions that it calls/would call) supposed
> > to modify the message structure?  If not, we should make it const.
> 
> That would be nice, but lack of const in libnotmuch makes it difficult
> to do this (for example, notmuch_message_get_filename, which
> show_message_body calls, takes a non-const notmuch_message_t *).
> 
> OTOH, since functions like notmuch_message_get_filename lazily compute
> fields of notmuch_message_t and C has no equivalent of C++'s mutable,
> it's not clear making the message const is even the right thing to do.
> 

If there are fields that are computed lazily (I just did not know it),
we can not make it const.


The patch looks good.  I think it can be pushed before the rest of the
patches are reviewed/ready.

Regards,
  Dmitry

> > I would also make all pointers constant (i.e. const notmuch_message_t
> > *const message), but I can not insist since it is not common in notmuch.
> > 
> > Regards,
> >   Dmitry

[PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Dmitry Kurochkin]

On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements  wrote:
> In addition to simplifying the code, we'll need the notmuch_message_t*
> in show_message_body shortly.
> ---
>  notmuch-client.h |2 +-
>  notmuch-reply.c  |3 +--
>  notmuch-show.c   |3 +--
>  show-message.c   |3 ++-
>  4 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index b50cb38..d7fb6ee 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -162,7 +162,7 @@ char *
>  query_string_from_args (void *ctx, int argc, char *argv[]);
>  
>  notmuch_status_t
> -show_message_body (const char *filename,
> +show_message_body (notmuch_message_t *message,
>  const notmuch_show_format_t *format,
>  notmuch_show_params_t *params);
>  
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 7ac879f..f8d5f64 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
>   notmuch_message_get_header (message, "date"),
>   notmuch_message_get_header (message, "from"));
>  
> - show_message_body (notmuch_message_get_filename (message),
> -format, params);
> + show_message_body (message, format, params);
>  
>   notmuch_message_destroy (message);
>  }
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..1dee3aa 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -753,8 +753,7 @@ show_message (void *ctx,
>  }
>  
>  if (format->part_content)
> - show_message_body (notmuch_message_get_filename (message),
> -format, params);
> + show_message_body (message, format, params);
>  
>  if (params->part <= 0) {
>   fputs (format->body_end, stdout);
> diff --git a/show-message.c b/show-message.c
> index d83f04e..09fa607 100644
> --- a/show-message.c
> +++ b/show-message.c
> @@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
>  }
>  
>  notmuch_status_t
> -show_message_body (const char *filename,
> +show_message_body (notmuch_message_t *message,
>  const notmuch_show_format_t *format,
>  notmuch_show_params_t *params)

Is show_message_body() (or functions that it calls/would call) supposed
to modify the message structure?  If not, we should make it const.

I would also make all pointers constant (i.e. const notmuch_message_t
*const message), but I can not insist since it is not common in notmuch.

Regards,
  Dmitry

>  {
> @@ -183,6 +183,7 @@ show_message_body (const char *filename,
>  GMimeParser *parser = NULL;
>  GMimeMessage *mime_message = NULL;
>  notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> +const char *filename = notmuch_message_get_filename (message);
>  FILE *file = NULL;
>  show_message_state_t state;
>  
> -- 
> 1.7.5.4
> 

[PATCH v2 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

Hi Austin.

On Fri, 9 Dec 2011 12:39:13 -0500, Austin Clements  wrote:
> Just a reminder that this series awaits review and is the first step
> in a better, simpler, and cleaner show that will make possible things
> like improvements to the JSON format, better encoding handling, proper
> raw support, and hierarchical part numbering.
> 

Thank you very much for this work.  And shame on me for not reviewing
it.  But I have been too busy with other stuff lately.  I hope I can get
to it this weekend.

Regards,
  Dmitry

> (I/git screwed up the CC line on the original patch series email.  It
> was supposed to be CC'd to everyone who had expressed an interest in
> this; sorry for any confusion.)
> 
> Quoth myself on Dec 04 at  2:31 pm:
> > This version addresses the comments in Jani's review
> > (id:8739d6u4ju.fsf at nikula.org).

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Dmitry Kurochkin]

Hi Austin.

+/* The number of children of this part. */
+int children;

Consider renaming to children_count or similar to make it clear that it
is a counter and not the actual children.

+notmuch_bool_t decrypt_success;

Perhaps s/decrypt_success/is_decrypted/ for consistency with
is_encrypted?

+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif

#include should go above declarations.

+mime_node_t *out = talloc_zero (parent, mime_node_t);

Perhaps s/out/node/?

+   /* this violates RFC 3156 section 4, so we won't bother with it. */
+   fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
+"message (should be exactly 2)\n",
+out->children);

Perhaps s/should be exactly/must be exactly/?  That is what the
RFC says.  Same for signature.

+   out->is_encrypted = TRUE;
+   out->is_signed = TRUE;

These are set only if we do decryption/verification.  But their
names and comments imply that they should reflect properties of
the part and be set independently from whether we are actually
doing decryption or verification.

+   /* For some reason the GMimeSignatureValidity returned
+* here is not a const (inconsistent with that
+* returned by
+* g_mime_multipart_encrypted_get_signature_validity,
+* and therefore needs to be properly disposed of.
+* Hopefully the API will become more consistent. */

Ouch.  In gmime 2.6 this API has changed, but looks like the
issue is still there.  Is there a bug for it?  If yes, we should
add a reference to the comment.  Otherwise, we should file the
bug and then add a reference to the comment :)

Both decryption and verification use cryptoctx.  But decryption
is done iff decrypt is true (without checking if cryptoctx is
set) and verification is done iff cryptoctx is set (without any
special flag).  Why is it asymmetric?  Do we need to check if
cryptoctx is set for decryption?

In mime_node_child():

+   GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);
+   if (child == 1 && parent->decrypted_child)
+   return _mime_node_create (parent, parent->decrypted_child);

Multipart is not needed here, please move it's declaration below
the condition.

+   GMimeObject *child = g_mime_message_get_mime_part (message);

The child variable eclipses the (int child) parameter.  Consider
using a different name for the variable (or parameter).

+   return _mime_node_create (parent, parent->decrypted_child);
+   return _mime_node_create (parent, g_mime_multipart_get_part (multipart, 
child));
...
+   return _mime_node_create (parent, child);

All returns are similar.  Consider declaring a local variable for
storing the part and using a single return, i.e.:

  GMimeObject *part;
  if (...)
part = ...;
  else ...
part = ...;

  return _mime_node_create (parent, part);

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Dmitry Kurochkin]

On Fri, 9 Dec 2011 14:54:26 -0500, Austin Clements  wrote:
> Quoth Dmitry Kurochkin on Dec 09 at 11:05 pm:
> > On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements  
> > wrote:
> > >  }
> > >  
> > >  notmuch_status_t
> > > -show_message_body (const char *filename,
> > > +show_message_body (notmuch_message_t *message,
> > >  const notmuch_show_format_t *format,
> > >  notmuch_show_params_t *params)
> > 
> > Is show_message_body() (or functions that it calls/would call) supposed
> > to modify the message structure?  If not, we should make it const.
> 
> That would be nice, but lack of const in libnotmuch makes it difficult
> to do this (for example, notmuch_message_get_filename, which
> show_message_body calls, takes a non-const notmuch_message_t *).
> 
> OTOH, since functions like notmuch_message_get_filename lazily compute
> fields of notmuch_message_t and C has no equivalent of C++'s mutable,
> it's not clear making the message const is even the right thing to do.
> 

If there are fields that are computed lazily (I just did not know it),
we can not make it const.


The patch looks good.  I think it can be pushed before the rest of the
patches are reviewed/ready.

Regards,
  Dmitry

> > I would also make all pointers constant (i.e. const notmuch_message_t
> > *const message), but I can not insist since it is not common in notmuch.
> > 
> > Regards,
> >   Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] show: Pass notmuch_message_t instead of path to show_message_body.

[Dmitry Kurochkin]

On Sun,  4 Dec 2011 14:31:37 -0500, Austin Clements  wrote:
> In addition to simplifying the code, we'll need the notmuch_message_t*
> in show_message_body shortly.
> ---
>  notmuch-client.h |2 +-
>  notmuch-reply.c  |3 +--
>  notmuch-show.c   |3 +--
>  show-message.c   |3 ++-
>  4 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index b50cb38..d7fb6ee 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -162,7 +162,7 @@ char *
>  query_string_from_args (void *ctx, int argc, char *argv[]);
>  
>  notmuch_status_t
> -show_message_body (const char *filename,
> +show_message_body (notmuch_message_t *message,
>  const notmuch_show_format_t *format,
>  notmuch_show_params_t *params);
>  
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 7ac879f..f8d5f64 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -546,8 +546,7 @@ notmuch_reply_format_default(void *ctx,
>   notmuch_message_get_header (message, "date"),
>   notmuch_message_get_header (message, "from"));
>  
> - show_message_body (notmuch_message_get_filename (message),
> -format, params);
> + show_message_body (message, format, params);
>  
>   notmuch_message_destroy (message);
>  }
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..1dee3aa 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -753,8 +753,7 @@ show_message (void *ctx,
>  }
>  
>  if (format->part_content)
> - show_message_body (notmuch_message_get_filename (message),
> -format, params);
> + show_message_body (message, format, params);
>  
>  if (params->part <= 0) {
>   fputs (format->body_end, stdout);
> diff --git a/show-message.c b/show-message.c
> index d83f04e..09fa607 100644
> --- a/show-message.c
> +++ b/show-message.c
> @@ -175,7 +175,7 @@ show_message_part (GMimeObject *part,
>  }
>  
>  notmuch_status_t
> -show_message_body (const char *filename,
> +show_message_body (notmuch_message_t *message,
>  const notmuch_show_format_t *format,
>  notmuch_show_params_t *params)

Is show_message_body() (or functions that it calls/would call) supposed
to modify the message structure?  If not, we should make it const.

I would also make all pointers constant (i.e. const notmuch_message_t
*const message), but I can not insist since it is not common in notmuch.

Regards,
  Dmitry

>  {
> @@ -183,6 +183,7 @@ show_message_body (const char *filename,
>  GMimeParser *parser = NULL;
>  GMimeMessage *mime_message = NULL;
>  notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> +const char *filename = notmuch_message_get_filename (message);
>  FILE *file = NULL;
>  show_message_state_t state;
>  
> -- 
> 1.7.5.4
> 
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 0/4] First step of 'show' rewrite

[Dmitry Kurochkin]

Hi Austin.

On Fri, 9 Dec 2011 12:39:13 -0500, Austin Clements  wrote:
> Just a reminder that this series awaits review and is the first step
> in a better, simpler, and cleaner show that will make possible things
> like improvements to the JSON format, better encoding handling, proper
> raw support, and hierarchical part numbering.
> 

Thank you very much for this work.  And shame on me for not reviewing
it.  But I have been too busy with other stuff lately.  I hope I can get
to it this weekend.

Regards,
  Dmitry

> (I/git screwed up the CC line on the original patch series email.  It
> was supposed to be CC'd to everyone who had expressed an interest in
> this; sorry for any confusion.)
> 
> Quoth myself on Dec 04 at  2:31 pm:
> > This version addresses the comments in Jani's review
> > (id:8739d6u4ju@nikula.org).
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] debian: add a recommends for w3m-el or w3m-el-snapshot to notmuch-emacs

[Dmitry Kurochkin]

On Thu, 08 Dec 2011 08:32:12 -0400, David Bremner  wrote:
> On Thu, 08 Dec 2011 13:33:09 +0400, Dmitry Kurochkin  gmail.com> wrote:
> 
> > I am not sure recommending w3m-el is a good idea.  It has very annoying
> > bugs with scrolling (C-n): at some point the cursor jumps back above,
> > i.e. it goes in a loop.  I did not investigate the issue though.
> 
> Hi Dmitry;
> 
> What do you think should be the default setup for rendering HTML in
> notmuch?
> 

I think standalone w3m is currently better than w3m-el because of the
cursor move issue I described above.  It has issues with charset, but it
should not affect most (i.e. English-only) users (and should be fixed
some day :)).

w3m-el would be better if we investigate and fix the cursor move issue.

Regards,
  Dmitry

> d

[PATCH] debian: add a recommends for w3m-el or w3m-el-snapshot to notmuch-emacs

[Dmitry Kurochkin]

Hi David.

I am not sure recommending w3m-el is a good idea.  It has very annoying
bugs with scrolling (C-n): at some point the cursor jumps back above,
i.e. it goes in a loop.  I did not investigate the issue though.

Regards,
  Dmitry

Re: [PATCH] debian: add a recommends for w3m-el or w3m-el-snapshot to notmuch-emacs

[Dmitry Kurochkin]

On Thu, 08 Dec 2011 08:32:12 -0400, David Bremner  wrote:
> On Thu, 08 Dec 2011 13:33:09 +0400, Dmitry Kurochkin 
>  wrote:
> 
> > I am not sure recommending w3m-el is a good idea.  It has very annoying
> > bugs with scrolling (C-n): at some point the cursor jumps back above,
> > i.e. it goes in a loop.  I did not investigate the issue though.
> 
> Hi Dmitry;
> 
> What do you think should be the default setup for rendering HTML in
> notmuch?
> 

I think standalone w3m is currently better than w3m-el because of the
cursor move issue I described above.  It has issues with charset, but it
should not affect most (i.e. English-only) users (and should be fixed
some day :)).

w3m-el would be better if we investigate and fix the cursor move issue.

Regards,
  Dmitry

> d
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] debian: add a recommends for w3m-el or w3m-el-snapshot to notmuch-emacs

[Dmitry Kurochkin]

Hi David.

I am not sure recommending w3m-el is a good idea.  It has very annoying
bugs with scrolling (C-n): at some point the cursor jumps back above,
i.e. it goes in a loop.  I did not investigate the issue though.

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/2] emacs: remove some code duplication in notmuch-show

[Dmitry Kurochkin]

David, this seems ready for push as well.

Regards,
  Dmitry

[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 1/2] emacs: remove some code duplication in notmuch-show

[Dmitry Kurochkin]

David, this seems ready for push as well.

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] test: cleanup gdb external dependency in atomicity tests

[Dmitry Kurochkin]

Change atomicity tests to use the new external binary dependencies.
This simplifies the code and makes output consistent.
---
 test/atomicity |   12 
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/test/atomicity b/test/atomicity
index ad7d4a3..6df0a00 100755
--- a/test/atomicity
+++ b/test/atomicity
@@ -7,8 +7,7 @@ test_description='atomicity'
 # final database contents should be the same regardless of when (or
 # if) it is killed and restarted.

-if which gdb 1>/dev/null 2>&1; then
-test_set_prereq GDB
+if test_require_external_prereq gdb; then

 # Create a maildir structure to also stress flag synchronization
 mkdir $MAIL_DIR/cur
@@ -91,14 +90,11 @@ if which gdb 1>/dev/null 2>&1; then
i=$(expr $end - 1)
fi
 done
-else
-say_color info "%-6s" "WARNING"
-echo " Missing test prerequisite GDB"
-fi  
+fi

 test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
-test_expect_equal_file GDB searchall expectall
+test_expect_equal_file searchall expectall

-test_expect_success GDB "detected $outcount>10 abort points" "test $outcount 
-gt 10"
+test_expect_success "detected $outcount>10 abort points" "test $outcount -gt 
10"

 test_done
-- 
1.7.7.3

[PATCH] test: cleanup gdb external dependency in atomicity tests

[Dmitry Kurochkin]

Change atomicity tests to use the new external binary dependencies.
This simplifies the code and makes output consistent.
---
 test/atomicity |   12 
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/test/atomicity b/test/atomicity
index ad7d4a3..6df0a00 100755
--- a/test/atomicity
+++ b/test/atomicity
@@ -7,8 +7,7 @@ test_description='atomicity'
 # final database contents should be the same regardless of when (or
 # if) it is killed and restarted.
 
-if which gdb 1>/dev/null 2>&1; then
-test_set_prereq GDB
+if test_require_external_prereq gdb; then
 
 # Create a maildir structure to also stress flag synchronization
 mkdir $MAIL_DIR/cur
@@ -91,14 +90,11 @@ if which gdb 1>/dev/null 2>&1; then
i=$(expr $end - 1)
fi
 done
-else
-say_color info "%-6s" "WARNING"
-echo " Missing test prerequisite GDB"
-fi  
+fi
 
 test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
-test_expect_equal_file GDB searchall expectall
+test_expect_equal_file searchall expectall
 
-test_expect_success GDB "detected $outcount>10 abort points" "test $outcount 
-gt 10"
+test_expect_success "detected $outcount>10 abort points" "test $outcount -gt 
10"
 
 test_done
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] emacs: Use notmuch-command variable in process-lines.

[Dmitry Kurochkin]

+1

Regards,
  Dmitry

[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 at notmuchmail.org'
 emacs_deliver_message \
 'Message with audio/mpeg attachment' \
-- 
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 at 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 at 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 at 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 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" < "$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 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 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  wrote:
> Hi Dmitry.
> 
> On Tue, 29 Nov 2011 01:26:39 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > Hi Tomi.
> > 
> > On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila  
> > wrote:
> > > On Mon, 28 Nov 2011 07:28:13 +0400, 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" < > > > +#!/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" < > > #!/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.' 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. 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. 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] emacs: Use notmuch-command variable in process-lines.

[Dmitry Kurochkin]

+1

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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" < "$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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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-attachm...@notmuchmail.org'
 emacs_deliver_message \
 'Message with audio/mpeg attachment' \
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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-attachm...@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-attachm...@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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

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

Hi Tomi.

On Tue, 29 Nov 2011 14:58:00 +0200, Tomi Ollila  wrote:
> Hi Dmitry.
> 
> On Tue, 29 Nov 2011 01:26:39 +0400, Dmitry Kurochkin 
>  wrote:
> > Hi Tomi.
> > 
> > On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila  wrote:
> > > On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin 
> > >  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" < > > > +#!/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" < > > #!/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.' 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. 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. 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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Tue, 29 Nov 2011 02:13:53 +0400, Dmitry Kurochkin 
 wrote:
> On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin 
>  wrote:
> > On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila  wrote:
> > > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin 
> > >  wrote:
> > > > There is existing support for general prerequisites in the test suite.
> > > > But it is not very convenient to use: every test case has to keep
> > > > track for it's dependencies and they have to be explicitly listed.
> > > > 
> > > > The patch aims to add better support for a particular type of external
> > > > dependencies: external executables.  The main idea is to replace
> > > > missing external binaries with shell functions that have the same
> > > > name.  These functions always fail and keep track of missing
> > > > dependencies for a subtest.  The result reporting functions later can
> > > > check that an external binaries are missing and correctly report SKIP
> > > > result instead of FAIL.  The primary benefit is that the test cases do
> > > > not need to declare their dependencies or be changed in any way.
> > > > ---
> > > >  test/test-lib.sh |   49 
> > > > +
> > > >  1 files changed, 41 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > > index f21e45e..ab8c6fd 100755
> > > > --- a/test/test-lib.sh
> > > > +++ b/test/test-lib.sh
> > > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> > > >  # - Implicitly by specifying the prerequisite tag in the calls to
> > > >  #   test_expect_{success,failure,code}.
> > > >  #
> > > >  # The single parameter is the prerequisite tag (a simple word, in all
> > > >  # capital letters by convention).
> > > >  
> > > >  test_set_prereq () {
> > > > satisfied="$satisfied$1 "
> > > >  }
> > > >  satisfied=" "
> > > >  
> > > >  test_have_prereq () {
> > > > case $satisfied in
> > > > *" $1 "*)
> > > > : yes, have it ;;
> > > > *)
> > > > ! : nope ;;
> > > > esac
> > > >  }
> > > >  
> > > > +# declare prerequisite for the given external binary
> > > > +test_declare_external_prereq () {
> > > > +   binary="$1"
> > > > +   test "$#" = 2 && name=$2 || name="$binary(1)"
> > > > +
> > > > +hash $binary 2>/dev/null || eval "
> > > > +$1 () {
> > > > +   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e 
> > > > \" $name \" ||
> > > > +   
> > > > test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
> > > >  $name\"
> > > > +   false
> > > > +}"
> > > > +}
> > > > +
> > > 
> > > Does this work right ?
> > 
> > It does not.
> > 
> > Moreover, there is a missing backslash before
> > $test_subtest_missing_external_prereqs_ (and that is why I did not
> > notice the bug during my poor testing).
> > 
> > > ... the grep -e \" $name \"   -- part requires
> > > spaces on both side of, but next line does not write trailing space...
> > > ... and is there leading space (and also the latest
> > > $test_subtest_missing_external_prereqs_ (just before 'false') is 
> > > evaluated) ?
> > > 
> > > This could perhaps be written like the above test_set_prereq &
> > > test_save_prereq:
> > > ...
> > > hash $binary 2>/dev/null || eval "
> > > $binary () {
> > >   test_missing_external_prereq_${binary}_=t
> > >   case \$test_subtest_missing_external_prereqs_ in
> > >   *' $name '*) ;;
> > > *) 
> > > test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name
> > >  \"
> > > esac
> > >   false
> > > }
> > > 
> > > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> > > 
> > 
> > Well, this approach would obviously be better

Re: [PATCH 1/2] emacs: remove some code duplication in notmuch-show

[Dmitry Kurochkin]

Hi Jamie.

On Mon, 28 Nov 2011 06:24:19 -0800, Jameson Graef Rollins 
 wrote:
> On Sat, 26 Nov 2011 02:23:30 +0400, Dmitry Kurochkin 
>  wrote:
> > -(defun notmuch-show-get-header (header)
> > +(defun notmuch-show-get-header (header &optional props)
> >"Return the named header of the current message, if any."
> > -  (plist-get (notmuch-show-get-prop :headers) header))
> > +  (plist-get (notmuch-show-get-prop :headers props) header))
> 
> Hey, Dmitry.  It looks like the new plist-get call is assuming props is
> defined, but it looks like it's only optional in the argument list.
> Wouldn't the function fail if the props argument is not supplied?
> 

If props is not supplied it is bound to nil.  There is no special
"undefined" value.

Regards,
  Dmitry

> jamie.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 4/4] test: fix error messages for missing binary dependencies

[Dmitry Kurochkin]

The fake missing binary functions check if the binary has already be
added to the diagnostic message to avoid duplicates.  Unfortunately,
this check was buggy because the message string does not have the
trailing space.
---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 2861d88..a975957 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -556,7 +556,7 @@ test_declare_external_prereq () {
hash $binary 2>/dev/null || eval "
test_missing_external_prereq_${binary}_=t
 $binary () {
-   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -qe \" 
$name \" ||
+   echo -n \"\$test_subtest_missing_external_prereqs_ \" | grep -qe \" 
$name \" ||

test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
 $name\"
false
 }"
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 3/4] test: fix spurious output from missing external binaries functions

[Dmitry Kurochkin]

The grep(1) command used in the fake binary functions was missing the
quiet option.
---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 9dcb2d2..2861d88 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -556,7 +556,7 @@ test_declare_external_prereq () {
hash $binary 2>/dev/null || eval "
test_missing_external_prereq_${binary}_=t
 $binary () {
-   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
\" ||
+   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -qe \" 
$name \" ||

test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
 $name\"
false
 }"
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/4] test: add missing escape backslash in test_declare_external_prereq()

[Dmitry Kurochkin]

---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index d7282ff..9dcb2d2 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -557,7 +557,7 @@ test_declare_external_prereq () {
test_missing_external_prereq_${binary}_=t
 $binary () {
echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
\" ||
-   
test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
 $name\"
+   
test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
 $name\"
false
 }"
 }
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/4] test: fix test_require_external_prereq()

[Dmitry Kurochkin]

test_missing_external_prereq_${binary}_ variable indicates that the
binary is missing.  It must be set in test_declare_external_prereq()
outside of the fake $binary() function.
---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 11e6646..d7282ff 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -554,8 +554,8 @@ test_declare_external_prereq () {
test "$#" = 2 && name=$2 || name="$binary(1)"
 
hash $binary 2>/dev/null || eval "
-$binary () {
test_missing_external_prereq_${binary}_=t
+$binary () {
echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
\" ||

test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
 $name\"
false
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 0/4] fix a bunch of bugs in recently added binary dependencies

[Dmitry Kurochkin]

Hello.

This is a series of trivial but important fixes for the recently added
binary dependencies.

Special thanks goes to Tomi Ollila who did review of the original
patches.

Regards,
  Dmitry

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin 
 wrote:
> On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila  wrote:
> > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin 
> >  wrote:
> > > There is existing support for general prerequisites in the test suite.
> > > But it is not very convenient to use: every test case has to keep
> > > track for it's dependencies and they have to be explicitly listed.
> > > 
> > > The patch aims to add better support for a particular type of external
> > > dependencies: external executables.  The main idea is to replace
> > > missing external binaries with shell functions that have the same
> > > name.  These functions always fail and keep track of missing
> > > dependencies for a subtest.  The result reporting functions later can
> > > check that an external binaries are missing and correctly report SKIP
> > > result instead of FAIL.  The primary benefit is that the test cases do
> > > not need to declare their dependencies or be changed in any way.
> > > ---
> > >  test/test-lib.sh |   49 +
> > >  1 files changed, 41 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > index f21e45e..ab8c6fd 100755
> > > --- a/test/test-lib.sh
> > > +++ b/test/test-lib.sh
> > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> > >  # - Implicitly by specifying the prerequisite tag in the calls to
> > >  #   test_expect_{success,failure,code}.
> > >  #
> > >  # The single parameter is the prerequisite tag (a simple word, in all
> > >  # capital letters by convention).
> > >  
> > >  test_set_prereq () {
> > >   satisfied="$satisfied$1 "
> > >  }
> > >  satisfied=" "
> > >  
> > >  test_have_prereq () {
> > >   case $satisfied in
> > >   *" $1 "*)
> > >   : yes, have it ;;
> > >   *)
> > >   ! : nope ;;
> > >   esac
> > >  }
> > >  
> > > +# declare prerequisite for the given external binary
> > > +test_declare_external_prereq () {
> > > + binary="$1"
> > > + test "$#" = 2 && name=$2 || name="$binary(1)"
> > > +
> > > +hash $binary 2>/dev/null || eval "
> > > +$1 () {
> > > + echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
> > > \" ||
> > > + 
> > > test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
> > >  $name\"
> > > + false
> > > +}"
> > > +}
> > > +
> > 
> > Does this work right ?
> 
> It does not.
> 
> Moreover, there is a missing backslash before
> $test_subtest_missing_external_prereqs_ (and that is why I did not
> notice the bug during my poor testing).
> 
> > ... the grep -e \" $name \"   -- part requires
> > spaces on both side of, but next line does not write trailing space...
> > ... and is there leading space (and also the latest
> > $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) 
> > ?
> > 
> > This could perhaps be written like the above test_set_prereq &
> > test_save_prereq:
> > ...
> > hash $binary 2>/dev/null || eval "
> > $binary () {
> > test_missing_external_prereq_${binary}_=t
> > case \$test_subtest_missing_external_prereqs_ in
> > *' $name '*) ;;
> > *) 
> > test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name
> >  \"
> > esac
> > false
> > }
> > 
> > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> > 
> 
> Well, this approach would obviously be better, at least because it does
> work.  But IMO it is too complex, and essentially has the same problem
> as the current code: it mixes the check with diagnostic message.
> 
> We already have a proper way to check if dependency is missing already:
> test_missing_external_prereq_${binary}_.  We should check it and add the
> binary to test_subtest_missing_external_prereqs_ if it is not set.  And
> move setting of test_missing_external_prereq_${binary}_ below.
> 
> > (I took some code from current git head also perhaps instead of 
> > setting test_missing_external_prereq_${binary}_=t, (bash)

Re: [PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila  wrote:
> On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin 
>  wrote:
> > There is existing support for general prerequisites in the test suite.
> > But it is not very convenient to use: every test case has to keep
> > track for it's dependencies and they have to be explicitly listed.
> > 
> > The patch aims to add better support for a particular type of external
> > dependencies: external executables.  The main idea is to replace
> > missing external binaries with shell functions that have the same
> > name.  These functions always fail and keep track of missing
> > dependencies for a subtest.  The result reporting functions later can
> > check that an external binaries are missing and correctly report SKIP
> > result instead of FAIL.  The primary benefit is that the test cases do
> > not need to declare their dependencies or be changed in any way.
> > ---
> >  test/test-lib.sh |   49 +
> >  1 files changed, 41 insertions(+), 8 deletions(-)
> > 
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index f21e45e..ab8c6fd 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> >  # - Implicitly by specifying the prerequisite tag in the calls to
> >  #   test_expect_{success,failure,code}.
> >  #
> >  # The single parameter is the prerequisite tag (a simple word, in all
> >  # capital letters by convention).
> >  
> >  test_set_prereq () {
> > satisfied="$satisfied$1 "
> >  }
> >  satisfied=" "
> >  
> >  test_have_prereq () {
> > case $satisfied in
> > *" $1 "*)
> > : yes, have it ;;
> > *)
> > ! : nope ;;
> > esac
> >  }
> >  
> > +# declare prerequisite for the given external binary
> > +test_declare_external_prereq () {
> > +   binary="$1"
> > +   test "$#" = 2 && name=$2 || name="$binary(1)"
> > +
> > +hash $binary 2>/dev/null || eval "
> > +$1 () {
> > +   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
> > \" ||
> > +   
> > test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
> >  $name\"
> > +   false
> > +}"
> > +}
> > +
> 
> Does this work right ?

It does not.

Moreover, there is a missing backslash before
$test_subtest_missing_external_prereqs_ (and that is why I did not
notice the bug during my poor testing).

> ... the grep -e \" $name \"   -- part requires
> spaces on both side of, but next line does not write trailing space...
> ... and is there leading space (and also the latest
> $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?
> 
> This could perhaps be written like the above test_set_prereq &
> test_save_prereq:
> ...
> hash $binary 2>/dev/null || eval "
> $binary () {
>   test_missing_external_prereq_${binary}_=t
>   case \$test_subtest_missing_external_prereqs_ in
>   *' $name '*) ;;
> *) 
> test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name
>  \"
> esac
>   false
> }
> 
> and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> 

Well, this approach would obviously be better, at least because it does
work.  But IMO it is too complex, and essentially has the same problem
as the current code: it mixes the check with diagnostic message.

We already have a proper way to check if dependency is missing already:
test_missing_external_prereq_${binary}_.  We should check it and add the
binary to test_subtest_missing_external_prereqs_ if it is not set.  And
move setting of test_missing_external_prereq_${binary}_ below.

> (I took some code from current git head also perhaps instead of 
> setting test_missing_external_prereq_${binary}_=t, (bash) associative
> arrays could be taken into use -- the eval to read that information
> is a bit hairy -- well, at least that part works for sure :D )
> 

Yes!  I like using hash here.

> Hmm... how about this:
> 
> hash $binary 2>/dev/null || eval "
> $binary () {
> if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]
> then
>   test_missing_external_prereq_[$binary]=t
> 
> test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
>  $name\"
> fi
>   false
> }
> 

There is some inconsistent indenting in the above code (mixed tabs and
spaces).  Also test_require_external_prereq() should be changed as well.

Otherwise I like it.  Would you please submit a patch?

Regards,
  Dmitry

> and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like 
> now.
> 
> Tomi
___
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]

Hi Tomi.

On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila  wrote:
> On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin 
>  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" < > +#!/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" < #!/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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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-attachm...@notmuchmail.org'
 emacs_deliver_message \
 'Message with audio/mpeg attachment' \
-- 
1.7.7.3

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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-attachm...@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-attachm...@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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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" < "$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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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@taco2.nixu.fi"

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Tue, 29 Nov 2011 02:13:53 +0400, Dmitry Kurochkin  wrote:
> On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila  
> > wrote:
> > > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin  > > gmail.com> wrote:
> > > > There is existing support for general prerequisites in the test suite.
> > > > But it is not very convenient to use: every test case has to keep
> > > > track for it's dependencies and they have to be explicitly listed.
> > > > 
> > > > The patch aims to add better support for a particular type of external
> > > > dependencies: external executables.  The main idea is to replace
> > > > missing external binaries with shell functions that have the same
> > > > name.  These functions always fail and keep track of missing
> > > > dependencies for a subtest.  The result reporting functions later can
> > > > check that an external binaries are missing and correctly report SKIP
> > > > result instead of FAIL.  The primary benefit is that the test cases do
> > > > not need to declare their dependencies or be changed in any way.
> > > > ---
> > > >  test/test-lib.sh |   49 
> > > > +
> > > >  1 files changed, 41 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > > index f21e45e..ab8c6fd 100755
> > > > --- a/test/test-lib.sh
> > > > +++ b/test/test-lib.sh
> > > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> > > >  # - Implicitly by specifying the prerequisite tag in the calls to
> > > >  #   test_expect_{success,failure,code}.
> > > >  #
> > > >  # The single parameter is the prerequisite tag (a simple word, in all
> > > >  # capital letters by convention).
> > > >  
> > > >  test_set_prereq () {
> > > > satisfied="$satisfied$1 "
> > > >  }
> > > >  satisfied=" "
> > > >  
> > > >  test_have_prereq () {
> > > > case $satisfied in
> > > > *" $1 "*)
> > > > : yes, have it ;;
> > > > *)
> > > > ! : nope ;;
> > > > esac
> > > >  }
> > > >  
> > > > +# declare prerequisite for the given external binary
> > > > +test_declare_external_prereq () {
> > > > +   binary="$1"
> > > > +   test "$#" = 2 && name=$2 || name="$binary(1)"
> > > > +
> > > > +hash $binary 2>/dev/null || eval "
> > > > +$1 () {
> > > > +   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e 
> > > > \" $name \" ||
> > > > +   
> > > > test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
> > > >  $name\"
> > > > +   false
> > > > +}"
> > > > +}
> > > > +
> > > 
> > > Does this work right ?
> > 
> > It does not.
> > 
> > Moreover, there is a missing backslash before
> > $test_subtest_missing_external_prereqs_ (and that is why I did not
> > notice the bug during my poor testing).
> > 
> > > ... the grep -e \" $name \"   -- part requires
> > > spaces on both side of, but next line does not write trailing space...
> > > ... and is there leading space (and also the latest
> > > $test_subtest_missing_external_prereqs_ (just before 'false') is 
> > > evaluated) ?
> > > 
> > > This could perhaps be written like the above test_set_prereq &
> > > test_save_prereq:
> > > ...
> > > hash $binary 2>/dev/null || eval "
> > > $binary () {
> > >   test_missing_external_prereq_${binary}_=t
> > >   case \$test_subtest_missing_external_prereqs_ in
> > >   *' $name '*) ;;
> > > *) 
> > > test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name
> > >  \"
> > > esac
> > >   false
> > > }
> > > 
> > > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> > > 
> > 
> &

[PATCH 4/4] test: fix error messages for missing binary dependencies

[Dmitry Kurochkin]

The fake missing binary functions check if the binary has already be
added to the diagnostic message to avoid duplicates.  Unfortunately,
this check was buggy because the message string does not have the
trailing space.
---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 2861d88..a975957 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -556,7 +556,7 @@ test_declare_external_prereq () {
hash $binary 2>/dev/null || eval "
test_missing_external_prereq_${binary}_=t
 $binary () {
-   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -qe \" 
$name \" ||
+   echo -n \"\$test_subtest_missing_external_prereqs_ \" | grep -qe \" 
$name \" ||

test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
 $name\"
false
 }"
-- 
1.7.7.3

[PATCH 3/4] test: fix spurious output from missing external binaries functions

[Dmitry Kurochkin]

The grep(1) command used in the fake binary functions was missing the
quiet option.
---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 9dcb2d2..2861d88 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -556,7 +556,7 @@ test_declare_external_prereq () {
hash $binary 2>/dev/null || eval "
test_missing_external_prereq_${binary}_=t
 $binary () {
-   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
\" ||
+   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -qe \" 
$name \" ||

test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
 $name\"
false
 }"
-- 
1.7.7.3

[PATCH 2/4] test: add missing escape backslash in test_declare_external_prereq()

[Dmitry Kurochkin]

---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index d7282ff..9dcb2d2 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -557,7 +557,7 @@ test_declare_external_prereq () {
test_missing_external_prereq_${binary}_=t
 $binary () {
echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
\" ||
-   
test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
 $name\"
+   
test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
 $name\"
false
 }"
 }
-- 
1.7.7.3

[PATCH 1/4] test: fix test_require_external_prereq()

[Dmitry Kurochkin]

test_missing_external_prereq_${binary}_ variable indicates that the
binary is missing.  It must be set in test_declare_external_prereq()
outside of the fake $binary() function.
---
 test/test-lib.sh |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 11e6646..d7282ff 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -554,8 +554,8 @@ test_declare_external_prereq () {
test "$#" = 2 && name=$2 || name="$binary(1)"

hash $binary 2>/dev/null || eval "
-$binary () {
test_missing_external_prereq_${binary}_=t
+$binary () {
echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
\" ||

test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
 $name\"
false
-- 
1.7.7.3

[PATCH 0/4] fix a bunch of bugs in recently added binary dependencies

[Dmitry Kurochkin]

Hello.

This is a series of trivial but important fixes for the recently added
binary dependencies.

Special thanks goes to Tomi Ollila who did review of the original
patches.

Regards,
  Dmitry

[PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin  wrote:
> On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila  wrote:
> > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin  > gmail.com> wrote:
> > > There is existing support for general prerequisites in the test suite.
> > > But it is not very convenient to use: every test case has to keep
> > > track for it's dependencies and they have to be explicitly listed.
> > > 
> > > The patch aims to add better support for a particular type of external
> > > dependencies: external executables.  The main idea is to replace
> > > missing external binaries with shell functions that have the same
> > > name.  These functions always fail and keep track of missing
> > > dependencies for a subtest.  The result reporting functions later can
> > > check that an external binaries are missing and correctly report SKIP
> > > result instead of FAIL.  The primary benefit is that the test cases do
> > > not need to declare their dependencies or be changed in any way.
> > > ---
> > >  test/test-lib.sh |   49 +
> > >  1 files changed, 41 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > index f21e45e..ab8c6fd 100755
> > > --- a/test/test-lib.sh
> > > +++ b/test/test-lib.sh
> > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> > >  # - Implicitly by specifying the prerequisite tag in the calls to
> > >  #   test_expect_{success,failure,code}.
> > >  #
> > >  # The single parameter is the prerequisite tag (a simple word, in all
> > >  # capital letters by convention).
> > >  
> > >  test_set_prereq () {
> > >   satisfied="$satisfied$1 "
> > >  }
> > >  satisfied=" "
> > >  
> > >  test_have_prereq () {
> > >   case $satisfied in
> > >   *" $1 "*)
> > >   : yes, have it ;;
> > >   *)
> > >   ! : nope ;;
> > >   esac
> > >  }
> > >  
> > > +# declare prerequisite for the given external binary
> > > +test_declare_external_prereq () {
> > > + binary="$1"
> > > + test "$#" = 2 && name=$2 || name="$binary(1)"
> > > +
> > > +hash $binary 2>/dev/null || eval "
> > > +$1 () {
> > > + echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
> > > \" ||
> > > + 
> > > test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
> > >  $name\"
> > > + false
> > > +}"
> > > +}
> > > +
> > 
> > Does this work right ?
> 
> It does not.
> 
> Moreover, there is a missing backslash before
> $test_subtest_missing_external_prereqs_ (and that is why I did not
> notice the bug during my poor testing).
> 
> > ... the grep -e \" $name \"   -- part requires
> > spaces on both side of, but next line does not write trailing space...
> > ... and is there leading space (and also the latest
> > $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) 
> > ?
> > 
> > This could perhaps be written like the above test_set_prereq &
> > test_save_prereq:
> > ...
> > hash $binary 2>/dev/null || eval "
> > $binary () {
> > test_missing_external_prereq_${binary}_=t
> > case \$test_subtest_missing_external_prereqs_ in
> > *' $name '*) ;;
> > *) 
> > test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name
> >  \"
> > esac
> > false
> > }
> > 
> > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> > 
> 
> Well, this approach would obviously be better, at least because it does
> work.  But IMO it is too complex, and essentially has the same problem
> as the current code: it mixes the check with diagnostic message.
> 
> We already have a proper way to check if dependency is missing already:
> test_missing_external_prereq_${binary}_.  We should check it and add the
> binary to test_subtest_missing_external_prereqs_ if it is not set.  And
> move setting of test_missing_external_prereq_${binary}_ below.
> 
> > (I took some code from current git head also perhaps instead of 
> > setting test_missing_external_prereq_${binary}_

[PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila  wrote:
> On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > There is existing support for general prerequisites in the test suite.
> > But it is not very convenient to use: every test case has to keep
> > track for it's dependencies and they have to be explicitly listed.
> > 
> > The patch aims to add better support for a particular type of external
> > dependencies: external executables.  The main idea is to replace
> > missing external binaries with shell functions that have the same
> > name.  These functions always fail and keep track of missing
> > dependencies for a subtest.  The result reporting functions later can
> > check that an external binaries are missing and correctly report SKIP
> > result instead of FAIL.  The primary benefit is that the test cases do
> > not need to declare their dependencies or be changed in any way.
> > ---
> >  test/test-lib.sh |   49 +
> >  1 files changed, 41 insertions(+), 8 deletions(-)
> > 
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index f21e45e..ab8c6fd 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> >  # - Implicitly by specifying the prerequisite tag in the calls to
> >  #   test_expect_{success,failure,code}.
> >  #
> >  # The single parameter is the prerequisite tag (a simple word, in all
> >  # capital letters by convention).
> >  
> >  test_set_prereq () {
> > satisfied="$satisfied$1 "
> >  }
> >  satisfied=" "
> >  
> >  test_have_prereq () {
> > case $satisfied in
> > *" $1 "*)
> > : yes, have it ;;
> > *)
> > ! : nope ;;
> > esac
> >  }
> >  
> > +# declare prerequisite for the given external binary
> > +test_declare_external_prereq () {
> > +   binary="$1"
> > +   test "$#" = 2 && name=$2 || name="$binary(1)"
> > +
> > +hash $binary 2>/dev/null || eval "
> > +$1 () {
> > +   echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name 
> > \" ||
> > +   
> > test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_
> >  $name\"
> > +   false
> > +}"
> > +}
> > +
> 
> Does this work right ?

It does not.

Moreover, there is a missing backslash before
$test_subtest_missing_external_prereqs_ (and that is why I did not
notice the bug during my poor testing).

> ... the grep -e \" $name \"   -- part requires
> spaces on both side of, but next line does not write trailing space...
> ... and is there leading space (and also the latest
> $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?
> 
> This could perhaps be written like the above test_set_prereq &
> test_save_prereq:
> ...
> hash $binary 2>/dev/null || eval "
> $binary () {
>   test_missing_external_prereq_${binary}_=t
>   case \$test_subtest_missing_external_prereqs_ in
>   *' $name '*) ;;
> *) 
> test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name
>  \"
> esac
>   false
> }
> 
> and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> 

Well, this approach would obviously be better, at least because it does
work.  But IMO it is too complex, and essentially has the same problem
as the current code: it mixes the check with diagnostic message.

We already have a proper way to check if dependency is missing already:
test_missing_external_prereq_${binary}_.  We should check it and add the
binary to test_subtest_missing_external_prereqs_ if it is not set.  And
move setting of test_missing_external_prereq_${binary}_ below.

> (I took some code from current git head also perhaps instead of 
> setting test_missing_external_prereq_${binary}_=t, (bash) associative
> arrays could be taken into use -- the eval to read that information
> is a bit hairy -- well, at least that part works for sure :D )
> 

Yes!  I like using hash here.

> Hmm... how about this:
> 
> hash $binary 2>/dev/null || eval "
> $binary () {
> if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]
> then
>   test_missing_external_prereq_[$binary]=t
> 
> test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_
>  $name\"
> fi
>   false
> }
> 

There is some inconsistent indenting in the above code (mixed tabs and
spaces).  Also test_require_external_prereq() should be changed as well.

Otherwise I like it.  Would you please submit a patch?

Regards,
  Dmitry

> and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like 
> now.
> 
> Tomi

[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  wrote:
> On Mon, 28 Nov 2011 07:28:13 +0400, 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" < > +#!/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" < #!/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

[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 at notmuchmail.org'
 emacs_deliver_message \
 'Message with audio/mpeg attachment' \
-- 
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 at 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 at 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 at 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 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" < "$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 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 at taco2.nixu.fi"

[PATCH 1/2] emacs: remove some code duplication in notmuch-show

[Dmitry Kurochkin]

Hi Jamie.

On Mon, 28 Nov 2011 06:24:19 -0800, Jameson Graef Rollins  wrote:
> On Sat, 26 Nov 2011 02:23:30 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > -(defun notmuch-show-get-header (header)
> > +(defun notmuch-show-get-header (header &optional props)
> >"Return the named header of the current message, if any."
> > -  (plist-get (notmuch-show-get-prop :headers) header))
> > +  (plist-get (notmuch-show-get-prop :headers props) header))
> 
> Hey, Dmitry.  It looks like the new plist-get call is assuming props is
> defined, but it looks like it's only optional in the argument list.
> Wouldn't the function fail if the props argument is not supplied?
> 

If props is not supplied it is bound to nil.  There is no special
"undefined" value.

Regards,
  Dmitry

> jamie.

[PATCH v6 1/2] emacs: User-defined sections in notmuch-hello

[Dmitry Kurochkin]

Hi Daniel.

On Thu, 24 Nov 2011 15:01:01 +0100, Daniel Schoepe  
wrote:
> On Thu, 24 Nov 2011 09:54:50 -0400, David Bremner  
> wrote:
> > On Mon, 10 Oct 2011 15:39:41 +0200, Daniel Schoepe  
> > wrote:
> > > From: Daniel Schoepe 
> > > 
> > > This patch makes the notmuch-hello screen fully customizable
> > > by allowing the user to add and remove arbitrary sections. It
> > > also provides some convenience functions for constructing sections,
> > > e.g. showing the unread message count for each tag.
> > > 
> > 
> > Yow, that is a big patch, I think it needs some more review.
> 
> Yes; Unfortunately I didn't manage to come up with good partitioning
> into smaller patches, because the changes are quite interconnected.
> 
> Is there something I can do to help with the review?
> 

Looking at earlier threads, I found an unanswered email from Austin [1].
IMO it has some good ideas and I would like to see your answer to it.

Sorry if I just missed it.  Would appreciate a link if that is the case.

Regards,
  Dmitry

[1] id:"BANLkTinKCLg1epsGvfxtyy-_G2Hm=gjUMQ at mail.gmail.com"

> I think there's also one performance improvement written by Michal
> Sojka, that isn't included in this version yet, but since patches are
> getting applied more timely now, I guess that can be merged afterwards.
> 
> Cheers,
> Daniel
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[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 at notmuchmail.org'
 emacs_deliver_message \
 'Message with audio/mpeg attachment' \
-- 
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 198c27b..20f8449 100755
--- a/test/emacs
+++ b/test/emacs
@@ -472,4 +472,35 @@ test_emacs '(notmuch-show 
"id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae at 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 at 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 at 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 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" < "$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