[PATCH 0/9] argument parsing fixes and improvements

[PATCH 0/9] argument parsing fixes and improvements

[Jani Nikula]

I had some other things in mind, but ended up with this prep series
instead... The main thing is patch 6 adding --no-argument style negating
arguments for boolean and keyword flag args. The rest is mostly just
refactoring and tests to make that happen.

BR,
Jani.

Jani Nikula (9):
  hex-xcode: use notmuch_bool_t for boolean arguments
  cli: use notmuch_bool_t for boolean argument in show
  cli: refactor boolean argument processing
  cli: change while to for in keyword argument processing
  cli: reduce indent in keyword argument processing
  cli: add support for --no- prefixed boolean and keyword flag arguments
  cli: use the negating boolean support for new and insert --no-hooks
  test: add boolean argument to arg-test
  test: expand argument parsing sanity checks

 command-line-arguments.c      | 93 ++++++++++++++++++++++++++++---------------
 notmuch-insert.c              |  6 +--
 notmuch-new.c                 |  8 ++--
 notmuch-show.c                |  2 +-
 test/T410-argument-parsing.sh | 40 ++++++++++++++++++-
 test/arg-test.c               |  5 +++
 test/hex-xcode.c              |  2 +-
 7 files changed, 113 insertions(+), 43 deletions(-)

-- 
2.11.0

[PATCH 1/9] hex-xcode: use notmuch_bool_t for boolean arguments

[Jani Nikula]

Pedantically correct, although they're the same underlying type.
---
 test/hex-xcode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/hex-xcode.c b/test/hex-xcode.c
index 65d49564a3e1..28c22e06b69e 100644
--- a/test/hex-xcode.c
+++ b/test/hex-xcode.c
@@ -45,7 +45,7 @@ main (int argc, char **argv)
 {
 
     enum direction dir = DECODE;
-    int omit_newline = FALSE;
+    notmuch_bool_t omit_newline = FALSE;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &dir, "direction", 'd',
-- 
2.11.0

[PATCH 2/9] cli: use notmuch_bool_t for boolean argument in show

[Jani Nikula]

Pedantically correct, although they're the same underlying type.
---
 notmuch-show.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index cdcc2a982bd9..9d1c3ef040c6 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1085,7 +1085,7 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	.output_body = TRUE,
     };
     int format = NOTMUCH_FORMAT_NOT_SPECIFIED;
-    int exclude = TRUE;
+    notmuch_bool_t exclude = TRUE;
 
     /* This value corresponds to neither true nor false being passed
      * on the command line */
-- 
2.11.0

[PATCH 3/9] cli: refactor boolean argument processing

[Jani Nikula]

Clean up the control flow to prepare for future changes. No functional
changes.
---
 command-line-arguments.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index dc517b06ff60..7fd58165278f 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -41,21 +41,20 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 
 static notmuch_bool_t
 _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
-
-    if (next == '\0') {
-	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
-	return TRUE;
-    }
-    if (strcmp (arg_str, "false") == 0) {
-	*((notmuch_bool_t *)arg_desc->output_var) = FALSE;
-	return TRUE;
-    }
-    if (strcmp (arg_str, "true") == 0) {
-	*((notmuch_bool_t *)arg_desc->output_var) = TRUE;
-	return TRUE;
+    notmuch_bool_t value;
+
+    if (next == '\0' || strcmp (arg_str, "true") == 0) {
+	value = TRUE;
+    } else if (strcmp (arg_str, "false") == 0) {
+	value = FALSE;
+    } else {
+	fprintf (stderr, "Unknown argument \"%s\" for (boolean) option \"%s\".\n", arg_str, arg_desc->name);
+	return FALSE;
     }
-    fprintf (stderr, "Unknown argument \"%s\" for (boolean) option \"%s\".\n", arg_str, arg_desc->name);
-    return FALSE;
+
+    *((notmuch_bool_t *)arg_desc->output_var) = value;
+
+    return TRUE;
 }
 
 static notmuch_bool_t
-- 
2.11.0

[PATCH 4/9] cli: change while to for in keyword argument processing

[Jani Nikula]

Using a for loop makes it easier to use continue, in preparation for
future changes. No functional changes.
---
 command-line-arguments.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 7fd58165278f..1373dbbb5225 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -13,14 +13,14 @@
 static notmuch_bool_t
 _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
