[Patch v7 0/6] Allow JSON to use non-entire thread, and use for elide

[Patch v7 0/6] Allow JSON to use non-entire thread, and use for elide

[Mark Walters]

This is version 7 of this patch set: the previous version is at
id:"1338106946-7611-1-git-send-email-markwalters1009@gmail.com".

I have fixed the whitespace errors, and simplified the logic in
notmuch-show.c as suggested by Peter.

I did not update the error output string as what is there is
consistent with the other error cases. However, I have a followup
patch which rejigs the error handling considerably, as there are some
other oddities: for example currently 

notmuch show --part id:<a message>

(with no number to "part") silently fails. It is a very simple but
relatively large patch: I will post it as a reply to this thread but
it is not logically part of the series (although it touches a lot of
the same code).

Best wishes

Mark

Mark Walters (6):
  cli: command line parsing: allow default for keyword options
  cli: Let json output "null" messages for non --entire-thread
  cli: make --entire-thread=false work for format=json.
  Update devel/schemata for --entire-thread=false
  emacs: make elide messages use notmuch-show for omitting messages.
  cli: notmuch-show.c fix whitespace error

 command-line-arguments.c |   17 +++++++++++---
 devel/TODO               |    2 -
 devel/schemata           |    2 +-
 emacs/notmuch-show.el    |   18 ++++++++------
 notmuch-client.h         |    1 +
 notmuch-show.c           |   55 +++++++++++++++++++++++++++++++++++++--------
 6 files changed, 70 insertions(+), 25 deletions(-)

-- 
1.7.9.1

[Patch v7 1/6] cli: command line parsing: allow default for keyword options

[Mark Walters]

This changes the parsing for "keyword" options so that if the option
is specified with no argument the argument is parsed as if it were
passed an empty string. This make it easier to add options to existing
boolean arguments (the existing --option can default to TRUE).
---
 command-line-arguments.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 76b185f..b0a0dab 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -11,10 +11,15 @@
 */
 
 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 */
+	arg_str = "";
+    }
+
     while (keywords->name) {
 	if (strcmp (arg_str, keywords->name) == 0) {
 	    if (arg_desc->output_var) {
@@ -24,7 +29,10 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
 	}
 	keywords++;
     }
-    fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    if (next != 0)
+	fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    else
+	fprintf (stderr, "option %s needs a keyword\n", arg_desc->name);
     return FALSE;
 }
 
@@ -99,7 +107,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 +119,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

[Patch v7 2/6] cli: Let json output "null" messages for non --entire-thread

[Mark Walters]

All formats except Json can output empty messages for non
entire-thread, but in Json format we output "null" to keep the other
elements (e.g. the replies to the omitted message) in the correct
place.
---
 notmuch-client.h |    1 +
 notmuch-show.c   |   20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 19b7f01..08540a7 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -72,6 +72,7 @@ typedef struct notmuch_show_format {
 			      const struct notmuch_show_params *params);
     const char *message_set_sep;
     const char *message_set_end;
