(no subject)

[David Bremner]

I decided to start a new thread since the other one
id:"1322808114-11854-1-git-send-email-david@tethera.net" was getting
pretty long, and there was no actual relevant discussion there.

I think these patches are getting close to ready. 

One thing to discuss is the inclusion of single element options. These
are not currently processed and are a essentially an artifact of an
earlier implementation; on the other hand it might be nice to have
them in place so we just need to (in theory) update notmuch-opts.[ch]
to support short options.

[PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.

[David Bremner]

From: David Bremner <bremner@debian.org>

As we noticed when Jani kindly converted things to getopt_long, much
of the work in argument parsing in notmuch is due to the the key-value
style arguments like --format=(raw|json|text).

In this version I implement Austin Clements' suggestion of basing the
main API on taking pointers to output variables.
---
 Makefile.local |    1 +
 notmuch-opts.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch-opts.h |   44 ++++++++++++++++++
 3 files changed, 181 insertions(+), 0 deletions(-)
 create mode 100644 notmuch-opts.c
 create mode 100644 notmuch-opts.h

diff --git a/Makefile.local b/Makefile.local
index c94402b..6606be8 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -303,6 +303,7 @@ notmuch_client_srcs =		\
 	notmuch-count.c		\
 	notmuch-dump.c		\
 	notmuch-new.c		\
+	notmuch-opts.c		\
 	notmuch-reply.c		\
 	notmuch-restore.c	\
 	notmuch-search.c	\
diff --git a/notmuch-opts.c b/notmuch-opts.c
new file mode 100644
index 0000000..62d94bf
--- /dev/null
+++ b/notmuch-opts.c
@@ -0,0 +1,136 @@
+#include <assert.h>
+#include <string.h>
+#include <stdio.h>
+#include "error_util.h"
+#include "notmuch-opts.h"
+
+/*
+  Search the list of keywords for a given argument, assigning the
+  output variable to the corresponding value.  Return FALSE if nothing
+  matches.
+*/
+
+static notmuch_bool_t
+_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
+
+    if (arg_str[0] != ':' && arg_str[0] != '=') {
+	return FALSE;
+    }
+
+    /* skip delimiter */
+    arg_str++;
+
+    notmuch_keyword_t *keywords = arg_desc->keywords;
+
+    while (keywords->name) {
+	if (strcmp (arg_str, keywords->name) == 0) {
+	    if (arg_desc->output_var) {
+		*((int *)arg_desc->output_var) = keywords->value;
+	    }
+	    return TRUE;
+	}
+	keywords++;
+    }
+    fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    return FALSE;
+}
+
+/*
+   Search for the {pos_arg_index}th position argument, return FALSE if
+   that does not exist.
+*/
+
+notmuch_bool_t
+parse_position_arg (const char *arg_str, int pos_arg_index, const notmuch_opt_desc_t *arg_desc) {
+
+    int pos_arg_counter = 0;
+    while (arg_desc->name){
+	if (arg_desc->opt_type == NOTMUCH_OPT_POSITION) {
+	    if (pos_arg_counter == pos_arg_index) {
+		if (arg_desc->output_var) {
+		    *((const char **)arg_desc->output_var) = arg_str;
+		}
+		return TRUE;
+	    }
+	    pos_arg_counter++;
+	}
+	arg_desc++;
+    }
+    return FALSE;
+}
+
+notmuch_bool_t
+parse_option (const char *arg,
+	      const notmuch_opt_desc_t *options){
+
+    assert(arg);
+    assert(options);
+
+    arg += 2;
+
+    const notmuch_opt_desc_t *try = options;
+    while (try->name) {
+	if (strncmp (arg, try->name, strlen(try->name)) == 0) {
+
+	    switch (try->opt_type) {
+	    case NOTMUCH_OPT_KEYWORD:
+		return  _process_keyword_arg (try, arg+strlen(try->name));
+		break;
+	    case NOTMUCH_OPT_BOOLEAN:
+		if (try->output_var)
+		    *((notmuch_bool_t *)try->output_var) = TRUE;
+		return TRUE;
+		break;
+	    case NOTMUCH_OPT_INT:
+		if (try->output_var)
+		    *((int *)try->output_var) =
+			atol (arg + strlen (try->name) +  1);
+		return TRUE;
+		break;
+
+	    case NOTMUCH_OPT_POSITION:
+	    case NOTMUCH_OPT_NULL:
+	    default:
+		INTERNAL_ERROR ("unknown or unhandled option type %d", try->opt_type);
+		/*UNREACHED*/
+	    }
+	}
+	try++;
+    }
+    fprintf (stderr, "Unrecognized option: --%s\n", arg);
+    return FALSE;
+}
+
+int
+notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){
+
+    int pos_arg_index = 0;
+    notmuch_bool_t more_args = TRUE;
+
+    while (more_args && opt_index < argc) {
+	if (strncmp (argv[opt_index],"--",2) != 0) {
+
+	    more_args = parse_position_arg (argv[opt_index], pos_arg_index, options);
+
+	    if (more_args) {
+		pos_arg_index++;
+		opt_index++;
+	    }
+
+	} else {
+
+	    if (strlen (argv[opt_index]) == 2)
+		return opt_index+1;
+
+	    more_args = parse_option (argv[opt_index], options);
+	    if (more_args) {
+		opt_index++;
+	    } else {
+		opt_index = -1;
+	    }
+
+	}
+    }
+
+    return opt_index;
+}
diff --git a/notmuch-opts.h b/notmuch-opts.h
new file mode 100644
index 0000000..2280fea
--- /dev/null
+++ b/notmuch-opts.h
@@ -0,0 +1,44 @@
+#ifndef NOTMUCH_OPTS_H
+#define NOTMUCH_OPTS_H
+
+#include "notmuch.h"
+
+enum notmuch_opt_type {
+    NOTMUCH_OPT_NULL = 0,
+    NOTMUCH_OPT_BOOLEAN,
+    NOTMUCH_OPT_INT,
+    NOTMUCH_OPT_KEYWORD,
+    NOTMUCH_OPT_POSITION
+};
+
+typedef struct notmuch_keyword {
+    const char *name;
+    int value;
+} notmuch_keyword_t;
+
+typedef struct notmuch_opt_desc {
+    const char *name;
+    int  arg_id;
+    enum notmuch_opt_type opt_type;
+    struct notmuch_keyword *keywords;
+    void *output_var;
+} notmuch_opt_desc_t;
+
+typedef struct notmuch_opt {
+    int arg_id;
+    int keyword_id;
+    const char *string;
+} notmuch_opt_t;
+
+notmuch_bool_t
+parse_option (const char *arg, const notmuch_opt_desc_t* options);
+
+notmuch_bool_t
+parse_position_arg (const char *arg,
+		    int position_arg_index,
+		    const notmuch_opt_desc_t* options);
+
+int
+notmuch_parse_args(int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index);
+
+#endif
-- 
1.7.7.3

[PATCH 2/4] notmuch-dump: convert to notmuch-opts argument handling.

[David Bremner]

From: David Bremner <bremner@debian.org>

The output file is handled via positional arguments. There are
currently no "normal" options.
---
 notmuch-dump.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index a490917..6fbdc81 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -19,6 +19,7 @@
  */
 
 #include "notmuch-client.h"
+#include "notmuch-opts.h"
 
 int
 notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
@@ -41,27 +42,34 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
     if (notmuch == NULL)
 	return 1;
 
-    argc--; argv++; /* skip subcommand argument */
+    char *output_file_name = NULL;
+    int opt_index;
 