-    const notmuch_keyword_t *keywords = arg_desc->keywords;
+    const notmuch_keyword_t *keywords;
 
     if (next == '\0') {
 	/* No keyword given */
 	arg_str = "";
     }
 
-    while (keywords->name) {
+    for (keywords = arg_desc->keywords; keywords->name; keywords++) {
 	if (strcmp (arg_str, keywords->name) == 0) {
 	    if (arg_desc->output_var) {
 		if (arg_desc->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS)
@@ -30,7 +30,6 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	    }
 	    return TRUE;
 	}
-	keywords++;
     }
     if (next != '\0')
 	fprintf (stderr, "Unknown keyword argument \"%s\" for option \"%s\".\n", arg_str, arg_desc->name);
-- 
2.11.0

[PATCH 5/9] cli: reduce indent in keyword argument processing

[Jani Nikula]

Reducing indent makes future changes easier. No functional changes.
---
 command-line-arguments.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index 1373dbbb5225..a79afcaf8a15 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -21,15 +21,17 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
     }
 
     for (keywords = arg_desc->keywords; keywords->name; keywords++) {
-	if (strcmp (arg_str, keywords->name) == 0) {
-	    if (arg_desc->output_var) {
-		if (arg_desc->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS)
-		    *((int *)arg_desc->output_var) |= keywords->value;
-		else
-		    *((int *)arg_desc->output_var) = keywords->value;
-	    }
-	    return TRUE;
+	if (strcmp (arg_str, keywords->name) != 0)
+	    continue;
+
+	if (arg_desc->output_var) {
+	    if (arg_desc->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS)
+		*((int *)arg_desc->output_var) |= keywords->value;
+	    else
+		*((int *)arg_desc->output_var) = keywords->value;
 	}
+
+	return TRUE;
     }
     if (next != '\0')
 	fprintf (stderr, "Unknown keyword argument \"%s\" for option \"%s\".\n", arg_str, arg_desc->name);
-- 
2.11.0

[PATCH 6/9] cli: add support for --no- prefixed boolean and keyword flag arguments

[Jani Nikula]

Add transparent support for negating boolean and keyword flag
arguments using --no-argument style on the command line. That is, if
the option description contains a boolean or a keyword flag argument
named "argument", --no-argument will match and negate it.

For boolean arguments this obviously means the logical NOT. For
keyword flag arguments this means bitwise AND of the bitwise NOT,
i.e. masking out the specified bits instead of OR'ing them in.

For example, you can use --no-exclude instead of --exclude=false in
notmuch show. If we had keyword flag arguments with some flags
defaulting to on, say --include=tags in notmuch dump/restore, this
would allow --no-include=tags to switch that off while not affecting
other flags.

As a curiosity, you should be able to warp your brain using
--no-exclude=true meaning false and --no-exclude=false meaning true if
you wish.

Specifying both "argument" and "no-argument" style arguments in the
same option description should be avoided. In this case, --no-argument
would match whichever is specified first, and --argument would only
match "argument".
---
 command-line-arguments.c | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index a79afcaf8a15..d61d345285b9 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -11,7 +11,8 @@
 */
 
 static notmuch_bool_t
-_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+_process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next,
+		      const char *arg_str, notmuch_bool_t negate) {
 
     const notmuch_keyword_t *keywords;
 
@@ -25,7 +26,9 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	    continue;
 
 	if (arg_desc->output_var) {
-	    if (arg_desc->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS)
+	    if (arg_desc->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS && negate)
+		*((int *)arg_desc->output_var) &= ~keywords->value;
+	    else if (arg_desc->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS)
 		*((int *)arg_desc->output_var) |= keywords->value;
 	    else
 		*((int *)arg_desc->output_var) = keywords->value;
@@ -41,7 +44,8 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 }
 
 static notmuch_bool_t
-_process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
+_process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next,
+		      const char *arg_str, notmuch_bool_t negate) {
     notmuch_bool_t value;
 
     if (next == '\0' || strcmp (arg_str, "true") == 0) {
@@ -53,7 +57,7 @@ _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next, const char
 	return FALSE;
     }
 
-    *((notmuch_bool_t *)arg_desc->output_var) = value;
+    *((notmuch_bool_t *)arg_desc->output_var) = negate ? !value : value;
 
     return TRUE;
 }
@@ -116,6 +120,8 @@ parse_position_arg (const char *arg_str, int pos_arg_index,
     return FALSE;
 }
 
