From: Michal Sojka <sojkam1@fel.cvut.cz>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 07/10] cli: search: Convert --output to keyword
 argument
In-Reply-To: <87389z4a1g.fsf@qmul.ac.uk>
References: <1415058622-21162-1-git-send-email-sojkam1@fel.cvut.cz>
	<1415058622-21162-8-git-send-email-sojkam1@fel.cvut.cz>
	<87389z4a1g.fsf@qmul.ac.uk>
User-Agent: Notmuch/0.18.2+157~ga00d359 (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 04 Nov 2014 12:26:02 +0100
Message-ID: <87d293mcl1.fsf@steelpick.2x.cz>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Tue, Nov 04 2014, Mark Walters wrote:
> 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). 

You can do this even with this patch.

> Thus I would like to continue to allow --output=sender
> --output=recipients.

This is allowed in notmuch address. This patch modifies only notmuch
search. Currently it makes no sense to run --output=files --output=tags
or other output combinations.

> 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.

Does this comment refer to 7/10 or to 6/10?

I'd say it refers to the following line from 6/10.

search_context.output = OUTPUT_SENDER | OUTPUT_RECIPIENTS;

Even there you can override the default by command line option. This get
executed only if no --output appears on the command line.

Thanks
-Michal


>
> 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