From: Mark Walters <markwalters1009@gmail.com>
To: Michal Sojka <sojkam1@fel.cvut.cz>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 07/10] cli: search: Convert --output to keyword
 argument
In-Reply-To: <1415058622-21162-8-git-send-email-sojkam1@fel.cvut.cz>
References: <1415058622-21162-1-git-send-email-sojkam1@fel.cvut.cz>
	<1415058622-21162-8-git-send-email-sojkam1@fel.cvut.cz>
User-Agent: Notmuch/0.18.1+86~gef5e66a (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Tue, 04 Nov 2014 08:58:19 +0000
Message-ID: <87389z4a1g.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi

On Mon, 03 Nov 2014, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> Now, when address related outputs are in a separate command, it makes
> no sense to combine multiple --output options in search command line.
> Using switch statement to handle different outputs is more readable
> than a series of if statements.

I am not keen on this change: I think the user should always be able to
force the default output by setting command line options (which should
protect against future changes in the default). Thus I would like to
continue to allow  --output=sender --output=recipients. 

I do approve of making the default do something useful but whether it
should be both or just sender (which is much faster) is unclear to me.

Best wishes

Mark

 
> ---
>  doc/man1/notmuch-search.rst |  3 ---
>  notmuch-search.c            | 25 +++++++++++++------------
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/doc/man1/notmuch-search.rst b/doc/man1/notmuch-search.rst
> index 65df288..0cc2911 100644
> --- a/doc/man1/notmuch-search.rst
> +++ b/doc/man1/notmuch-search.rst
> @@ -78,9 +78,6 @@ Supported options for **search** include
>              by null characters (--format=text0), as a JSON array
>              (--format=json), or as an S-Expression list (--format=sexp).
>  
> -	This option can be given multiple times to combine different
> -	outputs.
> -
>      ``--sort=``\ (**newest-first**\ \|\ **oldest-first**)
>          This option can be used to present results in either
>          chronological order (**oldest-first**) or reverse chronological
> diff --git a/notmuch-search.c b/notmuch-search.c
> index cbd84f5..402e860 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -593,7 +593,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>      int opt_index, ret;
>  
>      notmuch_opt_desc_t options[] = {
> -	{ NOTMUCH_OPT_KEYWORD_FLAGS, &ctx->output, "output", 'o',
> +	{ NOTMUCH_OPT_KEYWORD, &ctx->output, "output", 'o',
>  	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
>  				  { "threads", OUTPUT_THREADS },
>  				  { "messages", OUTPUT_MESSAGES },
> @@ -607,13 +607,11 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>  	{ 0, 0, 0, 0, 0 }
>      };
>  
> +    ctx->output = OUTPUT_SUMMARY;
>      opt_index = parse_arguments (argc, argv, options, 1);
>      if (opt_index < 0)
>  	return EXIT_FAILURE;
>  
> -    if (! ctx->output)
> -	ctx->output = OUTPUT_SUMMARY;
> -
>      if (ctx->output != OUTPUT_FILES && ctx->output != OUTPUT_MESSAGES &&
>  	ctx->dupe != -1) {
>          fprintf (stderr, "Error: --duplicate=N is only supported with --output=files and --output=messages.\n");
> @@ -624,17 +622,20 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>  				 argc - opt_index, argv + opt_index))
>  	return EXIT_FAILURE;
>  
> -    if (ctx->output == OUTPUT_SUMMARY ||
> -	ctx->output == OUTPUT_THREADS)
> +    switch (ctx->output) {
> +    case OUTPUT_SUMMARY:
> +    case OUTPUT_THREADS:
>  	ret = do_search_threads (ctx);
> -    else if (ctx->output == OUTPUT_MESSAGES ||
> -	     ctx->output == OUTPUT_FILES)
> +	break;
> +    case OUTPUT_MESSAGES:
> +    case OUTPUT_FILES:
>  	ret = do_search_messages (ctx);
> -    else if (ctx->output == OUTPUT_TAGS)
> +	break;
> +    case OUTPUT_TAGS:
>  	ret = do_search_tags (ctx);
> -    else {
> -	fprintf (stderr, "Error: the combination of outputs is not supported.\n");
> -	ret = 1;
> +	break;
> +    default:
> +	INTERNAL_ERROR ("Unexpected output");
>      }
>  
>      _notmuch_search_cleanup (ctx);
> -- 
> 2.1.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch