Re: [PATCH v4 2/4] cli: make --entire-thread=false work for format=json.

[Mark Walters <markwalters1009@gmail.com>]

On Sat, 26 May 2012, Peter Wang <novalazy@gmail.com> wrote:
> On Tue, 24 Apr 2012 10:11:13 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>> The --entire-thread option in notmuch-show.c defaults to true when
>> format=json. Previously there was no way to turn this off. This patch
>> makes it respect --entire-thread=false.
>> 
>> The one subtlety is that we initialise a notmuch_bool_t to -1 to
>> indicate that the option parsing has not set it. This allows the code
>> to distinguish between the option being omitted from the command line,
>> and the option being set to false on the command line.
>> ---
>>  notmuch-show.c |   16 ++++++++++++++--
>>  1 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/notmuch-show.c b/notmuch-show.c
>> index 0d21f1a..48551bb 100644
>> --- a/notmuch-show.c
>> +++ b/notmuch-show.c
>> @@ -996,7 +996,13 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>>      char *query_string;
>>      int opt_index, ret;
>>      const notmuch_show_format_t *format = &format_text;
>> -    notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
>> +
>> +    /* We abuse the notmuch_bool_t variable params.entire-thread by
>> +     * setting it to -1 to denote that the command line parsing has
>> +     * not set it. We ensure it is set to TRUE or FALSE before passing
>> +     * it to any other function.*/
>> +    notmuch_show_params_t params = { .part = -1, .entire_thread = -1 };
>> +
>>      int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>>      notmuch_bool_t verify = FALSE;
>>      int exclude = EXCLUDE_TRUE;
>
> Hi Mark,
>
> As an alternative to the abuse, could you just treat it as with exclude,
> using an enum with three values (TRUE|FALSE|DEFAULT)?
> Then set params.entire_thread afterwards.

The reason I haven't done this is that the current command line parser
does not allow keyword options to take default values: in other words
--entire-thread without an "=<something>" would not be allowed.

It is easy to change the keyword parsing code to allow this: I include a
first draft of such a patch below. This would allow the solution you
suggest and thus avoid the hack/abuse.  What do people think?

Best wishes

Mark


---
 command-line-arguments.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 76b185f..d40c7e6 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -11,10 +11,16 @@
 */
 
 static notmuch_bool_t
-_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
+_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
     const notmuch_keyword_t *keywords = arg_desc->keywords;
 
+    if (next == 0) {
+/* No keyword given so return first option as default */
+	*((int *)arg_desc->output_var) = keywords->value;
+	return TRUE;
+    }
+
     while (keywords->name) {
 	if (strcmp (arg_str, keywords->name) == 0) {
 	    if (arg_desc->output_var) {
@@ -99,7 +105,8 @@ parse_option (const char *arg,
 	     */
 	    if (next != '=' && next != ':' && next != 0) return FALSE;
 	    if (next == 0) {
-		if (try->opt_type != NOTMUCH_OPT_BOOLEAN)
+		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
+		    try->opt_type != NOTMUCH_OPT_KEYWORD)
 		    return FALSE;
 	    } else {
 		if (value[0] == 0) return FALSE;
@@ -110,7 +117,7 @@ parse_option (const char *arg,
 
 	    switch (try->opt_type) {
 	    case NOTMUCH_OPT_KEYWORD:
-		return _process_keyword_arg (try, value);
+		return _process_keyword_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_BOOLEAN:
 		return _process_boolean_arg (try, next, value);
-- 
1.7.9.1