[PATCH] cli: notmuch address option defaults update

[PATCH] cli: notmuch address option defaults update

[Tomi Ollila]

When no --output option were given, change default to display senders
only. This is faster and provides useful-enough list of addresses.

When only --count option is given, display senders (in contrary to not
displaying anything).

Document how --count affects to "sort order" a bit more accurately.

Clean up some whitespace in the documentation.

One test updated to have --output=count without sender nor recipient
output option.
---

Some quick updates to the notmuch address interface which I hope will
be considered to be included in 0.19 release. As we are in feature freeze
I hope this gets quick feedback, in any way you desire.

Tomi

 doc/man1/notmuch-address.rst | 34 ++++++++++++++++++----------------
 notmuch-search.c             |  4 ++--
 test/T095-address.sh         | 14 +++++++-------
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/doc/man1/notmuch-address.rst b/doc/man1/notmuch-address.rst
index 359616e0dc5f..034607c434d2 100644
--- a/doc/man1/notmuch-address.rst
+++ b/doc/man1/notmuch-address.rst
@@ -32,28 +32,28 @@ Supported options for **address** include
     ``--output=(sender|recipients|count)``
 
         Controls which information appears in the output. This option
-	can be given multiple times to combine different outputs.
-	Omitting this option is equivalent to
-	--output=sender --output=recipients.
+        can be given multiple times to combine different outputs.
+        Omitting this option is equivalent to --output=sender.
 
-	**sender**
+        **sender**
             Output all addresses from the *From* header.
 
-	    Note: Searching for **sender** should be much faster than
-	    searching for **recipients**, because sender addresses are
-	    cached directly in the database whereas other addresses
-	    need to be fetched from message files.
+            Note: Searching for **sender** should be much faster than
+            searching for **recipients**, because sender addresses are
+            cached directly in the database whereas other addresses
+            need to be fetched from message files.
 
-	**recipients**
+        **recipients**
             Output all addresses from the *To*, *Cc* and *Bcc*
             headers.
 
-	**count**
-	    Print the count of how many times was the address
-	    encountered during search.
+        **count**
+            Print the count of how many times was the address
+            encountered during search.
 
-	    Note: With this option, addresses are printed only after
-	    the whole search is finished. This may take long time.
+            Note: With this option, addresses are printed only after
+            the whole search is finished (and in seemingly random
+            order). This may take long time.
 
     ``--sort=``\ (**newest-first**\ \|\ **oldest-first**)
         This option can be used to present results in either
@@ -63,7 +63,9 @@ Supported options for **address** include
         By default, results will be displayed in reverse chronological
         order, (that is, the newest results will be displayed first).
 
-	This option has no effect when used with --output=count.
+        This option affects the seemingly random output order when
+        used with --output=count.
+
 
     ``--exclude=(true|false)``
         A message is called "excluded" if it matches at least one tag in
@@ -95,4 +97,4 @@ SEE ALSO
 **notmuch-dump(1)**, **notmuch-hooks(5)**, **notmuch-insert(1)**,
 **notmuch-new(1)**, **notmuch-reply(1)**, **notmuch-restore(1)**,
 **notmuch-search-terms(7)**, **notmuch-show(1)**, **notmuch-tag(1)**,
-***notmuch-search(1)**
+**notmuch-search(1)**
diff --git a/notmuch-search.c b/notmuch-search.c
index 5036d8e44005..14b9f01c5ad1 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -735,8 +735,8 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
-    if (! ctx->output)
-	ctx->output = OUTPUT_SENDER | OUTPUT_RECIPIENTS;
+    if (! (ctx->output & (OUTPUT_SENDER | OUTPUT_RECIPIENTS)))
+	ctx->output |= OUTPUT_SENDER;
 
     if (_notmuch_search_prepare (ctx, config,
 				 argc - opt_index, argv + opt_index))
diff --git a/test/T095-address.sh b/test/T095-address.sh
index 033d0f4fd68c..ed0cac7807ff 100755
--- a/test/T095-address.sh
+++ b/test/T095-address.sh
@@ -27,6 +27,11 @@ Mikhail Gusarov <dottedmag@dottedmag.net>
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "without --output"
+notmuch address '*' >OUTPUT
+# Use EXPECTED from previous subtest
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "--output=sender --format=json"
 notmuch address --output=sender --format=json '*' >OUTPUT
 cat <<EOF >EXPECTED
@@ -91,11 +96,6 @@ Mikhail Gusarov <dottedmag@dottedmag.net>
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-test_begin_subtest "without --output"
-notmuch address '*' >OUTPUT
-# Use EXPECTED from previous subtest
-test_expect_equal_file OUTPUT EXPECTED
-
 test_begin_subtest "--output=sender --output=count"
 notmuch address --output=sender --output=count '*' | sort -n >OUTPUT
 cat <<EOF >EXPECTED