+    const char *null_message;
 } notmuch_show_format_t;
 
 typedef struct notmuch_show_params {
diff --git a/notmuch-show.c b/notmuch-show.c
index 95427d4..97da5cc 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -37,7 +37,8 @@ static const notmuch_show_format_t format_json = {
     .message_set_start = "[",
     .part = format_part_json_entry,
     .message_set_sep = ", ",
-    .message_set_end = "]"
+    .message_set_end = "]",
+    .null_message = "null"
 };
 
 static notmuch_status_t
@@ -800,6 +801,15 @@ format_part_raw (unused (const void *ctx), mime_node_t *node,
 }
 
 static notmuch_status_t
+show_null_message (const notmuch_show_format_t *format)
+{
+    /* Output a null message. Currently empty for all formats except Json */
+    if (format->null_message)
+	printf ("%s", format->null_message);
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+static notmuch_status_t
 show_message (void *ctx,
 	      const notmuch_show_format_t *format,
 	      notmuch_message_t *message,
@@ -862,11 +872,13 @@ show_messages (void *ctx,
 	    if (status && !res)
 		res = status;
 	    next_indent = indent + 1;
-
-	    if (!status && format->message_set_sep)
-		fputs (format->message_set_sep, stdout);
+	} else {
+	    status = show_null_message (format);
 	}
 
+	if (!status && format->message_set_sep)
+	    fputs (format->message_set_sep, stdout);
+
 	status = show_messages (ctx,
 				format,
 				notmuch_message_get_replies (message),
-- 
1.7.9.1

[Patch v7 3/6] 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.

To do this the patch moves the --entire-thread option to be a keyword
option using the new command line parsing to allow the existing
--entire-thread to keep working.
---
 notmuch-show.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 97da5cc..61e19b0 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -981,6 +981,12 @@ enum {
     NOTMUCH_FORMAT_RAW
 };
 
+enum {
+    ENTIRE_THREAD_DEFAULT,
+    ENTIRE_THREAD_TRUE,
+    ENTIRE_THREAD_FALSE,
+};
+
 /* The following is to allow future options to be added more easily */
 enum {
     EXCLUDE_TRUE,
@@ -1000,6 +1006,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
     notmuch_bool_t verify = FALSE;
     int exclude = EXCLUDE_TRUE;
+    int entire_thread = ENTIRE_THREAD_DEFAULT;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
@@ -1012,8 +1019,12 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
           (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
                                   { "false", EXCLUDE_FALSE },
                                   { 0, 0 } } },
+	{ NOTMUCH_OPT_KEYWORD, &entire_thread, "entire-thread", 't',
+	  (notmuch_keyword_t []){ { "true", ENTIRE_THREAD_TRUE },
+				  { "false", ENTIRE_THREAD_FALSE },
+				  { "", ENTIRE_THREAD_TRUE },
+				  { 0, 0 } } },
 	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
 	{ 0, 0, 0, 0, 0 }
@@ -1036,7 +1047,6 @@ 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;
 	break;
     case NOTMUCH_FORMAT_TEXT:
 	format = &format_text;
@@ -1059,6 +1069,19 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 	break;
     }
 
+    /* Default is entire-thread = FALSE except for format=json. */
+    if (entire_thread == ENTIRE_THREAD_DEFAULT) {
+	if (format == &format_json)
+	    entire_thread = ENTIRE_THREAD_TRUE;
+	else
+	    entire_thread = ENTIRE_THREAD_FALSE;
+    }
+
+    if (entire_thread == ENTIRE_THREAD_TRUE)
+	params.entire_thread = TRUE;
+    else
+	params.entire_thread = FALSE;
+
     if (params.decrypt || verify) {
 #ifdef GMIME_ATLEAST_26
 	/* TODO: GMimePasswordRequestFunc */
-- 
1.7.9.1

[Patch v7 4/6] Update devel/schemata for --entire-thread=false

[Mark Walters]

Also remove the Json --entire-thread item from devel/TODO.
---
 devel/TODO     |    2 --
 devel/schemata |    2 +-
 2 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/devel/TODO b/devel/TODO
index 7b750af..eb757af 100644
--- a/devel/TODO
+++ b/devel/TODO
@@ -92,8 +92,6 @@ and email address in the From: line. We could also then easily support
 "notmuch compose --from <something>" to support getting at alternate
 email addresses.
 
-Fix the --format=json option to not imply --entire-thread.
-
 Implement "notmuch search --exclude-threads=<search-terms>" to allow
 for excluding muted threads, (and any other negative, thread-based
 filtering that the user wants to do).
diff --git a/devel/schemata b/devel/schemata
index 977cea7..8fcab8e 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -32,7 +32,7 @@ thread = [thread_node*]
 
 # A message and its replies (show_messages)
 thread_node = [
-    message?,                 # present if --entire-thread or matched
+    message?,                 # null if not matched and not --entire-thread
     [thread_node*]            # children of message
 ]
 
-- 
1.7.9.1

[Patch v7 5/6] emacs: make elide messages use notmuch-show for omitting messages.

[Mark Walters]

Previously the elide messages code got the entire-thread from
notmuch-show.c and then threw away all non-matching messages. This
version calls notmuch-show.c without the --entire-thread flag so
it never receives the non-matching messages in the first place.

This makes it substantially faster.
---
 emacs/notmuch-show.el |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d318430..9cc68be 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -958,9 +958,9 @@ message at DEPTH in the current thread."
   "Insert the message tree TREE at depth DEPTH in the current thread."
   (let ((msg (car tree))
 	(replies (cadr tree)))
-    (if (or (not notmuch-show-elide-non-matching-messages)
-	    (plist-get msg :match))
-	(notmuch-show-insert-msg msg depth))
+    ;; We test whether there is a message or just some replies.
+    (when msg
+      (notmuch-show-insert-msg msg depth))
     (notmuch-show-insert-thread replies (1+ depth))))
 
 (defun notmuch-show-insert-thread (thread depth)
@@ -1041,16 +1041,18 @@ function is used."
 	     (args (if notmuch-show-query-context
 		       (append (list "\'") basic-args
 			       (list "and (" notmuch-show-query-context ")\'"))
-		     (append (list "\'") basic-args (list "\'")))))
-	(notmuch-show-insert-forest (notmuch-query-get-threads
-				     (cons "--exclude=false" args)))
+		     (append (list "\'") basic-args (list "\'"))))
+	     (cli-args (cons "--exclude=false"
+			     (when notmuch-show-elide-non-matching-messages
+			       (list "--entire-thread=false")))))
+
+	(notmuch-show-insert-forest (notmuch-query-get-threads (append cli-args args)))
 	;; If the query context reduced the results to nothing, run
 	;; the basic query.
 	(when (and (eq (buffer-size) 0)
 		   notmuch-show-query-context)
 	  (notmuch-show-insert-forest
-	   (notmuch-query-get-threads
-	    (cons "--exclude=false" basic-args)))))
+	   (notmuch-query-get-threads (append cli-args basic-args)))))
 
       (jit-lock-register #'notmuch-show-buttonise-links)
 
-- 
1.7.9.1

[Patch v7 6/6] cli: notmuch-show.c fix whitespace error

[Mark Walters]

Fix an existing whitespace error since it is right next to
the changes of this series.
---
 notmuch-show.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 61e19b0..b789853 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1015,10 +1015,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 				  { "mbox", NOTMUCH_FORMAT_MBOX },
 				  { "raw", NOTMUCH_FORMAT_RAW },
 				  { 0, 0 } } },
