[PATCH 1/2] cli: fix notmuch top level argument parsing

[PATCH 1/2] cli: fix notmuch top level argument parsing

[Jani Nikula]

Use strcmp instead of STRNCMP_LITERAL, which matches the prefix
instead of the whole argument.
---
 notmuch.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/notmuch.c b/notmuch.c
index 477a09c..4ff66e3 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -245,10 +245,10 @@ main (int argc, char *argv[])
     if (argc == 1)
 	return notmuch (local);
 
-    if (STRNCMP_LITERAL (argv[1], "--help") == 0)
+    if (strcmp (argv[1], "--help") == 0)
 	return notmuch_help_command (NULL, argc - 1, &argv[1]);
 
-    if (STRNCMP_LITERAL (argv[1], "--version") == 0) {
+    if (strcmp (argv[1], "--version") == 0) {
 	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
 	return 0;
     }
-- 
1.7.10.4

[PATCH 2/2] cli: convert "notmuch new" to the argument parser

[Jani Nikula]

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 },
+	{ 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;
-- 
1.7.10.4

Re: [PATCH 1/2] cli: fix notmuch top level argument parsing

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Mon, Dec 03 2012, Jani Nikula wrote:
> Use strcmp instead of STRNCMP_LITERAL, which matches the prefix
> instead of the whole argument.

Perhaps add and use this instead:

#define STRCMP_LITERAL(var, literal) \
    strncmp ((var), (literal), sizeof (literal))

Than again, it's argument parsing so hardly a performance critical path,
so maybe readability is more important.

> ---
>  notmuch.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/notmuch.c b/notmuch.c
> index 477a09c..4ff66e3 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -245,10 +245,10 @@ main (int argc, char *argv[])
>      if (argc == 1)
>  	return notmuch (local);
>  
> -    if (STRNCMP_LITERAL (argv[1], "--help") == 0)
> +    if (strcmp (argv[1], "--help") == 0)
>  	return notmuch_help_command (NULL, argc - 1, &argv[1]);
>  
> -    if (STRNCMP_LITERAL (argv[1], "--version") == 0) {
> +    if (strcmp (argv[1], "--version") == 0) {
>  	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
>  	return 0;
>      }

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 1/2] cli: fix notmuch top level argument parsing

[Jani Nikula]

On Mon, 03 Dec 2012, Michal Nazarewicz <mina86@mina86.com> wrote:
> On Mon, Dec 03 2012, Jani Nikula wrote:
>> Use strcmp instead of STRNCMP_LITERAL, which matches the prefix
>> instead of the whole argument.
>
> Perhaps add and use this instead:
>
> #define STRCMP_LITERAL(var, literal) \
>     strncmp ((var), (literal), sizeof (literal))

That's broken the same way STRNCMP_LITERAL is broken in this use case:
it matches if literal is a prefix of var.

BR,
Jani.


>
> Than again, it's argument parsing so hardly a performance critical path,
> so maybe readability is more important.
>
>> ---
>>  notmuch.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/notmuch.c b/notmuch.c
>> index 477a09c..4ff66e3 100644
>> --- a/notmuch.c
>> +++ b/notmuch.c
>> @@ -245,10 +245,10 @@ main (int argc, char *argv[])
>>      if (argc == 1)
>>  	return notmuch (local);
>>  
>> -    if (STRNCMP_LITERAL (argv[1], "--help") == 0)
>> +    if (strcmp (argv[1], "--help") == 0)
>>  	return notmuch_help_command (NULL, argc - 1, &argv[1]);
>>  
>> -    if (STRNCMP_LITERAL (argv[1], "--version") == 0) {
>> +    if (strcmp (argv[1], "--version") == 0) {
>>  	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
>>  	return 0;
>>      }
>
> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

Re: [PATCH 2/2] cli: convert "notmuch new" to the argument parser

[Austin Clements]

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;

Re: [PATCH 1/2] cli: fix notmuch top level argument parsing

[Jani Nikula]

On Tue, 04 Dec 2012, Jani Nikula <jani@nikula.org> wrote:
> On Mon, 03 Dec 2012, Michal Nazarewicz <mina86@mina86.com> wrote:
>> On Mon, Dec 03 2012, Jani Nikula wrote:
>>> Use strcmp instead of STRNCMP_LITERAL, which matches the prefix
>>> instead of the whole argument.
>>
>> Perhaps add and use this instead:
>>
>> #define STRCMP_LITERAL(var, literal) \
>>     strncmp ((var), (literal), sizeof (literal))
>
> That's broken the same way STRNCMP_LITERAL is broken in this use case:
> it matches if literal is a prefix of var.

Err, that's bollocks. Missed the missing -1 there. ;)

>> Than again, it's argument parsing so hardly a performance critical path,
>> so maybe readability is more important.

I'll go with this. And I'm not even sure strncmp is faster than strcmp,
as it has to keep track of count.


BR,
Jani.



>>
>>> ---
>>>  notmuch.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/notmuch.c b/notmuch.c
>>> index 477a09c..4ff66e3 100644
>>> --- a/notmuch.c
>>> +++ b/notmuch.c
>>> @@ -245,10 +245,10 @@ main (int argc, char *argv[])
>>>      if (argc == 1)
>>>  	return notmuch (local);
>>>  
>>> -    if (STRNCMP_LITERAL (argv[1], "--help") == 0)
>>> +    if (strcmp (argv[1], "--help") == 0)
>>>  	return notmuch_help_command (NULL, argc - 1, &argv[1]);
>>>  
>>> -    if (STRNCMP_LITERAL (argv[1], "--version") == 0) {
>>> +    if (strcmp (argv[1], "--version") == 0) {
>>>  	printf ("notmuch " STRINGIFY(NOTMUCH_VERSION) "\n");
>>>  	return 0;
>>>      }
>>
>> -- 
>> Best regards,                                         _     _
>> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
>> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
>> ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

Re: [PATCH 2/2] cli: convert "notmuch new" to the argument parser

[Jani Nikula]

On Tue, 04 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> -    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.

Agreed, although I believe we'd have to review all of the single
character arguments before actually enabling any of them. At least my
choice of them has been rather whimsical. I'll leave this up to David to
decide.

BR,
Jani.

Re: [PATCH 1/2] cli: fix notmuch top level argument parsing

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Use strcmp instead of STRNCMP_LITERAL, which matches the prefix
> instead of the whole argument.

pushed both, 

d

Re: [PATCH 1/2] cli: fix notmuch top level argument parsing

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

On Mon, Dec 03 2012, Jani Nikula wrote:
> And I'm not even sure strncmp is faster than strcmp, as it has to keep
> track of count.

Good point.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]