@@ -119,10 +119,10 @@ cat <<EOF >EXPECTED
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-test_begin_subtest "--output=sender --output=count --format=json"
+test_begin_subtest "--output=count --format=json"
 # Since the iteration order of GHashTable is not specified, we
 # preprocess and sort the results to keep the order stable here.
-notmuch address --output=sender --output=count --format=json '*' | \
+notmuch address --output=count --format=json '*' | \
     sed -e 's/^\[//' -e 's/]$//' -e 's/,$//' | sort >OUTPUT
 cat <<EOF >EXPECTED
 {"name": "Adrian Perez de Castro", "address": "aperez@igalia.com", "name-addr": "Adrian Perez de Castro <aperez@igalia.com>", "count": 1}
-- 
2.0.0

Re: [PATCH] cli: notmuch address option defaults update

[Mark Walters]

Hi

On Thu, 06 Nov 2014, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> When no --output option were given, change default to display senders
> only. This is faster and provides useful-enough list of addresses.
>
> When only --count option is given, display senders (in contrary to not
> displaying anything).
>
> Document how --count affects to "sort order" a bit more accurately.
>
> Clean up some whitespace in the documentation.
>
> One test updated to have --output=count without sender nor recipient
> output option.
> ---
>
> Some quick updates to the notmuch address interface which I hope will
> be considered to be included in 0.19 release. As we are in feature freeze
> I hope this gets quick feedback, in any way you desire.

I have tested it and it basically looks good. (As mentioned
on irc) I am mildly in favour of the change to defaulting to
output=sender that this patch does.

A couple of comments below. Feel free to ignore.

> Tomi
>
>  doc/man1/notmuch-address.rst | 34 ++++++++++++++++++----------------
>  notmuch-search.c             |  4 ++--
>  test/T095-address.sh         | 14 +++++++-------
>  3 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/doc/man1/notmuch-address.rst b/doc/man1/notmuch-address.rst
> index 359616e0dc5f..034607c434d2 100644
> --- a/doc/man1/notmuch-address.rst
> +++ b/doc/man1/notmuch-address.rst
> @@ -32,28 +32,28 @@ Supported options for **address** include
>      ``--output=(sender|recipients|count)``
>  
>          Controls which information appears in the output. This option
> -	can be given multiple times to combine different outputs.
> -	Omitting this option is equivalent to
> -	--output=sender --output=recipients.
> +        can be given multiple times to combine different outputs.
> +        Omitting this option is equivalent to --output=sender.

Perhaps say, "if neither --output=sender nor --output=recipients is
specified then this defaults to --output=sender". (Otherwise the manual
does not specify what happens in --output=count case)

> -	**sender**
> +        **sender**
>              Output all addresses from the *From* header.
>  
> -	    Note: Searching for **sender** should be much faster than
> -	    searching for **recipients**, because sender addresses are
> -	    cached directly in the database whereas other addresses
> -	    need to be fetched from message files.
> +            Note: Searching for **sender** should be much faster than
> +            searching for **recipients**, because sender addresses are
> +            cached directly in the database whereas other addresses
> +            need to be fetched from message files.
>  
> -	**recipients**
> +        **recipients**
>              Output all addresses from the *To*, *Cc* and *Bcc*
>              headers.
>  
> -	**count**
> -	    Print the count of how many times was the address
> -	    encountered during search.
> +        **count**
> +            Print the count of how many times was the address
> +            encountered during search.
>  
> -	    Note: With this option, addresses are printed only after
> -	    the whole search is finished. This may take long time.
> +            Note: With this option, addresses are printed only after
> +            the whole search is finished (and in seemingly random
> +            order). This may take long time.
>  
>      ``--sort=``\ (**newest-first**\ \|\ **oldest-first**)
>          This option can be used to present results in either
> @@ -63,7 +63,9 @@ Supported options for **address** include
>          By default, results will be displayed in reverse chronological
>          order, (that is, the newest results will be displayed first).
>  
> -	This option has no effect when used with --output=count.
> +        This option affects the seemingly random output order when
> +        used with --output=count.

I know what this means but it confused me at first.  How about "When
used with --output=count this option changes the output from one
seemingly random order to a different seemingly random order"? 

Best wishes

Mark