-        { NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
-          (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
-                                  { "false", EXCLUDE_FALSE },
-                                  { 0, 0 } } },
+	{ NOTMUCH_OPT_KEYWORD, &exclude, "exclude", 'x',
+	  (notmuch_keyword_t []){ { "true", EXCLUDE_TRUE },
+				  { "false", EXCLUDE_FALSE },
+				  { 0, 0 } } },
 	{ NOTMUCH_OPT_KEYWORD, &entire_thread, "entire-thread", 't',
 	  (notmuch_keyword_t []){ { "true", ENTIRE_THREAD_TRUE },
 				  { "false", ENTIRE_THREAD_FALSE },
-- 
1.7.9.1

[PATCH] cli: make the command line parser's errors more informative.

[Mark Walters]

Previously, the cli parser was a little erratic in what errors it
reported and would fail silently in many cases (for example, when no
argument was passed to an integer option). This was particularly
annoying as the user could not (easily) tell whether the command
failed or just there were no search results.

This patch tries to make the handling consistent and return a helpful
error message in all cases.
---

 command-line-arguments.c |   66 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index b0a0dab..ea56467 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -30,9 +30,9 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	keywords++;
     }
     if (next != 0)
-	fprintf (stderr, "unknown keyword: %s\n", arg_str);
+	fprintf (stderr, "Unknown argument \"%s\" for option \"%s\".\n", arg_str, arg_desc->name);
     else
-	fprintf (stderr, "option %s needs a keyword\n", arg_desc->name);
+	fprintf (stderr, "Option \"%s\" needs a keyword argument.\n", arg_desc->name);
     return FALSE;
 }
 
@@ -51,9 +51,43 @@ _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
 	return TRUE;
     }
+    fprintf (stderr, "Unknown argument \"%s\" for (boolean) option \"%s\".\n", arg_str, arg_desc->name);
     return FALSE;
 }
 