+#define NEGATIVE_PREFIX "no-"
+
 /*
  * Search for a non-positional (i.e. starting with --) argument matching arg,
  * parse a possible value, and assign to *output_var
@@ -132,6 +138,14 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
     assert(options);
 
     const char *arg = _arg + 2; /* _arg starts with -- */
+    const char *negative_arg = NULL;
+
+    /* See if this is a --no-argument */
+    if (strlen (arg) > strlen (NEGATIVE_PREFIX) &&
+	strncmp (arg, NEGATIVE_PREFIX, strlen (NEGATIVE_PREFIX)) == 0) {
+	negative_arg = arg + strlen (NEGATIVE_PREFIX);
+    }
+
     const notmuch_opt_desc_t *try;
 
     const char *next_arg = NULL;
@@ -148,11 +162,24 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
 	if (! try->name)
 	    continue;
 
-	if (strncmp (arg, try->name, strlen (try->name)) != 0)
+	char next;
+	const char *value;
+	notmuch_bool_t negate = FALSE;
+
+	if (strncmp (arg, try->name, strlen (try->name)) == 0) {
+	    next = arg[strlen (try->name)];
+	    value = arg + strlen (try->name) + 1;
+	} else if (negative_arg &&
+		   strncmp (negative_arg, try->name, strlen (try->name)) == 0 &&
+		   (try->opt_type == NOTMUCH_OPT_BOOLEAN ||
+		    try->opt_type == NOTMUCH_OPT_KEYWORD_FLAGS)) {
+	    next = negative_arg[strlen (try->name)];
+	    value = negative_arg + strlen (try->name) + 1;
+	    /* The argument part of --no-argument matches, negate the result. */
+	    negate = TRUE;
+	} else {
 	    continue;
-
-	char next = arg[strlen (try->name)];
-	const char *value = arg + strlen(try->name) + 1;
+	}
 
 	/*
 	 * If we have not reached the end of the argument (i.e. the
@@ -176,10 +203,10 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
 	switch (try->opt_type) {
 	case NOTMUCH_OPT_KEYWORD:
 	case NOTMUCH_OPT_KEYWORD_FLAGS:
-	    opt_status = _process_keyword_arg (try, next, value);
+	    opt_status = _process_keyword_arg (try, next, value, negate);
 	    break;
 	case NOTMUCH_OPT_BOOLEAN:
-	    opt_status = _process_boolean_arg (try, next, value);
+	    opt_status = _process_boolean_arg (try, next, value, negate);
 	    break;
 	case NOTMUCH_OPT_INT:
 	    opt_status = _process_int_arg (try, next, value);
-- 
2.11.0

[PATCH 7/9] cli: use the negating boolean support for new and insert --no-hooks

[Jani Nikula]

This lets us use the positive hooks variable in code, increasing
clarity.
---
 notmuch-insert.c | 6 +++---
 notmuch-new.c    | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 648bd944a7b1..f0a3a33812a8 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -455,7 +455,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
     notmuch_bool_t keep = FALSE;
-    notmuch_bool_t no_hooks = FALSE;
+    notmuch_bool_t hooks = TRUE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
@@ -466,7 +466,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ 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_BOOLEAN,  &hooks, "hooks", 'n', 0 },
 	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
@@ -575,7 +575,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	}
     }
 
-    if (! no_hooks && status == NOTMUCH_STATUS_SUCCESS) {
+    if (hooks && status == NOTMUCH_STATUS_SUCCESS) {
 	/* Ignore hook failures. */
 	notmuch_run_hook (db_path, "post-insert");
     }
diff --git a/notmuch-new.c b/notmuch-new.c
index faeb8f0a5896..cffecf745f1d 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -948,7 +948,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     int opt_index;
     unsigned int i;
     notmuch_bool_t timer_is_active = FALSE;
-    notmuch_bool_t no_hooks = FALSE;
+    notmuch_bool_t hooks = TRUE;
     notmuch_bool_t quiet = FALSE, verbose = FALSE;
     notmuch_status_t status;
 
@@ -956,7 +956,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN,  &quiet, "quiet", 'q', 0 },
 	{ 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_BOOLEAN,  &hooks, "hooks", 'n', 0 },
 	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
@@ -989,7 +989,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	}
     }
 
