[PATCH v2 0/2] cli: Allow true/false parameter for boolean

[PATCH v2 0/2] cli: Allow true/false parameter for boolean

[Mark Walters]

Hi

Here is a second version of a patch to allow parameters to boolean
options on the command line. This version allows parameters
(=true|false). My first version is at
id:"1331244944-7960-1-git-send-email-markwalters1009@gmail.com". Jani
posted an alternative version there. Jani's version and this one are
quite similar: the key difference is that this version abuses a
notmuch_bool_t by setting it to -1 (to indicate that the parser has
not set this option). This makes the code simpler but is definitely an
abuse. I will discuss this further in replies to Jani's series.

Best wishes

Mark

Mark Walters (2):
  cli: Parsing. Allow true/false parameter for boolean options.
  cli: make --entire-thread=false work for format=json.

 command-line-arguments.c |   34 ++++++++++++++++++++++++++--------
 notmuch-show.c           |   15 +++++++++++++--
 2 files changed, 39 insertions(+), 10 deletions(-)

-- 
1.7.9.1

[PATCH 1/2] cli: Parsing. Allow true/false parameter for boolean options.

[Mark Walters]

Allow NOTMUCH_OPT_BOOLEAN to take a true or false parameter.  In
particular it allows the user to turn off a boolean option with
--option=false.
---
 command-line-arguments.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index e711414..76b185f 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -28,6 +28,24 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
     return FALSE;
 }
 
+static notmuch_bool_t
+_process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+
+    if (next == 0) {
+	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
+	return TRUE;
+    }
+    if (strcmp (arg_str, "false") == 0) {
+	*((notmuch_bool_t *)arg_desc->output_var) = FALSE;
+	return TRUE;
+    }
+    if (strcmp (arg_str, "true") == 0) {
+	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
+	return TRUE;
+    }
+    return FALSE;
+}
+
 /*
    Search for the {pos_arg_index}th position argument, return FALSE if
    that does not exist.
@@ -76,14 +94,15 @@ parse_option (const char *arg,
 	    char *endptr;
 
 	    /* Everything but boolean arguments (switches) needs a
-	     * delimiter, and a non-zero length value
+	     * delimiter, and a non-zero length value. Boolean
+	     * arguments may take an optional =true or =false value.
 	     */
-
-	    if (try->opt_type != NOTMUCH_OPT_BOOLEAN) {
-		if (next != '=' && next != ':') return FALSE;
-		if (value[0] == 0) return FALSE;
+	    if (next != '=' && next != ':' && next != 0) return FALSE;
+	    if (next == 0) {
+		if (try->opt_type != NOTMUCH_OPT_BOOLEAN)
+		    return FALSE;
 	    } else {
-		if (next != 0) return FALSE;
+		if (value[0] == 0) return FALSE;
 	    }
 
 	    if (try->output_var == NULL)
@@ -94,8 +113,7 @@ parse_option (const char *arg,
 		return _process_keyword_arg (try, value);
 		break;
 	    case NOTMUCH_OPT_BOOLEAN:
-		*((notmuch_bool_t *)try->output_var) = TRUE;
-		return TRUE;
+		return _process_boolean_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_INT:
 		*((int *)try->output_var) = strtol (value, &endptr, 10);
-- 
1.7.9.1

Re: [PATCH 1/2] cli: Parsing. Allow true/false parameter for boolean options.

[David Bremner]

On Sat, 10 Mar 2012 11:05:32 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> Allow NOTMUCH_OPT_BOOLEAN to take a true or false parameter.  In
> particular it allows the user to turn off a boolean option with
> --option=false.

pushed this one patch, waiting for feedback on the next.

d

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

[Mark Walters]

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 |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 05d51b2..8da1845 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -983,7 +983,12 @@ 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 };
+
+    /* 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;
     notmuch_bool_t no_exclude = FALSE;
@@ -1020,7 +1025,9 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     switch (format_sel) {
     case NOTMUCH_FORMAT_JSON:
 	format = &format_json;
-	params.entire_thread = TRUE;
+	/* JSON defaults to entire-thread TRUE */
+	if (params.entire_thread == -1)
+	    params.entire_thread = TRUE;
 	break;
     case NOTMUCH_FORMAT_TEXT:
 	format = &format_text;
@@ -1042,6 +1049,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	params.raw = TRUE;
 	break;
     }
+    /* Default is entire-thread = FALSE except for format=json which
+     * is dealt with above. */
+    if (params.entire_thread == -1)
+	params.entire_thread = FALSE;
 
     if (params.decrypt || verify) {
 #ifdef GMIME_ATLEAST_26
-- 
1.7.9.1

Re: [PATCH v2 0/2] cli: Allow true/false parameter for boolean

[Austin Clements]

Quoth Mark Walters on Mar 10 at 11:05 am:
> Hi
> 
> Here is a second version of a patch to allow parameters to boolean
> options on the command line. This version allows parameters
> (=true|false). My first version is at
> id:"1331244944-7960-1-git-send-email-markwalters1009@gmail.com". Jani
> posted an alternative version there. Jani's version and this one are
> quite similar: the key difference is that this version abuses a
> notmuch_bool_t by setting it to -1 (to indicate that the parser has
> not set this option). This makes the code simpler but is definitely an
> abuse. I will discuss this further in replies to Jani's series.
> 
> Best wishes
> 
> Mark
> 
> Mark Walters (2):
>   cli: Parsing. Allow true/false parameter for boolean options.
>   cli: make --entire-thread=false work for format=json.

LGTM.  The one question I have is whether or not the resulting
non-entire-thread behavior of the JSON format is actually what we
*want*.  As a probably unintentional consequence of the current show
code, we get

# A message and its replies (show_messages)
thread_node = [
    message?,                 # present if --entire-thread or matched
    [thread_node*]            # children of message
]

But would it be better to have

thread_node = [
    message|null,             # non-null if --entire-thread or matched
    [thread_node*]            # children of message
]

?  The latter is much more natural for consumers to work with
(checking whether the message matched or not is more natural and the
index of the child list doesn't change), but would require a little
more code in show to support.