>  
>      ``--exclude=(true|false)``
>          A message is called "excluded" if it matches at least one tag in
> @@ -95,4 +97,4 @@ SEE ALSO
>  **notmuch-dump(1)**, **notmuch-hooks(5)**, **notmuch-insert(1)**,
>  **notmuch-new(1)**, **notmuch-reply(1)**, **notmuch-restore(1)**,
>  **notmuch-search-terms(7)**, **notmuch-show(1)**, **notmuch-tag(1)**,
> -***notmuch-search(1)**
> +**notmuch-search(1)**
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 5036d8e44005..14b9f01c5ad1 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -735,8 +735,8 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
>      if (opt_index < 0)
>  	return EXIT_FAILURE;
>  
> -    if (! ctx->output)
> -	ctx->output = OUTPUT_SENDER | OUTPUT_RECIPIENTS;
> +    if (! (ctx->output & (OUTPUT_SENDER | OUTPUT_RECIPIENTS)))
> +	ctx->output |= OUTPUT_SENDER;
>  
>      if (_notmuch_search_prepare (ctx, config,
>  				 argc - opt_index, argv + opt_index))
> diff --git a/test/T095-address.sh b/test/T095-address.sh
> index 033d0f4fd68c..ed0cac7807ff 100755
> --- a/test/T095-address.sh
> +++ b/test/T095-address.sh
> @@ -27,6 +27,11 @@ Mikhail Gusarov <dottedmag@dottedmag.net>
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> +test_begin_subtest "without --output"
> +notmuch address '*' >OUTPUT
> +# Use EXPECTED from previous subtest
> +test_expect_equal_file OUTPUT EXPECTED
> +
>  test_begin_subtest "--output=sender --format=json"
>  notmuch address --output=sender --format=json '*' >OUTPUT
>  cat <<EOF >EXPECTED
> @@ -91,11 +96,6 @@ Mikhail Gusarov <dottedmag@dottedmag.net>
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> -test_begin_subtest "without --output"
> -notmuch address '*' >OUTPUT
> -# Use EXPECTED from previous subtest
> -test_expect_equal_file OUTPUT EXPECTED
> -
>  test_begin_subtest "--output=sender --output=count"
>  notmuch address --output=sender --output=count '*' | sort -n >OUTPUT
>  cat <<EOF >EXPECTED
> @@ -119,10 +119,10 @@ cat <<EOF >EXPECTED
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
> -test_begin_subtest "--output=sender --output=count --format=json"
> +test_begin_subtest "--output=count --format=json"
>  # Since the iteration order of GHashTable is not specified, we
>  # preprocess and sort the results to keep the order stable here.
> -notmuch address --output=sender --output=count --format=json '*' | \
> +notmuch address --output=count --format=json '*' | \
>      sed -e 's/^\[//' -e 's/]$//' -e 's/,$//' | sort >OUTPUT
>  cat <<EOF >EXPECTED
>  {"name": "Adrian Perez de Castro", "address": "aperez@igalia.com", "name-addr": "Adrian Perez de Castro <aperez@igalia.com>", "count": 1}
> -- 
> 2.0.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] cli: notmuch address option defaults update

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>> -	This option has no effect when used with --output=count.
>> +        This option affects the seemingly random output order when
>> +        used with --output=count.
>
> I know what this means but it confused me at first.  How about "When
> used with --output=count this option changes the output from one
> seemingly random order to a different seemingly random order"? 

What about just saying "This option is not supported with
--output=count".
Then the fact that it doesn't crash or error out is a kind of bonus ;).
Of course we could (later?) add a check for --sort && --output=count and
report an errror.

d

Re: [PATCH] cli: notmuch address option defaults update

[Michal Sojka]

On Thu, Nov 06 2014, Tomi Ollila wrote:
> When no --output option were given, change default to display senders
> only. This is faster and provides useful-enough list of addresses.
>
> When only --count option is given, display senders (in contrary to not
> displaying anything).
>
> Document how --count affects to "sort order" a bit more accurately.
>
> Clean up some whitespace in the documentation.
>
> One test updated to have --output=count without sender nor recipient
> output option.
> ---
>
> Some quick updates to the notmuch address interface which I hope will
> be considered to be included in 0.19 release. As we are in feature freeze
> I hope this gets quick feedback, in any way you desire.
>
> Tomi
>
>  doc/man1/notmuch-address.rst | 34 ++++++++++++++++++----------------
>  notmuch-search.c             |  4 ++--
>  test/T095-address.sh         | 14 +++++++-------
>  3 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/doc/man1/notmuch-address.rst b/doc/man1/notmuch-address.rst
> index 359616e0dc5f..034607c434d2 100644
> --- a/doc/man1/notmuch-address.rst
> +++ b/doc/man1/notmuch-address.rst
> @@ -32,28 +32,28 @@ Supported options for **address** include
>      ``--output=(sender|recipients|count)``
>  
>          Controls which information appears in the output. This option
> -	can be given multiple times to combine different outputs.
> -	Omitting this option is equivalent to
> -	--output=sender --output=recipients.
> +        can be given multiple times to combine different outputs.
> +        Omitting this option is equivalent to --output=sender.

The implementation does a bit more. What about:

    When neither --output=sender nor --output=recipients is given,
    --output=sender is implied.

Otherwise it LGTM.

Thanks,
-Michal