argument parsing refactor, add shared options

argument parsing refactor, add shared options

[David Bremner]

As part of ongoing hacking on Austin's revision tracking patches, I
wanted to add an option to (almost) all subcommands. One thing led to
another and this series emerged. By itself, the gain in functionality
is probably not worthwhile (yay, we can type notmuch subcommand
--version), but internally it makes it easy to add further "global
options" that are accepted as

% notmuch --option [subcommand]

and

% notmuch subcommand --option

it would make sense, e.g. for --quiet and --verbose to be supported
this way.

Whatever people think about the shared options, I think the
refactoring of notmuch_help_command is probably worthwhile as it is
pretty simple and improves the readability of that code.

Although there are other ways of doing so, patch 4 of this series also
fixes a UI bug encountered by Rob recently, where "--config" is silently ignored
in

	notmuch setup --config=/tmp/foo.conf

In principle this series will need a documentation update, but note that

   notmuch search --help

is already undocumented.

[PATCH 1/4] cli: ignore config argument of notmuch_help_command

[David Bremner]

We call it with NULL at one point anyway, so it needs to work with
NULL. Since the only place we use talloc is right before exec, there
is no harm in always using NULL.
---
 notmuch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index a5b2877..31672c3 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -177,7 +177,7 @@ exec_man (const char *page)
 }
 
 static int
-notmuch_help_command (notmuch_config_t *config, int argc, char *argv[])
+notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
 {
     command_t *command;
     help_topic_t *topic;
@@ -202,14 +202,14 @@ notmuch_help_command (notmuch_config_t *config, int argc, char *argv[])
 
     command = find_command (argv[0]);
     if (command) {
-	char *page = talloc_asprintf (config, "notmuch-%s", command->name);
+	char *page = talloc_asprintf (NULL, "notmuch-%s", command->name);
 	exec_man (page);
     }
 
     for (i = 0; i < ARRAY_SIZE (help_topics); i++) {
 	topic = &help_topics[i];
 	if (strcmp (argv[0], topic->name) == 0) {
-	    char *page = talloc_asprintf (config, "notmuch-%s", topic->name);
+	    char *page = talloc_asprintf (NULL, "notmuch-%s", topic->name);
 	    exec_man (page);
 	}
     }
-- 
2.1.4

[PATCH 2/4] cli: refactor notmuch_help_command

[David Bremner]

Create a new private entry point _help_for so that we can call help
without simulating a command line invokation to set up the arguments.
---
 notmuch.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index 31672c3..bff941f 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -177,21 +177,19 @@ exec_man (const char *page)
 }
 
 static int
-notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
+_help_for (const char *topic_name)
 {
     command_t *command;
     help_topic_t *topic;
     unsigned int i;
 
-    argc--; argv++; /* Ignore "help" */
-
-    if (argc == 0) {
+    if (!topic_name) {
 	printf ("The notmuch mail system.\n\n");
 	usage (stdout);
 	return EXIT_SUCCESS;
     }
 
-    if (strcmp (argv[0], "help") == 0) {
+    if (strcmp (topic_name, "help") == 0) {
 	printf ("The notmuch help system.\n\n"
 		"\tNotmuch uses the man command to display help. In case\n"
 		"\tof difficulties check that MANPATH includes the pages\n"
@@ -200,7 +198,7 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 	return EXIT_SUCCESS;
     }
 
-    command = find_command (argv[0]);
+    command = find_command (topic_name);
     if (command) {
 	char *page = talloc_asprintf (NULL, "notmuch-%s", command->name);
 	exec_man (page);
@@ -208,7 +206,7 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 
     for (i = 0; i < ARRAY_SIZE (help_topics); i++) {
 	topic = &help_topics[i];
-	if (strcmp (argv[0], topic->name) == 0) {
+	if (strcmp (topic_name, topic->name) == 0) {
 	    char *page = talloc_asprintf (NULL, "notmuch-%s", topic->name);
 	    exec_man (page);
 	}
@@ -216,10 +214,22 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 
     fprintf (stderr,
 	     "\nSorry, %s is not a known command. There's not much I can do to help.\n\n",
-	     argv[0]);
+	     topic_name);
     return EXIT_FAILURE;
 }
 
+static int
+notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
+{
+    argc--; argv++; /* Ignore "help" */
+
+    if (argc == 0) {
+	return _help_for (NULL);
+    }
+
+    return _help_for (argv[0]);
+}
+
 /* Handle the case of "notmuch" being invoked with no command
  * argument. For now we just call notmuch_setup_command, but we plan
  * to be more clever about this in the future.
-- 
2.1.4

[PATCH 3/4] cli: define shared options, use for --help and --version

[David Bremner]

Unfortunately it seems trickier to support --config globally
---
 notmuch-client.h  |  2 ++
 notmuch-compact.c |  4 ++++
 notmuch-count.c   |  3 +++
 notmuch-dump.c    |  3 +++
 notmuch-insert.c  |  3 +++
 notmuch-new.c     |  3 +++
 notmuch-reply.c   |  3 +++
 notmuch-search.c  |  3 +++
 notmuch-show.c    |  3 +++
 notmuch-tag.c     |  3 +++
 notmuch.c         | 52 +++++++++++++++++++++++++++++++---------------------
 11 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index fb3021c..ab0d188 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -466,4 +466,6 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 		       notmuch_bool_t gzip_output);
 
 #include "command-line-arguments.h"
+extern const notmuch_opt_desc_t  notmuch_shared_options [];
+void notmuch_process_shared_options (const char* help_name);
 #endif
diff --git a/notmuch-compact.c b/notmuch-compact.c
index 2fc012a..5be551d 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -38,12 +38,16 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &backup_path, "backup", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0}
     };
 
     opt_index = parse_arguments (argc, argv, options, 1);
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (! quiet)
 	printf ("Compacting database...\n");
     ret = notmuch_database_compact (path, backup_path,
diff --git a/notmuch-count.c b/notmuch-count.c
index 6058f7c..57a88a8 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -146,6 +146,7 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -153,6 +154,8 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (input_file_name) {
 	batch = TRUE;
 	input = fopen (input_file_name, "r");
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 9c6ad7f..fab22bd 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -228,6 +228,7 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &output_file_name, "output", 'o', 0  },
 	{ NOTMUCH_OPT_BOOLEAN, &gzip_output, "gzip", 'z', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -235,6 +236,8 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (opt_index < argc) {
 	query_str = query_string_from_args (notmuch, argc - opt_index, argv + opt_index);
 	if (query_str == NULL) {
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 90fe3ba..697880f 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -466,6 +466,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -473,6 +474,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     db_path = notmuch_config_get_database_path (config);
     new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
diff --git a/notmuch-new.c b/notmuch-new.c
index e6c283e..895f5d9 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -934,6 +934,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -941,6 +942,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     /* quiet trumps verbose */
     if (quiet)
 	add_files_state.verbosity = VERBOSITY_QUIET;
diff --git a/notmuch-reply.c b/notmuch-reply.c
index d51fdfc..4464741 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -790,6 +790,7 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 				  { "sender", FALSE },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -797,6 +798,8 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (format == FORMAT_HEADERS_ONLY) {
 	reply_format_func = notmuch_reply_format_headers_only;
     } else if (format == FORMAT_JSON) {
diff --git a/notmuch-search.c b/notmuch-search.c
index b81ac01..994ae58 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -681,6 +681,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_INT, &ctx->limit, "limit", 'L', 0  },
 	{ NOTMUCH_OPT_INT, &ctx->dupe, "duplicate", 'D', 0  },
 	{ NOTMUCH_OPT_INHERIT, (void *) &common_options, NULL, 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -689,6 +690,8 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (ctx->output != OUTPUT_FILES && ctx->output != OUTPUT_MESSAGES &&
 	ctx->dupe != -1) {
         fprintf (stderr, "Error: --duplicate=N is only supported with --output=files and --output=messages.\n");
diff --git a/notmuch-show.c b/notmuch-show.c
index 43bf71c..b80933a 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1114,6 +1114,7 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.verify, "verify", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.output_body, "body", 'b', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.include_html, "include-html", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -1121,6 +1122,8 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     /* decryption implies verification */
     if (params.crypto.decrypt)
 	params.crypto.verify = TRUE;
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 5b2f1e4..35f971d 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -206,6 +206,7 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &remove_all, "remove-all", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -213,6 +214,8 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (input_file_name) {
 	batch = TRUE;
 	input = fopen (input_file_name, "r");
diff --git a/notmuch.c b/notmuch.c
index bff941f..c7f8c8f 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -43,6 +43,35 @@ notmuch_help_command (notmuch_config_t *config, int argc, char *argv[]);
 static int
 notmuch_command (notmuch_config_t *config, int argc, char *argv[]);
 
+static int
+_help_for (const char *topic);
+
+static notmuch_bool_t print_version = FALSE, print_help = FALSE;
+
+const notmuch_opt_desc_t notmuch_shared_options [] = {
+    { NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
+    { NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
+    {0, 0, 0, 0, 0}
+};
+
+/* any subcommand wanting to support these options should call
+ * inherit notmuch_shared_options and call
+ * notmuch_process_shared_options (subcommand_name);
+ */
+void
+notmuch_process_shared_options (const char *help_name) {
+    if (print_version) {
+	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
+	exit (EXIT_SUCCESS);
+    }
+
+    if (print_help) {
+	int ret = _help_for (help_name);
+	exit (ret);
+    }
+}
+
+
 static command_t commands[] = {
     { NULL, notmuch_command, TRUE,
       "Notmuch main command." },
@@ -295,14 +324,12 @@ main (int argc, char *argv[])
     command_t *command;
     char *config_file_name = NULL;
     notmuch_config_t *config = NULL;
-    notmuch_bool_t print_help=FALSE, print_version=FALSE;
     int opt_index;
     int ret;
 
     notmuch_opt_desc_t options[] = {
-	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
 	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -324,24 +351,7 @@ main (int argc, char *argv[])
 	goto DONE;
     }
 
-    /* Handle notmuch --help [command] and notmuch command --help. */
-    if (print_help ||
-	(opt_index + 1 < argc && strcmp (argv[opt_index + 1], "--help") == 0)) {
-	/*
-	 * Pass the first positional argument as argv[1] so the help
-	 * command can give help for it. The help command ignores the
-	 * argv[0] passed to it.
-	 */
-	ret = notmuch_help_command (NULL, argc - opt_index + 1,
-				    argv + opt_index - 1);
-	goto DONE;
-    }
-
-    if (print_version) {
-	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
-	ret = EXIT_SUCCESS;
-	goto DONE;
-    }
+    notmuch_process_shared_options (NULL);
 
     if (opt_index < argc)
 	command_name = argv[opt_index];
-- 
2.1.4

[PATCH 4/4] cli: add standard option processing to config and setup

[David Bremner]

In particular this fixes a recently encountered bug where the
"--config" argument to "notmuch setup" is silently ignored, which the
unpleasant consequence of overwriting the users config file.
---
 notmuch-config.c | 12 ++++++++++++
 notmuch-setup.c  | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/notmuch-config.c b/notmuch-config.c
index 2d5c297..054dd3b 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -872,6 +872,18 @@ int
 notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
 {
     int ret;
+    int opt_index;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    notmuch_process_shared_options (argv[0]);
 
     argc--; argv++; /* skip subcommand argument */
 
diff --git a/notmuch-setup.c b/notmuch-setup.c
index 36a6171..5fc6e25 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -133,6 +133,7 @@ notmuch_setup_command (notmuch_config_t *config,
     size_t new_tags_len;
     const char **search_exclude_tags;
     size_t search_exclude_tags_len;
+    int opt_index;
 
 #define prompt(format, ...)					\
     do {							\
@@ -145,6 +146,17 @@ notmuch_setup_command (notmuch_config_t *config,
 	chomp_newline (response);				\
     } while (0)
 
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    notmuch_process_shared_options (argv[0]);
+
     if (notmuch_config_is_new (config))
 	welcome_message_pre_setup ();
 
-- 
2.1.4

Re: [PATCH 4/4] cli: add standard option processing to config and setup

[David Bremner]

David Bremner <david@tethera.net> writes:

> In particular this fixes a recently encountered bug where the
> "--config" argument to "notmuch setup" is silently ignored, which the
> unpleasant consequence of overwriting the users config file.

Urgh. I had a longish cover letter for this series but somehow mashing
buttons with git-send-email caused it to be lost forever. Or maybe it's
en route still. Anyway, here is a quick summary:

- The first two patches are worthwhile just to improve understandability
  IMHO

- The third patch is a bit big for tiny improvement in functionality,
  but it does make it easier to define global arguments, which is
  something I want for the revision-tracking series
  (id:1413181203-1676-1-git-send-email-aclements@csail.mit.edu)

- the last patch fixes an annoying bug where --config is silently
  ignored in

  % notmuch setup --config=foobar

  This could of course be fixed in a way that doesn't depend on patch
  3/4.

argument parsing refactoring, round 2

[David Bremner]

Thanks to Mark for pointing out on IRC that the previous version had problems linking the test suite helpers. That's fixed here by adding a couple stubs.

Here's the diff, roughly with the previous version (actually the first
hunk is with a slightly later version, where I fixed one bug and
introduced another).

diff --git a/notmuch-config.c b/notmuch-config.c
index 568b3dc..f2cd6a8 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -885,9 +885,9 @@ notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_process_shared_options (argv[0]);
 
-    /* skip subcommand argument */
-    argc-= opt_index+1;
-    argv+= opt_index+1;
+    /* skip at least subcommand argument */
+    argc-= opt_index;
+    argv+= opt_index;
 
     if (argc < 1) {
 	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
diff --git a/notmuch-setup.c b/notmuch-setup.c
index 5fc6e25..6a020dc 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -155,7 +155,7 @@ notmuch_setup_command (notmuch_config_t *config,
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
-    notmuch_process_shared_options (argv[0]);
+    notmuch_process_shared_options ("setup");
 
     if (notmuch_config_is_new (config))
 	welcome_message_pre_setup ();
diff --git a/test/random-corpus.c b/test/random-corpus.c
index 790193d..6c467bb 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -114,6 +114,15 @@ random_utf8_string (void *ctx, size_t char_count)
     return buf;
 }
 
+/* stubs since we cannot link with notmuch.o */
+const notmuch_opt_desc_t notmuch_shared_options[] = {
+	{ 0, 0, 0, 0, 0 }
+};
+
+void
+notmuch_process_shared_options (unused (const char *dummy))
+{
+}
 
 int
 main (int argc, char **argv)

[Patch v2 1/4] cli: ignore config argument of notmuch_help_command

[David Bremner]

We call it with NULL at one point anyway, so it needs to work with
NULL. Since the only place we use talloc is right before exec, there
is no harm in always using NULL.
---
 notmuch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index a5b2877..31672c3 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -177,7 +177,7 @@ exec_man (const char *page)
 }
 
 static int
-notmuch_help_command (notmuch_config_t *config, int argc, char *argv[])
+notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
 {
     command_t *command;
     help_topic_t *topic;
@@ -202,14 +202,14 @@ notmuch_help_command (notmuch_config_t *config, int argc, char *argv[])
 
     command = find_command (argv[0]);
     if (command) {
-	char *page = talloc_asprintf (config, "notmuch-%s", command->name);
+	char *page = talloc_asprintf (NULL, "notmuch-%s", command->name);
 	exec_man (page);
     }
 
     for (i = 0; i < ARRAY_SIZE (help_topics); i++) {
 	topic = &help_topics[i];
 	if (strcmp (argv[0], topic->name) == 0) {
-	    char *page = talloc_asprintf (config, "notmuch-%s", topic->name);
+	    char *page = talloc_asprintf (NULL, "notmuch-%s", topic->name);
 	    exec_man (page);
 	}
     }
-- 
2.1.4

[Patch v2 2/4] cli: refactor notmuch_help_command

[David Bremner]

Create a new private entry point _help_for so that we can call help
without simulating a command line invokation to set up the arguments.
---
 notmuch.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index 31672c3..bff941f 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -177,21 +177,19 @@ exec_man (const char *page)
 }
 
 static int
-notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
+_help_for (const char *topic_name)
 {
     command_t *command;
     help_topic_t *topic;
     unsigned int i;
 
-    argc--; argv++; /* Ignore "help" */
-
-    if (argc == 0) {
+    if (!topic_name) {
 	printf ("The notmuch mail system.\n\n");
 	usage (stdout);
 	return EXIT_SUCCESS;
     }
 
-    if (strcmp (argv[0], "help") == 0) {
+    if (strcmp (topic_name, "help") == 0) {
 	printf ("The notmuch help system.\n\n"
 		"\tNotmuch uses the man command to display help. In case\n"
 		"\tof difficulties check that MANPATH includes the pages\n"
@@ -200,7 +198,7 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 	return EXIT_SUCCESS;
     }
 
-    command = find_command (argv[0]);
+    command = find_command (topic_name);
     if (command) {
 	char *page = talloc_asprintf (NULL, "notmuch-%s", command->name);
 	exec_man (page);
@@ -208,7 +206,7 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 
     for (i = 0; i < ARRAY_SIZE (help_topics); i++) {
 	topic = &help_topics[i];
-	if (strcmp (argv[0], topic->name) == 0) {
+	if (strcmp (topic_name, topic->name) == 0) {
 	    char *page = talloc_asprintf (NULL, "notmuch-%s", topic->name);
 	    exec_man (page);
 	}
@@ -216,10 +214,22 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 
     fprintf (stderr,
 	     "\nSorry, %s is not a known command. There's not much I can do to help.\n\n",
-	     argv[0]);
+	     topic_name);
     return EXIT_FAILURE;
 }
 
+static int
+notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
+{
+    argc--; argv++; /* Ignore "help" */
+
+    if (argc == 0) {
+	return _help_for (NULL);
+    }
+
+    return _help_for (argv[0]);
+}
+
 /* Handle the case of "notmuch" being invoked with no command
  * argument. For now we just call notmuch_setup_command, but we plan
  * to be more clever about this in the future.
-- 
2.1.4

[Patch v2 3/4] cli: define shared options, use for --help and --version

[David Bremner]

Unfortunately it seems trickier to support --config globally

The non-trivial changes are in notmuch.c; most of the other changes
consists of blindly inserting two lines into every subcommand.
---
 notmuch-client.h  |  2 ++
 notmuch-compact.c |  4 ++++
 notmuch-count.c   |  3 +++
 notmuch-dump.c    |  3 +++
 notmuch-insert.c  |  3 +++
 notmuch-new.c     |  3 +++
 notmuch-reply.c   |  3 +++
 notmuch-restore.c |  2 ++
 notmuch-search.c  |  3 +++
 notmuch-show.c    |  3 +++
 notmuch-tag.c     |  3 +++
 notmuch.c         | 52 +++++++++++++++++++++++++++++++---------------------
 12 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index fb3021c..ab0d188 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -466,4 +466,6 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 		       notmuch_bool_t gzip_output);
 
 #include "command-line-arguments.h"
+extern const notmuch_opt_desc_t  notmuch_shared_options [];
+void notmuch_process_shared_options (const char* help_name);
 #endif
diff --git a/notmuch-compact.c b/notmuch-compact.c
index 2fc012a..5be551d 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -38,12 +38,16 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &backup_path, "backup", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0}
     };
 
     opt_index = parse_arguments (argc, argv, options, 1);
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (! quiet)
 	printf ("Compacting database...\n");
     ret = notmuch_database_compact (path, backup_path,
diff --git a/notmuch-count.c b/notmuch-count.c
index 6058f7c..57a88a8 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -146,6 +146,7 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -153,6 +154,8 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (input_file_name) {
 	batch = TRUE;
 	input = fopen (input_file_name, "r");
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 9c6ad7f..fab22bd 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -228,6 +228,7 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &output_file_name, "output", 'o', 0  },
 	{ NOTMUCH_OPT_BOOLEAN, &gzip_output, "gzip", 'z', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -235,6 +236,8 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (opt_index < argc) {
 	query_str = query_string_from_args (notmuch, argc - opt_index, argv + opt_index);
 	if (query_str == NULL) {
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 90fe3ba..697880f 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -466,6 +466,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -473,6 +474,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     db_path = notmuch_config_get_database_path (config);
     new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
diff --git a/notmuch-new.c b/notmuch-new.c
index e6c283e..895f5d9 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -934,6 +934,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -941,6 +942,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     /* quiet trumps verbose */
     if (quiet)
 	add_files_state.verbosity = VERBOSITY_QUIET;
diff --git a/notmuch-reply.c b/notmuch-reply.c
index d51fdfc..4464741 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -790,6 +790,7 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 				  { "sender", FALSE },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -797,6 +798,8 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (format == FORMAT_HEADERS_ONLY) {
 	reply_format_func = notmuch_reply_format_headers_only;
     } else if (format == FORMAT_JSON) {
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 584d9f9..2a534dc 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -154,6 +154,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &accumulate, "accumulate", 'a', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -163,6 +164,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
 	goto DONE;
     }
 
+    notmuch_process_shared_options (argv[0]);
     name_for_error = input_file_name ? input_file_name : "stdin";
 
     if (! accumulate)
diff --git a/notmuch-search.c b/notmuch-search.c
index b81ac01..994ae58 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -681,6 +681,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_INT, &ctx->limit, "limit", 'L', 0  },
 	{ NOTMUCH_OPT_INT, &ctx->dupe, "duplicate", 'D', 0  },
 	{ NOTMUCH_OPT_INHERIT, (void *) &common_options, NULL, 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -689,6 +690,8 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (ctx->output != OUTPUT_FILES && ctx->output != OUTPUT_MESSAGES &&
 	ctx->dupe != -1) {
         fprintf (stderr, "Error: --duplicate=N is only supported with --output=files and --output=messages.\n");
diff --git a/notmuch-show.c b/notmuch-show.c
index 43bf71c..b80933a 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1114,6 +1114,7 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.verify, "verify", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.output_body, "body", 'b', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.include_html, "include-html", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -1121,6 +1122,8 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     /* decryption implies verification */
     if (params.crypto.decrypt)
 	params.crypto.verify = TRUE;
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 5b2f1e4..35f971d 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -206,6 +206,7 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &remove_all, "remove-all", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -213,6 +214,8 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (input_file_name) {
 	batch = TRUE;
 	input = fopen (input_file_name, "r");
diff --git a/notmuch.c b/notmuch.c
index bff941f..c7f8c8f 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -43,6 +43,35 @@ notmuch_help_command (notmuch_config_t *config, int argc, char *argv[]);
 static int
 notmuch_command (notmuch_config_t *config, int argc, char *argv[]);
 
+static int
+_help_for (const char *topic);
+
+static notmuch_bool_t print_version = FALSE, print_help = FALSE;
+
+const notmuch_opt_desc_t notmuch_shared_options [] = {
+    { NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
+    { NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
+    {0, 0, 0, 0, 0}
+};
+
+/* any subcommand wanting to support these options should call
+ * inherit notmuch_shared_options and call
+ * notmuch_process_shared_options (subcommand_name);
+ */
+void
+notmuch_process_shared_options (const char *help_name) {
+    if (print_version) {
+	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
+	exit (EXIT_SUCCESS);
+    }
+
+    if (print_help) {
+	int ret = _help_for (help_name);
+	exit (ret);
+    }
+}
+
+
 static command_t commands[] = {
     { NULL, notmuch_command, TRUE,
       "Notmuch main command." },
@@ -295,14 +324,12 @@ main (int argc, char *argv[])
     command_t *command;
     char *config_file_name = NULL;
     notmuch_config_t *config = NULL;
-    notmuch_bool_t print_help=FALSE, print_version=FALSE;
     int opt_index;
     int ret;
 
     notmuch_opt_desc_t options[] = {
-	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
 	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -324,24 +351,7 @@ main (int argc, char *argv[])
 	goto DONE;
     }
 
-    /* Handle notmuch --help [command] and notmuch command --help. */
-    if (print_help ||
-	(opt_index + 1 < argc && strcmp (argv[opt_index + 1], "--help") == 0)) {
-	/*
-	 * Pass the first positional argument as argv[1] so the help
-	 * command can give help for it. The help command ignores the
-	 * argv[0] passed to it.
-	 */
-	ret = notmuch_help_command (NULL, argc - opt_index + 1,
-				    argv + opt_index - 1);
-	goto DONE;
-    }
-
-    if (print_version) {
-	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
-	ret = EXIT_SUCCESS;
-	goto DONE;
-    }
+    notmuch_process_shared_options (NULL);
 
     if (opt_index < argc)
 	command_name = argv[opt_index];
-- 
2.1.4

[Patch v2 4/4] cli: add standard option processing to config and setup

[David Bremner]

In particular this fixes a recently encountered bug where the
"--config" argument to "notmuch setup" is silently ignored, which the
unpleasant consequence of overwriting the users config file.
---
 notmuch-config.c     | 16 +++++++++++++++-
 notmuch-setup.c      | 12 ++++++++++++
 test/random-corpus.c |  9 +++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 2d5c297..f2cd6a8 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -872,8 +872,22 @@ int
 notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
 {
     int ret;
+    int opt_index;
 
-    argc--; argv++; /* skip subcommand argument */
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    notmuch_process_shared_options (argv[0]);
+
+    /* skip at least subcommand argument */
+    argc-= opt_index;
+    argv+= opt_index;
 
     if (argc < 1) {
 	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
diff --git a/notmuch-setup.c b/notmuch-setup.c
index 36a6171..6a020dc 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -133,6 +133,7 @@ notmuch_setup_command (notmuch_config_t *config,
     size_t new_tags_len;
     const char **search_exclude_tags;
     size_t search_exclude_tags_len;
+    int opt_index;
 
 #define prompt(format, ...)					\
     do {							\
@@ -145,6 +146,17 @@ notmuch_setup_command (notmuch_config_t *config,
 	chomp_newline (response);				\
     } while (0)
 
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    notmuch_process_shared_options ("setup");
+
     if (notmuch_config_is_new (config))
 	welcome_message_pre_setup ();
 
diff --git a/test/random-corpus.c b/test/random-corpus.c
index 790193d..6c467bb 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -114,6 +114,15 @@ random_utf8_string (void *ctx, size_t char_count)
     return buf;
 }
 
+/* stubs since we cannot link with notmuch.o */
+const notmuch_opt_desc_t notmuch_shared_options[] = {
+	{ 0, 0, 0, 0, 0 }
+};
+
+void
+notmuch_process_shared_options (unused (const char *dummy))
+{
+}
 
 int
 main (int argc, char **argv)
-- 
2.1.4

Re: argument parsing refactoring, round 2

[Mark Walters]

On Mon, 06 Apr 2015, David Bremner <david@tethera.net> wrote:
> Thanks to Mark for pointing out on IRC that the previous version had problems linking the test suite helpers. That's fixed here by adding a couple stubs.
>
> Here's the diff, roughly with the previous version (actually the first
> hunk is with a slightly later version, where I fixed one bug and
> introduced another).

Hi 

I like the shared options and the series in general. I have some very
minor nits:

The first is that I think you missed the second command in
notmuch-search.c: i.e., notmuch_address_command so that doesn't respect
these options. (You could either just add it there or to common_options
if you prefer)

The second is utterly trivial: is it worth having the shared options for
help too? At the moment notmuch help --help just says "sorry --help is
not a command".

The other (very minor things) I will send in replies to the individual
patches.

Best wishes

Mark


>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 568b3dc..f2cd6a8 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -885,9 +885,9 @@ notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
>  
>      notmuch_process_shared_options (argv[0]);
>  
> -    /* skip subcommand argument */
> -    argc-= opt_index+1;
> -    argv+= opt_index+1;
> +    /* skip at least subcommand argument */
> +    argc-= opt_index;
> +    argv+= opt_index;
>  
>      if (argc < 1) {
>  	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
> diff --git a/notmuch-setup.c b/notmuch-setup.c
> index 5fc6e25..6a020dc 100644
> --- a/notmuch-setup.c
> +++ b/notmuch-setup.c
> @@ -155,7 +155,7 @@ notmuch_setup_command (notmuch_config_t *config,
>      if (opt_index < 0)
>  	return EXIT_FAILURE;
>  
> -    notmuch_process_shared_options (argv[0]);
> +    notmuch_process_shared_options ("setup");
>  
>      if (notmuch_config_is_new (config))
>  	welcome_message_pre_setup ();
> diff --git a/test/random-corpus.c b/test/random-corpus.c
> index 790193d..6c467bb 100644
> --- a/test/random-corpus.c
> +++ b/test/random-corpus.c
> @@ -114,6 +114,15 @@ random_utf8_string (void *ctx, size_t char_count)
>      return buf;
>  }
>  
> +/* stubs since we cannot link with notmuch.o */
> +const notmuch_opt_desc_t notmuch_shared_options[] = {
> +	{ 0, 0, 0, 0, 0 }
> +};
> +
> +void
> +notmuch_process_shared_options (unused (const char *dummy))
> +{
> +}
>  
>  int
>  main (int argc, char **argv)
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 3/4] cli: define shared options, use for --help and --version

[Mark Walters]

Hi

> +void notmuch_process_shared_options (const char* help_name);
> diff --git a/notmuch.c b/notmuch.c

> +/* any subcommand wanting to support these options should call
> + * inherit notmuch_shared_options and call
> + * notmuch_process_shared_options (subcommand_name);
> + */
> +void
> +notmuch_process_shared_options (const char *help_name) {

Perhaps call the argument something else as it might be used by other
commands later? Eg subcommand_name? (and in notmuch-client.h too)

Best wishes

Mark




> +    if (print_version) {
> +	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
> +	exit (EXIT_SUCCESS);
> +    }
> +
> +    if (print_help) {
> +	int ret = _help_for (help_name);
> +	exit (ret);
> +    }
> +}
> +
> +
>  static command_t commands[] = {
>      { NULL, notmuch_command, TRUE,
>        "Notmuch main command." },
> @@ -295,14 +324,12 @@ main (int argc, char *argv[])
>      command_t *command;
>      char *config_file_name = NULL;
>      notmuch_config_t *config = NULL;
> -    notmuch_bool_t print_help=FALSE, print_version=FALSE;
>      int opt_index;
>      int ret;
>  
>      notmuch_opt_desc_t options[] = {
> -	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
> -	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
>  	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
> +	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
>  
> @@ -324,24 +351,7 @@ main (int argc, char *argv[])
>  	goto DONE;
>      }
>  
> -    /* Handle notmuch --help [command] and notmuch command --help. */
> -    if (print_help ||
> -	(opt_index + 1 < argc && strcmp (argv[opt_index + 1], "--help") == 0)) {
> -	/*
> -	 * Pass the first positional argument as argv[1] so the help
> -	 * command can give help for it. The help command ignores the
> -	 * argv[0] passed to it.
> -	 */
> -	ret = notmuch_help_command (NULL, argc - opt_index + 1,
> -				    argv + opt_index - 1);
> -	goto DONE;
> -    }
> -
> -    if (print_version) {
> -	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
> -	ret = EXIT_SUCCESS;
> -	goto DONE;
> -    }
> +    notmuch_process_shared_options (NULL);
>  
>      if (opt_index < argc)
>  	command_name = argv[opt_index];
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 4/4] cli: add standard option processing to config and setup

[Mark Walters]

Hi

On Mon, 06 Apr 2015, David Bremner <david@tethera.net> wrote:
> In particular this fixes a recently encountered bug where the
> "--config" argument to "notmuch setup" is silently ignored, which the
> unpleasant consequence of overwriting the users config file.
> ---
>  notmuch-config.c     | 16 +++++++++++++++-
>  notmuch-setup.c      | 12 ++++++++++++
>  test/random-corpus.c |  9 +++++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 2d5c297..f2cd6a8 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -872,8 +872,22 @@ int
>  notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
>  {
>      int ret;
> +    int opt_index;
>  
> -    argc--; argv++; /* skip subcommand argument */
> +    notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
> +	{ 0, 0, 0, 0, 0 }
> +    };
> +
> +    opt_index = parse_arguments (argc, argv, options, 1);
> +    if (opt_index < 0)
> +	return EXIT_FAILURE;
> +
> +    notmuch_process_shared_options (argv[0]);
> +
> +    /* skip at least subcommand argument */
> +    argc-= opt_index;
> +    argv+= opt_index;
>  
>      if (argc < 1) {
>  	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
> diff --git a/notmuch-setup.c b/notmuch-setup.c
> index 36a6171..6a020dc 100644
> --- a/notmuch-setup.c
> +++ b/notmuch-setup.c
> @@ -133,6 +133,7 @@ notmuch_setup_command (notmuch_config_t *config,
>      size_t new_tags_len;
>      const char **search_exclude_tags;
>      size_t search_exclude_tags_len;
> +    int opt_index;
>  
>  #define prompt(format, ...)					\
>      do {							\
> @@ -145,6 +146,17 @@ notmuch_setup_command (notmuch_config_t *config,
>  	chomp_newline (response);				\
>      } while (0)
>  
> +    notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
> +	{ 0, 0, 0, 0, 0 }
> +    };
> +
> +    opt_index = parse_arguments (argc, argv, options, 1);
> +    if (opt_index < 0)
> +	return EXIT_FAILURE;
> +
> +    notmuch_process_shared_options ("setup");

Why "setup" here rather than argv[0] or similar? It seems inconsistent
with the other cases.

Best wishes

Mark



> +
>      if (notmuch_config_is_new (config))
>  	welcome_message_pre_setup ();
>  
> diff --git a/test/random-corpus.c b/test/random-corpus.c
> index 790193d..6c467bb 100644
> --- a/test/random-corpus.c
> +++ b/test/random-corpus.c
> @@ -114,6 +114,15 @@ random_utf8_string (void *ctx, size_t char_count)
>      return buf;
>  }
>  
> +/* stubs since we cannot link with notmuch.o */
> +const notmuch_opt_desc_t notmuch_shared_options[] = {
> +	{ 0, 0, 0, 0, 0 }
> +};
> +
> +void
> +notmuch_process_shared_options (unused (const char *dummy))
> +{
> +}
>  
>  int
>  main (int argc, char **argv)
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 4/4] cli: add standard option processing to config and setup

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>
> Why "setup" here rather than argv[0] or similar? It seems inconsistent
> with the other cases.
>
> Best wishes
>
> Mark

You're right this deserves a comment. notmuch_setup_command is actually
called with argv=NULL (hence the unused (char * argv). The argument
types are constrained by the fact we use a table of function pointers to
dispatch.

d

argument parsing refactoring round3

[David Bremner]

I think a dealt with all of Mark's comments, even "notmuch help
--help".

I ended up creating a new function for the places where we want to
process _only_ the shared options (config, setup, and help)

diff --git a/notmuch-client.h b/notmuch-client.h
index ab0d188..78680aa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -467,5 +467,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 
 #include "command-line-arguments.h"
 extern const notmuch_opt_desc_t  notmuch_shared_options [];
-void notmuch_process_shared_options (const char* help_name);
+void notmuch_process_shared_options (const char* subcommand_name);
+int notmuch_minimal_options (const char* subcommand_name,
+			     int argc, char **argv);
 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index f2cd6a8..9348278 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -874,17 +874,10 @@ notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
     int ret;
     int opt_index;
 
-    notmuch_opt_desc_t options[] = {
-	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
-	{ 0, 0, 0, 0, 0 }
-    };
-
-    opt_index = parse_arguments (argc, argv, options, 1);
+    opt_index = notmuch_minimal_options ("config", argc, argv);
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
-    notmuch_process_shared_options (argv[0]);
-
     /* skip at least subcommand argument */
     argc-= opt_index;
     argv+= opt_index;
diff --git a/notmuch-search.c b/notmuch-search.c
index 994ae58..b89a17e 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -740,6 +740,7 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
 				  { "false", NOTMUCH_EXCLUDE_FALSE },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_INHERIT, (void *) &common_options, NULL, 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -747,6 +748,8 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (! (ctx->output & (OUTPUT_SENDER | OUTPUT_RECIPIENTS)))
 	ctx->output |= OUTPUT_SENDER;
 
diff --git a/notmuch-setup.c b/notmuch-setup.c
index 21d074a..7dd5822 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -133,7 +133,6 @@ notmuch_setup_command (notmuch_config_t *config,
     size_t new_tags_len;
     const char **search_exclude_tags;
     size_t search_exclude_tags_len;
-    int opt_index;
 
 #define prompt(format, ...)					\
     do {							\
@@ -146,18 +145,9 @@ notmuch_setup_command (notmuch_config_t *config,
 	chomp_newline (response);				\
     } while (0)
 
-    notmuch_opt_desc_t options[] = {
-	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
-	{ 0, 0, 0, 0, 0 }
-    };
-
-    opt_index = parse_arguments (argc, argv, options, 1);
-    if (opt_index < 0)
+    if (notmuch_minimal_options ("setup", argc, argv) < 0)
 	return EXIT_FAILURE;
 
-    /* We can't use argv here as it is sometimes NULL */
-    notmuch_process_shared_options ("setup");
-
     if (notmuch_config_is_new (config))
 	welcome_message_pre_setup ();
 
diff --git a/notmuch.c b/notmuch.c
index c7f8c8f..2198b73 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -59,18 +59,40 @@ const notmuch_opt_desc_t notmuch_shared_options [] = {
  * notmuch_process_shared_options (subcommand_name);
  */
 void
-notmuch_process_shared_options (const char *help_name) {
+notmuch_process_shared_options (const char *subcommand_name) {
     if (print_version) {
 	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
 	exit (EXIT_SUCCESS);
     }
 
     if (print_help) {
-	int ret = _help_for (help_name);
+	int ret = _help_for (subcommand_name);
 	exit (ret);
     }
 }
 
+/* This is suitable for subcommands that do not actually open the
+ * database.
+ */
+int notmuch_minimal_options (const char *subcommand_name,
+				  int argc, char **argv)
+{
+    int opt_index;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0)
+	return -1;
+
+    /* We can't use argv here as it is sometimes NULL */
+    notmuch_process_shared_options (subcommand_name);
+    return opt_index;
+}
 
 static command_t commands[] = {
     { NULL, notmuch_command, TRUE,
@@ -250,7 +272,15 @@ _help_for (const char *topic_name)
 static int
 notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
 {
-    argc--; argv++; /* Ignore "help" */
+    int opt_index;
+
+    opt_index = notmuch_minimal_options ("help", argc, argv);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    /* skip at least subcommand argument */
+    argc-= opt_index;
+    argv+= opt_index;
 
     if (argc == 0) {
 	return _help_for (NULL);
diff --git a/test/random-corpus.c b/test/random-corpus.c
index 6c467bb..b377eb4 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -125,6 +125,14 @@ notmuch_process_shared_options (unused (const char *dummy))
 }
 
 int
+notmuch_minimal_options (unused (const char *subcommand),
+			 unused (int argc),
+			 unused (char **argv))
+{
+    return 0;
+}
+
+int
 main (int argc, char **argv)
 {
 

[PATCH 1/4] cli: ignore config argument of notmuch_help_command

[David Bremner]

We call it with NULL at one point anyway, so it needs to work with
NULL. Since the only place we use talloc is right before exec, there
is no harm in always using NULL.
---
 notmuch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index a5b2877..31672c3 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -177,7 +177,7 @@ exec_man (const char *page)
 }
 
 static int
-notmuch_help_command (notmuch_config_t *config, int argc, char *argv[])
+notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
 {
     command_t *command;
     help_topic_t *topic;
@@ -202,14 +202,14 @@ notmuch_help_command (notmuch_config_t *config, int argc, char *argv[])
 
     command = find_command (argv[0]);
     if (command) {
-	char *page = talloc_asprintf (config, "notmuch-%s", command->name);
+	char *page = talloc_asprintf (NULL, "notmuch-%s", command->name);
 	exec_man (page);
     }
 
     for (i = 0; i < ARRAY_SIZE (help_topics); i++) {
 	topic = &help_topics[i];
 	if (strcmp (argv[0], topic->name) == 0) {
-	    char *page = talloc_asprintf (config, "notmuch-%s", topic->name);
+	    char *page = talloc_asprintf (NULL, "notmuch-%s", topic->name);
 	    exec_man (page);
 	}
     }
-- 
2.1.4

[PATCH 2/4] cli: refactor notmuch_help_command

[David Bremner]

Create a new private entry point _help_for so that we can call help
without simulating a command line invokation to set up the arguments.
---
 notmuch.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index 31672c3..bff941f 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -177,21 +177,19 @@ exec_man (const char *page)
 }
 
 static int
-notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
+_help_for (const char *topic_name)
 {
     command_t *command;
     help_topic_t *topic;
     unsigned int i;
 
-    argc--; argv++; /* Ignore "help" */
-
-    if (argc == 0) {
+    if (!topic_name) {
 	printf ("The notmuch mail system.\n\n");
 	usage (stdout);
 	return EXIT_SUCCESS;
     }
 
-    if (strcmp (argv[0], "help") == 0) {
+    if (strcmp (topic_name, "help") == 0) {
 	printf ("The notmuch help system.\n\n"
 		"\tNotmuch uses the man command to display help. In case\n"
 		"\tof difficulties check that MANPATH includes the pages\n"
@@ -200,7 +198,7 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 	return EXIT_SUCCESS;
     }
 
-    command = find_command (argv[0]);
+    command = find_command (topic_name);
     if (command) {
 	char *page = talloc_asprintf (NULL, "notmuch-%s", command->name);
 	exec_man (page);
@@ -208,7 +206,7 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 
     for (i = 0; i < ARRAY_SIZE (help_topics); i++) {
 	topic = &help_topics[i];
-	if (strcmp (argv[0], topic->name) == 0) {
+	if (strcmp (topic_name, topic->name) == 0) {
 	    char *page = talloc_asprintf (NULL, "notmuch-%s", topic->name);
 	    exec_man (page);
 	}
@@ -216,10 +214,22 @@ notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[]
 
     fprintf (stderr,
 	     "\nSorry, %s is not a known command. There's not much I can do to help.\n\n",
-	     argv[0]);
+	     topic_name);
     return EXIT_FAILURE;
 }
 
+static int
+notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
+{
+    argc--; argv++; /* Ignore "help" */
+
+    if (argc == 0) {
+	return _help_for (NULL);
+    }
+
+    return _help_for (argv[0]);
+}
+
 /* Handle the case of "notmuch" being invoked with no command
  * argument. For now we just call notmuch_setup_command, but we plan
  * to be more clever about this in the future.
-- 
2.1.4

[PATCH 3/4] cli: define shared options, use for --help and --version

[David Bremner]

Unfortunately it seems trickier to support --config globally

The non-trivial changes are in notmuch.c; most of the other changes
consists of blindly inserting two lines into every subcommand.
---
 notmuch-client.h  |  2 ++
 notmuch-compact.c |  4 ++++
 notmuch-count.c   |  3 +++
 notmuch-dump.c    |  3 +++
 notmuch-insert.c  |  3 +++
 notmuch-new.c     |  3 +++
 notmuch-reply.c   |  3 +++
 notmuch-restore.c |  2 ++
 notmuch-search.c  |  6 ++++++
 notmuch-show.c    |  3 +++
 notmuch-tag.c     |  3 +++
 notmuch.c         | 52 +++++++++++++++++++++++++++++++---------------------
 12 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index fb3021c..8ecfac6 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -466,4 +466,6 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 		       notmuch_bool_t gzip_output);
 
 #include "command-line-arguments.h"
+extern const notmuch_opt_desc_t  notmuch_shared_options [];
+void notmuch_process_shared_options (const char* subcommand_name);
 #endif
diff --git a/notmuch-compact.c b/notmuch-compact.c
index 2fc012a..5be551d 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -38,12 +38,16 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &backup_path, "backup", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0}
     };
 
     opt_index = parse_arguments (argc, argv, options, 1);
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (! quiet)
 	printf ("Compacting database...\n");
     ret = notmuch_database_compact (path, backup_path,
diff --git a/notmuch-count.c b/notmuch-count.c
index 6058f7c..57a88a8 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -146,6 +146,7 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -153,6 +154,8 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (input_file_name) {
 	batch = TRUE;
 	input = fopen (input_file_name, "r");
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 9c6ad7f..fab22bd 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -228,6 +228,7 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &output_file_name, "output", 'o', 0  },
 	{ NOTMUCH_OPT_BOOLEAN, &gzip_output, "gzip", 'z', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -235,6 +236,8 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (opt_index < argc) {
 	query_str = query_string_from_args (notmuch, argc - opt_index, argv + opt_index);
 	if (query_str == NULL) {
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 90fe3ba..697880f 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -466,6 +466,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -473,6 +474,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     db_path = notmuch_config_get_database_path (config);
     new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
     synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
diff --git a/notmuch-new.c b/notmuch-new.c
index e6c283e..895f5d9 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -934,6 +934,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -941,6 +942,8 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     /* quiet trumps verbose */
     if (quiet)
 	add_files_state.verbosity = VERBOSITY_QUIET;
diff --git a/notmuch-reply.c b/notmuch-reply.c
index d51fdfc..4464741 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -790,6 +790,7 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 				  { "sender", FALSE },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -797,6 +798,8 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (format == FORMAT_HEADERS_ONLY) {
 	reply_format_func = notmuch_reply_format_headers_only;
     } else if (format == FORMAT_JSON) {
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 584d9f9..2a534dc 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -154,6 +154,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &accumulate, "accumulate", 'a', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -163,6 +164,7 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
 	goto DONE;
     }
 
+    notmuch_process_shared_options (argv[0]);
     name_for_error = input_file_name ? input_file_name : "stdin";
 
     if (! accumulate)
diff --git a/notmuch-search.c b/notmuch-search.c
index b81ac01..b89a17e 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -681,6 +681,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_INT, &ctx->limit, "limit", 'L', 0  },
 	{ NOTMUCH_OPT_INT, &ctx->dupe, "duplicate", 'D', 0  },
 	{ NOTMUCH_OPT_INHERIT, (void *) &common_options, NULL, 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -689,6 +690,8 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (ctx->output != OUTPUT_FILES && ctx->output != OUTPUT_MESSAGES &&
 	ctx->dupe != -1) {
         fprintf (stderr, "Error: --duplicate=N is only supported with --output=files and --output=messages.\n");
@@ -737,6 +740,7 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
 				  { "false", NOTMUCH_EXCLUDE_FALSE },
 				  { 0, 0 } } },
 	{ NOTMUCH_OPT_INHERIT, (void *) &common_options, NULL, 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -744,6 +748,8 @@ notmuch_address_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (! (ctx->output & (OUTPUT_SENDER | OUTPUT_RECIPIENTS)))
 	ctx->output |= OUTPUT_SENDER;
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 43bf71c..b80933a 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1114,6 +1114,7 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.verify, "verify", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.output_body, "body", 'b', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &params.include_html, "include-html", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -1121,6 +1122,8 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     /* decryption implies verification */
     if (params.crypto.decrypt)
 	params.crypto.verify = TRUE;
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 5b2f1e4..35f971d 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -206,6 +206,7 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &remove_all, "remove-all", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -213,6 +214,8 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
     if (opt_index < 0)
 	return EXIT_FAILURE;
 
+    notmuch_process_shared_options (argv[0]);
+
     if (input_file_name) {
 	batch = TRUE;
 	input = fopen (input_file_name, "r");
diff --git a/notmuch.c b/notmuch.c
index bff941f..3a9da90 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -43,6 +43,35 @@ notmuch_help_command (notmuch_config_t *config, int argc, char *argv[]);
 static int
 notmuch_command (notmuch_config_t *config, int argc, char *argv[]);
 
+static int
+_help_for (const char *topic);
+
+static notmuch_bool_t print_version = FALSE, print_help = FALSE;
+
+const notmuch_opt_desc_t notmuch_shared_options [] = {
+    { NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
+    { NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
+    {0, 0, 0, 0, 0}
+};
+
+/* any subcommand wanting to support these options should call
+ * inherit notmuch_shared_options and call
+ * notmuch_process_shared_options (subcommand_name);
+ */
+void
+notmuch_process_shared_options (const char *subcommand_name) {
+    if (print_version) {
+	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
+	exit (EXIT_SUCCESS);
+    }
+
+    if (print_help) {
+	int ret = _help_for (subcommand_name);
+	exit (ret);
+    }
+}
+
+
 static command_t commands[] = {
     { NULL, notmuch_command, TRUE,
       "Notmuch main command." },
@@ -295,14 +324,12 @@ main (int argc, char *argv[])
     command_t *command;
     char *config_file_name = NULL;
     notmuch_config_t *config = NULL;
-    notmuch_bool_t print_help=FALSE, print_version=FALSE;
     int opt_index;
     int ret;
 
     notmuch_opt_desc_t options[] = {
-	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
-	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
 	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -324,24 +351,7 @@ main (int argc, char *argv[])
 	goto DONE;
     }
 
-    /* Handle notmuch --help [command] and notmuch command --help. */
-    if (print_help ||
-	(opt_index + 1 < argc && strcmp (argv[opt_index + 1], "--help") == 0)) {
-	/*
-	 * Pass the first positional argument as argv[1] so the help
-	 * command can give help for it. The help command ignores the
-	 * argv[0] passed to it.
-	 */
-	ret = notmuch_help_command (NULL, argc - opt_index + 1,
-				    argv + opt_index - 1);
-	goto DONE;
-    }
-
-    if (print_version) {
-	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
-	ret = EXIT_SUCCESS;
-	goto DONE;
-    }
+    notmuch_process_shared_options (NULL);
 
     if (opt_index < argc)
 	command_name = argv[opt_index];
-- 
2.1.4

[PATCH 4/4] cli: add standard option processing to config, help and setup

[David Bremner]

In particular this fixes a recently encountered bug where the
"--config" argument to "notmuch setup" is silently ignored, which the
unpleasant consequence of overwriting the users config file.
---
 notmuch-client.h     |  2 ++
 notmuch-config.c     |  9 ++++++++-
 notmuch-setup.c      |  3 +++
 notmuch.c            | 32 +++++++++++++++++++++++++++++++-
 test/random-corpus.c |  9 +++++++++
 5 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 8ecfac6..78680aa 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -468,4 +468,6 @@ notmuch_database_dump (notmuch_database_t *notmuch,
 #include "command-line-arguments.h"
 extern const notmuch_opt_desc_t  notmuch_shared_options [];
 void notmuch_process_shared_options (const char* subcommand_name);
+int notmuch_minimal_options (const char* subcommand_name,
+			     int argc, char **argv);
 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index 2d5c297..9348278 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -872,8 +872,15 @@ int
 notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
 {
     int ret;
+    int opt_index;
 
-    argc--; argv++; /* skip subcommand argument */
+    opt_index = notmuch_minimal_options ("config", argc, argv);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    /* skip at least subcommand argument */
+    argc-= opt_index;
+    argv+= opt_index;
 
     if (argc < 1) {
 	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
diff --git a/notmuch-setup.c b/notmuch-setup.c
index 36a6171..7dd5822 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -145,6 +145,9 @@ notmuch_setup_command (notmuch_config_t *config,
 	chomp_newline (response);				\
     } while (0)
 
+    if (notmuch_minimal_options ("setup", argc, argv) < 0)
+	return EXIT_FAILURE;
+
     if (notmuch_config_is_new (config))
 	welcome_message_pre_setup ();
 
diff --git a/notmuch.c b/notmuch.c
index 3a9da90..2198b73 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -71,6 +71,28 @@ notmuch_process_shared_options (const char *subcommand_name) {
     }
 }
 
+/* This is suitable for subcommands that do not actually open the
+ * database.
+ */
+int notmuch_minimal_options (const char *subcommand_name,
+				  int argc, char **argv)
+{
+    int opt_index;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0)
+	return -1;
+
+    /* We can't use argv here as it is sometimes NULL */
+    notmuch_process_shared_options (subcommand_name);
+    return opt_index;
+}
 
 static command_t commands[] = {
     { NULL, notmuch_command, TRUE,
@@ -250,7 +272,15 @@ _help_for (const char *topic_name)
 static int
 notmuch_help_command (unused (notmuch_config_t * config), int argc, char *argv[])
 {
-    argc--; argv++; /* Ignore "help" */
+    int opt_index;
+
+    opt_index = notmuch_minimal_options ("help", argc, argv);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    /* skip at least subcommand argument */
+    argc-= opt_index;
+    argv+= opt_index;
 
     if (argc == 0) {
 	return _help_for (NULL);
diff --git a/test/random-corpus.c b/test/random-corpus.c
index 790193d..6c467bb 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -114,6 +114,15 @@ random_utf8_string (void *ctx, size_t char_count)
     return buf;
 }
 
+/* stubs since we cannot link with notmuch.o */
+const notmuch_opt_desc_t notmuch_shared_options[] = {
+	{ 0, 0, 0, 0, 0 }
+};
+
+void
+notmuch_process_shared_options (unused (const char *dummy))
+{
+}
 
 int
 main (int argc, char **argv)
-- 
2.1.4

[PATCH] fixup! cli: add standard option processing to config, help and setup

[David Bremner]

---
 test/random-corpus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/random-corpus.c b/test/random-corpus.c
index 6c467bb..b377eb4 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -125,6 +125,14 @@ notmuch_process_shared_options (unused (const char *dummy))
 }
 
 int
+notmuch_minimal_options (unused (const char *subcommand),
+			 unused (int argc),
+			 unused (char **argv))
+{
+    return 0;
+}
+
+int
 main (int argc, char **argv)
 {
 
-- 
2.1.4

Re: argument parsing refactoring round3

[guyzmo]

Hi David,

On Wed, Apr 08, 2015 at 04:30:38AM +0900, David Bremner wrote:
> I think a dealt with all of Mark's comments, even "notmuch help
> --help".
> 
> I ended up creating a new function for the places where we want to
> process _only_ the shared options (config, setup, and help)

I see you patching and repatching notmuch's CLI to improve it, and I was
wondering whether you had considered actually using `docopt` to generate
the CLI parser from the output.

It's possible to chain docopts to create a CLI UI very much alike the
git command, and it's more easily maintainable, as you're actually
generating the code from the `--help` page instead of the other way
around, making you focus on how you want the user to use the CLI only.

Here's the link to the C parser generator:

    https://github.com/docopt/docopt.c

it might not be perfect as is, but it could be worth trying out? I
actually never tried the .c version of docopt.

I had a more extensive experience with the python version, and since
then I totally dropped argparse.

    https://github.com/docopt/docopt.c

I even tend to believe that one could create a full CLI using
python+docopt to actually control notmuch using the python notmuch
interface… Even though I'm pretty sure some would yell at me and
want to burn me as an heretic just for suggesting that :-)

what do you believe?

-- 
Guyzmo

Re: argument parsing refactoring round3

[David Bremner]

guyzmo <guyzmo+notmuch@m0g.net> writes:

> Hi David,

[...]

> I see you patching and repatching notmuch's CLI to improve it, and I was
> wondering whether you had considered actually using `docopt` to generate
> the CLI parser from the output.
>
> It's possible to chain docopts to create a CLI UI very much alike the
> git command, and it's more easily maintainable, as you're actually
> generating the code from the `--help` page instead of the other way
> around, making you focus on how you want the user to use the CLI only.

[...]

> what do you believe?

It's an interesting idea, and if I was faced with writing CLI parser
from scratch (i.e. 4 years ago) I would investigate it more seriously.
As it stands, I'm not particularly annoyed with the notmuch argument
parsing code, so I mainly see negative issues about your proposal.

- I'm not sure how much this change would ripple through the rest of the
  notmuch code. At least all of the variables set by the current
  argument parsing code would have to be set foo=args.foo, or replaced
  everywhere with args.foo.
   
- It would require modifying the notmuch CLI to conform to the
  conventions of docopt. Of course, you might consider this a feature,
  but I think as many people would be annoyed as would be happy.

- The most dramatic example of an appartently missing feature from
  docopt is keyword arguments of the form
  
    --output=(messages|threads|summary).

  These are the reason we rolled our own parser in the first place,
  rather than using e.g. gnu getopt.  docopt says it doesn't do data
  validation, which is fine philosophically, but by the time we add back
  in validation code, I'm not sure we win very much in the
  maintainability department.
  
- I don't really know about the health of the docopt.c project, compared
  to the python version. It seems somewhat unfinished [1]; in particular
  a lack of positional arguments seems like a showstopper for us.
  arguments. There has never been a release (which is the norm for
  github, but not for dependencies of notmuch), and the last commit was
  in December of 2014.

- Since docopt_c_py is not widely in distros (it isn't in Debian, for
  example), we'd have to emded it the notmuch source.  It's only 217
  lines of fairly simple python, but embedding 3rd party code is
  something we try pretty hard to avoid.

As always, my lack of enthusiasm doesn't prevent someone else from
investigating further, but hopefully the points I listed above give
anyone doing such investigation some hints as to what I (and I suspect
not just I) would object to about any hypothetical patches.

David

[1]: From the README.md "Note, at this point the code generator handles
only options (positional arguments, commands and pattern matching will
follow)."

Re: argument parsing refactoring round3

[David Bremner]

David Bremner <david@tethera.net> writes:

> guyzmo <guyzmo+notmuch@m0g.net> writes:
>
>> I was wondering whether you had considered actually using `docopt` to
>> generate the CLI parser from the output.

> As it stands, I'm not particularly annoyed with the notmuch argument
> parsing code, so I mainly see negative issues about your proposal.

On the other hand, I do find the config handling code annoying, so if
people have some ideas about that, I am interested to hear them.  There
is now almost 1000 lines of C in notmuch-config.c, and despite some
efforts to streamline things, there is a lot of copying and pasting
every time someone wants to add a configuration option.

d

Re: argument parsing refactoring round3

[Tomi Ollila]

On Sat, Apr 11 2015, David Bremner <david@tethera.net> wrote:

> David Bremner <david@tethera.net> writes:
>
>> guyzmo <guyzmo+notmuch@m0g.net> writes:
>>
>>> I was wondering whether you had considered actually using `docopt` to
>>> generate the CLI parser from the output.
>
>> As it stands, I'm not particularly annoyed with the notmuch argument
>> parsing code, so I mainly see negative issues about your proposal.
>
> On the other hand, I do find the config handling code annoying, so if
> people have some ideas about that, I am interested to hear them.  There
> is now almost 1000 lines of C in notmuch-config.c, and despite some
> efforts to streamline things, there is a lot of copying and pasting
> every time someone wants to add a configuration option.

I reviewed the changes -- last ones 'cursory' as I think those don't 
break any "real" functionality -- and if help works bad then one can
always resort to testing and reading code >;)...

...but I did not test -- automated tests (to ensure it doesn't break
anything be handy... more complete would limit anyone's interest to
implement alternate option handling (even further).

As much I also liked "better" option handling code new code (vast of that)
would also require significant reviewing effort which no-one will like
to do >;/

With that said, +1 from me to the config handling changes. 
(or should I give it some hand-testing (how?)

Tomi

>
> d

Re: argument parsing refactoring round3

[David Bremner]

David Bremner <david@tethera.net> writes:

> I think a dealt with all of Mark's comments, even "notmuch help
> --help".
>

Pushed the amended version of these patches, as some small step to
getting the revision tracking patches merged.

d