[PATCH 2/3] devel/schemata: describe version 4

[PATCH 2/3] devel/schemata: describe version 4

[David Bremner]

Not all keys will necessarily be generated with a given version of gmime.
---
 devel/schemata | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/devel/schemata b/devel/schemata
index 00ebb7a6..30f6ac53 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -14,7 +14,7 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
 colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
 nil, true as t and false as nil.
 
-This is version 2 of the structured output format.
+This is version 4 of the structured output format.
 
 Version history
 ---------------
@@ -30,6 +30,9 @@ v3
 - Replaced message.filename string with a list of filenames.
 - Added part.content-disposition field.
 
+v4
+- replace signature error with a set of flags mirroring gmime 3.0 / gpgme signature status
+
 Common non-terminals
 --------------------
 
@@ -120,17 +123,33 @@ encstatus = [{status: "good"|"bad"}]
 sigstatus = [signature*]
 
 signature = {
-    # (signature_status_to_string)
-    status:         "none"|"good"|"bad"|"error"|"unknown",
-    # if status is "good":
+    flags:          sig_flags
+    # if flags["good"]:
     fingerprint?:   string,
     created?:       unix_time,
     expires?:       unix_time,
     userid?:        string
-    # if status is not "good":
+    # if flags["bad"] || flags["error"]:
     keyid?:         string
-    # if the signature has errors:
-    errors?:        int
+
+}
+
+sig_flags = {
+    # exactly one of the next three will be set
+    good?: bool,
+    bad?: bool,
+    error?: bool,
+    valid?: bool,
+    key-revoked?: bool,
+    key-expired?: bool,
+    sig-expired?: bool,
+    key-missing?: bool,
+    alg-unsupported?: bool,
+    crl-missing?: bool,
+    crl-too-old?: bool,
+    bad-policy?: bool,
+    sys-error?: bool,
+    tofu-conflict?: bool
 }
 
 notmuch search schema
-- 
2.11.0

Re: [PATCH 2/3] devel/schemata: describe version 4

[Daniel Kahn Gillmor]

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

On Wed 2017-05-31 08:45:34 -0300, David Bremner wrote:
> Not all keys will necessarily be generated with a given version of gmime.
> ---
>  devel/schemata | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/devel/schemata b/devel/schemata
> index 00ebb7a6..30f6ac53 100644
> --- a/devel/schemata
> +++ b/devel/schemata
> @@ -14,7 +14,7 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
>  colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
>  nil, true as t and false as nil.
>  
> -This is version 2 of the structured output format.
> +This is version 4 of the structured output format.
>  
>  Version history
>  ---------------
> @@ -30,6 +30,9 @@ v3
>  - Replaced message.filename string with a list of filenames.
>  - Added part.content-disposition field.
>  
> +v4
> +- replace signature error with a set of flags mirroring gmime 3.0 / gpgme signature status
> +
>  Common non-terminals
>  --------------------
>  
> @@ -120,17 +123,33 @@ encstatus = [{status: "good"|"bad"}]
>  sigstatus = [signature*]
>  
>  signature = {
> -    # (signature_status_to_string)
> -    status:         "none"|"good"|"bad"|"error"|"unknown",
> -    # if status is "good":
> +    flags:          sig_flags
> +    # if flags["good"]:
>      fingerprint?:   string,
>      created?:       unix_time,
>      expires?:       unix_time,
>      userid?:        string
> -    # if status is not "good":
> +    # if flags["bad"] || flags["error"]:
>      keyid?:         string
> -    # if the signature has errors:
> -    errors?:        int
> +
> +}
> +
> +sig_flags = {
> +    # exactly one of the next three will be set
> +    good?: bool,
> +    bad?: bool,
> +    error?: bool,
> +    valid?: bool,
> +    key-revoked?: bool,
> +    key-expired?: bool,
> +    sig-expired?: bool,
> +    key-missing?: bool,
> +    alg-unsupported?: bool,
> +    crl-missing?: bool,
> +    crl-too-old?: bool,
> +    bad-policy?: bool,
> +    sys-error?: bool,
> +    tofu-conflict?: bool
>  }

I understand that this is mapping to underlying gpgme/gmime status, but
the "exactly one of the next three will be set" business makes me
uncomfortable as an API.  what if it's not true?