+static notmuch_bool_t
+_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+
+    char *endptr;
+    if (next == 0 || arg_str[0] == 0) {
+	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
+	return FALSE;
+    }
+
+    *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10);
+    if (*endptr == 0)
+	return TRUE;
+
+    fprintf (stderr, "Unable to parse argument \"%s\" for option \"%s\" as an integer.\n",
+	     arg_str, arg_desc->name);
+    return FALSE;
+}
+
+static notmuch_bool_t
+_process_string_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+
+    if (next == 0) {
+	fprintf (stderr, "Option \"%s\" needs a string argument.\n", arg_desc->name);
+	return FALSE;
+    }
+    if (arg_str[0] == 0) {
+	fprintf (stderr, "String argument for option \"%s\" must be non-empty.\n", arg_desc->name);
+	return FALSE;
+    }
+    *((const char **)arg_desc->output_var) = arg_str;
+    return TRUE;
+}
+
 /*
    Search for the {pos_arg_index}th position argument, return FALSE if
    that does not exist.
@@ -99,20 +133,13 @@ parse_option (const char *arg,
 	    char next = arg[strlen (try->name)];
 	    const char *value= arg+strlen(try->name)+1;
 
-	    char *endptr;
-
-	    /* Everything but boolean arguments (switches) needs a
-	     * delimiter, and a non-zero length value. Boolean
-	     * arguments may take an optional =true or =false value.
-	     */
-	    if (next != '=' && next != ':' && next != 0) return FALSE;
-	    if (next == 0) {
-		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
-		    try->opt_type != NOTMUCH_OPT_KEYWORD)
-		    return FALSE;
-	    } else {
-		if (value[0] == 0) return FALSE;
-	    }
+	    /* If this is not the end of the argument (i.e. the next
+	     * character is not a space or a delimiter) we stop
+	     * parsing for this option but allow the parsing to
+	     * continue to for other options. This should allow
+	     * options to be initial segments of other options. */
+	    if (next != '=' && next != ':' && next != 0)
+		goto DONE_THIS_OPTION;
 
 	    if (try->output_var == NULL)
 		INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
@@ -125,12 +152,10 @@ parse_option (const char *arg,
 		return _process_boolean_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_INT:
-		*((int *)try->output_var) = strtol (value, &endptr, 10);
-		return (*endptr == 0);
+		return _process_int_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_STRING:
-		*((const char **)try->output_var) = value;
-		return TRUE;
+		return _process_string_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_POSITION:
 	    case NOTMUCH_OPT_END:
@@ -139,6 +164,7 @@ parse_option (const char *arg,
 		/*UNREACHED*/
 	    }
 	}
+    DONE_THIS_OPTION:
 	try++;
     }
     fprintf (stderr, "Unrecognized option: --%s\n", arg);
-- 
1.7.9.1

Re: [PATCH] cli: make the command line parser's errors more informative.

[Peter Wang]

On Sun,  3 Jun 2012 12:48:48 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>  
> +static notmuch_bool_t
> +_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
> +
> +    char *endptr;
> +    if (next == 0 || arg_str[0] == 0) {
> +	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
> +	return FALSE;
> +    }
> +
> +    *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10);
> +    if (*endptr == 0)
> +	return TRUE;

It's usually clearer to write '\0' for the null character.

> @@ -99,20 +133,13 @@ parse_option (const char *arg,
>  	    char next = arg[strlen (try->name)];
>  	    const char *value= arg+strlen(try->name)+1;
>  
> -	    char *endptr;
> -
> -	    /* Everything but boolean arguments (switches) needs a
> -	     * delimiter, and a non-zero length value. Boolean
> -	     * arguments may take an optional =true or =false value.
> -	     */
> -	    if (next != '=' && next != ':' && next != 0) return FALSE;
> -	    if (next == 0) {
> -		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
> -		    try->opt_type != NOTMUCH_OPT_KEYWORD)
> -		    return FALSE;
> -	    } else {
> -		if (value[0] == 0) return FALSE;
> -	    }
> +	    /* If this is not the end of the argument (i.e. the next
> +	     * character is not a space or a delimiter) we stop
> +	     * parsing for this option but allow the parsing to
> +	     * continue to for other options. This should allow
> +	     * options to be initial segments of other options. */

It took me a little while to figure out what the last sentence was
about.  Perhaps:

    If we have not reached the end of the argument (i.e. the next
    character is not a space or delimiter) then the argument could
    still match a longer option name later in the option table.

(otherwise, "continue to for other")