-    if (!no_hooks) {
+    if (hooks) {
 	ret = notmuch_run_hook (db_path, "pre-new");
 	if (ret)
 	    return EXIT_FAILURE;
@@ -1154,7 +1154,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_database_destroy (notmuch);
 
-    if (!no_hooks && !ret && !interrupted)
+    if (hooks && !ret && !interrupted)
 	ret = notmuch_run_hook (db_path, "post-new");
 
     if (ret || interrupted)
-- 
2.11.0

[PATCH 8/9] test: add boolean argument to arg-test

[Jani Nikula]

Surprisingly it's not there.
---
 test/T410-argument-parsing.sh | 3 ++-
 test/arg-test.c               | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/test/T410-argument-parsing.sh b/test/T410-argument-parsing.sh
index fad134e305c5..4505c58301ea 100755
--- a/test/T410-argument-parsing.sh
+++ b/test/T410-argument-parsing.sh
@@ -3,8 +3,9 @@ test_description="argument parsing"
 . ./test-lib.sh || exit 1
 
 test_begin_subtest "sanity check"
-$TEST_DIRECTORY/arg-test  pos1  --keyword=one --string=foo pos2 --int=7 --flag=one --flag=three > OUTPUT
+$TEST_DIRECTORY/arg-test  pos1  --keyword=one --boolean --string=foo pos2 --int=7 --flag=one --flag=three > OUTPUT
 cat <<EOF > EXPECTED
+boolean 1
 keyword 1
 flags 5
 int 7
diff --git a/test/arg-test.c b/test/arg-test.c
index 736686ded2c0..2f839c992c56 100644
--- a/test/arg-test.c
+++ b/test/arg-test.c
@@ -12,8 +12,10 @@ int main(int argc, char **argv){
     char *pos_arg1=NULL;
     char *pos_arg2=NULL;
     char *string_val=NULL;
+    notmuch_bool_t bool_val = FALSE;
 
     notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_BOOLEAN, &bool_val, "boolean", 'b', 0},
 	{ NOTMUCH_OPT_KEYWORD, &kw_val, "keyword", 'k',
 	  (notmuch_keyword_t []){ { "one", 1 },
 				  { "two", 2 },
@@ -34,6 +36,9 @@ int main(int argc, char **argv){
     if (opt_index < 0)
 	return 1;
 
+    if (bool_val)
+	printf("boolean %d\n", bool_val);
+
     if (kw_val)
 	printf("keyword %d\n", kw_val);
 
-- 
2.11.0

[PATCH 9/9] test: expand argument parsing sanity checks

[Jani Nikula]

Test the various boolean formats, --no- prefixed boolean and keyword
flag arguments, and space between option and option argument.
---
 test/T410-argument-parsing.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/test/T410-argument-parsing.sh b/test/T410-argument-parsing.sh
index 4505c58301ea..f3094e1e741a 100755
--- a/test/T410-argument-parsing.sh
+++ b/test/T410-argument-parsing.sh
@@ -15,4 +15,41 @@ positional arg 2 pos2
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "--boolean=true"
+$TEST_DIRECTORY/arg-test --boolean=true > OUTPUT
+cat <<EOF > EXPECTED
+boolean 1
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "--boolean=false"
+$TEST_DIRECTORY/arg-test --boolean=false > OUTPUT
+cat <<EOF > EXPECTED
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "--no-boolean"
+$TEST_DIRECTORY/arg-test --no-boolean > OUTPUT
+cat <<EOF > EXPECTED
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "--no-flag"
+$TEST_DIRECTORY/arg-test --flag=one --flag=three --no-flag=three > OUTPUT
+cat <<EOF > EXPECTED
+flags 1
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "--param SPC arg"
+$TEST_DIRECTORY/arg-test --keyword two --no-boolean --string foo pos2 --int 7 --flag one --flag three > OUTPUT
+cat <<EOF > EXPECTED
+keyword 2
+flags 5
+int 7
+string foo
+positional arg 1 pos2
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.11.0

Re: [PATCH 0/9] argument parsing fixes and improvements

[Daniel Kahn Gillmor]

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

On Tue 2017-09-19 23:39:20 +0300, Jani Nikula wrote:
> I had some other things in mind, but ended up with this prep series
> instead... The main thing is patch 6 adding --no-argument style negating
> arguments for boolean and keyword flag args. The rest is mostly just
> refactoring and tests to make that happen.

this series looks like it does what it says on the label to me, though i
haven't tested it.

I'm not sure that the ux improvement is particularly significant,
though.  Is there a concrete advantage of offering --no-foo on in
addition to an already-present --foo=false ?  do we *want* there to be
many ways to achieve the same result in the cli?

I'm a little concerned that "notmuch search --exclude" is a corner case
that doesn't make a lot of sense here.

It's documented as:

          --exclude=(true|false|all|flag)
                 A message is called "excluded" if it matches at least one tag
                 in  search.tag_exclude that does not appear explicitly in the
                 search terms. This option specifies whether to omit  excluded
                 messages in the search process.

                 The  default  value,  true,  prevents  excluded messages from
                 matching the search terms.

                 all additionally prevents excluded messages from appearing in
                 displayed  results, in effect behaving as though the excluded
                 messages do not exist.

                 false allows excluded messages  to  match  search  terms  and
                 appear  in  displayed  results.  Excluded  messages are still
                 marked in the relevant outputs.

                 flag only has an effect when --output=summary. The output  is
                 almost  identical to false, but the "match count" is the num‐
                 ber of matching non-excluded messages in the  thread,  rather
                 than the number of matching messages.


It's "not quite" a boolean.  --exclude is mentioned in patch 6, but
isn't included here.

It's a NOTMUCH_OPT_KEYWORD, but it's not a flag, so the idea of a
bitwise boolean doesn't make sense for it.  i guess you just can't
"notmuch search --no-exclude=" then?


Also, this series should update the bash bindings so that the --no-
arguments are available.  And i think the documentation for the cli
should also be updated to mention --no-foo, right?

Regards,

        --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/9] argument parsing fixes and improvements

[Jani Nikula]

On Wed, Sep 20, 2017 at 12:43 AM, Daniel Kahn Gillmor
<dkg@fifthhorseman.net> wrote:
> On Tue 2017-09-19 23:39:20 +0300, Jani Nikula wrote:
>> I had some other things in mind, but ended up with this prep series
>> instead... The main thing is patch 6 adding --no-argument style negating
>> arguments for boolean and keyword flag args. The rest is mostly just
>> refactoring and tests to make that happen.
>
> this series looks like it does what it says on the label to me, though i
> haven't tested it.

Thanks for the review.

> I'm not sure that the ux improvement is particularly significant,
> though.  Is there a concrete advantage of offering --no-foo on in
> addition to an already-present --foo=false ?  do we *want* there to be
> many ways to achieve the same result in the cli?

Those are good questions, and up for debate. I was more interested in
getting the mask out behaviour for keyword flag arguments, and the
boolean part came practically free. Or, there was certain synergy in
having them behave similarly.

This started out as I was pondering the notmuch show options. There's
a hodgepodge of options to control the output: --entire-thread,
--body, --include-html. I'd prefer a --output=foo parameter to choose
stuff, like in notmuch address, but also with a way to disable default
enabled outputs. So you could combine, say, --output=entire-thread
--no-output=body --output=html. If we ever need to add even one more
output option to notmuch show, I think this'll be worth it. Even if we
have to retain the old stuff for backwards compatibility.

Now, I don't know if that's the best way to do this. Perhaps people
would prefer --output=!entire-thread or --output=no-entire-thread or
--output=entire-thread-disable or something. But the --no- prefix at
least has precedence in other software for booleans, and I think
that's a good reason to consider it. And having patches on the list
doing it one way generates better feedback and discussion than just
general musing on IRC.

I don't recall if we considered comma-separated values for multiple
outputs in e.g. notmuch address, say --output=sender,recipients but we
ended up with separate --output=sender --output=recipients instead.
Not sure if that's relevant in this discussion... perhaps if we want
to support that in the future. (Do we?)

> I'm a little concerned that "notmuch search --exclude" is a corner case
> that doesn't make a lot of sense here.

That was an unfortunate choice for an example. notmuch show --exclude
is a normal boolean option I was looking at. The search --exclude
option shouldn't be discussed as an example for anything. ;)

> Also, this series should update the bash bindings so that the --no-
> arguments are available.  And i think the documentation for the cli
> should also be updated to mention --no-foo, right?

Of course. But given that there's so much bikeshedding potential, I
decided to postpone that part. :) We also have mixed documentation of
boolean options as it is. Some just have --foo, others have
--foo=(true|false). Perhaps we use the former for default false
options, and the latter for default true options, but it's still a bit
inconsistent. With --no-foo support, we could document the default
true options with that, and it would convey the default as a bonus.

BR,
Jani.

Re: [PATCH 0/9] argument parsing fixes and improvements

[Daniel Kahn Gillmor]

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

Hi Jani--

On Wed 2017-09-20 12:16:33 +0300, Jani Nikula wrote:
> Those are good questions, and up for debate. I was more interested in
> getting the mask out behaviour for keyword flag arguments, and the
> boolean part came practically free. Or, there was certain synergy in
> having them behave similarly.

gotcha, that motivation makes sense, thanks!

> Now, I don't know if that's the best way to do this. Perhaps people
> would prefer --output=!entire-thread or --output=no-entire-thread or
> --output=entire-thread-disable or something. But the --no- prefix at
> least has precedence in other software for booleans, and I think
> that's a good reason to consider it. And having patches on the list
> doing it one way generates better feedback and discussion than just
> general musing on IRC.

yup.  fwiw, if we want negations like this, i'm fine with the --no-
prefix as you've proposed.

>> I'm a little concerned that "notmuch search --exclude" is a corner case
>> that doesn't make a lot of sense here.
>
> That was an unfortunate choice for an example. notmuch show --exclude
> is a normal boolean option I was looking at. The search --exclude
> option shouldn't be discussed as an example for anything. ;)

*you* didn't use "notmuch search --exclude" -- i was the one who brought
it up because i've been looking at argument parsing recently and it
stuck out as a bit of a weirdo.  It's too bad that it doesn't fit into
this framework, but meh -- a foolish consistency is the hobgoblin of
small minds.

> Of course. But given that there's so much bikeshedding potential, I
> decided to postpone that part. :) We also have mixed documentation of
> boolean options as it is. Some just have --foo, others have
> --foo=(true|false). Perhaps we use the former for default false
> options, and the latter for default true options, but it's still a bit
> inconsistent. With --no-foo support, we could document the default
> true options with that, and it would convey the default as a bonus.

right, about the defaults: as i'm sure you're aware, i'm trying to
introduce some boolean options whose default is "unset", meaning "do
whatever is set in the database config".  In particular, --try-decrypt
(for those subcommands which do indexing).  As you can see in
id:20170912230153.4175-8-dkg@fifthhorseman.net, i handle this by
declaring the internal variable as:

   int try_decrypt = -1;

And then i decide to act on it with:

   if (try_decrypt == TRUE || try_decrypt = FALSE) {
      /* act on it… */
   }

Otherwise, i invoke the internal functions and let them behave as the
database default does.

This sort of goes against the trend of your
id:5efab1e6d2250d87c909d9e5b435da5235efdc84.1505853159.git.jani@nikula.org,
which prefers to use notmuch_bool_t for those declarations (and is maybe
heading in the direction of stdbool.h).  How do you think we should
implement a future boolean flag whose default value is "unset" if we
head in this direction?

          --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/9] argument parsing fixes and improvements

[Jani Nikula]

On Wed, 20 Sep 2017, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> right, about the defaults: as i'm sure you're aware, i'm trying to
> introduce some boolean options whose default is "unset", meaning "do
> whatever is set in the database config".  In particular, --try-decrypt
> (for those subcommands which do indexing).  As you can see in
> id:20170912230153.4175-8-dkg@fifthhorseman.net, i handle this by
> declaring the internal variable as:
>
>    int try_decrypt = -1;
>
> And then i decide to act on it with:
>
>    if (try_decrypt == TRUE || try_decrypt = FALSE) {
>       /* act on it… */
>    }
>
> Otherwise, i invoke the internal functions and let them behave as the
> database default does.

I think in general having stateful defaults are bad. See the code or
manual page for notmuch show. See how many defaults depend on some
*other* option, for absolutely no good reason. If you play with the
interface, you'll be surprised many times over how changing one knob
changes another as well. For example, switch output format, you'll get
entire threads as well. If you don't specify output format, choosing a
part will change the output format. Some unsupported combos result in
errors, some in warnings. It's a mess.

So I think I'd prefer either strict booleans where the default is one or
the other, or an explicit tristate true|false|default. In your case,
perhaps --try-decrypt=(true|false|database), where the default can be
requested and clearly documented. And that's again not a bool, like
notmuch search --exclude, but then neither is your proposed option. It
has three options in reality, but you've hidden one.

> This sort of goes against the trend of your
> id:5efab1e6d2250d87c909d9e5b435da5235efdc84.1505853159.git.jani@nikula.org,
> which prefers to use notmuch_bool_t for those declarations (and is maybe
> heading in the direction of stdbool.h).  How do you think we should
> implement a future boolean flag whose default value is "unset" if we
> head in this direction?

Indeed the direction is standard bools. I really see no reason to use
notmuch_bool_t except in the library interface for historical
reasons. Both sides of the interface could use stdbool.

I'm sure we could modify the argument parser to provide information
about options present and not, but given the above, I'm leaning towards
making the values explicit instead.

Further reading on orthogonality [1].

BR,
Jani.


[1] http://www.catb.org/esr/writings/taoup/html/ch04s02.html#orthogonality

Re: [PATCH 0/9] argument parsing fixes and improvements

[Daniel Kahn Gillmor]

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

On Thu 2017-09-21 20:07:30 +0300, Jani Nikula wrote:
> So I think I'd prefer either strict booleans where the default is one or
> the other, or an explicit tristate true|false|default. In your case,
> perhaps --try-decrypt=(true|false|database), where the default can be
> requested and clearly documented. And that's again not a bool, like
> notmuch search --exclude, but then neither is your proposed option. It
> has three options in reality, but you've hidden one.

being able to set a default in the database is definitely useful, rather
than having to find all the places that you (or some other tool) might
be invoking a notmuch indexing subcommand.

If you want to make it an explicit third variant as stated above, that's
fine, but then we don't get --no-try-decrypt from your series here,
which is itself a different kind of interface inconsistency. :/

I have plans for a few more boolean variables like this for indexing
that will store their defaults in the database.

Perhaps we should introduce a new tristate (true|false|database) type to
make that simpler to deal with internally, but could also implement
--no- prefixes?  If that was something you're interested in providing,
i'd be fine adjusting my patches to make use of it.

      --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/9] argument parsing fixes and improvements

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Wed, 20 Sep 2017, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
>> right, about the defaults: as i'm sure you're aware, i'm trying to
>> introduce some boolean options whose default is "unset", meaning "do
>> whatever is set in the database config".  In particular, --try-decrypt
>> (for those subcommands which do indexing).  As you can see in
>> id:20170912230153.4175-8-dkg@fifthhorseman.net, i handle this by
>> declaring the internal variable as:
>>
>>    int try_decrypt = -1;
>>
>> And then i decide to act on it with:
>>
>>    if (try_decrypt == TRUE || try_decrypt = FALSE) {
>>       /* act on it… */
>>    }
>>
>> Otherwise, i invoke the internal functions and let them behave as the
>> database default does.

