Date: Mon, 3 Dec 2012 17:28:44 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH 2/2] cli: convert "notmuch new" to the argument parser
Message-ID: <20121203222844.GH1020@mit.edu>
References:
 <c4f2a1048763950dd14e13c99a4e7eb333be8c9d.1354568072.git.jani@nikula.org>
	<e31ac6b2ceed533f8ac5386a010f4c052e09502e.1354568072.git.jani@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <e31ac6b2ceed533f8ac5386a010f4c052e09502e.1354568072.git.jani@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

LGTM.  One minor nit below, but feel free to ignore it.

Quoth Jani Nikula on Dec 03 at 10:56 pm:
> Use the notmuch argument parser to handle arguments in "notmuch
> new". As a side effect, this fixes broken STRNCMP_LITERAL usage that
> accepts, for example, --verbosefoo for --verbose.
> ---
>  notmuch-new.c |   38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 718a538..feb9c32 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -36,8 +36,8 @@ typedef struct _filename_list {
>  
>  typedef struct {
>      int output_is_a_tty;
> -    int verbose;
> -    int debug;
> +    notmuch_bool_t verbose;
> +    notmuch_bool_t debug;
>      const char **new_tags;
>      size_t new_tags_length;
>      const char **new_ignore;
> @@ -853,28 +853,28 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>      char *dot_notmuch_path;
>      struct sigaction action;
>      _filename_node_t *f;
> +    int opt_index;
>      int i;
>      notmuch_bool_t timer_is_active = FALSE;
> -    notmuch_bool_t run_hooks = TRUE;
> +    notmuch_bool_t no_hooks = FALSE;
>  
> -    add_files_state.verbose = 0;
> -    add_files_state.debug = 0;
> +    add_files_state.verbose = FALSE;
> +    add_files_state.debug = FALSE;
>      add_files_state.output_is_a_tty = isatty (fileno (stdout));
>  
> -    argc--; argv++; /* skip subcommand argument */
> +    notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.verbose, "verbose", 'v', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },

Not that the single character arguments matter right now, but '-n' is
often used for "dry-run", which could actually apply to notmuch new.

> +	{ 0, 0, 0, 0, 0 }
> +    };
>  
> -    for (i = 0; i < argc && argv[i][0] == '-'; i++) {
> -	if (STRNCMP_LITERAL (argv[i], "--verbose") == 0) {
> -	    add_files_state.verbose = 1;
> -	} else if (strcmp (argv[i], "--debug") == 0) {
> -	    add_files_state.debug = 1;
> -	} else if (strcmp (argv[i], "--no-hooks") == 0) {
> -	    run_hooks = FALSE;
> -	} else {
> -	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
> -	    return 1;
> -	}
> +    opt_index = parse_arguments (argc, argv, options, 1);
> +    if (opt_index < 0) {
> +	/* diagnostics already printed */
> +	return 1;
>      }
> +
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
>  	return 1;
> @@ -884,7 +884,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>      add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>      db_path = notmuch_config_get_database_path (config);
>  
> -    if (run_hooks) {
> +    if (!no_hooks) {
>  	ret = notmuch_run_hook (db_path, "pre-new");
>  	if (ret)
>  	    return ret;
> @@ -1045,7 +1045,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      notmuch_database_destroy (notmuch);
>  
> -    if (run_hooks && !ret && !interrupted)
> +    if (!no_hooks && !ret && !interrupted)
>  	ret = notmuch_run_hook (db_path, "post-new");
>  
>      return ret || interrupted;