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

[Mark Walters <markwalters1009@gmail.com>]

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