I think there is two different discussions one could be having here; one
about the UI, the other about the implimentation.

From the UI point of view, it seems like the best thing is to use any
configuration to set the default for a given boolean flag. Conceptually
this would look something like (semi-pseudo-code)

    try_decrypt = false;
    
    notmuch_database_get_config(notmuch, "try_decrypt", &try_decrypt);

    parse_arguments(argc, argv, ...)

We have 3 possibilities, with the latest specified one winning.

In the implmentation, we need to cope with the fact that the database
probably can't be opened until after the command line arguments are
processed.  There are various ways that might be achieved, but I think
we should agree on the UI we are trying to achieve first.

Re: [PATCH 0/9] argument parsing fixes and improvements

[Daniel Kahn Gillmor]

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

On Mon 2017-09-25 08:34:13 -0300, David Bremner wrote:
> I think there is two different discussions one could be having here; one
> about the UI, the other about the implimentation.
>
> From the UI point of view,

Are you using the term "UI" to mean "API" here?  i tend to think of "UI"
as the CLI interface, which i think still has open questions (see below).

> it seems like the best thing is to use any
> configuration to set the default for a given boolean flag. Conceptually
> this would look something like (semi-pseudo-code)
>
>     try_decrypt = false;
>     
>     notmuch_database_get_config(notmuch, "try_decrypt", &try_decrypt);
>
>     parse_arguments(argc, argv, ...)
>
> We have 3 possibilities, with the latest specified one winning.
>
> In the implmentation, we need to cope with the fact that the database
> probably can't be opened until after the command line arguments are
> processed.

