[PATCHES v4] Encourage explicit arguments for --decrypt in "show" and "reply"

[PATCHES v4] Encourage explicit arguments for --decrypt in "show" and "reply"

[Daniel Kahn Gillmor]

This is a clean revision of the series that was first introduced at
id:20171212001858.706-1-dkg@fifthhorseman.net.  It needed to be
rebased after Jani's boolean/negation series.  It should now apply
cleanly.

I think it's important to apply this series before releasing 0.26,
because of the interaction between Jani's boolean/negation series and
the introduction of a keyword-based --decrypt for the indexing
subcommands.  For consistency, it would be unpleasant if some commands
offer --no-decrypt, while others require --decrypt=false.

Background
----------

The notmuch indexing subcommands ("new", "insert", and "reindex") now
have a --decrypt option that takes an argument (the decryption
policy), since the session-keys patches have landed.

But the viewing subcommands ("show" and "reply") have their
traditional --decrypt option that (as a boolean) need not take an
argument, having --decrypt not present means something different from
either --decrypt=true or --decrypt=false.

This series allows the user to explicitly choose --decrypt=auto for
the viewing subcommands, while allowing people to use the
argument-free form (as an alias for --decrypt=true), but warns the
user to encourage them to switch to using an explicit argument
instead.

This is useful normalizing work for the interface, so it's worthwhile
on its own. It is also necessary preparation in the event that we
decide we want to:

 * set up a notmuch configuration option that changes the default for
   --decrypt for the viewing subcommands

 * allow "notmuch show" to actually index encrypted messages upon
   their first encounter (e.g., via a new decryption policy, which
   i'll propose separately)

Regards,

        --dkg

[PATCH v4 1/3] cli: some keyword options can be supplied with no argument

[Daniel Kahn Gillmor]

We might change some notmuch command line tools that used to be
booleans into keyword arguments.

In that case, there are some legacy tools that will expect to be able
to do "notmuch foo --bar" instead of "notmuch foo --bar=baz".

This patch makes it possible to support that older API, while
providing a warning and an encouragement to upgrade.
---
 command-line-arguments.c      | 59 +++++++++++++++++++++++++++++++------------
 command-line-arguments.h      |  4 +++
 test/T410-argument-parsing.sh | 24 ++++++++++++++++++
 test/arg-test.c               | 12 ++++++++-
 4 files changed, 82 insertions(+), 17 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index a4a8bb4c..de9ce0cf 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -4,13 +4,19 @@
 #include "error_util.h"
 #include "command-line-arguments.h"
 
+typedef enum {
+    OPT_FAILED, /* false */
+    OPT_OK, /* good */
+    OPT_GIVEBACK, /* pop one of the arguments you thought you were getting off the stack */
+} opt_handled;
+
 /*
   Search the array of keywords for a given argument, assigning the
   output variable to the corresponding value.  Return false if nothing
   matches.
 */
 
-static bool
+static opt_handled
 _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next,
 		      const char *arg_str, bool negate)
 {
@@ -32,16 +38,32 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next,
 	else
 	    *arg_desc->opt_keyword = keywords->value;
 
-	return true;
+	return OPT_OK;
     }
+
+    if (arg_desc->opt_keyword && arg_desc->keyword_no_arg_value && next != ':' && next != '=') {
+	for (keywords = arg_desc->keywords; keywords->name; keywords++) {
+	    if (strcmp (arg_desc->keyword_no_arg_value, keywords->name) != 0)
+		continue;
+
+	    *arg_desc->opt_keyword = keywords->value;
+	    fprintf (stderr, "Warning: No known keyword option given for \"%s\", choosing value \"%s\"."
+		     "  Please specify the argument explicitly!\n", arg_desc->name, arg_desc->keyword_no_arg_value);
+
+	    return OPT_GIVEBACK;
+	}
+	fprintf (stderr, "No matching keyword for option \"%s\" and default value \"%s\" is invalid.\n", arg_str, arg_desc->name);
+	return OPT_FAILED;
+    }
+
     if (next != '\0')
 	fprintf (stderr, "Unknown keyword argument \"%s\" for option \"%s\".\n", arg_str, arg_desc->name);
     else
 	fprintf (stderr, "Option \"%s\" needs a keyword argument.\n", arg_desc->name);
-    return false;
+    return OPT_FAILED;
 }
 