> +	    if (next != '=' && next != ':' && next != 0)
> +		goto DONE_THIS_OPTION;

The `goto' could be expressed as a `continue' in a `for' loop, AFAICS.

>  
>  	    if (try->output_var == NULL)
>  		INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
> @@ -125,12 +152,10 @@ parse_option (const char *arg,
>  		return _process_boolean_arg (try, next, value);
>  		break;
>  	    case NOTMUCH_OPT_INT:
> -		*((int *)try->output_var) = strtol (value, &endptr, 10);
> -		return (*endptr == 0);
> +		return _process_int_arg (try, next, value);
>  		break;
>  	    case NOTMUCH_OPT_STRING:
> -		*((const char **)try->output_var) = value;
> -		return TRUE;
> +		return _process_string_arg (try, next, value);
>  		break;
>  	    case NOTMUCH_OPT_POSITION:
>  	    case NOTMUCH_OPT_END:
> @@ -139,6 +164,7 @@ parse_option (const char *arg,
>  		/*UNREACHED*/
>  	    }
>  	}
> +    DONE_THIS_OPTION:
>  	try++;
>      }
>      fprintf (stderr, "Unrecognized option: --%s\n", arg);

Peter

Re: [PATCH] cli: make the command line parser's errors more informative.

[Mark Walters]

On Tue, 05 Jun 2012, Peter Wang <novalazy@gmail.com> wrote:
> On Sun,  3 Jun 2012 12:48:48 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>>  
>> +static notmuch_bool_t
>> +_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
>> +
>> +    char *endptr;
>> +    if (next == 0 || arg_str[0] == 0) {
>> +	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
>> +	return FALSE;
>> +    }
>> +
>> +    *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10);
>> +    if (*endptr == 0)
>> +	return TRUE;
>
> It's usually clearer to write '\0' for the null character.

Yes I agree: fixed. I also changed the other instances.

>> @@ -99,20 +133,13 @@ parse_option (const char *arg,
>>  	    char next = arg[strlen (try->name)];
>>  	    const char *value= arg+strlen(try->name)+1;
>>  
>> -	    char *endptr;
>> -
>> -	    /* Everything but boolean arguments (switches) needs a
>> -	     * delimiter, and a non-zero length value. Boolean
>> -	     * arguments may take an optional =true or =false value.
>> -	     */
>> -	    if (next != '=' && next != ':' && next != 0) return FALSE;
>> -	    if (next == 0) {
>> -		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
>> -		    try->opt_type != NOTMUCH_OPT_KEYWORD)
>> -		    return FALSE;
>> -	    } else {
>> -		if (value[0] == 0) return FALSE;
>> -	    }
>> +	    /* If this is not the end of the argument (i.e. the next
>> +	     * character is not a space or a delimiter) we stop
>> +	     * parsing for this option but allow the parsing to
>> +	     * continue to for other options. This should allow
>> +	     * options to be initial segments of other options. */
>
> It took me a little while to figure out what the last sentence was
> about.  Perhaps:
>
>     If we have not reached the end of the argument (i.e. the next
>     character is not a space or delimiter) then the argument could
>     still match a longer option name later in the option table.

This is much clearer, thanks!

> (otherwise, "continue to for other")
>
>> +	    if (next != '=' && next != ':' && next != 0)
>> +		goto DONE_THIS_OPTION;
>
> The `goto' could be expressed as a `continue' in a `for' loop, AFAICS.

This is also much nicer. Updated patch follows

Thanks for the review!

Best wishes

Mark

[PATCH v2] cli: make the command line parser's errors more informative.

[Mark Walters]

Previously, the cli parser was a little erratic in what errors it
reported and would fail silently in many cases (for example, when no
argument was passed to an integer option). This was particularly
annoying as the user could not (easily) tell whether the command
failed or just there were no search results.

This patch tries to make the handling consistent and return a helpful
error message in all cases.
---
 command-line-arguments.c |   76 ++++++++++++++++++++++++++++++----------------
 1 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index b0a0dab..bf9aeca 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -15,7 +15,7 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 
     const notmuch_keyword_t *keywords = arg_desc->keywords;
 
-    if (next == 0) {
+    if (next == '\0') {
 	/* No keyword given */
 	arg_str = "";
     }
@@ -29,17 +29,17 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	}
 	keywords++;
     }