in my cleartext-index series, reading the config from the database
doesn't happen until it's needed, because it is deferred to the creation
of the indexopts object (which you can't get without a database).

So from an implementation point of view, it's definitely cleaner/simpler
to have an internally "explicitly unset" state for the CLI flags.

From a CLI UI perspective: do we want to be able to send --foo=default
for a boolean explicitly?

    --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/9] argument parsing fixes and improvements

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> On Mon 2017-09-25 08:34:13 -0300, David Bremner wrote:
>> I think there is two different discussions one could be having here; one
>> about the UI, the other about the implimentation.
>>
>> From the UI point of view,
>
> Are you using the term "UI" to mean "API" here?  i tend to think of "UI"
> as the CLI interface, which i think still has open questions (see below).
>

when I say UI I mean CLI here.

> So from an implementation point of view, it's definitely cleaner/simpler
> to have an internally "explicitly unset" state for the CLI flags.

I'm trying to separate-out/defer impliementation questions here, at
least until I'm clear on the UI.

> From a CLI UI perspective: do we want to be able to send --foo=default
> for a boolean explicitly?

I have the feeling that maybe Jani does, but I'm not sure (as sometimes
happens) why my way of thinking about it isn't the only obvious way ;).

d

Re: [PATCH 0/9] argument parsing fixes and improvements