-static bool
+static opt_handled
 _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next,
 		      const char *arg_str, bool negate)
 {
@@ -53,45 +75,45 @@ _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next,
 	value = false;
     } else {
 	fprintf (stderr, "Unknown argument \"%s\" for (boolean) option \"%s\".\n", arg_str, arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
 
     *arg_desc->opt_bool = negate ? !value : value;
 
-    return true;
+    return OPT_OK;
 }
 
-static bool
+static opt_handled
 _process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
     char *endptr;
     if (next == '\0' || arg_str[0] == '\0') {
 	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
 
     *arg_desc->opt_int = strtol (arg_str, &endptr, 10);
     if (*endptr == '\0')
-	return true;
+	return OPT_OK;
 
     fprintf (stderr, "Unable to parse argument \"%s\" for option \"%s\" as an integer.\n",
 	     arg_str, arg_desc->name);
-    return false;
+    return OPT_FAILED;
 }
 
-static bool
+static opt_handled
 _process_string_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
     if (next == '\0') {
 	fprintf (stderr, "Option \"%s\" needs a string argument.\n", arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
     if (arg_str[0] == '\0' && ! arg_desc->allow_empty) {
 	fprintf (stderr, "String argument for option \"%s\" must be non-empty.\n", arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
     *arg_desc->opt_string = arg_str;
-    return true;
+    return OPT_OK;
 }
 
 /* Return number of non-NULL opt_* fields in opt_desc. */
@@ -212,13 +234,15 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
 	if (next != '=' && next != ':' && next != '\0')
 	    continue;
 
+	bool incremented = false;
 	if (next == '\0' && next_arg != NULL && ! try->opt_bool) {
 	    next = ' ';
 	    value = next_arg;
+	    incremented = true;
 	    opt_index ++;
 	}
 
-	bool opt_status = false;
+	opt_handled opt_status = OPT_FAILED;
 	if (try->opt_keyword || try->opt_flags)
 	    opt_status = _process_keyword_arg (try, next, value, negate);
 	else if (try->opt_bool)
@@ -230,9 +254,12 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
 	else
 	    INTERNAL_ERROR ("unknown or unhandled option \"%s\"", try->name);
 
-	if (! opt_status)
+	if (opt_status == OPT_FAILED)
 	    return -1;
 
+	if (incremented && opt_status == OPT_GIVEBACK)
+	    opt_index --;
+
 	if (try->present)
 	    *try->present = true;
 
diff --git a/command-line-arguments.h b/command-line-arguments.h
index c0228f7c..f722f97d 100644
--- a/command-line-arguments.h
+++ b/command-line-arguments.h
@@ -26,6 +26,10 @@ typedef struct notmuch_opt_desc {
     const char **opt_string;
     const char **opt_position;
 
+    /* for opt_keyword only: if no matching arguments were found, and
+     * keyword_no_arg_value is set, then use keyword_no_arg_value instead. */
+    const char *keyword_no_arg_value;
+
     /* Must be set except for opt_inherit and opt_position. */
     const char *name;
 
diff --git a/test/T410-argument-parsing.sh b/test/T410-argument-parsing.sh
index 192133c5..3f8a78a3 100755
--- a/test/T410-argument-parsing.sh
+++ b/test/T410-argument-parsing.sh
@@ -65,4 +65,28 @@ flags 1
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "test keyword arguments without value"
+$TEST_DIRECTORY/arg-test --boolkeyword bananas > OUTPUT
+cat <<EOF > EXPECTED
+boolkeyword 1
+positional arg 1 bananas
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "test keyword arguments without value at the end"
+$TEST_DIRECTORY/arg-test bananas --boolkeyword > OUTPUT
+cat <<EOF > EXPECTED
+boolkeyword 1
+positional arg 1 bananas
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "test keyword arguments without value but with = (should be an error)"
+$TEST_DIRECTORY/arg-test bananas --boolkeyword= > OUTPUT 2>&1
+cat <<EOF > EXPECTED
+Unknown keyword argument "" for option "boolkeyword".
+Unrecognized option: --boolkeyword=
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
diff --git a/test/arg-test.c b/test/arg-test.c
index 7aff8255..a218f967 100644
--- a/test/arg-test.c
+++ b/test/arg-test.c
@@ -7,13 +7,14 @@ int main(int argc, char **argv){
     int opt_index=1;
 
     int kw_val=0;
+    int kwb_val=0;
     int fl_val=0;
     int int_val=0;
     const char *pos_arg1=NULL;
     const char *pos_arg2=NULL;
     const char *string_val=NULL;
     bool bool_val = false;
-    bool fl_set = false, int_set = false, bool_set = false,
+    bool fl_set = false, int_set = false, bool_set = false, kwb_set = false,
 	kw_set = false, string_set = false, pos1_set = false, pos2_set = false;
 
     notmuch_opt_desc_t parent_options[] = {
@@ -33,6 +34,12 @@ int main(int argc, char **argv){
 				  { "one", 1 },
 				  { "two", 2 },
 				  { 0, 0 } } },
+	{ .opt_keyword = &kwb_val, .name = "boolkeyword", .present = &kwb_set,
+	  .keyword_no_arg_value = "true", .keywords =
+	  (notmuch_keyword_t []){ { "false", 0 },
+				  { "true", 1 },
+				  { "auto", 2 },
+				  { 0, 0 } } },
 	{ .opt_inherit = parent_options },
 	{ .opt_string = &string_val, .name = "string", .present = &string_set },
 	{ .opt_position = &pos_arg1, .present = &pos1_set },
@@ -51,6 +58,9 @@ int main(int argc, char **argv){
     if (kw_set)
 	printf("keyword %d\n", kw_val);
 
+    if (kwb_set)
+	printf("boolkeyword %d\n", kwb_val);
+
     if (fl_set)
 	printf("flags %d\n", fl_val);
 
-- 
2.15.1

Re: [PATCH v4 1/3] cli: some keyword options can be supplied with no argument

[David Bremner]

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

>  
> +	bool incremented = false;
>  	if (next == '\0' && next_arg != NULL && ! try->opt_bool) {
>  	    next = ' ';
>  	    value = next_arg;
> +	    incremented = true;
>  	    opt_index ++;
>  	}

Is incremented == true exactly when next == ' ' ? It might be nice to
make that more explicit by setting one based on the other. You could
also use (next == ' ') as your test condition, but I understand that
might not be that obvious to read.

>  
> diff --git a/test/T410-argument-parsing.sh b/test/T410-argument-parsing.sh
> index 192133c5..3f8a78a3 100755
> --- a/test/T410-argument-parsing.sh
> +++ b/test/T410-argument-parsing.sh
> @@ -65,4 +65,28 @@ flags 1
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
> +test_begin_subtest "test keyword arguments without value"
> +$TEST_DIRECTORY/arg-test --boolkeyword bananas > OUTPUT
> +cat <<EOF > EXPECTED
> +boolkeyword 1
> +positional arg 1 bananas
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +test_begin_subtest "test keyword arguments without value at the end"
> +$TEST_DIRECTORY/arg-test bananas --boolkeyword > OUTPUT
> +cat <<EOF > EXPECTED
> +boolkeyword 1
> +positional arg 1 bananas
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +test_begin_subtest "test keyword arguments without value but with = (should be an error)"
> +$TEST_DIRECTORY/arg-test bananas --boolkeyword= > OUTPUT 2>&1
> +cat <<EOF > EXPECTED
> +Unknown keyword argument "" for option "boolkeyword".
> +Unrecognized option: --boolkeyword=
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +

The thing I'm most nervous about here is the interaction between this
new code and the relatively recent code that permits ' ' as a
seperator. Would you mind adding one or more tests for that case? For
example, that I checked that

         ./notmuch show --format=json --decrypt true $id

continues to work, and that's great, but it seems like something to
check on the argument parsing level, i.e

--keyword␣non-default-value

(pardon my unicode)

[PATCH] cli: some keyword options can be supplied with no argument

[David Bremner]

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

We might change some notmuch command line tools that used to be
booleans into keyword arguments.

In that case, there are some legacy tools that will expect to be able
to do "notmuch foo --bar" instead of "notmuch foo --bar=baz".

This patch makes it possible to support that older API, while
providing a warning and an encouragement to upgrade.
---
 command-line-arguments.c      | 61 +++++++++++++++++++++++++++++++------------
 command-line-arguments.h      |  4 +++
 test/T410-argument-parsing.sh | 32 +++++++++++++++++++++++
 test/arg-test.c               | 12 ++++++++-
 4 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index a4a8bb4c..d64aa85b 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -4,13 +4,19 @@
 #include "error_util.h"
 #include "command-line-arguments.h"
 
+typedef enum {
+    OPT_FAILED, /* false */
+    OPT_OK, /* good */
+    OPT_GIVEBACK, /* pop one of the arguments you thought you were getting off the stack */
+} opt_handled;
+
 /*
   Search the array of keywords for a given argument, assigning the
   output variable to the corresponding value.  Return false if nothing
   matches.
 */
 
-static bool
+static opt_handled
 _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next,
 		      const char *arg_str, bool negate)
 {
@@ -32,16 +38,32 @@ _process_keyword_arg (const notmuch_opt_desc_t *arg_desc, char next,
 	else
 	    *arg_desc->opt_keyword = keywords->value;
 
-	return true;
+	return OPT_OK;
+    }
+
+    if (arg_desc->opt_keyword && arg_desc->keyword_no_arg_value && next != ':' && next != '=') {
+	for (keywords = arg_desc->keywords; keywords->name; keywords++) {
+	    if (strcmp (arg_desc->keyword_no_arg_value, keywords->name) != 0)
+		continue;
+
+	    *arg_desc->opt_keyword = keywords->value;
+	    fprintf (stderr, "Warning: No known keyword option given for \"%s\", choosing value \"%s\"."
+		     "  Please specify the argument explicitly!\n", arg_desc->name, arg_desc->keyword_no_arg_value);
+
+	    return OPT_GIVEBACK;
+	}
+	fprintf (stderr, "No matching keyword for option \"%s\" and default value \"%s\" is invalid.\n", arg_str, arg_desc->name);
+	return OPT_FAILED;
     }
+
     if (next != '\0')
 	fprintf (stderr, "Unknown keyword argument \"%s\" for option \"%s\".\n", arg_str, arg_desc->name);
     else
 	fprintf (stderr, "Option \"%s\" needs a keyword argument.\n", arg_desc->name);
-    return false;
+    return OPT_FAILED;
 }
 
-static bool
+static opt_handled
 _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next,
 		      const char *arg_str, bool negate)
 {
@@ -53,45 +75,45 @@ _process_boolean_arg (const notmuch_opt_desc_t *arg_desc, char next,
 	value = false;
     } else {
 	fprintf (stderr, "Unknown argument \"%s\" for (boolean) option \"%s\".\n", arg_str, arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
 
     *arg_desc->opt_bool = negate ? !value : value;
 
-    return true;
+    return OPT_OK;
 }
 
-static bool
+static opt_handled
 _process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
     char *endptr;
     if (next == '\0' || arg_str[0] == '\0') {
 	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
 
     *arg_desc->opt_int = strtol (arg_str, &endptr, 10);
     if (*endptr == '\0')
-	return true;
+	return OPT_OK;
 
     fprintf (stderr, "Unable to parse argument \"%s\" for option \"%s\" as an integer.\n",
 	     arg_str, arg_desc->name);
-    return false;
+    return OPT_FAILED;
 }
 
-static bool
+static opt_handled
 _process_string_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
 
     if (next == '\0') {
 	fprintf (stderr, "Option \"%s\" needs a string argument.\n", arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
     if (arg_str[0] == '\0' && ! arg_desc->allow_empty) {
 	fprintf (stderr, "String argument for option \"%s\" must be non-empty.\n", arg_desc->name);
-	return false;
+	return OPT_FAILED;
     }
     *arg_desc->opt_string = arg_str;
-    return true;
+    return OPT_OK;
 }
 
 /* Return number of non-NULL opt_* fields in opt_desc. */
@@ -212,13 +234,15 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
 	if (next != '=' && next != ':' && next != '\0')
 	    continue;
 
-	if (next == '\0' && next_arg != NULL && ! try->opt_bool) {
+	bool lookahead = (next == '\0' && next_arg != NULL && ! try->opt_bool);
+
+	if (lookahead) {
 	    next = ' ';
 	    value = next_arg;
 	    opt_index ++;
 	}
 
-	bool opt_status = false;
+	opt_handled opt_status = OPT_FAILED;
 	if (try->opt_keyword || try->opt_flags)
 	    opt_status = _process_keyword_arg (try, next, value, negate);
 	else if (try->opt_bool)
@@ -230,9 +254,12 @@ parse_option (int argc, char **argv, const notmuch_opt_desc_t *options, int opt_
 	else
 	    INTERNAL_ERROR ("unknown or unhandled option \"%s\"", try->name);
 
-	if (! opt_status)
+	if (opt_status == OPT_FAILED)
 	    return -1;
 
+	if (lookahead && opt_status == OPT_GIVEBACK)
+	    opt_index --;
+
 	if (try->present)
 	    *try->present = true;
 
diff --git a/command-line-arguments.h b/command-line-arguments.h
index c0228f7c..f722f97d 100644
--- a/command-line-arguments.h
+++ b/command-line-arguments.h
@@ -26,6 +26,10 @@ typedef struct notmuch_opt_desc {
     const char **opt_string;
     const char **opt_position;
 
+    /* for opt_keyword only: if no matching arguments were found, and
+     * keyword_no_arg_value is set, then use keyword_no_arg_value instead. */
+    const char *keyword_no_arg_value;
+
     /* Must be set except for opt_inherit and opt_position. */
     const char *name;
 
diff --git a/test/T410-argument-parsing.sh b/test/T410-argument-parsing.sh
index 192133c5..a384ce86 100755
--- a/test/T410-argument-parsing.sh
+++ b/test/T410-argument-parsing.sh
@@ -65,4 +65,36 @@ flags 1
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "test keyword arguments without value"
+$TEST_DIRECTORY/arg-test --boolkeyword bananas > OUTPUT
+cat <<EOF > EXPECTED
+boolkeyword 1
+positional arg 1 bananas
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "test keyword arguments with non-default value separted by a space"
+$TEST_DIRECTORY/arg-test --boolkeyword false bananas > OUTPUT
+cat <<EOF > EXPECTED
+boolkeyword 0
+positional arg 1 bananas
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "test keyword arguments without value at the end"
+$TEST_DIRECTORY/arg-test bananas --boolkeyword > OUTPUT
+cat <<EOF > EXPECTED
+boolkeyword 1
+positional arg 1 bananas
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "test keyword arguments without value but with = (should be an error)"
+$TEST_DIRECTORY/arg-test bananas --boolkeyword= > OUTPUT 2>&1
+cat <<EOF > EXPECTED
+Unknown keyword argument "" for option "boolkeyword".
+Unrecognized option: --boolkeyword=
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
diff --git a/test/arg-test.c b/test/arg-test.c
index 7aff8255..a218f967 100644
--- a/test/arg-test.c
+++ b/test/arg-test.c
@@ -7,13 +7,14 @@ int main(int argc, char **argv){
     int opt_index=1;
 
     int kw_val=0;
+    int kwb_val=0;
     int fl_val=0;
     int int_val=0;
     const char *pos_arg1=NULL;
     const char *pos_arg2=NULL;
     const char *string_val=NULL;
     bool bool_val = false;
-    bool fl_set = false, int_set = false, bool_set = false,
+    bool fl_set = false, int_set = false, bool_set = false, kwb_set = false,
 	kw_set = false, string_set = false, pos1_set = false, pos2_set = false;
 
     notmuch_opt_desc_t parent_options[] = {
@@ -33,6 +34,12 @@ int main(int argc, char **argv){
 				  { "one", 1 },
 				  { "two", 2 },
 				  { 0, 0 } } },
+	{ .opt_keyword = &kwb_val, .name = "boolkeyword", .present = &kwb_set,
+	  .keyword_no_arg_value = "true", .keywords =
+	  (notmuch_keyword_t []){ { "false", 0 },
+				  { "true", 1 },
+				  { "auto", 2 },
+				  { 0, 0 } } },
 	{ .opt_inherit = parent_options },
 	{ .opt_string = &string_val, .name = "string", .present = &string_set },
 	{ .opt_position = &pos_arg1, .present = &pos1_set },
@@ -51,6 +58,9 @@ int main(int argc, char **argv){
     if (kw_set)
 	printf("keyword %d\n", kw_val);
 
+    if (kwb_set)
+	printf("boolkeyword %d\n", kwb_val);
+
     if (fl_set)
 	printf("flags %d\n", fl_val);
 
-- 
2.15.1

Re: [PATCH] cli: some keyword options can be supplied with no argument

[David Bremner]

David Bremner <david@tethera.net> writes:

> From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
>
> We might change some notmuch command line tools that used to be
> booleans into keyword arguments.

pushed my #1 and the original #2 and #3 to release

Re: [PATCH v4 1/3] cli: some keyword options can be supplied with no argument

[Daniel Kahn Gillmor]

On Sat 2017-12-23 10:29:30 -0400, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>>  
>> +	bool incremented = false;
>>  	if (next == '\0' && next_arg != NULL && ! try->opt_bool) {
>>  	    next = ' ';
>>  	    value = next_arg;
>> +	    incremented = true;
>>  	    opt_index ++;
>>  	}
>
> Is incremented == true exactly when next == ' ' ? It might be nice to
> make that more explicit by setting one based on the other. You could
> also use (next == ' ') as your test condition, but I understand that
> might not be that obvious to read.

yes, i was aiming for readability.  I'm assuming that the compiler can
optimize this as needed.

> The thing I'm most nervous about here is the interaction between this
> new code and the relatively recent code that permits ' ' as a
> seperator. Would you mind adding one or more tests for that case? For
> example, that I checked that
>
>          ./notmuch show --format=json --decrypt true $id
>
> continues to work, and that's great, but it seems like something to
> check on the argument parsing level, i.e
>
> --keyword␣non-default-value
>
> (pardon my unicode)

I'm fine with that, and with your proposed revision of this patch that
includes the amended test.  thanks for pushing it forward.

         --dkg

[PATCH v4 2/3] cli/show: make --decrypt take a keyword.

[Daniel Kahn Gillmor]

We also expand tab completion for it, update the emacs bindings, and
update T350, T357, and T450 to match.

Make use of the bool-to-keyword backward-compatibility feature.
---
 completion/notmuch-completion.bash |  6 +++++-
 doc/man1/notmuch-show.rst          | 37 +++++++++++++++++++++----------------
 emacs/notmuch-lib.el               |  2 +-
 emacs/notmuch-query.el             |  2 +-
 notmuch-show.c                     | 22 +++++++++-------------
 test/T350-crypto.sh                | 12 ++++++------
 test/T357-index-decryption.sh      |  6 +++---
 test/T450-emacs-show.sh            |  2 +-
 8 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
index adf64a0a..17f3c5ec 100644
--- a/completion/notmuch-completion.bash
+++ b/completion/notmuch-completion.bash
@@ -517,10 +517,14 @@ _notmuch_show()
 	    COMPREPLY=( $( compgen -W "text json sexp mbox raw" -- "${cur}" ) )
 	    return
 	    ;;
-	--exclude|--body|--decrypt)
+	--exclude|--body)
 	    COMPREPLY=( $( compgen -W "true false" -- "${cur}" ) )
 	    return
 	    ;;
+        --decrypt)
+	    COMPREPLY=( $( compgen -W "true auto false" -- "${cur}" ) )
+	    return
+	    ;;
     esac
 
     ! $split &&
diff --git a/doc/man1/notmuch-show.rst b/doc/man1/notmuch-show.rst
index 64caa7a6..7d2b38cb 100644
--- a/doc/man1/notmuch-show.rst
+++ b/doc/man1/notmuch-show.rst
@@ -115,22 +115,27 @@ Supported options for **show** include
         supported with --format=json and --format=sexp), and the
         multipart/signed part will be replaced by the signed data.
 
-    ``--decrypt``
-        Decrypt any MIME encrypted parts found in the selected content
-        (ie. "multipart/encrypted" parts). Status of the decryption will
-        be reported (currently only supported with --format=json and
-        --format=sexp) and on successful decryption the
-        multipart/encrypted part will be replaced by the decrypted
-        content.
-
-        If a session key is already known for the message, then it
-        will be decrypted automatically unless the user explicitly
-        sets ``--decrypt=false``.
-
-        Decryption expects a functioning **gpg-agent(1)** to provide any
-        needed credentials. Without one, the decryption will fail.
-
-        Implies --verify.
+    ``--decrypt=(false|auto|true)``
+        If ``true``, decrypt any MIME encrypted parts found in the
+        selected content (i.e. "multipart/encrypted" parts). Status of
+        the decryption will be reported (currently only supported
+        with --format=json and --format=sexp) and on successful
+        decryption the multipart/encrypted part will be replaced by
+        the decrypted content.
+
+        If ``auto``, and a session key is already known for the
+        message, then it will be decrypted, but notmuch will not try
+        to access the user's keys.
+
+        Use ``false`` to avoid even automatic decryption.
+
+        Non-automatic decryption expects a functioning
+        **gpg-agent(1)** to provide any needed credentials. Without
+        one, the decryption will fail.
+
+        Note: ``true`` implies --verify.
+
+        Default: ``auto``
 
     ``--exclude=(true|false)``
         Specify whether to omit threads only matching
diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 010be454..a7e02710 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -593,7 +593,7 @@ the given type."
 		       (set-buffer-multibyte nil))
 		     (let ((args `("show" "--format=raw"
 				   ,(format "--part=%s" (plist-get part :id))
-				   ,@(when process-crypto '("--decrypt"))
+				   ,@(when process-crypto '("--decrypt=true"))
 				   ,(notmuch-id-to-query (plist-get msg :id))))
 			   (coding-system-for-read
 			    (if binaryp 'no-conversion
diff --git a/emacs/notmuch-query.el b/emacs/notmuch-query.el
index 592fd8f1..563e4acf 100644
--- a/emacs/notmuch-query.el
+++ b/emacs/notmuch-query.el
@@ -32,7 +32,7 @@ is a possibly empty forest of replies.
 "
   (let ((args '("show" "--format=sexp" "--format-version=4")))
     (if notmuch-show-process-crypto
-	(setq args (append args '("--decrypt"))))
+	(setq args (append args '("--decrypt=true"))))
     (setq args (append args search-terms))
     (apply #'notmuch-call-notmuch-sexp args)))
 
diff --git a/notmuch-show.c b/notmuch-show.c
index d5adc370..9871159d 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1085,8 +1085,6 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
     bool exclude = true;
     bool entire_thread_set = false;
     bool single_message;
-    bool decrypt = false;
-    bool decrypt_set = false;
 
     notmuch_opt_desc_t options[] = {
 	{ .opt_keyword = &format, .name = "format", .keywords =
@@ -1101,7 +1099,12 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	{ .opt_bool = &params.entire_thread, .name = "entire-thread",
 	  .present = &entire_thread_set },
 	{ .opt_int = &params.part, .name = "part" },
-	{ .opt_bool = &decrypt, .name = "decrypt", .present = &decrypt_set },
+	{ .opt_keyword = (int*)(&params.crypto.decrypt), .name = "decrypt",
+	  .keyword_no_arg_value = "true", .keywords =
+	  (notmuch_keyword_t []){ { "false", NOTMUCH_DECRYPT_FALSE },
+				  { "auto", NOTMUCH_DECRYPT_AUTO },
+				  { "true", NOTMUCH_DECRYPT_NOSTASH },
+				  { 0, 0 } } },
 	{ .opt_bool = &params.crypto.verify, .name = "verify" },
 	{ .opt_bool = &params.output_body, .name = "body" },
 	{ .opt_bool = &params.include_html, .name = "include-html" },
@@ -1115,16 +1118,9 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_process_shared_options (argv[0]);
 
-    if (decrypt_set) {
-	if (decrypt) {
-	    /* we do not need or want to ask for session keys */
-	    params.crypto.decrypt = NOTMUCH_DECRYPT_NOSTASH;
-	    /* decryption implies verification */
-	    params.crypto.verify = true;
-	} else {
-	    params.crypto.decrypt = NOTMUCH_DECRYPT_FALSE;
-	}
-    }
+    /* explicit decryption implies verification */
+    if (params.crypto.decrypt == NOTMUCH_DECRYPT_NOSTASH)
+	params.crypto.verify = true;
 
     /* specifying a part implies single message display */
     single_message = params.part >= 0;
diff --git a/test/T350-crypto.sh b/test/T350-crypto.sh
index 38615677..761ff553 100755
--- a/test/T350-crypto.sh
+++ b/test/T350-crypto.sh
@@ -223,7 +223,7 @@ output=$(notmuch search mimetype:multipart/encrypted and mimetype:application/pg
 test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message 001 (encrypted inbox)"
 
 test_begin_subtest "decryption, --format=text"
-output=$(notmuch show --format=text --decrypt subject:"test encrypted message 001" \
+output=$(notmuch show --format=text --decrypt=true subject:"test encrypted message 001" \
     | notmuch_show_sanitize_all \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
 expected='\fmessage{ id:XXXXX depth:0 match:1 excluded:0 filename:XXXXX
@@ -255,7 +255,7 @@ test_expect_equal \
     "$expected"
 
 test_begin_subtest "decryption, --format=json"
-output=$(notmuch show --format=json --decrypt subject:"test encrypted message 001" \
+output=$(notmuch show --format=json --decrypt=true subject:"test encrypted message 001" \
     | notmuch_json_show_sanitize \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
 expected='[[[{"id": "XXXXX",
@@ -293,7 +293,7 @@ test_expect_equal_json \
     "$expected"
 
 test_begin_subtest "decryption, --format=json, --part=4"
-output=$(notmuch show --format=json --part=4 --decrypt subject:"test encrypted message 001" \
+output=$(notmuch show --format=json --part=4 --decrypt=true subject:"test encrypted message 001" \
     | notmuch_json_show_sanitize \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
 expected='{"id": 4,
@@ -307,13 +307,13 @@ test_begin_subtest "decrypt attachment (--part=5 --format=raw)"
 notmuch show \
     --format=raw \
     --part=5 \
-    --decrypt \
+    --decrypt=true \
     subject:"test encrypted message 001" >OUTPUT
 test_expect_equal_file TESTATTACHMENT OUTPUT
 
 test_begin_subtest "decryption failure with missing key"
 mv "${GNUPGHOME}"{,.bak}
-output=$(notmuch show --format=json --decrypt subject:"test encrypted message 001" \
+output=$(notmuch show --format=json --decrypt=true subject:"test encrypted message 001" \
     | notmuch_json_show_sanitize \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
 expected='[[[{"id": "XXXXX",
@@ -351,7 +351,7 @@ test_expect_success \
 
 test_begin_subtest "decryption + signature verification"
 test_subtest_broken_gmime_2
-output=$(notmuch show --format=json --decrypt subject:"test encrypted message 002" \
+output=$(notmuch show --format=json --decrypt=true subject:"test encrypted message 002" \
     | notmuch_json_show_sanitize \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
 expected='[[[{"id": "XXXXX",
diff --git a/test/T357-index-decryption.sh b/test/T357-index-decryption.sh
index 6b8a8261..2b8e05b8 100755
--- a/test/T357-index-decryption.sh
+++ b/test/T357-index-decryption.sh
@@ -197,14 +197,14 @@ test_expect_equal \
     "$output" \
     "$expected"
 
-test_begin_subtest "show one of the messages with --decrypt"
-output=$(notmuch show --decrypt thread:0000000000000001 | awk '/^\014part}/{ f=0 }; { if (f) { print $0 } } /^\014part{ ID: 3/{ f=1 }')
+test_begin_subtest "show one of the messages with --decrypt=true"
+output=$(notmuch show --decrypt=true thread:0000000000000001 | awk '/^\014part}/{ f=0 }; { if (f) { print $0 } } /^\014part{ ID: 3/{ f=1 }')
 expected='This is a test encrypted message with a wumpus.'
 test_expect_equal \
     "$output" \
     "$expected"
 
-test_begin_subtest "Ensure that we cannot show the message without --decrypt"
+test_begin_subtest "Ensure that we cannot show the message with --decrypt=auto"
 output=$(notmuch show thread:0000000000000001 | awk '/^\014part}/{ f=0 }; { if (f) { print $0 } } /^\014part{ ID: 3/{ f=1 }')
 expected='Non-text part: application/octet-stream'
 test_expect_equal \
diff --git a/test/T450-emacs-show.sh b/test/T450-emacs-show.sh
index 8db0e49b..3555a939 100755
--- a/test/T450-emacs-show.sh
+++ b/test/T450-emacs-show.sh
@@ -191,7 +191,7 @@ This is an error (see *Notmuch errors* for more details)
 === ERROR ===
 [XXX]
 This is an error
-command: YYY/notmuch_fail show --format\\=sexp --format-version\\=4 --decrypt --exclude\\=false \\' \\* \\'
+command: YYY/notmuch_fail show --format\\=sexp --format-version\\=4 --decrypt\\=true --exclude\\=false \\' \\* \\'
 exit status: 1
 stderr:
 This is an error
-- 
2.15.1

Re: [PATCH v4 2/3] cli/show: make --decrypt take a keyword.

[David Bremner]

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

> -	{ .opt_bool = &decrypt, .name = "decrypt", .present = &decrypt_set },
> +	{ .opt_keyword = (int*)(&params.crypto.decrypt), .name = "decrypt",
> +	  .keyword_no_arg_value = "true", .keywords =
> +	  (notmuch_keyword_t []){ { "false", NOTMUCH_DECRYPT_FALSE },
> +				  { "auto", NOTMUCH_DECRYPT_AUTO },
> +				  { "true", NOTMUCH_DECRYPT_NOSTASH },
> +				  { 0, 0 } } },

Should we explicitely allow --decrypt=nostash for consistency? Also
maybe stability in case we change what --true means.

> +    if (params.crypto.decrypt == NOTMUCH_DECRYPT_NOSTASH)
> +	params.crypto.verify = true;

One thing that gave me pause is the fact that --decrypt=auto does not
verify by default. What are the security implications of this? Do we
verify when indexing? Does this require more documentation?

Re: [PATCH v4 2/3] cli/show: make --decrypt take a keyword.

[Daniel Kahn Gillmor]

On Sat 2017-12-23 10:39:47 -0400, David Bremner wrote:
> One thing that gave me pause is the fact that --decrypt=auto does not
> verify by default. What are the security implications of this?

The only issue is that some messages that are correctly signed are
marked as unsigned if the user hasn't chosen to explicitly --decrypt.
The reverse failure would be the real security problem, but that's not
happening here :)

> Do we verify when indexing? Does this require more documentation?

We do not verify when indexing at all, because we have nothing to do
with any statement about signature verification during indexing (we
aren't caching it in the database, for example)

If notmuch was to do something more clever with signature verification,
i'd be interested in working on it, but i'm not currently clear what
that would be, specifically (e.g. Patrick Brunschwig and i discussed
some interesting thoughts about signature verification for enigmail that
resulted in this mailing list post [0]).

But i don't think these decisions are ones that need to be made in the
context of this particular series.

all that said, if anyone has been thinking about signature verification
for e-mail, i'd be happy to hear your thoughts on how notmuch could do
better there.

     --dkg

[0] https://admin.hostpoint.ch/pipermail/enigmail-users_enigmail.net/2017-November/004683.html

[PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[Daniel Kahn Gillmor]

This brings the --decrypt argument to "notmuch reply" into line with
the other --decrypt arguments (in "show", "new", "insert", and
"reindex").  This patch is really just about bringing consistency to
the user interface.

We also use the recommended form in the emacs MUA when replying, and
update test T350 to match.
---
 completion/notmuch-completion.bash |  2 +-
 doc/man1/notmuch-reply.rst         | 34 ++++++++++++++++++++--------------
 emacs/notmuch-mua.el               |  2 +-
 notmuch-reply.c                    | 11 ++++++-----
 test/T350-crypto.sh                |  2 +-
 5 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
index 17f3c5ec..249b9664 100644
--- a/completion/notmuch-completion.bash
+++ b/completion/notmuch-completion.bash
@@ -351,7 +351,7 @@ _notmuch_reply()
 	    return
 	    ;;
 	--decrypt)
-	    COMPREPLY=( $( compgen -W "true false" -- "${cur}" ) )
+	    COMPREPLY=( $( compgen -W "true auto false" -- "${cur}" ) )
 	    return
 	    ;;
     esac
diff --git a/doc/man1/notmuch-reply.rst b/doc/man1/notmuch-reply.rst
index ede77930..1b62e075 100644
--- a/doc/man1/notmuch-reply.rst
+++ b/doc/man1/notmuch-reply.rst
@@ -72,20 +72,26 @@ Supported options for **reply** include
             in this order, and copy values from the first that contains
             something other than only the user's addresses.
 
-    ``--decrypt``
-        Decrypt any MIME encrypted parts found in the selected content
-        (ie. "multipart/encrypted" parts). Status of the decryption will
-        be reported (currently only supported with --format=json and
-        --format=sexp) and on successful decryption the
-        multipart/encrypted part will be replaced by the decrypted
-        content.
-
-        If a session key is already known for the message, then it
-        will be decrypted automatically unless the user explicitly
-        sets ``--decrypt=false``.
-
-        Decryption expects a functioning **gpg-agent(1)** to provide any
-        needed credentials. Without one, the decryption will likely fail.
+    ``--decrypt=(false|auto|true)``
+
+        If ``true``, decrypt any MIME encrypted parts found in the
+        selected content (i.e., "multipart/encrypted" parts). Status
+        of the decryption will be reported (currently only supported
+        with --format=json and --format=sexp), and on successful
+        decryption the multipart/encrypted part will be replaced by
+        the decrypted content.
+
+        If ``auto``, and a session key is already known for the
+        message, then it will be decrypted, but notmuch will not try
+        to access the user's secret keys.
+
+        Use ``false`` to avoid even automatic decryption.
+
+        Non-automatic decryption expects a functioning
+        **gpg-agent(1)** to provide any needed credentials. Without
+        one, the decryption will likely fail.
+
+        Default: ``auto``
 
 See **notmuch-search-terms(7)** for details of the supported syntax for
 <search-terms>.
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 7a341ebf..59b546a6 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -181,7 +181,7 @@ mutiple parts get a header."
 	reply
 	original)
     (when process-crypto
-      (setq args (append args '("--decrypt"))))
+      (setq args (append args '("--decrypt=true"))))
 
     (if reply-all
 	(setq args (append args '("--reply-to=all")))
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 5cdf642b..75cf7ecb 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -704,8 +704,6 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     };
     int format = FORMAT_DEFAULT;
     int reply_all = true;
-    bool decrypt = false;
-    bool decrypt_set = false;
 
     notmuch_opt_desc_t options[] = {
 	{ .opt_keyword = &format, .name = "format", .keywords =
@@ -719,7 +717,12 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	  (notmuch_keyword_t []){ { "all", true },
 				  { "sender", false },
 				  { 0, 0 } } },
-	{ .opt_bool = &decrypt, .name = "decrypt", .present = &decrypt_set },
+	{ .opt_keyword = (int*)(&params.crypto.decrypt), .name = "decrypt",
+	  .keyword_no_arg_value = "true", .keywords =
+	  (notmuch_keyword_t []){ { "false", NOTMUCH_DECRYPT_FALSE },
+				  { "auto", NOTMUCH_DECRYPT_AUTO },
+				  { "true", NOTMUCH_DECRYPT_NOSTASH },
+				  { 0, 0 } } },
 	{ .opt_inherit = notmuch_shared_options },
 	{ }
     };
@@ -729,8 +732,6 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
 
     notmuch_process_shared_options (argv[0]);
-    if (decrypt_set)
-	params.crypto.decrypt = decrypt ? NOTMUCH_DECRYPT_NOSTASH : NOTMUCH_DECRYPT_FALSE;
 
     notmuch_exit_if_unsupported_format ();
 
diff --git a/test/T350-crypto.sh b/test/T350-crypto.sh
index 761ff553..a776ec35 100755
--- a/test/T350-crypto.sh
+++ b/test/T350-crypto.sh
@@ -384,7 +384,7 @@ test_expect_equal_json \
     "$expected"
 
 test_begin_subtest "reply to encrypted message"
-output=$(notmuch reply --decrypt subject:"test encrypted message 002" \
+output=$(notmuch reply --decrypt=true subject:"test encrypted message 002" \
     | notmuch_drop_mail_headers In-Reply-To References)
 expected='From: Notmuch Test Suite <test_suite@notmuchmail.org>
 Subject: Re: test encrypted message 002
-- 
2.15.1

Re: [PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
> +	{ .opt_keyword = (int*)(&params.crypto.decrypt), .name = "decrypt",
> +	  .keyword_no_arg_value = "true", .keywords =
> +	  (notmuch_keyword_t []){ { "false", NOTMUCH_DECRYPT_FALSE },
> +				  { "auto", NOTMUCH_DECRYPT_AUTO },
> +				  { "true", NOTMUCH_DECRYPT_NOSTASH },
> +				  { 0, 0 } } },

same question as for show, should we support --decrypt=nostash ?

d

Re: [PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[Daniel Kahn Gillmor]

On Sat 2017-12-23 10:47:52 -0400, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>> +	{ .opt_keyword = (int*)(&params.crypto.decrypt), .name = "decrypt",
>> +	  .keyword_no_arg_value = "true", .keywords =
>> +	  (notmuch_keyword_t []){ { "false", NOTMUCH_DECRYPT_FALSE },
>> +				  { "auto", NOTMUCH_DECRYPT_AUTO },
>> +				  { "true", NOTMUCH_DECRYPT_NOSTASH },
>> +				  { 0, 0 } } },
>
> same question as for show, should we support --decrypt=nostash ?

No, we should not support --decrypt=nostash for show or reply.  The
semantics of the display commands (show, reply) are such that they
*never* modify the index or stash anything in there.  The equivalent for
the indexing (new, insert, reindex) commands' "--decrypt=nostash" in the
display commands is simply "--decrypt=true".

If you take a look at the subsequent series that follows this one,
you'll see that i offer a "--decrypt=stash" command "notmuch show", but
it makes some unusual assumptions (in particular, about opening the
database read/write, which "notmuch show" has never needed to do
before).

So, in case it wasn't clear: i've thought pretty hard about these
commands and the options that users are likely to want or need.

But the current series (under discussion in this thread) should be
applied *without* those additional changes -- i don't want to have the
whole controversy about "notmuch show --decrypt=stash" now. :)

      --dkg

Re: [PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[David Bremner]

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

> No, we should not support --decrypt=nostash for show or reply.  The
> semantics of the display commands (show, reply) are such that they
> *never* modify the index or stash anything in there.  The equivalent for
> the indexing (new, insert, reindex) commands' "--decrypt=nostash" in the
> display commands is simply "--decrypt=true".
>

I'm not sure I completely agree, but its a trivial matter to add nostash
later if desired. And it's always easier to add API / command options
than to take them away.

d

Re: [PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[Daniel Kahn Gillmor]

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

On Fri 2017-12-29 10:30:00 -0400, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> No, we should not support --decrypt=nostash for show or reply.  The
>> semantics of the display commands (show, reply) are such that they
>> *never* modify the index or stash anything in there.  The equivalent for
>> the indexing (new, insert, reindex) commands' "--decrypt=nostash" in the
>> display commands is simply "--decrypt=true".
>
> I'm not sure I completely agree, but its a trivial matter to add nostash
> later if desired. And it's always easier to add API / command options
> than to take them away.

yes, true that we can always expand out, and it's more parsimonious to
start small.  :)

It sounds like you were suggesting "--decrypt=nostash" as a synonym for
"--decrypt=true" on show/reply, which i confess i didn't fully
understand when i wrote my response.  If it really makes you feel better
to add the alias/synonym, i wouldn't block such a change.

But I think the documentation is tricky to write (and trickier to read
and trickier still to understand!)  if "--decrypt=nostash" means the
same thing as "--decrypt=true" in one context, but they mean different
things in a different context.

I'm still trying to aim for that sweet spot where the smallest possible
API guides the user to sensible decisions while still understanding
what's going on. :)

       --dkg

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

Re: [PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[David Bremner]

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

> It sounds like you were suggesting "--decrypt=nostash" as a synonym for
> "--decrypt=true" on show/reply, which i confess i didn't fully
> understand when i wrote my response.  If it really makes you feel better
> to add the alias/synonym, i wouldn't block such a change.

Yes, that's what I was suggesting.

> But I think the documentation is tricky to write (and trickier to read
> and trickier still to understand!)  if "--decrypt=nostash" means the
> same thing as "--decrypt=true" in one context, but they mean different
> things in a different context.

I need more time to think about this, so I'd rather defer till after the
release in any case. But at some point we collectively (I think? Maybe
Jamie and I browbeat you into it) decided that it was OK for
--decrypt=true to have context dependent behaviour. It seems to me that
"different things in a different context" issue already exists.

d

Re: [PATCH v4 3/3] cli/reply: make --decrypt take a keyword

[Daniel Kahn Gillmor]

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

On Sat 2017-12-30 09:05:40 -0400, David Bremner wrote:
> I need more time to think about this, so I'd rather defer till after the
> release in any case. 

are you saying that you want to defer this whole series until after the
release?  that would be a real shame, since it would mean we'd have
both:

    notmuch show --decrypt

and

    notmuch new --decrypt=true

which seems particularly troubling.  please let's at least make it a
keyword in all cases.

> But at some point we collectively (I think? Maybe Jamie and I browbeat
> you into it) decided that it was OK for --decrypt=true to have context
> dependent behaviour.  It seems to me that "different things in a
> different context" issue already exists.

Oh, i'm not saying that "notmuch show --decrypt=true" must mean exactly
the same thing as "notmuch new --decrypt=true" -- i understand that it
does not mean the same thing, because the contexts are different.  My
complaint was that documenting "notmuch show --decrypt=nostash" seems
like it introduces confusion around the fact that --decrypt=nostash *is*
identical to --decrypt=true in one place, and is functionally
(significantly) different from --decrypt=true in another place.

iow: i'm fine with --decrypt=true being a coherent policy about message
decryption that does different things in different contexts.  I think
explaining about --decrypt=nostash being the same as that policy in some
contexts and different in others is pretty awkward, but if people really
want it, i won't block on it, and i look forward to seeing the patches
to the documentation.

   --dkg

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