Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query

2020-05-21 Thread Anton Khirnov
ping

-- 
Anton Khirnov
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query

2020-05-21 Thread David Bremner
Anton Khirnov  writes:

> ping

I'm hoping Floris will find a chance to review your patches.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: test suite: FIXED messages are misordered with tests

2020-05-21 Thread Daniel Kahn Gillmor
On Thu 2020-05-21 00:16:48 +0300, Tomi Ollila wrote:
> (just tested this latest works)

Thanks for looking into this, Tomi!

Do you have a patch to propose?

   --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

2020-05-21 Thread David Bremner
Daniel Kahn Gillmor  writes:

> When checking cryptographic signatures, Notmuch relies on GMime to
> tell it whether the certificate that signs a message has a valid User
> ID or not.
>
> If the User ID is not valid, then notmuch does not report the signer's
> User ID to the user.  This means that the consumer of notmuch's
> cryptographic summary of a message (or of its protected headers) can
> be confident in relaying the reported identity to the user.
>
> However, some versions of GMime before 3.2.7 cannot report Certificate
> validity for X.509 certificates.  This is resolved upstream in GMime
> at https://github.com/jstedfast/gmime/pull/90.
>
> We adapt to this by marking tests of reported User IDs for
> S/MIME-signed messages as known-broken if GMime is older than 3.2.7
> and has not been patched.
>
> If GMime >= 3.2.7 and certificate validity still doesn't work for
> X.509 certs, then there has likely been a regression in GMime and we
> should fail early, during ./configure.
>
> To break out these specific User ID checks from other checks, i had to
> split some tests into two parts, and reuse $output across the two
> subtests.
>
> Signed-off-by: Daniel Kahn Gillmor 
> ---
>  configure  | 79 ++
>  test/T355-smime.sh | 17 +---
>  test/T356-protected-headers.sh | 13 +-
>  3 files changed, 100 insertions(+), 9 deletions(-)
>
> diff --git a/configure b/configure
> index 0cfdaa6f..92e5bd1b 100755
> --- a/configure
> +++ b/configure
> @@ -536,6 +536,82 @@ EOF
>  if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
>  rm -rf "$TEMP_GPG"
>  fi
> +
> +# see https://github.com/jstedfast/gmime/pull/90
> +# should be fixed in GMime in 3.2.7, but some distros might patch
> +printf "Checking for GMime X.509 certificate validity... "
> +
> +cat > _check_x509_validity.c < +#include 
> +#include 
> +
> +int main () {
> +GError *error = NULL;
> +GMimeParser *parser = NULL;
> +GMimeApplicationPkcs7Mime *body = NULL;
> +GMimeSignatureList *sig_list = NULL;
> +GMimeSignature *sig = NULL;
> +GMimeCertificate *cert = NULL;
> +GMimeObject *output = NULL;
> +GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
> +int len;
> +
> +g_mime_init ();
> +parser = g_mime_parser_new ();
> +g_mime_parser_init_with_stream (parser, 
> g_mime_stream_file_open("test/corpora/pkcs7/smime-onepart-signed.eml", "r", 
> ));
> +if (error) return !! fprintf (stderr, "failed to instantiate parser with 
> test/corpora/pkcs7/smime-onepart-signed.eml\n");
> +
> +body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part 
> (g_mime_parser_construct_message (parser, NULL)));
> +if (body == NULL) return !!  fprintf (stderr, "did not find a 
> application/pkcs7 message\n");

I find these long lines with !! in the middle pretty surprising. Is
there some reason for this style? It doesn't seem to fit with the usual
conventions.

This line in particular has a tab in the middle.


> +elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c 
> ${gmime_ldflags} -o _check_x509_validity \

The other test files are cleaned up in configure (source and binary)
once we are done with them.

As far as I could follow, the changes to the tests themselves look
reasonable.

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] emacs: add notmuch-expr, sexp-style queries

2020-05-21 Thread Daniel Kahn Gillmor
On Wed 2020-05-13 20:00:24 +1000, Tom Fitzhenry wrote:
> notmuch-expr allows you to write notmuch search queries in sexp style like:
>
> (notmuch-expr
>   '(and
> (to "emacs-devel")
> "info manual"
> (or
>   (not (is "spam"))
>   (is "important"
>
> which will generate the textual query:
>
> "to:emacs-devel AND (NOT is:spam OR is:important) AND \"info manual\""

I like this idea!

> +(defun notmuch-expr--quote (s)
> +  ;; FIXME Escape s.
> +  (concat "\"" s "\""))

Shouldn't this FIXME be resolved before we consider merging?

  --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: test suite: FIXED messages are misordered with tests

2020-05-21 Thread Tomi Ollila
On Thu, May 21 2020, Daniel Kahn Gillmor wrote:

> On Thu 2020-05-21 00:16:48 +0300, Tomi Ollila wrote:
>> (just tested this latest works)
>
> Thanks for looking into this, Tomi!
>
> Do you have a patch to propose?

Looked a bit (now). Somewhat complicated to make perfect (enemy of good)
change.

print_test_description could be added as first line in 
test_known_broken_ok_ (then inconsistent with test_known_broken_failure_)
or in test_ok_ just before calling test_known_broken_ok_ 
(test_known_broken_ok_ is called only once in test-lib.sh)

but it looks like test_check_missing_external_prereqs_ and
test_report_skip_ may be called before print_test_description is called
(and there may be more...)

We've accumulated quite a bit of mess during these years to the test
system, which makes it harder to do larger adjustments (and not (yet)
mentioning even larger refactorings...).

>
>--dkg

Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH 2/2 v3] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

2020-05-21 Thread Daniel Kahn Gillmor
When checking cryptographic signatures, Notmuch relies on GMime to
tell it whether the certificate that signs a message has a valid User
ID or not.

If the User ID is not valid, then notmuch does not report the signer's
User ID to the user.  This means that the consumer of notmuch's
cryptographic summary of a message (or of its protected headers) can
be confident in relaying the reported identity to the user.

However, some versions of GMime before 3.2.7 cannot report Certificate
validity for X.509 certificates.  This is resolved upstream in GMime
at https://github.com/jstedfast/gmime/pull/90.

We adapt to this by marking tests of reported User IDs for
S/MIME-signed messages as known-broken if GMime is older than 3.2.7
and has not been patched.

If GMime >= 3.2.7 and certificate validity still doesn't work for
X.509 certs, then there has likely been a regression in GMime and we
should fail early, during ./configure.

To break out these specific User ID checks from other checks, i had to
split some tests into two parts, and reuse $output across the two
subtests.

Signed-off-by: Daniel Kahn Gillmor 
---
 configure  | 83 +-
 test/T355-smime.sh | 17 ---
 test/T356-protected-headers.sh | 13 +-
 3 files changed, 102 insertions(+), 11 deletions(-)

diff --git a/configure b/configure
index 0cfdaa6f..37368bda 100755
--- a/configure
+++ b/configure
@@ -494,7 +494,7 @@ int main () {
 if (error) return !! fprintf (stderr, "failed to instantiate parser with 
test/corpora/crypto/basic-encrypted.eml\n");
 
 body = GMIME_MULTIPART_ENCRYPTED(g_mime_message_get_mime_part 
(g_mime_parser_construct_message (parser, NULL)));
-if (body == NULL) return !!fprintf (stderr, "did not find a 
multipart encrypted message\n");
+if (body == NULL) return !! fprintf (stderr, "did not find a multipart 
encrypted message\n");
 
 output = g_mime_multipart_encrypted_decrypt (body, 
GMIME_DECRYPT_EXPORT_SESSION_KEY, NULL, _result, );
 if (error || output == NULL) return !! fprintf (stderr, "decryption 
failed\n");
@@ -536,6 +536,82 @@ EOF
 if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
 rm -rf "$TEMP_GPG"
 fi
+
+# see https://github.com/jstedfast/gmime/pull/90
+# should be fixed in GMime in 3.2.7, but some distros might patch
+printf "Checking for GMime X.509 certificate validity... "
+
+cat > _check_x509_validity.c <
+#include 
+
+int main () {
+GError *error = NULL;
+GMimeParser *parser = NULL;
+GMimeApplicationPkcs7Mime *body = NULL;
+GMimeSignatureList *sig_list = NULL;
+GMimeSignature *sig = NULL;
+GMimeCertificate *cert = NULL;
+GMimeObject *output = NULL;
+GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
+int len;
+
+g_mime_init ();
+parser = g_mime_parser_new ();
+g_mime_parser_init_with_stream (parser, 
g_mime_stream_file_open("$srcdir/test/corpora/pkcs7/smime-onepart-signed.eml", 
"r", ));
+if (error) return !! fprintf (stderr, "failed to instantiate parser with 
test/corpora/pkcs7/smime-onepart-signed.eml\n");
+
+body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part 
(g_mime_parser_construct_message (parser, NULL)));
+if (body == NULL) return !!fprintf (stderr, "did not find a 
application/pkcs7 message\n");
+
+sig_list = g_mime_application_pkcs7_mime_verify (body, GMIME_VERIFY_NONE, 
, );
+if (error || output == NULL) return !! fprintf (stderr, "verify failed\n");
+
+if (sig_list == NULL) return !! fprintf (stderr, "no GMimeSignatureList 
found\n");
+len = g_mime_signature_list_length (sig_list);
+if (len != 1) return !! fprintf (stderr, "expected 1 signature, got %d\n", 
len);
+sig = g_mime_signature_list_get_signature (sig_list, 0);
+if (sig == NULL) return !! fprintf (stderr, "no GMimeSignature found at 
position 0\n");
+cert = g_mime_signature_get_certificate (sig);
+if (cert == NULL) return !! fprintf (stderr, "no GMimeCertificate 
found\n");
+validity = g_mime_certificate_get_id_validity (cert);
+if (validity != GMIME_VALIDITY_FULL) return !! fprintf (stderr, "Got 
validity %d, expected %d\n", validity, GMIME_VALIDITY_FULL);
+
+return 0;
+}
+EOF
+if ! TEMP_GPG=$(mktemp -d "${TMPDIR:-/tmp}/notmuch.XX"); then
+printf 'No.\nCould not make tempdir for testing X.509 certificate 
validity support.\n'
+errors=$((errors + 1))
+elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c 
${gmime_ldflags} -o _check_x509_validity \
+&& echo disable-crl-checks > "$TEMP_GPG/gpgsm.conf" \
+&& echo 
"4D:E0:FF:63:C0:E9:EC:01:29:11:C8:7A:EE:DA:3A:9A:7F:6E:C1:0D S" >> 
"$TEMP_GPG/trustlist.txt" \
+&& GNUPGHOME=${TEMP_GPG} gpgsm --batch --quiet --import < 
"$srcdir"/test/smime/ca.crt
+then
+if GNUPGHOME=${TEMP_GPG} ./_check_x509_validity; then
+gmime_x509_cert_validity=1
+printf "Yes.\n"

Re: [PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

2020-05-21 Thread Daniel Kahn Gillmor
Thanks for the review, and for the poke about out-of-tree builds on IRC,
Bremner.  Another revision is coming in a minute.  Notes below…

On Thu 2020-05-21 20:29:05 -0300, David Bremner wrote:
> I find these long lines with !! in the middle pretty surprising. Is
> there some reason for this style? It doesn't seem to fit with the usual
> conventions.

Hm, do we even have conventions for inlined C in ./configure?

If you'd rather i expand these to something more verbose, i can do so,
but i was under the impression that we wanted to keep these C
interstitials fairly compact so that ./configure is still (somewhat)
readable as a shell script.

The "return !! fprintf…" idiom is a compact way to get a non-zero
process error code and an error message to stderr without introducing a
code block.  The only way for fprintf to return 0 (which would result in
the process returning 0) is if 0 bytes are written and no error
occurred, which isn't possible with any of the format strings supplied
here.

Another approach would be to pull the C entirely out of ./configure, but
that could have a problem when dealing with out-of-tree builds.  (i just
noticed a problem for this test with out-of-tree builds, which i'll
revise in a minute)

> This line in particular has a tab in the middle.

i dunno how that got there, i'll have it fixed in the upcoming revision.

>> +elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c 
>> ${gmime_ldflags} -o _check_x509_validity \
>
> The other test files are cleaned up in configure (source and binary)
> once we are done with them.

good point, i'll ensure that they get cleaned up alongside
_check_session_keys* in the upcoming revision.

> As far as I could follow, the changes to the tests themselves look
> reasonable.

thanks for the thoughtful review!

   --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 3/5] build: optionally build python-cffi bindings

2020-05-21 Thread Daniel Kahn Gillmor
On Mon 2019-11-04 23:26:25 +0200, Tomi Ollila wrote:
> how bad does out-of-tree build break with this -- do we need to do
> the same as with ruby bindings (copy sources -- do we still do so)? or does
> python provide better alternative..?

I've just posted id:20200522010359.715688-1-...@fifthhorseman.net to
apply the same hack as the ruby bindings here.  I welcome any better
proposals, but think we should at least have the build complete!

--dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query

2020-05-21 Thread Floris Bruynooghe
Hi Anton,

Thanks for improving the bindings!  Any my apologies for the late
response, I failed to spot this mail the first time round.

Also, this is a pretty serious bug, thanks for finding it.

This looks pretty solid, a few small style comments that aren't very
important notwithstanding.  Though I do think you should be able to add
a test for this in the pytest tests.  I think just triggering the
use-after-free you demonstrate in the commit message is fine as it will
work with your patch.  The fact it will crash rather then fail without
is unfortunate but probably fine.

On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote:

> Any messages retrieved from a query - either directly via
> search_messages() or indirectly via thread objects - are owned by that
> query. Retrieving the same message (i.e. corresponding to the same
> message ID / database object) several times will always yield the same
> C object.
>
> The caller is allowed to destroy message objects owned by a query before
> the query itself - which can save memory for long-lived queries.
> However, that message must then never be retrieved again from that
> query.
>
> The python-notmuch2 bindings will currently destroy every message object
> in Message._destroy(), which will lead to an invalid free if the same
> message is then retrieved again. E.g. the following python program leads
> to libtalloc abort()ing:
>
> import notmuch2
> db   = notmuch2.Database(mode = notmuch2.Database.MODE.READ_ONLY)
> t= next(db.threads('*'))
> msgs = list(zip(t.toplevel(), t.toplevel()))
> msgs = list(zip(t.toplevel(), t.toplevel()))
>
> Fix this issue by creating a subclass of Message, which is used for
> "standalone" message which have to be freed by the caller. Message class
> is then used only for messages descended from a query, which do not need
> to be freed by the caller.
> ---
>  bindings/python-cffi/notmuch2/_database.py |  6 ++--
>  bindings/python-cffi/notmuch2/_message.py  | 42 ++
>  2 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_database.py 
> b/bindings/python-cffi/notmuch2/_database.py
> index 95f59ca0..f14eac78 100644
> --- a/bindings/python-cffi/notmuch2/_database.py
> +++ b/bindings/python-cffi/notmuch2/_database.py
> @@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
>capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
>  if ret not in ok:
>  raise errors.NotmuchError(ret)
> -msg = message.Message(self, msg_pp[0], db=self)
> +msg = message.StandaloneMessage(self, msg_pp[0], db=self)
>  if sync_flags:
>  msg.tags.from_maildir_flags()
>  return self.AddedMessage(
> @@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
>  msg_p = msg_pp[0]
>  if msg_p == capi.ffi.NULL:
>  raise LookupError
> -msg = message.Message(self, msg_p, db=self)
> +msg = message.StandaloneMessage(self, msg_p, db=self)
>  return msg
>  
>  def get(self, filename):
> @@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
>  msg_p = msg_pp[0]
>  if msg_p == capi.ffi.NULL:
>  raise LookupError
> -msg = message.Message(self, msg_p, db=self)
> +msg = message.StandaloneMessage(self, msg_p, db=self)
>  return msg
>  
>  @property
> diff --git a/bindings/python-cffi/notmuch2/_message.py 
> b/bindings/python-cffi/notmuch2/_message.py
> index c5fdbf6d..416ce7ca 100644
> --- a/bindings/python-cffi/notmuch2/_message.py
> +++ b/bindings/python-cffi/notmuch2/_message.py
> @@ -14,7 +14,7 @@ __all__ = ['Message']
>  
>  
>  class Message(base.NotmuchObject):
> -"""An email message stored in the notmuch database.
> +"""An email message stored in the notmuch database retrieved via a query.
>  
>  This should not be directly created, instead it will be returned
>  by calling methods on :class:`Database`.  A message keeps a
> @@ -61,22 +61,10 @@ class Message(base.NotmuchObject):
>  
>  @property
>  def alive(self):
> -if not self._parent.alive:
> -return False
> -try:
> -self._msg_p
> -except errors.ObjectDestroyedError:
> -return False
> -else:
> -return True
> -
> -def __del__(self):
> -self._destroy()
> +return self._parent.alive
>  
>  def _destroy(self):
> -if self.alive:
> -capi.lib.notmuch_message_destroy(self._msg_p)
> -self._msg_p = None
> +pass

I feel like some more description from the commit message could live
here to explain why this isn't destroying the message rather than having
to find it in the commit message.

>  
>  @property
>  def messageid(self):
> @@ -375,6 +363,30 @@ class Message(base.NotmuchObject):
>  if isinstance(other, self.__class__):
>  return self.messageid == other.messageid
>  
> +class 

[PATCH] tests: fix test_json_nodes() in out-of-tree builds

2020-05-21 Thread Daniel Kahn Gillmor
In out-of-tree builds, $TEST_DIRECTORY doesn't contain
json_check_nodes.py.  This caused 27 tests to fail in such an
environment.

Signed-off-by: Daniel Kahn Gillmor 
---
 test/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 58972339..792b1cb9 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -556,7 +556,7 @@ test_json_nodes () {
 
if ! test_skip "$test_subtest_name"
then
-   output=$(PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON 
"$TEST_DIRECTORY"/json_check_nodes.py "$@")
+   output=$(PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON 
"$NOTMUCH_SRCDIR"/test/json_check_nodes.py "$@")
if [ "$?" = 0 ]
then
test_ok_
-- 
2.26.2

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: waiting tag

2020-05-21 Thread Daniel Kahn Gillmor
On Mon 2020-05-04 09:25:59 +0200, Gregor Zattler wrote:
> * Keegan Carruthers-Smith  [2020-05-03; 22:37]:
>>   notmuch tag -waiting -- tag:waiting and 'thread:{tag:new}'
>
> but this removes the waiting tag if there is some response
> to some message in the thread, not necessary to the message
> tagged waiting, isn't it?

Yes, you're right, this is not exactly the thing you were asking for.

fwiw, i don't know how to provide the thing you were asking for either
:(

--dkg
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] emacs docstrings: consistent indentation, newlines, periods

2020-05-21 Thread Daniel Kahn Gillmor
On Mon 2020-05-04 00:21:36 +0300, Tomi Ollila wrote:
> Fixed emacs docstrings to be consistent. No functional change.
>
> - removed some (accidental) indentation
> - removed some trailing newlines
> - added trailing periods where missing (some exclusions)

This all looks good to me, except for the following changes.

I think if the last line of the docstring is an example, deliberately
indented to indicate that it is verbatim code, adding a trailing period
is a mistake, because it encourages naive users to make the obvious
error of including the period in their text.

These were the examples i noticed in a skim of the changeset:

> --- a/emacs/notmuch-draft.el
> +++ b/emacs/notmuch-draft.el
> @@ -45,7 +45,7 @@ (defcustom notmuch-draft-tags '("+draft")
>  For example, if you wanted to give the message a \"draft\" tag
>  but not the (normally added by default) \"inbox\" tag, you would
>  set:
> -(\"+draft\" \"-inbox\")"
> +(\"+draft\" \"-inbox\")."
>:type '(repeat string)
>:group 'notmuch-draft)
>  


> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -145,7 +145,7 @@ (defcustom notmuch-archive-tags '("-inbox")
>  
>  For example, if you wanted to remove an \"inbox\" tag and add an
>  \"archived\" tag, you would set:
> -(\"-inbox\" \"+archived\")"
> +(\"-inbox\" \"+archived\")."
>:type '(repeat string)
>:group 'notmuch-search
>:group 'notmuch-show)


> --- a/emacs/notmuch-message.el
> +++ b/emacs/notmuch-message.el
> @@ -33,7 +33,7 @@ (defcustom notmuch-message-replied-tags '("+replied")
>  
>  For example, if you wanted to add a \"replied\" tag and remove
>  the \"inbox\" and \"todo\" tags, you would set:
> -(\"+replied\" \"-inbox\" \"-todo\")"
> +(\"+replied\" \"-inbox\" \"-todo\")."
>:type '(repeat string)
>:group 'notmuch-send)
>  
> @@ -46,7 +46,7 @@ (defcustom notmuch-message-forwarded-tags '("+forwarded")
>  
>  For example, if you wanted to add a \"forwarded\" tag and remove
>  the \"inbox\" tag, you would set:
> -(\"+forwarded\" \"-inbox\")"
> +(\"+forwarded\" \"-inbox\")."
>:type '(repeat string)
>:group 'notmuch-send)


> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -237,7 +237,7 @@ (defcustom notmuch-show-mark-read-tags '("-unread")
>  
>  For example, if you wanted to remove an \"unread\" tag and add a
>  \"read\" tag (which would make little sense), you would set:
> -(\"-unread\" \"+read\")"
> +(\"-unread\" \"+read\")."
>:type '(repeat string)
>:group 'notmuch-show)


I'd prefer this changeset to not have trailing periods when the
docstring ends in an example.   (and maybe also to clean up any trailing
periods that might already be in such a docstring, if they exist)

I recognize this is something of an aesthetic position with no
objectively correct answer, but so is the question of trailing periods,
i suppose.  I'm not going to die on this hill, though.

Regards,

--dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] emacs: split-window-sensibly in tree mode with open message

2020-05-21 Thread Daniel Kahn Gillmor
On Sat 2020-05-02 20:11:09 -0400, Radu Butoi wrote:

> This uses the standard Emacs function `split-window-sensibly` to split a
> window horizontally or vertically depending on space when opening a
> message in tree view. By default, split-width-threshold is 160 columns
> (and -height- is nil), so screens wider than 160 will be split
> horizontally.
>
> This is based on an older proposal [1] which manually did the
> calculation of width. The main issues there were (1) lack of
> configurability and (2) lack of testing. I don't have an answer for
> testing, but this allows users to configure two thresholds using
> built-in variables, an improvement.

I like this proposal, and the simplification that it gives to the
notmuch-emacs codebase.  However, this thread is the first place i've
learned about split-window-sensibly, so i'm probably not eligible to
really judge the merits here.

As far as testing goes, a test would be nice -- is this something you
could add to test/T460-emacs-tree.sh ?  Testing UI/UX issues is always
pretty tough though, and it's not clear to me that we're actually
already testing the existing "(/ (window-height) 4)" business anyway.

   --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: test suite: FIXED messages are misordered with tests

2020-05-21 Thread Daniel Kahn Gillmor
On Fri 2020-05-22 00:57:14 +0300, Tomi Ollila wrote:
> We've accumulated quite a bit of mess during these years to the test
> system, which makes it harder to do larger adjustments (and not (yet)
> mentioning even larger refactorings...).

The last significant refactoring was probably when jrollins and i got
the test suite running in parallel, and that might not even qualify as
"major".  Of course, the most effective tool for encouraging any
refactoring is having a robust and reliable test suite to make sure that
nothing broke.  But now we're talking about a test suite for the test
suite…  i dunno how far we want to go down the rabbit hole!

--dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


[PATCH] python-cffi: enable out-of-tree builds

2020-05-21 Thread Daniel Kahn Gillmor
This is a simple hack to enable out-of-tree builds, a concern raised
by Tomi in id:m24kzjib9a@guru.guru-group.fi

This change at least enables "make check" to complete without error,
but I'm sure it could be improved.  I am not expert enough in
setuptools to know how.

Signed-off-by: Daniel Kahn Gillmor 
---
 configure | 8 
 1 file changed, 8 insertions(+)

diff --git a/configure b/configure
index 37368bda..f0472e8e 100755
--- a/configure
+++ b/configure
@@ -70,6 +70,14 @@ if [ "$srcdir" != "." ]; then
 mkdir bindings/ruby
 cp -a "$srcdir"/bindings/ruby/*.[ch] bindings/ruby
 cp -a "$srcdir"/bindings/ruby/extconf.rb bindings/ruby
+
+# Use the same hack to replicate python-cffi source for
+# out-of-tree builds (again, not ideal).
+mkdir bindings/python-cffi
+cp -a "$srcdir"/bindings/python-cffi/tests \
+   "$srcdir"/bindings/python-cffi/notmuch2 \
+   "$srcdir"/bindings/python-cffi/setup.py \
+   bindings/python-cffi/
 fi
 
 # Set several defaults (optionally specified by the user in
-- 
2.26.2

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


python-cffi and ruby test suites fail in out-of-tree builds

2020-05-21 Thread Daniel Kahn Gillmor
Hey folks--

I just did a bit of testing and cleanup for out-of-tree builds (see the
minor patches that should have landed on the list in the last hour or
two).

For me, "make check" in an out-of-tree build works fine now, with the
exception of T391-python-cffi.sh and T395-ruby.sh.

I'm afraid i don't know enough about either python or ruby to see the
obvious fix for these builds, but i do note that they're the only builds
which depend on copying their source into the out-of-tree location
(assuming no one proposes an improvement for
id:20200522010359.715688-1-...@fifthhorseman.net).

Just thought i'd flag this as an outstanding problem that hopefully
someone clever feels like digging into.

--dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] emacs docstrings: consistent indentation, newlines, periods

2020-05-21 Thread Tomi Ollila
On Thu, May 21 2020, Daniel Kahn Gillmor wrote:

> On Mon 2020-05-04 00:21:36 +0300, Tomi Ollila wrote:
>> Fixed emacs docstrings to be consistent. No functional change.
>>
>> - removed some (accidental) indentation
>> - removed some trailing newlines
>> - added trailing periods where missing (some exclusions)
>
> This all looks good to me, except for the following changes.
>
> I think if the last line of the docstring is an example, deliberately
> indented to indicate that it is verbatim code, adding a trailing period
> is a mistake, because it encourages naive users to make the obvious
> error of including the period in their text.
>
> These were the examples i noticed in a skim of the changeset:
>
>> --- a/emacs/notmuch-draft.el
>> +++ b/emacs/notmuch-draft.el
>> @@ -45,7 +45,7 @@ (defcustom notmuch-draft-tags '("+draft")
>>  For example, if you wanted to give the message a \"draft\" tag
>>  but not the (normally added by default) \"inbox\" tag, you would
>>  set:
>> -(\"+draft\" \"-inbox\")"
>> +(\"+draft\" \"-inbox\")."
>>:type '(repeat string)
>>:group 'notmuch-draft)
>>  
>
>
>> --- a/emacs/notmuch-lib.el
>> +++ b/emacs/notmuch-lib.el
>> @@ -145,7 +145,7 @@ (defcustom notmuch-archive-tags '("-inbox")
>>  
>>  For example, if you wanted to remove an \"inbox\" tag and add an
>>  \"archived\" tag, you would set:
>> -(\"-inbox\" \"+archived\")"
>> +(\"-inbox\" \"+archived\")."
>>:type '(repeat string)
>>:group 'notmuch-search
>>:group 'notmuch-show)
>
>
>> --- a/emacs/notmuch-message.el
>> +++ b/emacs/notmuch-message.el
>> @@ -33,7 +33,7 @@ (defcustom notmuch-message-replied-tags '("+replied")
>>  
>>  For example, if you wanted to add a \"replied\" tag and remove
>>  the \"inbox\" and \"todo\" tags, you would set:
>> -(\"+replied\" \"-inbox\" \"-todo\")"
>> +(\"+replied\" \"-inbox\" \"-todo\")."
>>:type '(repeat string)
>>:group 'notmuch-send)
>>  
>> @@ -46,7 +46,7 @@ (defcustom notmuch-message-forwarded-tags '("+forwarded")
>>  
>>  For example, if you wanted to add a \"forwarded\" tag and remove
>>  the \"inbox\" tag, you would set:
>> -(\"+forwarded\" \"-inbox\")"
>> +(\"+forwarded\" \"-inbox\")."
>>:type '(repeat string)
>>:group 'notmuch-send)
>
>
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -237,7 +237,7 @@ (defcustom notmuch-show-mark-read-tags '("-unread")
>>  
>>  For example, if you wanted to remove an \"unread\" tag and add a
>>  \"read\" tag (which would make little sense), you would set:
>> -(\"-unread\" \"+read\")"
>> +(\"-unread\" \"+read\")."
>>:type '(repeat string)
>>:group 'notmuch-show)
>
>
> I'd prefer this changeset to not have trailing periods when the
> docstring ends in an example.   (and maybe also to clean up any trailing
> periods that might already be in such a docstring, if they exist)
>
> I recognize this is something of an aesthetic position with no
> objectively correct answer, but so is the question of trailing periods,
> i suppose.  I'm not going to die on this hill, though.

I agree with your point. Like I mentioned I excluded trailing period at
least in one case where missing, probably similar to this. I have to
reconsider also to re-indent some content after For example: 

How I did the changeset I used David's rstdoc.el to get long list of all
docstrings and then looked through that and went editing original .el
files -- monotonic changes makes one blind of all kinds of considerations.

>
> Regards,
>
> --dkg

Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 2/2] python/notmuch2: add bindings for the database config strings

2020-05-21 Thread Floris Bruynooghe
Thanks for adding more of the API!

This mostly is fine as well, again I'd mainly ask to add tests however.
At least the things which are implemented directly I guess: setitem,
getitem, iter and len.


On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote:

> ---
>  bindings/python-cffi/notmuch2/_build.py| 17 +
>  bindings/python-cffi/notmuch2/_config.py   | 84 ++
>  bindings/python-cffi/notmuch2/_database.py | 23 ++
>  3 files changed, 124 insertions(+)
>  create mode 100644 bindings/python-cffi/notmuch2/_config.py
>
> diff --git a/bindings/python-cffi/notmuch2/_build.py 
> b/bindings/python-cffi/notmuch2/_build.py
> index 5e1fcac1..f269f2a1 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -314,6 +314,23 @@ ffibuilder.cdef(
>  notmuch_indexopts_get_decrypt_policy (const notmuch_indexopts_t 
> *indexopts);
>  void
>  notmuch_indexopts_destroy (notmuch_indexopts_t *options);
> +
> +notmuch_status_t
> +notmuch_database_set_config (notmuch_database_t *db, const char *key, 
> const char *value);
> +notmuch_status_t
> +notmuch_database_get_config (notmuch_database_t *db, const char *key, 
> char **value);
> +notmuch_status_t
> +notmuch_database_get_config_list (notmuch_database_t *db, const char 
> *prefix, notmuch_config_list_t **out);
> +notmuch_bool_t
> +notmuch_config_list_valid (notmuch_config_list_t *config_list);
> +const char *
> +notmuch_config_list_key (notmuch_config_list_t *config_list);
> +const char *
> +notmuch_config_list_value (notmuch_config_list_t *config_list);
> +void
> +notmuch_config_list_move_to_next (notmuch_config_list_t *config_list);
> +void
> +notmuch_config_list_destroy (notmuch_config_list_t *config_list);
>  """
>  )
>  
> diff --git a/bindings/python-cffi/notmuch2/_config.py 
> b/bindings/python-cffi/notmuch2/_config.py
> new file mode 100644
> index ..58383c16
> --- /dev/null
> +++ b/bindings/python-cffi/notmuch2/_config.py
> @@ -0,0 +1,84 @@
> +import collections.abc
> +
> +import notmuch2._base as base
> +import notmuch2._capi as capi
> +import notmuch2._errors as errors
> +
> +__all__ = ['ConfigMapping']
> +

Same style thing about 2 blank lines

> +class ConfigIter(base.NotmuchIter):
> +def __init__(self, parent, iter_p):
> +super().__init__(
> +parent, iter_p,
> +fn_destroy=capi.lib.notmuch_config_list_destroy,
> +fn_valid=capi.lib.notmuch_config_list_valid,
> +fn_get=capi.lib.notmuch_config_list_key,
> +fn_next=capi.lib.notmuch_config_list_move_to_next)
> +
> +def __next__(self):
> +item = super().__next__()
> +return base.BinString.from_cffi(item)
> +
> +class ConfigMapping(base.NotmuchObject, collections.abc.MutableMapping):
> +"""The config key/value pairs stored in the database.
> +
> +The entries are exposed as a :class:`collections.abc.MutableMapping` 
> object.
> +Note that setting a value to an empty string is the same as deleting it.
> +
> +:param parent: the parent object
> +:param ptr_name: the name of the attribute on the parent which will
> +   return the memory pointer.  This allows this object to
> +   access the pointer via the parent's descriptor and thus
> +   trigger :class:`MemoryPointer`'s memory safety.
> +"""
> +
> +def __init__(self, parent, ptr_name):
> +self._parent = parent
> +self._ptr = lambda: getattr(parent, ptr_name)
> +
> +@property
> +def alive(self):
> +return self._parent.alive
> +
> +def _destroy(self):
> +pass
> +
> +def __getitem__(self, key):
> +if isinstance(key, str):
> +key = key.encode('utf-8')
> +val_pp = capi.ffi.new('char**')
> +ret = capi.lib.notmuch_database_get_config(self._ptr(), key, val_pp)
> +if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +raise errors.NotmuchError(ret)
> +if val_pp[0] == "":
> +capi.lib.free(val_pp[0])
> +raise KeyError
> +val = base.BinString.from_cffi(val_pp[0])
> +capi.lib.free(val_pp[0])
> +return val
> +
> +def __setitem__(self, key, val):
> +if isinstance(key, str):
> +key = key.encode('utf-8')
> +if isinstance(val, str):
> +val = val.encode('utf-8')
> +ret = capi.lib.notmuch_database_set_config(self._ptr(), key, val)
> +if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +raise errors.NotmuchError(ret)
> +
> +def __delitem__(self, key):
> +self[key] = ""
> +
> +def __iter__(self):
> +"""Return an iterator over the config items.
> +
> +:raises NullPointerError: If the iterator can not be created.
> +"""
> +configlist_pp = capi.ffi.new('notmuch_config_list_t**')
> +ret = 

Re: [PATCH] emacs: split-window-sensibly in tree mode with open message

2020-05-21 Thread Radu Butoi
Daniel Kahn Gillmor  writes:

> I like this proposal, and the simplification that it gives to the
> notmuch-emacs codebase.  However, this thread is the first place i've
> learned about split-window-sensibly, so i'm probably not eligible to
> really judge the merits here.

For some historical context, split-window-sensibly was introduced in 2009 [1] 
and the (split-window-vertically (/ (window-height) 4)) code in 2012 [2]. The 
two functions seem pretty interchangeable.

> As far as testing goes, a test would be nice -- is this something you
> could add to test/T460-emacs-tree.sh ?  Testing UI/UX issues is always
> pretty tough though, and it's not clear to me that we're actually
> already testing the existing "(/ (window-height) 4)" business anyway.

I did look at that file, and there is no tests for the current split 
functionality. All of the tests use the test-output function which outputs the 
contents of a buffer -- we need the whole frame. This might need to be done at 
a layer above Emacs and may complicate the test. I took a "screenshot" by 
copying my terminal contents with tmux, but I'm not sure how that would look 
like in a test.

If I manually do the split on the test case [3], the fourth line does cut off 
some of the text since it goes beyond 160 columns, the default threshold. It is 
still viewable by pressing C-e, like anything too long.

So I agree that testing would be nice but it seems to be an existing problem 
here. I'm not sure how, or if, other Emacs packages do character-perfect tests 
like these.

-Radu

[1]: 
git.savannah.gnu.org/cgit/emacs.git/commit/?id=8b10a2d19895041340296a703ba956c77541ec88

[2]: 
git.notmuchmail.org/git?p=notmuch;a=commit;h=3d92a257c8adbb36615bc61be9e668c8188006dc

[3]: 
https://gist.githubusercontent.com/rbutoi/78c5b30fc5e64585bda18abbc5bf1fbf/raw/cba7df184f3cd534c92a6fb9bf39aa121c19ecd7/gistfile1.txt
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch