[PATCH 1/2] cli/config: don't try to open config file for 'notmuch help'

[PATCH 1/2] cli/config: don't try to open config file for 'notmuch help'

[Jani Nikula]

The help command does not really need to try to open the config
file. So don't.

---

This will allow better error reporting such as
id:1483570332-11820-1-git-send-email-tomi.ollila@iki.fi while still
ensuring 'notmuch help' succeeds in the absence of config files.
---
 notmuch-client.h |  7 ++++++-
 notmuch-config.c | 12 ++++++++----
 notmuch.c        | 34 +++++++++++++++++-----------------
 3 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index 21b087980a17..1099122426af 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -263,10 +263,15 @@ json_quote_str (const void *ctx, const char *str);
 
 /* notmuch-config.c */
 
+typedef enum {
+    NOTMUCH_CONFIG_OPEN	= 1 << 0,
+    NOTMUCH_CONFIG_CREATE = 1 << 1,
+} notmuch_config_mode_t;
+
 notmuch_config_t *
 notmuch_config_open (void *ctx,
 		     const char *filename,
-		     notmuch_bool_t create_new);
+		     notmuch_config_mode_t config_mode);
 
 void
 notmuch_config_close (notmuch_config_t *config);
diff --git a/notmuch-config.c b/notmuch-config.c
index b202bb1e2299..959410cce7b8 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -322,7 +322,7 @@ out:
 notmuch_config_t *
 notmuch_config_open (void *ctx,
 		     const char *filename,
-		     notmuch_bool_t create_new)
+		     notmuch_config_mode_t config_mode)
 {
     GError *error = NULL;
     size_t tmp;
@@ -356,9 +356,13 @@ notmuch_config_open (void *ctx,
 
     config->key_file = g_key_file_new ();
 
-    if (! get_config_from_file (config, create_new)) {
-	talloc_free (config);
-	return NULL;
+    if (config_mode & NOTMUCH_CONFIG_OPEN) {
+	notmuch_bool_t create_new = (config_mode & NOTMUCH_CONFIG_CREATE) != 0;
+
+	if (! get_config_from_file (config, create_new)) {
+	    talloc_free (config);
+	    return NULL;
+	}
     }
 
     /* Whenever we know of configuration sections that don't appear in
diff --git a/notmuch.c b/notmuch.c
index b9c320329dd5..8e332ce64410 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -33,7 +33,7 @@ typedef int (*command_function_t) (notmuch_config_t *config, int argc, char *arg
 typedef struct command {
     const char *name;
     command_function_t function;
-    notmuch_bool_t create_config;
+    notmuch_config_mode_t config_mode;
     const char *summary;
 } command_t;
 
@@ -97,35 +97,35 @@ int notmuch_minimal_options (const char *subcommand_name,
 }
 
 static command_t commands[] = {
-    { NULL, notmuch_command, TRUE,
+    { NULL, notmuch_command, NOTMUCH_CONFIG_OPEN | NOTMUCH_CONFIG_CREATE,
       "Notmuch main command." },
-    { "setup", notmuch_setup_command, TRUE,
+    { "setup", notmuch_setup_command, NOTMUCH_CONFIG_OPEN | NOTMUCH_CONFIG_CREATE,
       "Interactively set up notmuch for first use." },
-    { "new", notmuch_new_command, FALSE,
+    { "new", notmuch_new_command, NOTMUCH_CONFIG_OPEN,
       "Find and import new messages to the notmuch database." },
-    { "insert", notmuch_insert_command, FALSE,
+    { "insert", notmuch_insert_command, NOTMUCH_CONFIG_OPEN,
       "Add a new message into the maildir and notmuch database." },
-    { "search", notmuch_search_command, FALSE,
+    { "search", notmuch_search_command, NOTMUCH_CONFIG_OPEN,
       "Search for messages matching the given search terms." },
-    { "address", notmuch_address_command, FALSE,
+    { "address", notmuch_address_command, NOTMUCH_CONFIG_OPEN,
       "Get addresses from messages matching the given search terms." },
-    { "show", notmuch_show_command, FALSE,
+    { "show", notmuch_show_command, NOTMUCH_CONFIG_OPEN,
       "Show all messages matching the search terms." },
-    { "count", notmuch_count_command, FALSE,
+    { "count", notmuch_count_command, NOTMUCH_CONFIG_OPEN,
       "Count messages matching the search terms." },
-    { "reply", notmuch_reply_command, FALSE,
+    { "reply", notmuch_reply_command, NOTMUCH_CONFIG_OPEN,
       "Construct a reply template for a set of messages." },
-    { "tag", notmuch_tag_command, FALSE,
+    { "tag", notmuch_tag_command, NOTMUCH_CONFIG_OPEN,
       "Add/remove tags for all messages matching the search terms." },
-    { "dump", notmuch_dump_command, FALSE,
+    { "dump", notmuch_dump_command, NOTMUCH_CONFIG_OPEN,
       "Create a plain-text dump of the tags for each message." },
-    { "restore", notmuch_restore_command, FALSE,
+    { "restore", notmuch_restore_command, NOTMUCH_CONFIG_OPEN,
       "Restore the tags from the given dump file (see 'dump')." },
-    { "compact", notmuch_compact_command, FALSE,
+    { "compact", notmuch_compact_command, NOTMUCH_CONFIG_OPEN,
       "Compact the notmuch database." },
-    { "config", notmuch_config_command, FALSE,
+    { "config", notmuch_config_command, NOTMUCH_CONFIG_OPEN,
       "Get or set settings in the notmuch configuration file." },
-    { "help", notmuch_help_command, TRUE, /* create but don't save config */
+    { "help", notmuch_help_command, NOTMUCH_CONFIG_CREATE, /* create but don't save config */
       "This message, or more detailed help for the named command." }
 };
 
@@ -447,7 +447,7 @@ main (int argc, char *argv[])
 	goto DONE;
     }
 
-    config = notmuch_config_open (local, config_file_name, command->create_config);
+    config = notmuch_config_open (local, config_file_name, command->config_mode);
     if (!config) {
 	ret = EXIT_FAILURE;
 	goto DONE;
-- 
2.11.0

[PATCH 2/2] notmuch-config: ENOENT vs generic handling when file open fails.

[Jani Nikula]

From: Tomi Ollila <tomi.ollila@iki.fi>

When opening configuration file fails, ENOENT (file not found) is
handled specially -- in setup missing file is ok (often expected),
and otherwise user can be informed to run notmuch setup.

In any other case the the reason is unknown, so there is no other
option but to print generic error message to stderr.
---
 notmuch-config.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 959410cce7b8..e4aaef610173 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -215,24 +215,23 @@ get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new)
 
     FILE *fp = fopen(config->filename, "r");
     if (fp == NULL) {
-	/* If create_new is true, then the caller is prepared for a
-	 * default configuration file in the case of FILE NOT FOUND.
-	 */
-	if (create_new) {
-	    config->is_new = TRUE;
-	    ret = TRUE;
-	    goto out;
-	} else if (errno == ENOENT) {
-	    fprintf (stderr, "Configuration file %s not found.\n"
-		     "Try running 'notmuch setup' to create a configuration.\n",
-		     config->filename);
-	    goto out;
+	if (errno == ENOENT) {
+	    /* If create_new is true, then the caller is prepared for a
+	     * default configuration file in the case of FILE NOT FOUND.
+	     */
+	    if (create_new) {
+		config->is_new = TRUE;
+		ret = TRUE;
+	    } else {
+		fprintf (stderr, "Configuration file %s not found.\n"
+			 "Try running 'notmuch setup' to create a configuration.\n",
+			 config->filename);
+	    }
 	} else {
-	    fprintf (stderr, "Error opening config file '%s': %s\n"
-		     "Try running 'notmuch setup' to create a configuration.\n",
+	    fprintf (stderr, "Error opening config file '%s': %s\n",
 		     config->filename, strerror(errno));
-	    goto out;
 	}
+	goto out;
     }
 
     config_str = talloc_zero_array (config, char, config_bufsize);
-- 
2.11.0

Re: [PATCH 1/2] cli/config: don't try to open config file for 'notmuch help'

[Tomi Ollila]

On Sun, Feb 26 2017, Jani Nikula <jani@nikula.org> wrote:

> The help command does not really need to try to open the config
> file. So don't.
>
> ---

I've forgotten this ... added notmuch::0.24 to this series and will
check carefully and test tomorrow...

Tomi

>
> This will allow better error reporting such as
> id:1483570332-11820-1-git-send-email-tomi.ollila@iki.fi while still
> ensuring 'notmuch help' succeeds in the absence of config files.
> ---
>  notmuch-client.h |  7 ++++++-
>  notmuch-config.c | 12 ++++++++----
>  notmuch.c        | 34 +++++++++++++++++-----------------
>  3 files changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 21b087980a17..1099122426af 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -263,10 +263,15 @@ json_quote_str (const void *ctx, const char *str);
>  
>  /* notmuch-config.c */
>  
> +typedef enum {
> +    NOTMUCH_CONFIG_OPEN	= 1 << 0,
> +    NOTMUCH_CONFIG_CREATE = 1 << 1,
> +} notmuch_config_mode_t;
> +
>  notmuch_config_t *
>  notmuch_config_open (void *ctx,
>  		     const char *filename,
> -		     notmuch_bool_t create_new);
> +		     notmuch_config_mode_t config_mode);
>  
>  void
>  notmuch_config_close (notmuch_config_t *config);
> diff --git a/notmuch-config.c b/notmuch-config.c
> index b202bb1e2299..959410cce7b8 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -322,7 +322,7 @@ out:
>  notmuch_config_t *
>  notmuch_config_open (void *ctx,
>  		     const char *filename,
> -		     notmuch_bool_t create_new)
> +		     notmuch_config_mode_t config_mode)
>  {
>      GError *error = NULL;
>      size_t tmp;
> @@ -356,9 +356,13 @@ notmuch_config_open (void *ctx,
>  
>      config->key_file = g_key_file_new ();
>  
> -    if (! get_config_from_file (config, create_new)) {
> -	talloc_free (config);
> -	return NULL;
> +    if (config_mode & NOTMUCH_CONFIG_OPEN) {
> +	notmuch_bool_t create_new = (config_mode & NOTMUCH_CONFIG_CREATE) != 0;
> +
> +	if (! get_config_from_file (config, create_new)) {
> +	    talloc_free (config);
> +	    return NULL;
> +	}
>      }
>  
>      /* Whenever we know of configuration sections that don't appear in
> diff --git a/notmuch.c b/notmuch.c
> index b9c320329dd5..8e332ce64410 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -33,7 +33,7 @@ typedef int (*command_function_t) (notmuch_config_t *config, int argc, char *arg
>  typedef struct command {
>      const char *name;
>      command_function_t function;
> -    notmuch_bool_t create_config;
> +    notmuch_config_mode_t config_mode;
>      const char *summary;
>  } command_t;
>  
> @@ -97,35 +97,35 @@ int notmuch_minimal_options (const char *subcommand_name,
>  }
>  
>  static command_t commands[] = {
> -    { NULL, notmuch_command, TRUE,
> +    { NULL, notmuch_command, NOTMUCH_CONFIG_OPEN | NOTMUCH_CONFIG_CREATE,
>        "Notmuch main command." },
> -    { "setup", notmuch_setup_command, TRUE,
> +    { "setup", notmuch_setup_command, NOTMUCH_CONFIG_OPEN | NOTMUCH_CONFIG_CREATE,
>        "Interactively set up notmuch for first use." },
> -    { "new", notmuch_new_command, FALSE,
> +    { "new", notmuch_new_command, NOTMUCH_CONFIG_OPEN,
>        "Find and import new messages to the notmuch database." },
> -    { "insert", notmuch_insert_command, FALSE,
> +    { "insert", notmuch_insert_command, NOTMUCH_CONFIG_OPEN,
>        "Add a new message into the maildir and notmuch database." },
> -    { "search", notmuch_search_command, FALSE,
> +    { "search", notmuch_search_command, NOTMUCH_CONFIG_OPEN,
>        "Search for messages matching the given search terms." },
> -    { "address", notmuch_address_command, FALSE,
> +    { "address", notmuch_address_command, NOTMUCH_CONFIG_OPEN,
>        "Get addresses from messages matching the given search terms." },
> -    { "show", notmuch_show_command, FALSE,
> +    { "show", notmuch_show_command, NOTMUCH_CONFIG_OPEN,
>        "Show all messages matching the search terms." },
> -    { "count", notmuch_count_command, FALSE,
> +    { "count", notmuch_count_command, NOTMUCH_CONFIG_OPEN,
>        "Count messages matching the search terms." },
> -    { "reply", notmuch_reply_command, FALSE,
> +    { "reply", notmuch_reply_command, NOTMUCH_CONFIG_OPEN,
>        "Construct a reply template for a set of messages." },
> -    { "tag", notmuch_tag_command, FALSE,
> +    { "tag", notmuch_tag_command, NOTMUCH_CONFIG_OPEN,
>        "Add/remove tags for all messages matching the search terms." },
> -    { "dump", notmuch_dump_command, FALSE,
> +    { "dump", notmuch_dump_command, NOTMUCH_CONFIG_OPEN,
>        "Create a plain-text dump of the tags for each message." },
> -    { "restore", notmuch_restore_command, FALSE,
> +    { "restore", notmuch_restore_command, NOTMUCH_CONFIG_OPEN,
>        "Restore the tags from the given dump file (see 'dump')." },
> -    { "compact", notmuch_compact_command, FALSE,
> +    { "compact", notmuch_compact_command, NOTMUCH_CONFIG_OPEN,
>        "Compact the notmuch database." },
> -    { "config", notmuch_config_command, FALSE,
> +    { "config", notmuch_config_command, NOTMUCH_CONFIG_OPEN,
>        "Get or set settings in the notmuch configuration file." },
> -    { "help", notmuch_help_command, TRUE, /* create but don't save config */
> +    { "help", notmuch_help_command, NOTMUCH_CONFIG_CREATE, /* create but don't save config */
>        "This message, or more detailed help for the named command." }
>  };
>  
> @@ -447,7 +447,7 @@ main (int argc, char *argv[])
>  	goto DONE;
>      }
>  
> -    config = notmuch_config_open (local, config_file_name, command->create_config);
> +    config = notmuch_config_open (local, config_file_name, command->config_mode);
>      if (!config) {
>  	ret = EXIT_FAILURE;
>  	goto DONE;
> -- 
> 2.11.0

Re: [PATCH 1/2] cli/config: don't try to open config file for 'notmuch help'

[Tomi Ollila]

On Sun, Feb 26 2017, Jani Nikula <jani@nikula.org> wrote:

> The help command does not really need to try to open the config
> file. So don't.
>
> ---

This series looks good, tests pass, repost of my patch seems to be
unmodified (the power of running 2 emacses w/ notmuch rules!)

In addition to `make test` I ran the following 3 commands:

  104  HOME=/tmp/xyxz ./notmuch help
  105  HOME=/tmp/xyxz ./notmuch nre
  106  HOME=/tmp/xyxz ./notmuch new
  107  HOME=/tmp/xyxz ./notmuch setup

and these worked as expected.

Tomi

>
> This will allow better error reporting such as
> id:1483570332-11820-1-git-send-email-tomi.ollila@iki.fi while still
> ensuring 'notmuch help' succeeds in the absence of config files.
> ---
>  notmuch-client.h |  7 ++++++-
>  notmuch-config.c | 12 ++++++++----
>  notmuch.c        | 34 +++++++++++++++++-----------------
>  3 files changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 21b087980a17..1099122426af 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -263,10 +263,15 @@ json_quote_str (const void *ctx, const char *str);
>  
>  /* notmuch-config.c */
>  
> +typedef enum {
> +    NOTMUCH_CONFIG_OPEN	= 1 << 0,
> +    NOTMUCH_CONFIG_CREATE = 1 << 1,
> +} notmuch_config_mode_t;
> +
>  notmuch_config_t *
>  notmuch_config_open (void *ctx,
>  		     const char *filename,
> -		     notmuch_bool_t create_new);
> +		     notmuch_config_mode_t config_mode);
>  
>  void
>  notmuch_config_close (notmuch_config_t *config);
> diff --git a/notmuch-config.c b/notmuch-config.c
> index b202bb1e2299..959410cce7b8 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -322,7 +322,7 @@ out:
>  notmuch_config_t *
>  notmuch_config_open (void *ctx,
>  		     const char *filename,
> -		     notmuch_bool_t create_new)
> +		     notmuch_config_mode_t config_mode)
>  {
>      GError *error = NULL;
>      size_t tmp;
> @@ -356,9 +356,13 @@ notmuch_config_open (void *ctx,
>  
>      config->key_file = g_key_file_new ();
>  
> -    if (! get_config_from_file (config, create_new)) {
> -	talloc_free (config);
> -	return NULL;
> +    if (config_mode & NOTMUCH_CONFIG_OPEN) {
> +	notmuch_bool_t create_new = (config_mode & NOTMUCH_CONFIG_CREATE) != 0;
> +
> +	if (! get_config_from_file (config, create_new)) {
> +	    talloc_free (config);
> +	    return NULL;
> +	}
>      }
>  
>      /* Whenever we know of configuration sections that don't appear in
> diff --git a/notmuch.c b/notmuch.c
> index b9c320329dd5..8e332ce64410 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -33,7 +33,7 @@ typedef int (*command_function_t) (notmuch_config_t *config, int argc, char *arg
>  typedef struct command {
>      const char *name;
>      command_function_t function;
> -    notmuch_bool_t create_config;
> +    notmuch_config_mode_t config_mode;
>      const char *summary;
>  } command_t;
>  
> @@ -97,35 +97,35 @@ int notmuch_minimal_options (const char *subcommand_name,
>  }
>  
>  static command_t commands[] = {
> -    { NULL, notmuch_command, TRUE,
> +    { NULL, notmuch_command, NOTMUCH_CONFIG_OPEN | NOTMUCH_CONFIG_CREATE,
>        "Notmuch main command." },
> -    { "setup", notmuch_setup_command, TRUE,
> +    { "setup", notmuch_setup_command, NOTMUCH_CONFIG_OPEN | NOTMUCH_CONFIG_CREATE,
>        "Interactively set up notmuch for first use." },
> -    { "new", notmuch_new_command, FALSE,
> +    { "new", notmuch_new_command, NOTMUCH_CONFIG_OPEN,
>        "Find and import new messages to the notmuch database." },
> -    { "insert", notmuch_insert_command, FALSE,
> +    { "insert", notmuch_insert_command, NOTMUCH_CONFIG_OPEN,
>        "Add a new message into the maildir and notmuch database." },
> -    { "search", notmuch_search_command, FALSE,
> +    { "search", notmuch_search_command, NOTMUCH_CONFIG_OPEN,
>        "Search for messages matching the given search terms." },
> -    { "address", notmuch_address_command, FALSE,
> +    { "address", notmuch_address_command, NOTMUCH_CONFIG_OPEN,
>        "Get addresses from messages matching the given search terms." },
> -    { "show", notmuch_show_command, FALSE,
> +    { "show", notmuch_show_command, NOTMUCH_CONFIG_OPEN,
>        "Show all messages matching the search terms." },
> -    { "count", notmuch_count_command, FALSE,
> +    { "count", notmuch_count_command, NOTMUCH_CONFIG_OPEN,
>        "Count messages matching the search terms." },
> -    { "reply", notmuch_reply_command, FALSE,
> +    { "reply", notmuch_reply_command, NOTMUCH_CONFIG_OPEN,
>        "Construct a reply template for a set of messages." },
> -    { "tag", notmuch_tag_command, FALSE,
> +    { "tag", notmuch_tag_command, NOTMUCH_CONFIG_OPEN,
>        "Add/remove tags for all messages matching the search terms." },
> -    { "dump", notmuch_dump_command, FALSE,
> +    { "dump", notmuch_dump_command, NOTMUCH_CONFIG_OPEN,
>        "Create a plain-text dump of the tags for each message." },
> -    { "restore", notmuch_restore_command, FALSE,
> +    { "restore", notmuch_restore_command, NOTMUCH_CONFIG_OPEN,
>        "Restore the tags from the given dump file (see 'dump')." },
> -    { "compact", notmuch_compact_command, FALSE,
> +    { "compact", notmuch_compact_command, NOTMUCH_CONFIG_OPEN,
>        "Compact the notmuch database." },
> -    { "config", notmuch_config_command, FALSE,
> +    { "config", notmuch_config_command, NOTMUCH_CONFIG_OPEN,
>        "Get or set settings in the notmuch configuration file." },
> -    { "help", notmuch_help_command, TRUE, /* create but don't save config */
> +    { "help", notmuch_help_command, NOTMUCH_CONFIG_CREATE, /* create but don't save config */
>        "This message, or more detailed help for the named command." }
>  };
>  
> @@ -447,7 +447,7 @@ main (int argc, char *argv[])
>  	goto DONE;
>      }
>  
> -    config = notmuch_config_open (local, config_file_name, command->create_config);
> +    config = notmuch_config_open (local, config_file_name, command->config_mode);
>      if (!config) {
>  	ret = EXIT_FAILURE;
>  	goto DONE;
> -- 
> 2.11.0

Re: [PATCH 1/2] cli/config: don't try to open config file for 'notmuch help'

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> The help command does not really need to try to open the config
> file. So don't.

series pushed to master