If we've got a bitfield, we should expose it as a bitfield.  but if we
know that there is additionally a status component that has one of
exactly three values, we should expose it as its own element.

So i'd prefer:

  status: "good"|"bad"|"error",
  flags: sig_flags,
  # if status is "good":
   …

and then excise good? error? bad? from sig_flags.

what do y'all think?

     --dkg

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

Re: [PATCH 2/3] devel/schemata: describe version 4

[David Bremner]

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

> If we've got a bitfield, we should expose it as a bitfield.  but if we
> know that there is additionally a status component that has one of
> exactly three values, we should expose it as its own element.
>
> So i'd prefer:
>
>   status: "good"|"bad"|"error",
>   flags: sig_flags,
>   # if status is "good":

GMime 3.0 is just a thin wrapper around gpgme here, so we may as well consult
the latter docs. If I understand them [1] correctly, there are two
distinct "good" states, one called "VALID" and the other called
"GREEN". Alas, I had to look at the gpgme source to really understand
the intent here.

GREEN means the following

,----
|   if (sig->validity == GPGME_VALIDITY_FULL
|       || sig->validity == GPGME_VALIDITY_ULTIMATE)
|     {
|       if (gpg_err_code (sig->status) == GPG_ERR_NO_ERROR
| 	  || gpg_err_code (sig->status) == GPG_ERR_SIG_EXPIRED
| 	  || gpg_err_code (sig->status) == GPG_ERR_KEY_EXPIRED)
| 	sum |= GPGME_SIGSUM_GREEN;
|     }
`----

while VALID means in addition that the SIG_EXPIRED and KEY_EXPIRED
errors don't occur.

The subtlety here is that "error" and "green" are overlapping states
from gpgme's point of view.  One way of bridging this gap is have status
only reflect the "usability" [2] of the signature

status: "good" | "warning" | "bad" | "unknown" 

where the interested client can check the flags for details in the
latter 3 cases.

[1]: https://www.gnupg.org/documentation/manuals/gpgme/Verify.html
[2]: naming is hard

v2 human readable signature status

[David Bremner]

Based on the feedback from dkg, this is a less intrusive change to the
API, and just expands out the error flags.

This obsoletes [1]. I think that the current status values can be
recovered from the gmime 3.0 signature status, but it remains a bit of
an open question whether we should mimic the behaviour of gpgme in
reporting multiple good states.  See [2] for a discussion of that.
Given we can preserve our current api, perhaps we can just defer that
change to a later date.

[1] id:20170531114535.10506-1-david@tethera.net
[2] id:87k24x6qvq.fsf@tesseract.cs.unb.ca

[PATCH 1/3] emacs: convert to use format-version 3

[David Bremner]

Keeping the emacs interface running old format-versions makes it
impossible to test new format-versions or deprecate old ones.
---
 emacs/notmuch-address.el | 2 +-
 emacs/notmuch-mua.el     | 2 +-
 emacs/notmuch-query.el   | 2 +-
 emacs/notmuch-show.el    | 3 ++-
 emacs/notmuch-tree.el    | 2 +-
 emacs/notmuch.el         | 2 +-
 test/T310-emacs.sh       | 2 +-
 test/T450-emacs-show.sh  | 2 +-
 8 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index d504ff2d..74a4584f 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -317,7 +317,7 @@ execution, CALLBACK is called when harvesting finishes."
 			    (when config-query
 			      (format " and (%s)" config-query)))
 		  from-or-to-me-query))
-	 (args `("address" "--format=sexp" "--format-version=2"
+	 (args `("address" "--format=sexp" "--format-version=3"
 		 ,(if sent "--output=recipients" "--output=sender")
 		 "--deduplicate=address"
 		 ,query)))
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 93747b1c..ddfb9811 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -176,7 +176,7 @@ mutiple parts get a header."
   (unless (bolp) (insert "\n")))
 
 (defun notmuch-mua-reply (query-string &optional sender reply-all)
-  (let ((args '("reply" "--format=sexp" "--format-version=1"))
+  (let ((args '("reply" "--format=sexp" "--format-version=3"))
 	(process-crypto notmuch-show-process-crypto)
 	reply
 	original)
diff --git a/emacs/notmuch-query.el b/emacs/notmuch-query.el
index 436ad160..48acb551 100644
--- a/emacs/notmuch-query.el
+++ b/emacs/notmuch-query.el
@@ -30,7 +30,7 @@ A thread is a forest or list of trees. A tree is a two element
 list where the first element is a message, and the second element
 is a possibly empty forest of replies.
 "
-  (let ((args '("show" "--format=sexp" "--format-version=1")))
+  (let ((args '("show" "--format=sexp" "--format-version=3")))
     (if notmuch-show-process-crypto
 	(setq args (append args '("--decrypt"))))
     (setq args (append args search-terms))
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e7c22da0..21fcc036 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1660,9 +1660,10 @@ current thread."
 
 ;; dme: Would it make sense to use a macro for many of these?
 
+;; XXX TODO figure out what to do about multiple filenames
 (defun notmuch-show-get-filename ()
   "Return the filename of the current message."
-  (notmuch-show-get-prop :filename))
+  (car (notmuch-show-get-prop :filename)))
 
 (defun notmuch-show-get-header (header &optional props)
   "Return the named header of the current message, if any."
diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index d4d40761..6812d7e9 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -917,7 +917,7 @@ the same as for the function notmuch-tree."
     (notmuch-tag-clear-cache)
     (let ((proc (notmuch-start-notmuch
 		 "notmuch-tree" (current-buffer) #'notmuch-tree-process-sentinel
-		 "show" "--body=false" "--format=sexp" "--format-version=2"
+		 "show" "--body=false" "--format=sexp" "--format-version=3"
 		 message-arg search-args))
 	  ;; Use a scratch buffer to accumulate partial output.
 	  ;; This buffer will be killed by the sentinel, which
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 40b9fabd..a4a7fd8b 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -990,7 +990,7 @@ the configured default sort order."
       (save-excursion
 	(let ((proc (notmuch-start-notmuch
 		     "notmuch-search" buffer #'notmuch-search-process-sentinel
-		     "search" "--format=sexp" "--format-version=2"
+		     "search" "--format=sexp" "--format-version=3"
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
index ef599849..8a881941 100755
--- a/test/T310-emacs.sh
+++ b/test/T310-emacs.sh
@@ -1014,7 +1014,7 @@ YYY/notmuch_fail exited with status 1 (see *Notmuch errors* for more details)
 === ERROR ===
 [XXX]
 YYY/notmuch_fail exited with status 1
-command: YYY/notmuch_fail search --format\=sexp --format-version\=2 --sort\=newest-first tag\:inbox
+command: YYY/notmuch_fail search --format\=sexp --format-version\=3 --sort\=newest-first tag\:inbox
 exit status: 1"
 
 test_begin_subtest "Search handles subprocess warnings"
diff --git a/test/T450-emacs-show.sh b/test/T450-emacs-show.sh
index 74d546bd..d302efb6 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\\=1 --exclude\\=false \\' \\* \\'
+command: YYY/notmuch_fail show --format\\=sexp --format-version\\=3 --exclude\\=false \\' \\* \\'
 exit status: 1
 stderr:
 This is an error
-- 
2.11.0

[PATCH 2/3] devel/schemata: describe version 4

[David Bremner]

Replace numeric errors with human readable flags.  Not all sig_error
keys will necessarily be generated with a given version of gmime.

Drop status "none" as it's currrently unused and I don't know what
it's for.
---
 devel/schemata | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/devel/schemata b/devel/schemata
index 00ebb7a6..048b05c0 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -14,7 +14,7 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
 colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
 nil, true as t and false as nil.
 
-This is version 2 of the structured output format.
+This is version 4 of the structured output format.
 
 Version history
 ---------------
@@ -30,6 +30,10 @@ v3
 - Replaced message.filename string with a list of filenames.
 - Added part.content-disposition field.
 
+v4
+- replace signature error integer bitmask with a set of flags for
+  individual errors.
+
 Common non-terminals
 --------------------
 
@@ -121,7 +125,7 @@ sigstatus = [signature*]
 
 signature = {
     # (signature_status_to_string)
-    status:         "none"|"good"|"bad"|"error"|"unknown",
+    status:         "good"|"bad"|"error"|"unknown",
     # if status is "good":
     fingerprint?:   string,
     created?:       unix_time,
@@ -129,8 +133,20 @@ signature = {
     userid?:        string
     # if status is not "good":
     keyid?:         string
-    # if the signature has errors:
-    errors?:        int
+    errors: 	    sig_errors
+}
+
+sig_errors = {
+    key-revoked?: bool,
+    key-expired?: bool,
+    sig-expired?: bool,
+    key-missing?: bool,
+    alg-unsupported?: bool,
+    crl-missing?: bool,
+    crl-too-old?: bool,
+    bad-policy?: bool,
+    sys-error?: bool,
+    tofu-conflict?: bool
 }
 
 notmuch search schema
-- 
2.11.0

[PATCH 3/3] cli: implement structured output version 4

[David Bremner]

Since the error field is unused by the emacs front end, no changes are
needed other than bumping the format version number.
---
 emacs/notmuch-query.el  |  2 +-
 notmuch-client.h        |  2 +-
 notmuch-show.c          | 46 ++++++++++++++++++++++++++++++++++++++++++----
 test/T350-crypto.sh     |  4 ++--
 test/T355-smime.sh      |  4 ++--
 test/T450-emacs-show.sh |  2 +-
 6 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/emacs/notmuch-query.el b/emacs/notmuch-query.el
index 48acb551..592fd8f1 100644
--- a/emacs/notmuch-query.el
+++ b/emacs/notmuch-query.el
@@ -30,7 +30,7 @@ A thread is a forest or list of trees. A tree is a two element
 list where the first element is a message, and the second element
 is a possibly empty forest of replies.
 "
-  (let ((args '("show" "--format=sexp" "--format-version=3")))
+  (let ((args '("show" "--format=sexp" "--format-version=4")))
     (if notmuch-show-process-crypto
 	(setq args (append args '("--decrypt"))))
     (setq args (append args search-terms))
diff --git a/notmuch-client.h b/notmuch-client.h
index 62d4bcec..77b34184 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -145,7 +145,7 @@ chomp_newline (char *str)
  * this.  New (required) map fields can be added without increasing
  * this.
  */
-#define NOTMUCH_FORMAT_CUR 3
+#define NOTMUCH_FORMAT_CUR 4
 /* The minimum supported structured output format version.  Requests
  * for format versions below this will return an error. */
 #define NOTMUCH_FORMAT_MIN 1
diff --git a/notmuch-show.c b/notmuch-show.c
index accea48a..fdb7e79c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -340,6 +340,40 @@ signature_status_to_string (GMimeSignatureStatus x)
     return "unknown";
 }
 
+
+/* Print signature flags */
+static void
+format_signature_errors (sprinter_t *sp, GMimeSignature *signature)
+{
+    GMimeSignatureError errors = g_mime_signature_get_errors (signature);
+
+    if (errors == GMIME_SIGNATURE_ERROR_NONE)
+	return;
+
+    struct {
+	GMimeSignatureError bit;
+	const char * string;
+    } key_map[] = {
+	{ GMIME_SIGNATURE_ERROR_EXPSIG, "sig-expired" },
+	{ GMIME_SIGNATURE_ERROR_NO_PUBKEY, "key-missing"},
+	{ GMIME_SIGNATURE_ERROR_EXPKEYSIG, "key-expired"},
+	{ GMIME_SIGNATURE_ERROR_REVKEYSIG, "key-revoked"},
+	{ GMIME_SIGNATURE_ERROR_UNSUPP_ALGO, "alg-unsupported"},
+    };
+
+    sp->map_key (sp, "errors");
+    sp->begin_map (sp);
+
+    for (unsigned int i = 0; i < ARRAY_SIZE (key_map); i++) {
+	if (errors & key_map[i].bit) {
+	    sp->map_key (sp, key_map[i].string);
+	    sp->boolean (sp, TRUE);
+	}
+    }
+
+    sp->end (sp);
+}
+
 /* Signature status sprinter (GMime 2.6) */
 static void
 format_part_sigstatus_sprinter (sprinter_t *sp, mime_node_t *node)
@@ -404,10 +438,14 @@ format_part_sigstatus_sprinter (sprinter_t *sp, mime_node_t *node)
 	    }
 	}
 
-	GMimeSignatureError errors = g_mime_signature_get_errors (signature);
-	if (errors != GMIME_SIGNATURE_ERROR_NONE) {
-	    sp->map_key (sp, "errors");
-	    sp->integer (sp, errors);
+	if (notmuch_format_version <= 3) {
+	    GMimeSignatureError errors = g_mime_signature_get_errors (signature);
+	    if (errors != GMIME_SIGNATURE_ERROR_NONE) {
+		sp->map_key (sp, "errors");
+		sp->integer (sp, errors);
+	    }
+	} else {
+	    format_signature_errors (sp, signature);
 	}
 
 	sp->end (sp);
diff --git a/test/T350-crypto.sh b/test/T350-crypto.sh
index d21cad14..0753acf3 100755
--- a/test/T350-crypto.sh
+++ b/test/T350-crypto.sh
@@ -123,7 +123,7 @@ expected='[[[{"id": "XXXXX",
  "body": [{"id": 1,
  "sigstatus": [{"status": "error",
  "keyid": "'$(echo $FINGERPRINT | cut -c 25-)'",
- "errors": 2}],
+ "errors": {"key-missing": true}}],
  "content-type": "multipart/signed",
  "content": [{"id": 2,
  "content-type": "text/plain",
@@ -367,7 +367,7 @@ expected='[[[{"id": "XXXXX",
  "body": [{"id": 1,
  "sigstatus": [{"status": "error",
  "keyid": "6D92612D94E46381",
- "errors": 8}],
+ "errors": {"key-revoked": true}}],
  "content-type": "multipart/signed",
  "content": [{"id": 2,
  "content-type": "text/plain",
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 0f39bc69..03d24581 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -64,8 +64,8 @@ expected='[[[{"id": "XXXXX",
  "To": "test_suite@notmuchmail.org",
  "Date": "Sat, 01 Jan 2000 12:00:00 +0000"},
  "body": [{"id": 1,
- "sigstatus": [{"status": "good",
- "fingerprint": "'$FINGERPRINT'",
+ "sigstatus": [{"fingerprint": "'$FINGERPRINT'",
+ "status": "good",
  "expires": 424242424,
  "created": 946728000}],
  "content-type": "multipart/signed",
diff --git a/test/T450-emacs-show.sh b/test/T450-emacs-show.sh
index d302efb6..c4bc5ce0 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\\=3 --exclude\\=false \\' \\* \\'
+command: YYY/notmuch_fail show --format\\=sexp --format-version\\=4 --exclude\\=false \\' \\* \\'
 exit status: 1
 stderr:
 This is an error
-- 
2.11.0

Re: [PATCH 2/3] devel/schemata: describe version 4

[David Bremner]

David Bremner <david@tethera.net> writes:

> Replace numeric errors with human readable flags.  Not all sig_error
> keys will necessarily be generated with a given version of gmime.
>
> Drop status "none" as it's currrently unused and I don't know what
> it's for.
> ---
>  devel/schemata | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/devel/schemata b/devel/schemata
> index 00ebb7a6..048b05c0 100644
> --- a/devel/schemata
> +++ b/devel/schemata
> @@ -14,7 +14,7 @@ are interleaved. Keys are printed as keywords (symbols preceded by a
>  colon), e.g. (:id "123" :time 54321 :from "foobar"). Null is printed as
>  nil, true as t and false as nil.
>  
> -This is version 2 of the structured output format.
> +This is version 4 of the structured output format.
>  
>  Version history
>  ---------------
> @@ -30,6 +30,10 @@ v3
>  - Replaced message.filename string with a list of filenames.
>  - Added part.content-disposition field.
>  
> +v4
> +- replace signature error integer bitmask with a set of flags for
> +  individual errors.
> +
>  Common non-terminals
>  --------------------
>  
> @@ -121,7 +125,7 @@ sigstatus = [signature*]
>  
>  signature = {
>      # (signature_status_to_string)
> -    status:         "none"|"good"|"bad"|"error"|"unknown",
> +    status:         "good"|"bad"|"error"|"unknown",
>      # if status is "good":
>      fingerprint?:   string,
>      created?:       unix_time,
> @@ -129,8 +133,20 @@ signature = {
>      userid?:        string
>      # if status is not "good":
>      keyid?:         string
> -    # if the signature has errors:
> -    errors?:        int
> +    errors: 	    sig_errors

I vacillated back and forth, but currently the code only shows the error
map when it's non-empty. So I've updated that to errors?: in git.