-    if (argc && strcmp (argv[0], "--") != 0) {
+    notmuch_opt_desc_t options[] = {
+	{ "out-file", 'o', NOTMUCH_OPT_POSITION, 0, &output_file_name },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = notmuch_parse_args (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	exit(1);
+    }
+
+    if (output_file_name) {
 	fprintf (stderr, "Warning: the output file argument of dump is deprecated.\n");
-	output = fopen (argv[0], "w");
+	output = fopen (output_file_name, "w");
 	if (output == NULL) {
 	    fprintf (stderr, "Error opening %s for writing: %s\n",
-		     argv[0], strerror (errno));
+		     output_file_name, strerror (errno));
 	    return 1;
 	}
-	argc--;
-	argv++;
     }
 
-    if (argc && strcmp (argv[0], "--") == 0){
-	argc--;
-	argv++;
-    }
 
-    if (argc) {
-	query_str = query_string_from_args (notmuch, argc, argv);
+    if (opt_index < argc) {
+	query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
 	if (query_str == NULL) {
 	    fprintf (stderr, "Out of memory.\n");
 	    return 1;
-- 
1.7.7.3

[PATCH 3/4] notmuch-restore: convert to notmuch-opts argument handling.

[David Bremner]

From: David Bremner <bremner@debian.org>

The new argument handling is a bit more concise, and bit more
flexible.  It allows the input file name to go before the --accumulate
option.
---
 notmuch-restore.c |   38 ++++++++++++++++----------------------
 1 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 13b4325..d0aa7a8 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -18,9 +18,8 @@
  * Author: Carl Worth <cworth@cworth.org>
  */
 
-#include <getopt.h>
-
 #include "notmuch-client.h"
+#include "notmuch-opts.h"
 
 int
 notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
@@ -29,12 +28,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     notmuch_database_t *notmuch;
     notmuch_bool_t synchronize_flags;
     notmuch_bool_t accumulate = FALSE;
+    char *input_file_name = NULL;
     FILE *input = stdin;
     char *line = NULL;
     size_t line_size;
     ssize_t line_len;
     regex_t regex;
     int rerr;
+    int opt_index;
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -47,37 +48,30 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
 
-    struct option options[] = {
-	{ "accumulate",   no_argument,       0, 'a' },
-	{ 0, 0, 0, 0}
+    notmuch_opt_desc_t options[] = {
+	{ "in-file", 'i', NOTMUCH_OPT_POSITION, 0, &input_file_name },
+	{ "accumulate", 'a',  NOTMUCH_OPT_BOOLEAN, 0, &accumulate },
+	{ 0, 0, 0, 0, 0 }
     };
 
-    int opt;
-    do {
-	opt = getopt_long (argc, argv, "", options, NULL);
+    opt_index = notmuch_parse_args (argc, argv, options, 1);
 
-	switch (opt) {
-	case 'a':
-	    accumulate = 1;
-	    break;
-	case '?':
-	    return 1;
-	    break;
-	}
-
-    } while (opt != -1);
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	exit(1);
+    }
 
-    if (optind < argc) {
-	input = fopen (argv[optind], "r");
+    if (input_file_name) {
+	input = fopen (input_file_name, "r");
 	if (input == NULL) {
 	    fprintf (stderr, "Error opening %s for reading: %s\n",
-		     argv[optind], strerror (errno));
+		     input_file_name, strerror (errno));
 	    return 1;
 	}
 	optind++;
     }
 
-    if (optind < argc) {
+    if (opt_index < argc) {
 	fprintf (stderr,
 	 "Cannot read dump from more than one file: %s\n",
 		 argv[optind]);
-- 
1.7.7.3

[PATCH 4/4] notmuch-search: convert to notmuch-opts argument parsing.

[David Bremner]

From: David Bremner <bremner@debian.org>

The switch on format_sel is slightly clunky, but it doesn't seem worth
special casing argument processing for function pointers, when I think
the function pointer approach will be modified/abandoned.
---
 notmuch-search.c |  110 ++++++++++++++++++++---------------------------------
 1 files changed, 42 insertions(+), 68 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 36686d1..a1cecb0 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -19,6 +19,7 @@
  */
 
 #include "notmuch-client.h"
+#include "notmuch-opts.h"
 
 typedef enum {
     OUTPUT_SUMMARY,
@@ -415,81 +416,54 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     notmuch_database_t *notmuch;
     notmuch_query_t *query;
     char *query_str;
-    char *opt;
     notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST;
     const search_format_t *format = &format_text;
-    int i, ret;
+    int opt_index, ret;
     output_t output = OUTPUT_SUMMARY;
     int offset = 0;
     int limit = -1; /* unlimited */
 
-    argc--; argv++; /* skip subcommand argument */
-
-    for (i = 0; i < argc && argv[i][0] == '-'; i++) {
-	if (strcmp (argv[i], "--") == 0) {
-	    i++;
-	    break;
-	}
-        if (STRNCMP_LITERAL (argv[i], "--sort=") == 0) {
-	    opt = argv[i] + sizeof ("--sort=") - 1;
-	    if (strcmp (opt, "oldest-first") == 0) {
-		sort = NOTMUCH_SORT_OLDEST_FIRST;
-	    } else if (strcmp (opt, "newest-first") == 0) {
-		sort = NOTMUCH_SORT_NEWEST_FIRST;
-	    } else {
-		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--offset=") == 0) {
-	    char *p;
-	    opt = argv[i] + sizeof ("--offset=") - 1;
-	    offset = strtol (opt, &p, 10);
-	    if (*opt == '\0' || p == opt || *p != '\0') {
-		fprintf (stderr, "Invalid value for --offset: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--limit=") == 0) {
-	    char *p;
-	    opt = argv[i] + sizeof ("--limit=") - 1;
-	    limit = strtoul (opt, &p, 10);
-	    if (*opt == '\0' || p == opt || *p != '\0') {
-		fprintf (stderr, "Invalid value for --limit: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {
-	    opt = argv[i] + sizeof ("--format=") - 1;
-	    if (strcmp (opt, "text") == 0) {
-		format = &format_text;
-	    } else if (strcmp (opt, "json") == 0) {
-		format = &format_json;
-	    } else {
-		fprintf (stderr, "Invalid value for --format: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--output=") == 0) {
-	    opt = argv[i] + sizeof ("--output=") - 1;
-	    if (strcmp (opt, "summary") == 0) {
-		output = OUTPUT_SUMMARY;
-	    } else if (strcmp (opt, "threads") == 0) {
-		output = OUTPUT_THREADS;
-	    } else if (strcmp (opt, "messages") == 0) {
-		output = OUTPUT_MESSAGES;
-	    } else if (strcmp (opt, "files") == 0) {
-		output = OUTPUT_FILES;
-	    } else if (strcmp (opt, "tags") == 0) {
-		output = OUTPUT_TAGS;
-	    } else {
-		fprintf (stderr, "Invalid value for --output: %s\n", opt);
-		return 1;
-	    }
-	} else {
-	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
-	    return 1;
-	}
+    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
+	format_sel = NOTMUCH_FORMAT_TEXT;
+
+    notmuch_opt_desc_t options[] = {
+	{ "sort", 's', NOTMUCH_OPT_KEYWORD,
+	  (notmuch_keyword_t []){ { "oldest-first", NOTMUCH_SORT_OLDEST_FIRST },
+				  { "newest-first", NOTMUCH_SORT_NEWEST_FIRST },
+				  {0, 0} },
+	  &sort },
+	{ "format", 'f', NOTMUCH_OPT_KEYWORD,
+	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
+				  { "text", NOTMUCH_FORMAT_TEXT },
+				  {0, 0} },
+	  &format_sel },
+	{ "output", 'o', NOTMUCH_OPT_KEYWORD,
+	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
+				  { "threads", OUTPUT_THREADS },
+				  { "messages", OUTPUT_MESSAGES },
+				  { "files", OUTPUT_FILES },
+				  { "tags", OUTPUT_TAGS },
+				  {0, 0} },
+	  &output },
+	{ "offset", 'O', NOTMUCH_OPT_INT, 0, &offset },
+	{ "limit", 'L', NOTMUCH_OPT_INT, 0, &limit },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = notmuch_parse_args (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	exit(1);
     }
 
-    argc -= i;
-    argv += i;
+    switch (format_sel) {
+    case NOTMUCH_FORMAT_TEXT:
+	format = &format_text;
+	break;
+    case NOTMUCH_FORMAT_JSON:
+	format = &format_json;
+	break;
+    }
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -500,7 +474,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     if (notmuch == NULL)
 	return 1;
 
-    query_str = query_string_from_args (notmuch, argc, argv);
+    query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
     if (query_str == NULL) {
 	fprintf (stderr, "Out of memory.\n");
 	return 1;
-- 
1.7.7.3

Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.

[Jani Nikula]

On Sun,  4 Dec 2011 11:47:52 -0400, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@debian.org>
> 
> As we noticed when Jani kindly converted things to getopt_long, much
> of the work in argument parsing in notmuch is due to the the key-value
> style arguments like --format=(raw|json|text).

Hi David -

Apologies for not finding time to do proper review earlier. All in all I
think this is good work, and I especially like the simplicity of
defining arguments in the commands. Even if it makes me slightly sad to
have to reinvent the argument parsing wheel... but this is much better
than just using getopt_long() like I did.

So the design is good, but there are a few issues and nitpicks; please
find my comments below.

BR,
Jani.

> In this version I implement Austin Clements' suggestion of basing the
> main API on taking pointers to output variables.
> ---
>  Makefile.local |    1 +
>  notmuch-opts.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  notmuch-opts.h |   44 ++++++++++++++++++
>  3 files changed, 181 insertions(+), 0 deletions(-)
>  create mode 100644 notmuch-opts.c
>  create mode 100644 notmuch-opts.h
> 
> diff --git a/Makefile.local b/Makefile.local
> index c94402b..6606be8 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -303,6 +303,7 @@ notmuch_client_srcs =		\
>  	notmuch-count.c		\
>  	notmuch-dump.c		\
>  	notmuch-new.c		\
> +	notmuch-opts.c		\

We chatted about reserving notmuch-*.c to notmuch commands, but I guess
it was only after you sent these. I think it would make sense though.

>  	notmuch-reply.c		\
>  	notmuch-restore.c	\
>  	notmuch-search.c	\
> diff --git a/notmuch-opts.c b/notmuch-opts.c
> new file mode 100644
> index 0000000..62d94bf
> --- /dev/null
> +++ b/notmuch-opts.c
> @@ -0,0 +1,136 @@
> +#include <assert.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include "error_util.h"
> +#include "notmuch-opts.h"
> +
> +/*
> +  Search the list of keywords for a given argument, assigning the
> +  output variable to the corresponding value.  Return FALSE if nothing
> +  matches.
> +*/

Array of keywords to be really pedantic.

> +
> +static notmuch_bool_t
> +_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
> +
> +    if (arg_str[0] != ':' && arg_str[0] != '=') {
> +	return FALSE;
> +    }
> +
> +    /* skip delimiter */
> +    arg_str++;

I think the caller should check and skip the delimiters. See my comments
below where this gets called.

> +
> +    notmuch_keyword_t *keywords = arg_desc->keywords;
> +
> +    while (keywords->name) {
> +	if (strcmp (arg_str, keywords->name) == 0) {
> +	    if (arg_desc->output_var) {
> +		*((int *)arg_desc->output_var) = keywords->value;
> +	    }

So if ->output_var is NULL, the parameter is accepted but silently
ignored? I'm not sure if I should consider this a feature or a bug. :)

This applies below in similar places; I'll not repeat myself.

> +	    return TRUE;
> +	}
> +	keywords++;
> +    }
> +    fprintf (stderr, "unknown keyword: %s\n", arg_str);
> +    return FALSE;
> +}
> +
> +/*
> +   Search for the {pos_arg_index}th position argument, return FALSE if
> +   that does not exist.
> +*/
> +
> +notmuch_bool_t
> +parse_position_arg (const char *arg_str, int pos_arg_index, const notmuch_opt_desc_t *arg_desc) {
> +
> +    int pos_arg_counter = 0;
> +    while (arg_desc->name){
> +	if (arg_desc->opt_type == NOTMUCH_OPT_POSITION) {
> +	    if (pos_arg_counter == pos_arg_index) {
> +		if (arg_desc->output_var) {
> +		    *((const char **)arg_desc->output_var) = arg_str;
> +		}
> +		return TRUE;
> +	    }
> +	    pos_arg_counter++;
> +	}
> +	arg_desc++;
> +    }
> +    return FALSE;
> +}
> +
> +notmuch_bool_t
> +parse_option (const char *arg,
> +	      const notmuch_opt_desc_t *options){

Missing space between ) and {.

> +
> +    assert(arg);
> +    assert(options);
> +
> +    arg += 2;
> +
> +    const notmuch_opt_desc_t *try = options;
> +    while (try->name) {
> +	if (strncmp (arg, try->name, strlen(try->name)) == 0) {

I think here you should check that arg[strlen(try->name)] is '=' or ':'
for NOTMUCH_OPT_KEYWORD and NOTMUCH_OPT_INT, and '\0' otherwise. After
the check, you could pass just the value part to _process_keyword_arg().

I can't figure out if and how you handle arguments with arbitrary string
values (for example --file=/path/to/file). You do specify --in-file and
--out-file in later patches, but those are with NOTMUCH_OPT_POSITION,
which, AFAICT, would fall in the internal error path below. They'll
*only* be handled as positional arguments, not option args.

I'm not sure if much weight should be put to getopt_long()
compatibility, but it accepts "--parameter value" as well as
"--parameter=value".

> +
> +	    switch (try->opt_type) {
> +	    case NOTMUCH_OPT_KEYWORD:
> +		return  _process_keyword_arg (try, arg+strlen(try->name));

Two spaces after return.

> +		break;
> +	    case NOTMUCH_OPT_BOOLEAN:
> +		if (try->output_var)
> +		    *((notmuch_bool_t *)try->output_var) = TRUE;
> +		return TRUE;
> +		break;
> +	    case NOTMUCH_OPT_INT:
> +		if (try->output_var)
> +		    *((int *)try->output_var) =
> +			atol (arg + strlen (try->name) +  1);

As implied above, the "+1" here might skip any character. Also, two
spaces after +.

> +		return TRUE;
> +		break;
> +
> +	    case NOTMUCH_OPT_POSITION:
> +	    case NOTMUCH_OPT_NULL:
> +	    default:
> +		INTERNAL_ERROR ("unknown or unhandled option type %d", try->opt_type);
> +		/*UNREACHED*/
> +	    }
> +	}
> +	try++;
> +    }
> +    fprintf (stderr, "Unrecognized option: --%s\n", arg);
> +    return FALSE;
> +}
> +

I think notmuch_parse_args() is a complicated function enough to warrant
a proper description here.

> +int
> +notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){

Missing space between ) and {.

> +
> +    int pos_arg_index = 0;
> +    notmuch_bool_t more_args = TRUE;
> +
> +    while (more_args && opt_index < argc) {
> +	if (strncmp (argv[opt_index],"--",2) != 0) {
> +
> +	    more_args = parse_position_arg (argv[opt_index], pos_arg_index, options);
> +
> +	    if (more_args) {
> +		pos_arg_index++;
> +		opt_index++;
> +	    }
> +
> +	} else {
> +
> +	    if (strlen (argv[opt_index]) == 2)
> +		return opt_index+1;
> +
> +	    more_args = parse_option (argv[opt_index], options);
> +	    if (more_args) {
> +		opt_index++;
> +	    } else {
> +		opt_index = -1;
> +	    }
> +
> +	}
> +    }
> +
> +    return opt_index;
> +}
> diff --git a/notmuch-opts.h b/notmuch-opts.h
> new file mode 100644
> index 0000000..2280fea
> --- /dev/null
> +++ b/notmuch-opts.h
> @@ -0,0 +1,44 @@
> +#ifndef NOTMUCH_OPTS_H
> +#define NOTMUCH_OPTS_H
> +
> +#include "notmuch.h"
> +
> +enum notmuch_opt_type {
> +    NOTMUCH_OPT_NULL = 0,
> +    NOTMUCH_OPT_BOOLEAN,
> +    NOTMUCH_OPT_INT,
> +    NOTMUCH_OPT_KEYWORD,
> +    NOTMUCH_OPT_POSITION
> +};
> +
> +typedef struct notmuch_keyword {
> +    const char *name;
> +    int value;
> +} notmuch_keyword_t;
> +
> +typedef struct notmuch_opt_desc {
> +    const char *name;
> +    int  arg_id;
> +    enum notmuch_opt_type opt_type;
> +    struct notmuch_keyword *keywords;
> +    void *output_var;
> +} notmuch_opt_desc_t;
> +
> +typedef struct notmuch_opt {
> +    int arg_id;
> +    int keyword_id;
> +    const char *string;
> +} notmuch_opt_t;

You're not using this for anything, are you?

> +
> +notmuch_bool_t
> +parse_option (const char *arg, const notmuch_opt_desc_t* options);
> +
> +notmuch_bool_t
> +parse_position_arg (const char *arg,
> +		    int position_arg_index,
> +		    const notmuch_opt_desc_t* options);

Is there a good reason the above are not static?

> +
> +int
> +notmuch_parse_args(int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index);
> +
> +#endif
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/4] notmuch-dump: convert to notmuch-opts argument handling.

[Jani Nikula]

On Sun,  4 Dec 2011 11:47:53 -0400, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@debian.org>
> 
> The output file is handled via positional arguments. There are
> currently no "normal" options.
> ---
>  notmuch-dump.c |   32 ++++++++++++++++++++------------
>  1 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/notmuch-dump.c b/notmuch-dump.c
> index a490917..6fbdc81 100644
> --- a/notmuch-dump.c
> +++ b/notmuch-dump.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "notmuch-client.h"
> +#include "notmuch-opts.h"
>  
>  int
>  notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
> @@ -41,27 +42,34 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
>      if (notmuch == NULL)
>  	return 1;
>  
> -    argc--; argv++; /* skip subcommand argument */
> +    char *output_file_name = NULL;
> +    int opt_index;
>  
> -    if (argc && strcmp (argv[0], "--") != 0) {
> +    notmuch_opt_desc_t options[] = {
> +	{ "out-file", 'o', NOTMUCH_OPT_POSITION, 0, &output_file_name },
> +	{ 0, 0, 0, 0, 0 }
> +    };
> +
> +    opt_index = notmuch_parse_args (argc, argv, options, 1);
> +
> +    if (opt_index < 0) {
> +	/* diagnostics already printed */
> +	exit(1);

return 1 rather than exit(1)?

BR,
Jani.

> +    }
> +
> +    if (output_file_name) {
>  	fprintf (stderr, "Warning: the output file argument of dump is deprecated.\n");
> -	output = fopen (argv[0], "w");
> +	output = fopen (output_file_name, "w");
>  	if (output == NULL) {
>  	    fprintf (stderr, "Error opening %s for writing: %s\n",
> -		     argv[0], strerror (errno));
> +		     output_file_name, strerror (errno));
>  	    return 1;
>  	}
> -	argc--;
> -	argv++;
>      }
>  
> -    if (argc && strcmp (argv[0], "--") == 0){
> -	argc--;
> -	argv++;
> -    }
>  
> -    if (argc) {
> -	query_str = query_string_from_args (notmuch, argc, argv);
> +    if (opt_index < argc) {
> +	query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
>  	if (query_str == NULL) {
>  	    fprintf (stderr, "Out of memory.\n");
>  	    return 1;
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/4] notmuch-restore: convert to notmuch-opts argument handling.

[Jani Nikula]

On Sun,  4 Dec 2011 11:47:54 -0400, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@debian.org>
> 
> The new argument handling is a bit more concise, and bit more
> flexible.  It allows the input file name to go before the --accumulate
> option.
> ---
>  notmuch-restore.c |   38 ++++++++++++++++----------------------
>  1 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 13b4325..d0aa7a8 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -18,9 +18,8 @@
>   * Author: Carl Worth <cworth@cworth.org>
>   */
>  
> -#include <getopt.h>
> -
>  #include "notmuch-client.h"
> +#include "notmuch-opts.h"

I guess this will eventually be included almost everywhere, so in that
sense could be put in notmuch-client.h too.

>  
>  int
>  notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
> @@ -29,12 +28,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>      notmuch_database_t *notmuch;
>      notmuch_bool_t synchronize_flags;
>      notmuch_bool_t accumulate = FALSE;
> +    char *input_file_name = NULL;
>      FILE *input = stdin;
>      char *line = NULL;
>      size_t line_size;
>      ssize_t line_len;
>      regex_t regex;
>      int rerr;
> +    int opt_index;
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
> @@ -47,37 +48,30 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  
>      synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>  
> -    struct option options[] = {
> -	{ "accumulate",   no_argument,       0, 'a' },
> -	{ 0, 0, 0, 0}
> +    notmuch_opt_desc_t options[] = {
> +	{ "in-file", 'i', NOTMUCH_OPT_POSITION, 0, &input_file_name },

Unless I'm mistaken in review of patch 1, specifying the input file
using --in-file=file doesn't work; it only works as a positional
argument.

> +	{ "accumulate", 'a',  NOTMUCH_OPT_BOOLEAN, 0, &accumulate },
> +	{ 0, 0, 0, 0, 0 }
>      };
>  
> -    int opt;
> -    do {
> -	opt = getopt_long (argc, argv, "", options, NULL);
> +    opt_index = notmuch_parse_args (argc, argv, options, 1);
>  
> -	switch (opt) {
> -	case 'a':
> -	    accumulate = 1;
> -	    break;
> -	case '?':
> -	    return 1;
> -	    break;
> -	}
> -
> -    } while (opt != -1);
> +    if (opt_index < 0) {
> +	/* diagnostics already printed */
> +	exit(1);

return 1 rather than exit(1)?

BR,
Jani.

> +    }
>  
> -    if (optind < argc) {
> -	input = fopen (argv[optind], "r");
> +    if (input_file_name) {
> +	input = fopen (input_file_name, "r");
>  	if (input == NULL) {
>  	    fprintf (stderr, "Error opening %s for reading: %s\n",
> -		     argv[optind], strerror (errno));
> +		     input_file_name, strerror (errno));
>  	    return 1;
>  	}
>  	optind++;
>      }
>  
> -    if (optind < argc) {
> +    if (opt_index < argc) {
>  	fprintf (stderr,
>  	 "Cannot read dump from more than one file: %s\n",
>  		 argv[optind]);
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.

[Jani Nikula]

On Sun,  4 Dec 2011 11:47:52 -0400, David Bremner <david@tethera.net> wrote:
> +	    case NOTMUCH_OPT_INT:
> +		if (try->output_var)
> +		    *((int *)try->output_var) =
> +			atol (arg + strlen (try->name) +  1);

Looking at patch 4 and the existing handling of int params, I'd really
like this one to check that the string is non-empty and that all of the
string is parsed (and doesn't contain garbage at the end).

BR,
Jani.

Re: [PATCH 1/4] notmuch-opts.[ch]: new argument parsing framework for notmuch.

[David Bremner]

On Tue, 06 Dec 2011 22:41:23 +0200, Jani Nikula <jani@nikula.org> wrote:
> 
> We chatted about reserving notmuch-*.c to notmuch commands, but I guess
> it was only after you sent these. I think it would make sense though.

renamed to "command-line-arguments.[ch]". Hard luck for those of you on
8.3 file systems.  

> > +/*
> > +  Search the list of keywords for a given argument, assigning the
> > +  output variable to the corresponding value.  Return FALSE if nothing
> > +  matches.
> > +*/
> 
> Array of keywords to be really pedantic.

Done.

> > +
> > +    /* skip delimiter */
> > +    arg_str++;
> 
> I think the caller should check and skip the delimiters. See my comments
> below where this gets called.

done, and error checking tightened up.

> 
> So if ->output_var is NULL, the parameter is accepted but silently
> ignored? I'm not sure if I should consider this a feature or a bug. :)

From one extreme to another, it is now an INTERNAL_ERROR to have
output_var NULL. I couldn't see a use case for silently ignoring command
line arguments.

> > +parse_option (const char *arg,
> > +	      const notmuch_opt_desc_t *options){
> 
> Missing space between ) and {.

done. but you missed some other missing spaces ;).

> I think here you should check that arg[strlen(try->name)] is '=' or ':'
> for NOTMUCH_OPT_KEYWORD and NOTMUCH_OPT_INT, and '\0' otherwise. After
> the check, you could pass just the value part to _process_keyword_arg().

done.

> I can't figure out if and how you handle arguments with arbitrary string
> values (for example --file=/path/to/file). You do specify --in-file and
> --out-file in later patches, but those are with NOTMUCH_OPT_POSITION,

there is now NOTMUCH_OPT_STRING which does this, but it is untested as
notmuch doesn't take these kind of arguments at the moment (restore
--match is one example, but those patches are unmerged so far).

> I'm not sure if much weight should be put to getopt_long()
> compatibility, but it accepts "--parameter value" as well as
> "--parameter=value".

Yeah, maybe I shouldn't let the implementation drive things this much,
but having one argmument per element of argv simplfies things
nicely. For now, I will skip this.

> 
> I think notmuch_parse_args() is a complicated function enough to warrant
> a proper description here.

Done.

> 
> > +int
> > +notmuch_parse_args (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index){
> 
> Missing space between ) and {.

Done.

> > +typedef struct notmuch_opt {
> > +    int arg_id;
> > +    int keyword_id;
> > +    const char *string;
> > +} notmuch_opt_t;
> 
> You're not using this for anything, are you?

Oops, deleted.
> 
> > +
> > +notmuch_bool_t
> > +parse_option (const char *arg, const notmuch_opt_desc_t* options);
> > +
> > +notmuch_bool_t
> > +parse_position_arg (const char *arg,
> > +		    int position_arg_index,
> > +		    const notmuch_opt_desc_t* options);
> 
> Is there a good reason the above are not static?

I had in mind to provide the user with the option of a getopt style loop
if the loop in parse_arguments was not flexible enough.  I could leave
them as static until such a need arises, I suppose.

Thanks for the review! Updated series to follow.

d

[PATCH 1/4] command-line-arguments.[ch]: new argument parsing framework for notmuch.

[David Bremner]

From: David Bremner <bremner@debian.org>

As we noticed when Jani kindly converted things to getopt_long, much
of the work in argument parsing in notmuch is due to the the key-value
style arguments like --format=(raw|json|text).

The framework here provides positional arguments, simple switches,
and --key=value style arguments that can take a value being an integer,
a string, or one of a set of keywords.
---
 Makefile.local           |    1 +
 command-line-arguments.c |  159 ++++++++++++++++++++++++++++++++++++++++++++++
 command-line-arguments.h |   39 +++++++++++
 notmuch-client.h         |    1 +
 4 files changed, 200 insertions(+), 0 deletions(-)
 create mode 100644 command-line-arguments.c
 create mode 100644 command-line-arguments.h

diff --git a/Makefile.local b/Makefile.local
index 15e6d88..28e371a 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -295,6 +295,7 @@ clean:
 distclean: clean
 
 notmuch_client_srcs =		\
+	command-line-arguments.c\
 	debugger.c		\
 	gmime-filter-reply.c	\
 	gmime-filter-headers.c	\
diff --git a/command-line-arguments.c b/command-line-arguments.c
new file mode 100644
index 0000000..96ed9ba
--- /dev/null
+++ b/command-line-arguments.c
@@ -0,0 +1,159 @@
+#include <assert.h>
+#include <string.h>
+#include <stdio.h>
+#include "error_util.h"
+#include "command-line-arguments.h"
+
+/*
+  Search the array of keywords for a given argument, assigning the
+  output variable to the corresponding value.  Return FALSE if nothing
+  matches.
+*/
+
+static notmuch_bool_t
+_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
+
+    notmuch_keyword_t *keywords = arg_desc->keywords;
+
+    while (keywords->name) {
+	if (strcmp (arg_str, keywords->name) == 0) {
+	    if (arg_desc->output_var) {
+		*((int *)arg_desc->output_var) = keywords->value;
+	    }
+	    return TRUE;
+	}
+	keywords++;
+    }
+    fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    return FALSE;
+}
+
+/*
+   Search for the {pos_arg_index}th position argument, return FALSE if
+   that does not exist.
+*/
+
+notmuch_bool_t
+parse_position_arg (const char *arg_str, int pos_arg_index, const notmuch_opt_desc_t *arg_desc) {
+
+    int pos_arg_counter = 0;
+    while (arg_desc->name){
+	if (arg_desc->opt_type == NOTMUCH_OPT_POSITION) {
+	    if (pos_arg_counter == pos_arg_index) {
+		if (arg_desc->output_var) {
+		    *((const char **)arg_desc->output_var) = arg_str;
+		}
+		return TRUE;
+	    }
+	    pos_arg_counter++;
+	}
+	arg_desc++;
+    }
+    return FALSE;
+}
+
+notmuch_bool_t
+parse_option (const char *arg,
+	      const notmuch_opt_desc_t *options) {
+
+    assert(arg);
+    assert(options);
+
+    arg += 2;
+
+    const notmuch_opt_desc_t *try = options;
+    while (try->name) {
+	if (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
+	     */
+
+	    if (try->opt_type != NOTMUCH_OPT_BOOLEAN) {
+		if (next != '=' && next != ':') return FALSE;
+		if (value[0] == 0) return FALSE;
+	    } else {
+		if (next != 0) return FALSE;
+	    }
+
+	    if (try->output_var == NULL)
+		INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
+
+	    switch (try->opt_type) {
+	    case NOTMUCH_OPT_KEYWORD:
+		return _process_keyword_arg (try, value);
+		break;
+	    case NOTMUCH_OPT_BOOLEAN:
+		*((notmuch_bool_t *)try->output_var) = TRUE;
+		return TRUE;
+		break;
+	    case NOTMUCH_OPT_INT:
+		*((int *)try->output_var) = strtol (value, &endptr, 10);
+		return (*endptr == 0);
+		break;
+	    case NOTMUCH_OPT_STRING:
+		*((const char **)try->output_var) = value;
+	    case NOTMUCH_OPT_POSITION:
+	    case NOTMUCH_OPT_NULL:
+	    default:
+		INTERNAL_ERROR ("unknown or unhandled option type %d", try->opt_type);
+		/*UNREACHED*/
+	    }
+	}
+	try++;
+    }
+    fprintf (stderr, "Unrecognized option: --%s\n", arg);
+    return FALSE;
+}
+
+/*
+   Parse command line arguments according to structure options,
+   starting at position opt_index.
+
+   All output of parsed values is via pointers in options.
+
+   Parsing stops at -- (consumed) or at the (k+1)st argument
+   not starting with -- (a "positional argument") if options contains
+   k positional argument descriptors.
+
+   Returns the index of first non-parsed argument, or -1 in case of error.
+
+  */
+int
+parse_arguments (int argc, char **argv,
+		 const notmuch_opt_desc_t *options, int opt_index) {
+
+    int pos_arg_index = 0;
+    notmuch_bool_t more_args = TRUE;
+
+    while (more_args && opt_index < argc) {
+	if (strncmp (argv[opt_index],"--",2) != 0) {
+
+	    more_args = parse_position_arg (argv[opt_index], pos_arg_index, options);
+
+	    if (more_args) {
+		pos_arg_index++;
+		opt_index++;
+	    }
+
+	} else {
+
+	    if (strlen (argv[opt_index]) == 2)
+		return opt_index+1;
+
+	    more_args = parse_option (argv[opt_index], options);
+	    if (more_args) {
+		opt_index++;
+	    } else {
+		opt_index = -1;
+	    }
+
+	}
+    }
+
+    return opt_index;
+}
diff --git a/command-line-arguments.h b/command-line-arguments.h
new file mode 100644
index 0000000..caebe18
--- /dev/null
+++ b/command-line-arguments.h
@@ -0,0 +1,39 @@
+#ifndef NOTMUCH_OPTS_H
+#define NOTMUCH_OPTS_H
+
+#include "notmuch.h"
+
+enum notmuch_opt_type {
+    NOTMUCH_OPT_NULL = 0,
+    NOTMUCH_OPT_BOOLEAN,
+    NOTMUCH_OPT_INT,
+    NOTMUCH_OPT_KEYWORD,
+    NOTMUCH_OPT_STRING,
+    NOTMUCH_OPT_POSITION
+};
+
+typedef struct notmuch_keyword {
+    const char *name;
+    int value;
+} notmuch_keyword_t;
+
+typedef struct notmuch_opt_desc {
+    const char *name;
+    int  arg_id;
+    enum notmuch_opt_type opt_type;
+    struct notmuch_keyword *keywords;
+    void *output_var;
+} notmuch_opt_desc_t;
+
+notmuch_bool_t
+parse_option (const char *arg, const notmuch_opt_desc_t* options);
+
+notmuch_bool_t
+parse_position_arg (const char *arg,
+		    int position_arg_index,
+		    const notmuch_opt_desc_t* options);
+
+int
+parse_arguments(int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index);
+
+#endif
diff --git a/notmuch-client.h b/notmuch-client.h
index b50cb38..703f856 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -238,4 +238,5 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 notmuch_bool_t
 debugger_is_active (void);
 
+#include "command-line-arguments.h"
 #endif
-- 
1.7.7.3

[PATCH 2/4] notmuch-dump: convert to command-line-arguments

[David Bremner]

From: David Bremner <bremner@debian.org>

The output file is handled via positional arguments. There are
currently no "normal" options.
---
 notmuch-dump.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index a490917..c568734 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -41,27 +41,34 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
     if (notmuch == NULL)
 	return 1;
 
-    argc--; argv++; /* skip subcommand argument */
+    char *output_file_name = NULL;
+    int opt_index;
 
-    if (argc && strcmp (argv[0], "--") != 0) {
+    notmuch_opt_desc_t options[] = {
+	{ "out-file", 'o', NOTMUCH_OPT_POSITION, 0, &output_file_name },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	exit(1);
+    }
+
+    if (output_file_name) {
 	fprintf (stderr, "Warning: the output file argument of dump is deprecated.\n");
-	output = fopen (argv[0], "w");
+	output = fopen (output_file_name, "w");
 	if (output == NULL) {
 	    fprintf (stderr, "Error opening %s for writing: %s\n",
-		     argv[0], strerror (errno));
+		     output_file_name, strerror (errno));
 	    return 1;
 	}
-	argc--;
-	argv++;
     }
 
-    if (argc && strcmp (argv[0], "--") == 0){
-	argc--;
-	argv++;
-    }
 
-    if (argc) {
-	query_str = query_string_from_args (notmuch, argc, argv);
+    if (opt_index < argc) {
+	query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
 	if (query_str == NULL) {
 	    fprintf (stderr, "Out of memory.\n");
 	    return 1;
-- 
1.7.7.3

[PATCH 3/4] notmuch-restore: convert to command-line-arguments

[David Bremner]

From: David Bremner <bremner@debian.org>

The new argument handling is a bit more concise, and bit more
flexible.  It allows the input file name to go before the --accumulate
option.
---
 notmuch-restore.c |   37 +++++++++++++++----------------------
 1 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 13b4325..708f3e9 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -18,8 +18,6 @@
  * Author: Carl Worth <cworth@cworth.org>
  */
 
-#include <getopt.h>
-
 #include "notmuch-client.h"
 
 int
@@ -29,12 +27,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     notmuch_database_t *notmuch;
     notmuch_bool_t synchronize_flags;
     notmuch_bool_t accumulate = FALSE;
+    char *input_file_name = NULL;
     FILE *input = stdin;
     char *line = NULL;
     size_t line_size;
     ssize_t line_len;
     regex_t regex;
     int rerr;
+    int opt_index;
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -47,37 +47,30 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
 
-    struct option options[] = {
-	{ "accumulate",   no_argument,       0, 'a' },
-	{ 0, 0, 0, 0}
+    notmuch_opt_desc_t options[] = {
+	{ "in-file", 'i', NOTMUCH_OPT_POSITION, 0, &input_file_name },
+	{ "accumulate", 'a',  NOTMUCH_OPT_BOOLEAN, 0, &accumulate },
+	{ 0, 0, 0, 0, 0 }
     };
 
-    int opt;
-    do {
-	opt = getopt_long (argc, argv, "", options, NULL);
+    opt_index = parse_arguments (argc, argv, options, 1);
 
-	switch (opt) {
-	case 'a':
-	    accumulate = 1;
-	    break;
-	case '?':
-	    return 1;
-	    break;
-	}
-
-    } while (opt != -1);
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	exit(1);
+    }
 
-    if (optind < argc) {
-	input = fopen (argv[optind], "r");
+    if (input_file_name) {
+	input = fopen (input_file_name, "r");
 	if (input == NULL) {
 	    fprintf (stderr, "Error opening %s for reading: %s\n",
-		     argv[optind], strerror (errno));
+		     input_file_name, strerror (errno));
 	    return 1;
 	}
 	optind++;
     }
 
-    if (optind < argc) {
+    if (opt_index < argc) {
 	fprintf (stderr,
 	 "Cannot read dump from more than one file: %s\n",
 		 argv[optind]);
-- 
1.7.7.3

[PATCH 4/4] notmuch-search: convert to command-line-arguments

[David Bremner]

From: David Bremner <bremner@debian.org>

The switch on format_sel is slightly clunky, but it doesn't seem worth
special casing argument processing for function pointers, when I think
the function pointer approach will be modified/abandoned.
---
 notmuch-search.c |  109 ++++++++++++++++++++---------------------------------
 1 files changed, 41 insertions(+), 68 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 36686d1..2ae7597 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -415,81 +415,54 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     notmuch_database_t *notmuch;
     notmuch_query_t *query;
     char *query_str;
-    char *opt;
     notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST;
     const search_format_t *format = &format_text;
-    int i, ret;
+    int opt_index, ret;
     output_t output = OUTPUT_SUMMARY;
     int offset = 0;
     int limit = -1; /* unlimited */
 
-    argc--; argv++; /* skip subcommand argument */
-
-    for (i = 0; i < argc && argv[i][0] == '-'; i++) {
-	if (strcmp (argv[i], "--") == 0) {
-	    i++;
-	    break;
-	}
-        if (STRNCMP_LITERAL (argv[i], "--sort=") == 0) {
-	    opt = argv[i] + sizeof ("--sort=") - 1;
-	    if (strcmp (opt, "oldest-first") == 0) {
-		sort = NOTMUCH_SORT_OLDEST_FIRST;
-	    } else if (strcmp (opt, "newest-first") == 0) {
-		sort = NOTMUCH_SORT_NEWEST_FIRST;
-	    } else {
-		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--offset=") == 0) {
-	    char *p;
-	    opt = argv[i] + sizeof ("--offset=") - 1;
-	    offset = strtol (opt, &p, 10);
-	    if (*opt == '\0' || p == opt || *p != '\0') {
-		fprintf (stderr, "Invalid value for --offset: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--limit=") == 0) {
-	    char *p;
-	    opt = argv[i] + sizeof ("--limit=") - 1;
-	    limit = strtoul (opt, &p, 10);
-	    if (*opt == '\0' || p == opt || *p != '\0') {
-		fprintf (stderr, "Invalid value for --limit: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {
-	    opt = argv[i] + sizeof ("--format=") - 1;
-	    if (strcmp (opt, "text") == 0) {
-		format = &format_text;
-	    } else if (strcmp (opt, "json") == 0) {
-		format = &format_json;
-	    } else {
-		fprintf (stderr, "Invalid value for --format: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--output=") == 0) {
-	    opt = argv[i] + sizeof ("--output=") - 1;
-	    if (strcmp (opt, "summary") == 0) {
-		output = OUTPUT_SUMMARY;
-	    } else if (strcmp (opt, "threads") == 0) {
-		output = OUTPUT_THREADS;
-	    } else if (strcmp (opt, "messages") == 0) {
-		output = OUTPUT_MESSAGES;
-	    } else if (strcmp (opt, "files") == 0) {
-		output = OUTPUT_FILES;
-	    } else if (strcmp (opt, "tags") == 0) {
-		output = OUTPUT_TAGS;
-	    } else {
-		fprintf (stderr, "Invalid value for --output: %s\n", opt);
-		return 1;
-	    }
-	} else {
-	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
-	    return 1;
-	}
+    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
+	format_sel = NOTMUCH_FORMAT_TEXT;
+
+    notmuch_opt_desc_t options[] = {
+	{ "sort", 's', NOTMUCH_OPT_KEYWORD,
+	  (notmuch_keyword_t []){ { "oldest-first", NOTMUCH_SORT_OLDEST_FIRST },
+				  { "newest-first", NOTMUCH_SORT_NEWEST_FIRST },
+				  {0, 0} },
+	  &sort },
+	{ "format", 'f', NOTMUCH_OPT_KEYWORD,
+	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
+				  { "text", NOTMUCH_FORMAT_TEXT },
+				  {0, 0} },
+	  &format_sel },
+	{ "output", 'o', NOTMUCH_OPT_KEYWORD,
+	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
+				  { "threads", OUTPUT_THREADS },
+				  { "messages", OUTPUT_MESSAGES },
+				  { "files", OUTPUT_FILES },
+				  { "tags", OUTPUT_TAGS },
+				  {0, 0} },
+	  &output },
+	{ "offset", 'O', NOTMUCH_OPT_INT, 0, &offset },
+	{ "limit", 'L', NOTMUCH_OPT_INT, 0, &limit },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	exit(1);
     }
 
-    argc -= i;
-    argv += i;
+    switch (format_sel) {
+    case NOTMUCH_FORMAT_TEXT:
+	format = &format_text;
+	break;
+    case NOTMUCH_FORMAT_JSON:
+	format = &format_json;
+	break;
+    }
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -500,7 +473,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     if (notmuch == NULL)
 	return 1;
 
-    query_str = query_string_from_args (notmuch, argc, argv);
+    query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
     if (query_str == NULL) {
 	fprintf (stderr, "Out of memory.\n");
 	return 1;
-- 
1.7.7.3

(no subject)

[David Bremner]

So it turns out the completely untested NOTMUCH_OPT_STRING code was
(surpise!) buggy.  I also fixed a few remaining formatting issues, added
a unit test, and some documentation (in command-line-arguements.h).  

I also reordered option description struct so that the two mandatory
(in the sense of being non-zero) members come first.

[PATCH v4 1/5] command-line-arguments.[ch]: new argument parsing framework for notmuch.

[David Bremner]

From: David Bremner <bremner@debian.org>

As we noticed when Jani kindly converted things to getopt_long, much
of the work in argument parsing in notmuch is due to the the key-value
style arguments like --format=(raw|json|text).

The framework here provides positional arguments, simple switches,
and --key=value style arguments that can take a value being an integer,
a string, or one of a set of keywords.
---
 Makefile.local           |    1 +
 command-line-arguments.c |  155 ++++++++++++++++++++++++++++++++++++++++++++++
 command-line-arguments.h |   80 ++++++++++++++++++++++++
 notmuch-client.h         |    1 +
 4 files changed, 237 insertions(+), 0 deletions(-)
 create mode 100644 command-line-arguments.c
 create mode 100644 command-line-arguments.h

diff --git a/Makefile.local b/Makefile.local
index 15e6d88..28e371a 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -295,6 +295,7 @@ clean:
 distclean: clean
 
 notmuch_client_srcs =		\
+	command-line-arguments.c\
 	debugger.c		\
 	gmime-filter-reply.c	\
 	gmime-filter-headers.c	\
diff --git a/command-line-arguments.c b/command-line-arguments.c
new file mode 100644
index 0000000..3aa87aa
--- /dev/null
+++ b/command-line-arguments.c
@@ -0,0 +1,155 @@
+#include <assert.h>
+#include <string.h>
+#include <stdio.h>
+#include "error_util.h"
+#include "command-line-arguments.h"
+
+/*
+  Search the array of keywords for a given argument, assigning the
+  output variable to the corresponding value.  Return FALSE if nothing
+  matches.
+*/
+
+static notmuch_bool_t
+_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, const char *arg_str) {
+
+    notmuch_keyword_t *keywords = arg_desc->keywords;
+
+    while (keywords->name) {
+	if (strcmp (arg_str, keywords->name) == 0) {
+	    if (arg_desc->output_var) {
+		*((int *)arg_desc->output_var) = keywords->value;
+	    }
+	    return TRUE;
+	}
+	keywords++;
+    }
+    fprintf (stderr, "unknown keyword: %s\n", arg_str);
+    return FALSE;
+}
+
+/*
+   Search for the {pos_arg_index}th position argument, return FALSE if
+   that does not exist.
+*/
+
+notmuch_bool_t
+parse_position_arg (const char *arg_str, int pos_arg_index,
+		    const notmuch_opt_desc_t *arg_desc) {
+
+    int pos_arg_counter = 0;
+    while (arg_desc->opt_type != NOTMUCH_OPT_END){
+	if (arg_desc->opt_type == NOTMUCH_OPT_POSITION) {
+	    if (pos_arg_counter == pos_arg_index) {
+		if (arg_desc->output_var) {
+		    *((const char **)arg_desc->output_var) = arg_str;
+		}
+		return TRUE;
+	    }
+	    pos_arg_counter++;
+	}
+	arg_desc++;
+    }
+    return FALSE;
+}
+
+/*
+ * Search for a non-positional (i.e. starting with --) argument matching arg,
+ * parse a possible value, and assign to *output_var
+ */
+
+notmuch_bool_t
+parse_option (const char *arg,
+	      const notmuch_opt_desc_t *options) {
+
+    assert(arg);
+    assert(options);
+
+    arg += 2;
+
+    const notmuch_opt_desc_t *try = options;
+    while (try->opt_type != NOTMUCH_OPT_END) {
+	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
+	     */
+
+	    if (try->opt_type != NOTMUCH_OPT_BOOLEAN) {
+		if (next != '=' && next != ':') return FALSE;
+		if (value[0] == 0) return FALSE;
+	    } else {
+		if (next != 0) return FALSE;
+	    }
+
+	    if (try->output_var == NULL)
+		INTERNAL_ERROR ("output pointer NULL for option %s", try->name);
+
+	    switch (try->opt_type) {
+	    case NOTMUCH_OPT_KEYWORD:
+		return _process_keyword_arg (try, value);
+		break;
+	    case NOTMUCH_OPT_BOOLEAN:
+		*((notmuch_bool_t *)try->output_var) = TRUE;
+		return TRUE;
+		break;
+	    case NOTMUCH_OPT_INT:
+		*((int *)try->output_var) = strtol (value, &endptr, 10);
+		return (*endptr == 0);
+		break;
+	    case NOTMUCH_OPT_STRING:
+		*((const char **)try->output_var) = value;
+		return TRUE;
+		break;
+	    case NOTMUCH_OPT_POSITION:
+	    case NOTMUCH_OPT_END:
+	    default:
+		INTERNAL_ERROR ("unknown or unhandled option type %d", try->opt_type);
+		/*UNREACHED*/
+	    }
+	}
+	try++;
+    }
+    fprintf (stderr, "Unrecognized option: --%s\n", arg);
+    return FALSE;
+}
+
+/* See command-line-arguments.h for description */
+int
+parse_arguments (int argc, char **argv,
+		 const notmuch_opt_desc_t *options, int opt_index) {
+
+    int pos_arg_index = 0;
+    notmuch_bool_t more_args = TRUE;
+
+    while (more_args && opt_index < argc) {
+	if (strncmp (argv[opt_index],"--",2) != 0) {
+
+	    more_args = parse_position_arg (argv[opt_index], pos_arg_index, options);
+
+	    if (more_args) {
+		pos_arg_index++;
+		opt_index++;
+	    }
+
+	} else {
+
+	    if (strlen (argv[opt_index]) == 2)
+		return opt_index+1;
+
+	    more_args = parse_option (argv[opt_index], options);
+	    if (more_args) {
+		opt_index++;
+	    } else {
+		opt_index = -1;
+	    }
+
+	}
+    }
+
+    return opt_index;
+}
diff --git a/command-line-arguments.h b/command-line-arguments.h
new file mode 100644
index 0000000..af8b1ce
--- /dev/null
+++ b/command-line-arguments.h
@@ -0,0 +1,80 @@
+#ifndef NOTMUCH_OPTS_H
+#define NOTMUCH_OPTS_H
+
+#include "notmuch.h"
+
+enum notmuch_opt_type {
+    NOTMUCH_OPT_END = 0,
+    NOTMUCH_OPT_BOOLEAN,	/* --verbose              */
+    NOTMUCH_OPT_INT,		/* --frob=8               */
+    NOTMUCH_OPT_KEYWORD,	/* --format=raw|json|text */
+    NOTMUCH_OPT_STRING,		/* --file=/tmp/gnarf.txt  */
+    NOTMUCH_OPT_POSITION	/* notmuch dump pos_arg   */
+};
+
+/*
+ * Describe one of the possibilities for a keyword option
+ * 'value' will be copied to the output variable
+ */
+
+typedef struct notmuch_keyword {
+    const char *name;
+    int value;
+} notmuch_keyword_t;
+
+/*
+ * Describe one option.
+ *
+ * First two parameters are mandatory.
+ *
+ * name is mandatory _except_ for positional arguments.
+ *
+ * arg_id is currently unused, but could define short arguments.
+ *
+ * keywords is a (possibly NULL) pointer to an array of keywords
+ */
+typedef struct notmuch_opt_desc {
+    enum notmuch_opt_type opt_type;
+    void *output_var;
+    const char *name;
+    int  arg_id;
+    struct notmuch_keyword *keywords;
+} notmuch_opt_desc_t;
+
+
+/*
+  This is the main entry point for command line argument parsing.
+
+  Parse command line arguments according to structure options,
+  starting at position opt_index.
+
+  All output of parsed values is via pointers in options.
+
+  Parsing stops at -- (consumed) or at the (k+1)st argument
+  not starting with -- (a "positional argument") if options contains
+  k positional argument descriptors.
+
+  Returns the index of first non-parsed argument, or -1 in case of error.
+
+*/
+int
+parse_arguments (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_index);
+
+/*
+ * If the argument parsing loop provided by parse_arguments is not
+ * flexible enough, then the user might be interested in the following
+ * routines, but note that the API to parse_option might have to
+ * change. See command-line-arguments.c for descriptions of these
+ * functions.
+ */
+
+notmuch_bool_t
+parse_option (const char *arg, const notmuch_opt_desc_t* options);
+
+notmuch_bool_t
+parse_position_arg (const char *arg,
+		    int position_arg_index,
+		    const notmuch_opt_desc_t* options);
+
+
+#endif
diff --git a/notmuch-client.h b/notmuch-client.h
index b50cb38..703f856 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -238,4 +238,5 @@ notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,
 notmuch_bool_t
 debugger_is_active (void);
 
+#include "command-line-arguments.h"
 #endif
-- 
1.7.5.4

[PATCH v4 2/5] test: tests for command-line-arguments.c

[David Bremner]

From: David Bremner <bremner@debian.org>

This was needed because no current notmuch code exercises the
NOTMUCH_OPT_STRING style arguments.
---
 test/Makefile.local   |   11 ++++++++-
 test/arg-test.c       |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 test/argument-parsing |   16 +++++++++++++++
 test/basic            |    2 +-
 test/notmuch-test     |    1 +
 5 files changed, 79 insertions(+), 3 deletions(-)
 create mode 100644 test/arg-test.c
 create mode 100755 test/argument-parsing

diff --git a/test/Makefile.local b/test/Makefile.local
index bffbbdb..6cb6c82 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -2,12 +2,17 @@
 
 dir := test
 
+extra_cflags += -I.
+
 smtp_dummy_srcs =		\
 	$(notmuch_compat_srcs)	\
 	$(dir)/smtp-dummy.c
 
 smtp_dummy_modules = $(smtp_dummy_srcs:.c=.o)
 
+$(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libutil.a
+	$(call quiet,CC) -I. $^ -o $@
+
 $(dir)/smtp-dummy: $(smtp_dummy_modules)
 	$(call quiet,CC) $^ -o $@
 
@@ -16,11 +21,13 @@ $(dir)/symbol-test: $(dir)/symbol-test.o
 
 .PHONY: test check
 
-test-binaries: $(dir)/smtp-dummy $(dir)/symbol-test
+test-binaries: $(dir)/arg-test $(dir)/smtp-dummy $(dir)/symbol-test
 
 test:	all test-binaries
 	@${dir}/notmuch-test $(OPTIONS)
 
 check: test
 
-CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o $(dir)/symbol-test $(dir)/symbol-test.o
+CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
+	 $(dir)/symbol-test $(dir)/symbol-test.o \
+	 $(dir)/arg-test $(dir)/arg-test.o
diff --git a/test/arg-test.c b/test/arg-test.c
new file mode 100644
index 0000000..adc56e3
--- /dev/null
+++ b/test/arg-test.c
@@ -0,0 +1,52 @@
+#include <stdio.h>
+#include "command-line-arguments.h"
+
+
+int main(int argc, char **argv){
+
+    int opt_index=1;
+
+    int kw_val=0;
+    int int_val=0;
+    char *pos_arg1=NULL;
+    char *pos_arg2=NULL;
+    char *string_val=NULL;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_KEYWORD, &kw_val, "keyword", 'k',
+	  (notmuch_keyword_t []){ { "one", 1 },
+				  { "two", 2 },
+				  { 0, 0 } } },
+	{ NOTMUCH_OPT_INT, &int_val, "int", 'i', 0},
+	{ NOTMUCH_OPT_STRING, &string_val, "string", 's', 0},
+	{ NOTMUCH_OPT_POSITION, &pos_arg1, 0,0, 0},
+	{ NOTMUCH_OPT_POSITION, &pos_arg2, 0,0, 0},
+	{ 0, 0, 0, 0, 0 } };
+
+    opt_index = parse_arguments(argc, argv, options, 1);
+
+    if (opt_index < 0)
+	return 1;
+
+    if (kw_val)
+	printf("keyword %d\n", kw_val);
+
+    if (int_val)
+	printf("int %d\n", int_val);
+
+    if (string_val)
+	printf("string %s\n", string_val);
+
+    if (pos_arg1)
+	printf("positional arg 1 %s\n", pos_arg1);
+
+    if (pos_arg2)
+	printf("positional arg 2 %s\n", pos_arg1);
+
+
+    for ( ; opt_index < argc ; opt_index ++) {
+	printf("non parsed arg %d = %s\n", opt_index, argv[opt_index]);
+    }
+
+    return 0;
+}
diff --git a/test/argument-parsing b/test/argument-parsing
new file mode 100755
index 0000000..672de0b
--- /dev/null
+++ b/test/argument-parsing
@@ -0,0 +1,16 @@
+#!/usr/bin/env bash
+test_description="argument parsing"
+. ./test-lib.sh
+
+test_begin_subtest "sanity check"
+$TEST_DIRECTORY/arg-test  pos1  --keyword=one --string=foo pos2 --int=7 > OUTPUT
+cat <<EOF > EXPECTED
+keyword 1
+int 7
+string foo
+positional arg 1 pos1
+positional arg 2 pos1
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+test_done
diff --git a/test/basic b/test/basic
index 4edf831..d6aed24 100755
--- a/test/basic
+++ b/test/basic
@@ -54,7 +54,7 @@ test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
 eval $(sed -n -e '/^TESTS="$/,/^"$/p' $TEST_DIRECTORY/notmuch-test)
 tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
 available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -executable -printf '%f\n' | \
-    sed -r -e "/^(aggregate-results.sh|notmuch-test|smtp-dummy|test-verbose|symbol-test)$/d" | \
+    sed -r -e "/^(aggregate-results.sh|notmuch-test|smtp-dummy|test-verbose|symbol-test|arg-test)$/d" | \
     sort)
 test_expect_equal "$tests_in_suite" "$available"
 
diff --git a/test/notmuch-test b/test/notmuch-test
index 53ce355..d05bb38 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -48,6 +48,7 @@ TESTS="
   search-folder-coherence
   atomicity
   python
+  argument-parsing
 "
 TESTS=${NOTMUCH_TESTS:=$TESTS}
 
-- 
1.7.5.4

[PATCH v4 3/5] notmuch-dump: convert to command-line-arguments

[David Bremner]

From: David Bremner <bremner@debian.org>

The output file is handled via positional arguments. There are
currently no "normal" options.
---
 notmuch-dump.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index a490917..a735875 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -41,27 +41,34 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
     if (notmuch == NULL)
 	return 1;
 
-    argc--; argv++; /* skip subcommand argument */
+    char *output_file_name = NULL;
+    int opt_index;
 
-    if (argc && strcmp (argv[0], "--") != 0) {
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_POSITION, &output_file_name, 0, 0, 0  },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	return 1;
+    }
+
+    if (output_file_name) {
 	fprintf (stderr, "Warning: the output file argument of dump is deprecated.\n");
-	output = fopen (argv[0], "w");
+	output = fopen (output_file_name, "w");
 	if (output == NULL) {
 	    fprintf (stderr, "Error opening %s for writing: %s\n",
-		     argv[0], strerror (errno));
+		     output_file_name, strerror (errno));
 	    return 1;
 	}
-	argc--;
-	argv++;
     }
 
-    if (argc && strcmp (argv[0], "--") == 0){
-	argc--;
-	argv++;
-    }
 
-    if (argc) {
-	query_str = query_string_from_args (notmuch, argc, argv);
+    if (opt_index < argc) {
+	query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
 	if (query_str == NULL) {
 	    fprintf (stderr, "Out of memory.\n");
 	    return 1;
-- 
1.7.5.4

[PATCH v4 4/5] notmuch-restore: convert to command-line-arguments

[David Bremner]

From: David Bremner <bremner@debian.org>

The new argument handling is a bit more concise, and bit more
flexible.  It allows the input file name to go before the --accumulate
option.
---
 notmuch-restore.c |   37 +++++++++++++++----------------------
 1 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 13b4325..87d9772 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -18,8 +18,6 @@
  * Author: Carl Worth <cworth@cworth.org>
  */
 
-#include <getopt.h>
-
 #include "notmuch-client.h"
 
 int
@@ -29,12 +27,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     notmuch_database_t *notmuch;
     notmuch_bool_t synchronize_flags;
     notmuch_bool_t accumulate = FALSE;
+    char *input_file_name = NULL;
     FILE *input = stdin;
     char *line = NULL;
     size_t line_size;
     ssize_t line_len;
     regex_t regex;
     int rerr;
+    int opt_index;
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -47,37 +47,30 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
 
-    struct option options[] = {
-	{ "accumulate",   no_argument,       0, 'a' },
-	{ 0, 0, 0, 0}
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_POSITION, &input_file_name, 0, 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &accumulate, "accumulate", 'a', 0 },
+	{ 0, 0, 0, 0, 0 }
     };
 
-    int opt;
-    do {
-	opt = getopt_long (argc, argv, "", options, NULL);
-
-	switch (opt) {
-	case 'a':
-	    accumulate = 1;
-	    break;
-	case '?':
-	    return 1;
-	    break;
-	}
+    opt_index = parse_arguments (argc, argv, options, 1);
 
-    } while (opt != -1);
+    if (opt_index < 0) {
+	/* diagnostics already printed */
+	return 1;
+    }
 
-    if (optind < argc) {
-	input = fopen (argv[optind], "r");
+    if (input_file_name) {
+	input = fopen (input_file_name, "r");
 	if (input == NULL) {
 	    fprintf (stderr, "Error opening %s for reading: %s\n",
-		     argv[optind], strerror (errno));
+		     input_file_name, strerror (errno));
 	    return 1;
 	}
 	optind++;
     }
 
-    if (optind < argc) {
+    if (opt_index < argc) {
 	fprintf (stderr,
 	 "Cannot read dump from more than one file: %s\n",
 		 argv[optind]);
-- 
1.7.5.4

[PATCH v4 5/5] notmuch-search: convert to command-line-arguments

[David Bremner]

From: David Bremner <bremner@debian.org>

The switch on format_sel is slightly clunky, but it doesn't seem worth
special casing argument processing for function pointers, when I think
the function pointer approach will be modified/abandoned.
---
 notmuch-search.c |  106 +++++++++++++++++++----------------------------------
 1 files changed, 38 insertions(+), 68 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 36686d1..861c888 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -415,81 +415,51 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     notmuch_database_t *notmuch;
     notmuch_query_t *query;
     char *query_str;
-    char *opt;
     notmuch_sort_t sort = NOTMUCH_SORT_NEWEST_FIRST;
     const search_format_t *format = &format_text;
-    int i, ret;
+    int opt_index, ret;
     output_t output = OUTPUT_SUMMARY;
     int offset = 0;
     int limit = -1; /* unlimited */
 
-    argc--; argv++; /* skip subcommand argument */
-
-    for (i = 0; i < argc && argv[i][0] == '-'; i++) {
-	if (strcmp (argv[i], "--") == 0) {
-	    i++;
-	    break;
-	}
-        if (STRNCMP_LITERAL (argv[i], "--sort=") == 0) {
-	    opt = argv[i] + sizeof ("--sort=") - 1;
-	    if (strcmp (opt, "oldest-first") == 0) {
-		sort = NOTMUCH_SORT_OLDEST_FIRST;
-	    } else if (strcmp (opt, "newest-first") == 0) {
-		sort = NOTMUCH_SORT_NEWEST_FIRST;
-	    } else {
-		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--offset=") == 0) {
-	    char *p;
-	    opt = argv[i] + sizeof ("--offset=") - 1;
-	    offset = strtol (opt, &p, 10);
-	    if (*opt == '\0' || p == opt || *p != '\0') {
-		fprintf (stderr, "Invalid value for --offset: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--limit=") == 0) {
-	    char *p;
-	    opt = argv[i] + sizeof ("--limit=") - 1;
-	    limit = strtoul (opt, &p, 10);
-	    if (*opt == '\0' || p == opt || *p != '\0') {
-		fprintf (stderr, "Invalid value for --limit: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {
-	    opt = argv[i] + sizeof ("--format=") - 1;
-	    if (strcmp (opt, "text") == 0) {
-		format = &format_text;
-	    } else if (strcmp (opt, "json") == 0) {
-		format = &format_json;
-	    } else {
-		fprintf (stderr, "Invalid value for --format: %s\n", opt);
-		return 1;
-	    }
-	} else if (STRNCMP_LITERAL (argv[i], "--output=") == 0) {
-	    opt = argv[i] + sizeof ("--output=") - 1;
-	    if (strcmp (opt, "summary") == 0) {
-		output = OUTPUT_SUMMARY;
-	    } else if (strcmp (opt, "threads") == 0) {
-		output = OUTPUT_THREADS;
-	    } else if (strcmp (opt, "messages") == 0) {
-		output = OUTPUT_MESSAGES;
-	    } else if (strcmp (opt, "files") == 0) {
-		output = OUTPUT_FILES;
-	    } else if (strcmp (opt, "tags") == 0) {
-		output = OUTPUT_TAGS;
-	    } else {
-		fprintf (stderr, "Invalid value for --output: %s\n", opt);
-		return 1;
-	    }
-	} else {
-	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
-	    return 1;
-	}
+    enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
+	format_sel = NOTMUCH_FORMAT_TEXT;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_KEYWORD, &sort, "sort", 's',
+	  (notmuch_keyword_t []){ { "oldest-first", NOTMUCH_SORT_OLDEST_FIRST },
+				  { "newest-first", NOTMUCH_SORT_NEWEST_FIRST },
+				  { 0, 0 } } },
+	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
+	  (notmuch_keyword_t []){ { "json", NOTMUCH_FORMAT_JSON },
+				  { "text", NOTMUCH_FORMAT_TEXT },
+				  { 0, 0 } } },
+	{ NOTMUCH_OPT_KEYWORD, &output, "output", 'o',
+	  (notmuch_keyword_t []){ { "summary", OUTPUT_SUMMARY },
+				  { "threads", OUTPUT_THREADS },
+				  { "messages", OUTPUT_MESSAGES },
+				  { "files", OUTPUT_FILES },
+				  { "tags", OUTPUT_TAGS },
+				  { 0, 0 } } },
+	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
+	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	return 1;
     }
 
-    argc -= i;
-    argv += i;
+    switch (format_sel) {
+    case NOTMUCH_FORMAT_TEXT:
+	format = &format_text;
+	break;
+    case NOTMUCH_FORMAT_JSON:
+	format = &format_json;
+	break;
+    }
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -500,7 +470,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     if (notmuch == NULL)
 	return 1;
 
-    query_str = query_string_from_args (notmuch, argc, argv);
+    query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
     if (query_str == NULL) {
 	fprintf (stderr, "Out of memory.\n");
 	return 1;
-- 
1.7.5.4

Re: [PATCH v4 1/5] command-line-arguments.[ch]: new argument parsing framework for notmuch.

[David Bremner]

On Wed,  7 Dec 2011 15:26:35 -0400, David Bremner <david@tethera.net> wrote:
> The framework here provides positional arguments, simple switches,
> and --key=value style arguments that can take a value being an integer,
> a string, or one of a set of keywords.

and I pushed v4 of this series.

d