-    if (next != 0)
-	fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    if (next != '\0')
+	fprintf (stderr, "Unknown keyword argument \"%s\" for option \"%s\".\n", arg_str, arg_desc->name);
     else
-	fprintf (stderr, "option %s needs a keyword\n", arg_desc->name);
+	fprintf (stderr, "Option \"%s\" needs a keyword argument.\n", arg_desc->name);
     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) {
+    if (next == '\0') {
 	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
 	return TRUE;
     }
@@ -51,9 +51,43 @@ _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
 	return TRUE;
     }
+    fprintf (stderr, "Unknown argument \"%s\" for (boolean) option \"%s\".\n", arg_str, arg_desc->name);
     return FALSE;
 }
 
+static notmuch_bool_t
+_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+
+    char *endptr;
+    if (next == '\0' || arg_str[0] == '\0') {
+	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
+	return FALSE;
+    }
+
+    *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10);
+    if (*endptr == '\0')
+	return TRUE;
+
+    fprintf (stderr, "Unable to parse argument \"%s\" for option \"%s\" as an integer.\n",
+	     arg_str, arg_desc->name);
+    return FALSE;
+}
+
+static notmuch_bool_t
+_process_string_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+
+    if (next == '\0') {
+	fprintf (stderr, "Option \"%s\" needs a string argument.\n", arg_desc->name);
+	return FALSE;
+    }
+    if (arg_str[0] == '\0') {
+	fprintf (stderr, "String argument for option \"%s\" must be non-empty.\n", arg_desc->name);
+	return FALSE;
+    }
+    *((const char **)arg_desc->output_var) = arg_str;
+    return TRUE;
+}
+
 /*
    Search for the {pos_arg_index}th position argument, return FALSE if
    that does not exist.
@@ -93,26 +127,19 @@ parse_option (const char *arg,
 
     arg += 2;
 
-    const notmuch_opt_desc_t *try = options;
-    while (try->opt_type != NOTMUCH_OPT_END) {
+    const notmuch_opt_desc_t *try;
+    for (try = options; try->opt_type != NOTMUCH_OPT_END; try++) {
 	if (try->name && strncmp (arg, try->name, strlen (try->name)) == 0) {
 	    char next = arg[strlen (try->name)];
 	    const char *value= arg+strlen(try->name)+1;
 
-	    char *endptr;
-
-	    /* Everything but boolean arguments (switches) needs a
-	     * delimiter, and a non-zero length value. Boolean
-	     * arguments may take an optional =true or =false value.
-	     */
-	    if (next != '=' && next != ':' && next != 0) return FALSE;
-	    if (next == 0) {
-		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
-		    try->opt_type != NOTMUCH_OPT_KEYWORD)
-		    return FALSE;
-	    } else {
-		if (value[0] == 0) return FALSE;
-	    }
+	    /* If we have not reached the end of the argument
+	       (i.e. the next character is not a space or delimiter)
+	       then the argument could still match a longer option
+	       name later in the option table.
+	    */
+	    if (next != '=' && next != ':' && next != '\0')
+		continue;
 
 	    if (try->output_var == NULL)
 		INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
@@ -125,12 +152,10 @@ parse_option (const char *arg,
 		return _process_boolean_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_INT:
-		*((int *)try->output_var) = strtol (value, &endptr, 10);
-		return (*endptr == 0);
+		return _process_int_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_STRING:
-		*((const char **)try->output_var) = value;
-		return TRUE;
+		return _process_string_arg (try, next, value);
 		break;
 	    case NOTMUCH_OPT_POSITION:
 	    case NOTMUCH_OPT_END:
@@ -139,7 +164,6 @@ parse_option (const char *arg,
 		/*UNREACHED*/
 	    }
 	}
-	try++;
     }
     fprintf (stderr, "Unrecognized option: --%s\n", arg);
     return FALSE;
-- 
1.7.9.1

Re: [PATCH v2] cli: make the command line parser's errors more informative.

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Previously, the cli parser was a little erratic in what errors it
> reported and would fail silently in many cases (for example, when no
> argument was passed to an integer option). This was particularly
> annoying as the user could not (easily) tell whether the command
> failed or just there were no search results.

pushed,

d