[Jani Nikula]

On Mon, 25 Sep 2017, David Bremner <david@tethera.net> wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>> So from an implementation point of view, it's definitely cleaner/simpler
>> to have an internally "explicitly unset" state for the CLI flags.
>
> I'm trying to separate-out/defer impliementation questions here, at
> least until I'm clear on the UI.

Patches 1-5 and 8 in this series would be non-committal refactoring and
cleanup in the mean time. ;)

>> From a CLI UI perspective: do we want to be able to send --foo=default
>> for a boolean explicitly?
>
> I have the feeling that maybe Jani does, but I'm not sure (as sometimes
> happens) why my way of thinking about it isn't the only obvious way ;).

I'm undecided. Definitely maybe. Going by "worse is better", the
implementation *does* impact the decision, at least it impacts my
opinion. For example, I don't think we'll open the database before
argument parsing even if that turned out to be "the right thing".

Looking at the defaults from another angle, if we don't want the ability
to set --foo=default explicitly, I still think passing ints as booleans
to the argument parser and checking if a boolean is neither true nor
false is the wrong thing to do. I'd also like to convert to stdbool
more. But those should not be a reason to convert essentially boolean
arguments to keyword arguments. I think we need a way to have the
argument parser tell us if an argument was present or not.


BR,
Jani.

Re: [PATCH 0/9] argument parsing fixes and improvements

[Jani Nikula]

On Sat, 30 Sep 2017, Jani Nikula <jani@nikula.org> wrote:
> Looking at the defaults from another angle, if we don't want the ability
> to set --foo=default explicitly, I still think passing ints as booleans
> to the argument parser and checking if a boolean is neither true nor
> false is the wrong thing to do. I'd also like to convert to stdbool
> more. But those should not be a reason to convert essentially boolean
> arguments to keyword arguments. I think we need a way to have the
> argument parser tell us if an argument was present or not.

id:20170930213239.15392-1-jani@nikula.org would make it easier to add,
say, a notmuch_bool_t *present field to notmuch_opt_desc_t that we could
set whenever we see the option and we want to know the difference.

BR,
Jani.

Re: [PATCH 0/9] argument parsing fixes and improvements

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Sat, 30 Sep 2017, Jani Nikula <jani@nikula.org> wrote:
>> Looking at the defaults from another angle, if we don't want the ability
>> to set --foo=default explicitly, I still think passing ints as booleans
>> to the argument parser and checking if a boolean is neither true nor
>> false is the wrong thing to do. I'd also like to convert to stdbool
>> more. But those should not be a reason to convert essentially boolean
>> arguments to keyword arguments. I think we need a way to have the
>> argument parser tell us if an argument was present or not.
>
> id:20170930213239.15392-1-jani@nikula.org would make it easier to add,
> say, a notmuch_bool_t *present field to notmuch_opt_desc_t that we could
> set whenever we see the option and we want to know the difference.

OK, I've queued that patch. Care to add `present` so we can unblock this
discussion?

d

Re: [PATCH 0/9] argument parsing fixes and improvements

[Jani Nikula]

On Sun, 01 Oct 2017, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>> id:20170930213239.15392-1-jani@nikula.org would make it easier to add,
>> say, a notmuch_bool_t *present field to notmuch_opt_desc_t that we could
>> set whenever we see the option and we want to know the difference.
>
> OK, I've queued that patch. Care to add `present` so we can unblock this
> discussion?

Done. I got a bit carried away, but the important stuff is in the
beginning: id:cover.1506890421.git.jani@nikula.org


BR,
Jani.