[PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[Paul Wise]

This should be be slightly faster since it avoids forking a shell
and is less code in and less dependencies for the script.

Since String::ShellQuote isn't used elsewhere, drop mention of it.
---
 contrib/notmuch-mutt/README       | 2 --
 contrib/notmuch-mutt/notmuch-mutt | 6 +-----
 debian/control                    | 1 -
 3 files changed, 1 insertion(+), 8 deletions(-)

PS: I am not subscribed, please CC me in response.

diff --git a/contrib/notmuch-mutt/README b/contrib/notmuch-mutt/README
index 26996c4a..c7520228 100644
--- a/contrib/notmuch-mutt/README
+++ b/contrib/notmuch-mutt/README
@@ -39,8 +39,6 @@ To *run* notmuch-mutt you will need Perl with the following libraries:
   (Debian package: libmail-box-perl)
 - Mail::Header <https://metacpan.org/pod/Mail::Header>
   (Debian package: libmailtools-perl)
-- String::ShellQuote <https://metacpan.org/pod/String::ShellQuote>
-  (Debian package: libstring-shellquote-perl)
 - Term::ReadLine::Gnu <https://metacpan.org/pod/Term::ReadLine::Gnu>
   (Debian package: libterm-readline-gnu-perl)
 
diff --git a/contrib/notmuch-mutt/notmuch-mutt b/contrib/notmuch-mutt/notmuch-mutt
index d1e2c084..e0b2aceb 100755
--- a/contrib/notmuch-mutt/notmuch-mutt
+++ b/contrib/notmuch-mutt/notmuch-mutt
@@ -17,7 +17,6 @@ use Getopt::Long qw(:config no_getopt_compat);
 use Mail::Header;
 use Mail::Box::Maildir;
 use Pod::Usage;
-use String::ShellQuote;
 use Term::ReadLine;
 use Digest::SHA;
 
@@ -124,11 +123,8 @@ sub thread_action($$@) {
 	empty_maildir($results_dir);
 	die "notmuch-mutt: cannot find Message-Id, abort.\n";
     }
-    my $search_cmd = 'notmuch search --output=threads ' . shell_quote("id:$mid");
-    my $tid = `$search_cmd`;	# get thread id
-    chomp($tid);
 
-    search($results_dir, $remove_dups, $tid);
+    search($results_dir, $remove_dups, qq{thread:"{id:$mid}"});
 }
 
 sub tag_action(@) {
diff --git a/debian/control b/debian/control
index 2dcb8cc7..135eb7ce 100644
--- a/debian/control
+++ b/debian/control
@@ -227,7 +227,6 @@ Architecture: all
 Depends:
  libmail-box-perl,
  libmailtools-perl,
- libstring-shellquote-perl,
  libterm-readline-gnu-perl,
  notmuch (>= 0.4),
  ${misc:Depends},
-- 
2.39.2

Re: [PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[David Bremner]

Paul Wise <pabs3@bonedaddy.net> writes:

> +    search($results_dir, $remove_dups, qq{thread:"{id:$mid}"});

did you test this with a message-id with spaces in it? the quoting is a
delicate.

d

Re: [PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[Paul Wise]

[-- Attachment #1.1: Type: text/plain, Size: 2015 bytes --]

On Sat, 2023-04-08 at 08:42 -0300, David Bremner wrote:

> did you test this with a message-id with spaces in it?
> the quoting is a delicate.

You are correct that the quoting for this was wrong, as it was also
wrong in the code before my patch and I just copied that, however...

I noticed that when notmuch stores the Message-ID in the Xapian
database, it strips the spaces, so messages with spaces in the
Message-ID are not found unless the id: search has no spaces also.

After switching from testing using id: to subject: and reading the
manual page I was able to find the correct quoting for spaces in $mid,
but I also added the space removals for the thread query.

Then I figured I should test with double quote characters and that blew
up in my face too, so I added double quote doubling to workaround that.

Then I noticed that you can put parentheses inside a quoted query and
that changes the meaning of the query but I couldn't find any way to
prevent an arbitrary Message-Id from inserting parentheses into the
query. Also notmuch converts Message-Id (test)@hostname to just
@hostname in the Xapian database. I think that right now I am not sure
that re-implementing all the idiosyncrasies of notmuch queries and
Message-Id munging within notmuch-mutt is the way to go, especially
since Message-Id fields that intersect with notmuch features are rare.

So I think that it would be good if there were a library for notmuch
query construction that would take care of all the different quoting
levels, Message-Id munging etc. This could then get language bindings
so that notmuch based tools and MUAs could use them for interactive
graphical query construction. Also maybe Xapian needs or has a way to
construct queries without serialising them to a text string and the
notmuch query construction library I propose could base itself on that.

Anyway, will send an updated patch series including space/quote fixing.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[PATCH v2 1/2] notmuch-mutt: fix Xapian query construction

[Paul Wise]

Spaces need to be stripped when querying the Message-Id,
because notmuch stores them in Xapian with spaces stripped.

All double-quote characters need to be doubled to escape them,
otherwise they will be added as extra query terms outside the id.
---
 contrib/notmuch-mutt/notmuch-mutt | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/contrib/notmuch-mutt/notmuch-mutt b/contrib/notmuch-mutt/notmuch-mutt
index d1e2c084..1bb95ba6 100755
--- a/contrib/notmuch-mutt/notmuch-mutt
+++ b/contrib/notmuch-mutt/notmuch-mutt
@@ -124,7 +124,11 @@ sub thread_action($$@) {
 	empty_maildir($results_dir);
 	die "notmuch-mutt: cannot find Message-Id, abort.\n";
     }
-    my $search_cmd = 'notmuch search --output=threads ' . shell_quote("id:$mid");
+
+    $mid =~ s/ //g; # notmuch strips spaces before storing Message-Id
+    $mid =~ s/"/""/g; # escape all double quote characters
+
+    my $search_cmd = 'notmuch search --output=threads ' . shell_quote(qq{id:"$mid"});
     my $tid = `$search_cmd`;	# get thread id
     chomp($tid);
 
@@ -135,7 +139,10 @@ sub tag_action(@) {
     my $mid = get_message_id();
     defined $mid or die "notmuch-mutt: cannot find Message-Id, abort.\n";
 
-    system("notmuch", "tag", @_, "--", "id:$mid");
+    $mid =~ s/ //g; # notmuch strips spaces before storing Message-Id
+    $mid =~ s/"/""/g; # escape all double quote characters
+
+    system("notmuch", "tag", @_, "--", qq{id:"$mid"});
 }
 
 sub die_usage() {
-- 
2.39.2

[PATCH v2 2/2] notmuch-mutt: replace extra command with notmuch thread search feature

[Paul Wise]

This should be be slightly faster since it avoids forking a shell
and is less code in and less dependencies for the script.

Since String::ShellQuote isn't used elsewhere, drop mention of it.
---
 contrib/notmuch-mutt/README       | 2 --
 contrib/notmuch-mutt/notmuch-mutt | 9 ++-------
 debian/control                    | 1 -
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/contrib/notmuch-mutt/README b/contrib/notmuch-mutt/README
index 26996c4a..c7520228 100644
--- a/contrib/notmuch-mutt/README
+++ b/contrib/notmuch-mutt/README
@@ -39,8 +39,6 @@ To *run* notmuch-mutt you will need Perl with the following libraries:
   (Debian package: libmail-box-perl)
 - Mail::Header <https://metacpan.org/pod/Mail::Header>
   (Debian package: libmailtools-perl)
-- String::ShellQuote <https://metacpan.org/pod/String::ShellQuote>
-  (Debian package: libstring-shellquote-perl)
 - Term::ReadLine::Gnu <https://metacpan.org/pod/Term::ReadLine::Gnu>
   (Debian package: libterm-readline-gnu-perl)
 
diff --git a/contrib/notmuch-mutt/notmuch-mutt b/contrib/notmuch-mutt/notmuch-mutt
index 1bb95ba6..48cb5594 100755
--- a/contrib/notmuch-mutt/notmuch-mutt
+++ b/contrib/notmuch-mutt/notmuch-mutt
@@ -17,7 +17,6 @@ use Getopt::Long qw(:config no_getopt_compat);
 use Mail::Header;
 use Mail::Box::Maildir;
 use Pod::Usage;
-use String::ShellQuote;
 use Term::ReadLine;
 use Digest::SHA;
 
@@ -126,13 +125,9 @@ sub thread_action($$@) {
     }
 
     $mid =~ s/ //g; # notmuch strips spaces before storing Message-Id
-    $mid =~ s/"/""/g; # escape all double quote characters
-
-    my $search_cmd = 'notmuch search --output=threads ' . shell_quote(qq{id:"$mid"});
-    my $tid = `$search_cmd`;	# get thread id
-    chomp($tid);
+    $mid =~ s/"/""""/g; # escape all double quote characters twice
 
-    search($results_dir, $remove_dups, $tid);
+    search($results_dir, $remove_dups, qq{thread:"{id:""$mid""}"});
 }
 
 sub tag_action(@) {
diff --git a/debian/control b/debian/control
index 2dcb8cc7..135eb7ce 100644
--- a/debian/control
+++ b/debian/control
@@ -227,7 +227,6 @@ Architecture: all
 Depends:
  libmail-box-perl,
  libmailtools-perl,
- libstring-shellquote-perl,
  libterm-readline-gnu-perl,
  notmuch (>= 0.4),
  ${misc:Depends},
-- 
2.39.2

Re: [PATCH v2 2/2] notmuch-mutt: replace extra command with notmuch thread search feature

[David Bremner]

Paul Wise <pabs3@bonedaddy.net> writes:

> This should be be slightly faster since it avoids forking a shell
> and is less code in and less dependencies for the script.
>
> Since String::ShellQuote isn't used elsewhere, drop mention of it.

applied to master.

d

Re: [PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[David Bremner]

Paul Wise <pabs3@bonedaddy.net> writes:

> So I think that it would be good if there were a library for notmuch
> query construction that would take care of all the different quoting
> levels, Message-Id munging etc. This could then get language bindings
> so that notmuch based tools and MUAs could use them for interactive
> graphical query construction. Also maybe Xapian needs or has a way to
> construct queries without serialising them to a text string and the
> notmuch query construction library I propose could base itself on that.

You might be interested in the s-expression query parser. Part of the
goal is to be less dwim/quirky than the native Xapian query parser, and
to make it easier / safer to construct notmuch queries via programs.

It doesn't do the message-id munging you mention (yet?), but quoting is
IMHO saner.

It does require an extra dependency (sfsexpr).

Re: [PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[Paul Wise]

[-- Attachment #1.1: Type: text/plain, Size: 858 bytes --]

On Sun, 2023-04-09 at 13:00 -0300, David Bremner wrote:

> You might be interested in the s-expression query parser. Part of the
> goal is to be less dwim/quirky than the native Xapian query parser, and
> to make it easier / safer to construct notmuch queries via programs.

That does seem saner to manually construct queries in, but it would
still be better to have a query construction library, otherwise
programs are going to get it wrong all the time, even within notmuch.

> It doesn't do the message-id munging you mention (yet?),

This is a big part of the query construction headache, re-implementing
the space stripping is easy, the user part removal is harder and I
can't find any docs on what else is required here.

PS: I don't intend to work on switching notmuch-mutt to sexp.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

Re: [PATCH] notmuch-mutt: replace extra command with notmuch-native thread search feature

[David Bremner]

Paul Wise <pabs3@bonedaddy.net> writes:


> that changes the meaning of the query but I couldn't find any way to
> prevent an arbitrary Message-Id from inserting parentheses into the
> query. Also notmuch converts Message-Id (test)@hostname to just
> @hostname in the Xapian database. I think that right now I am not sure
> that re-implementing all the idiosyncrasies of notmuch queries and
> Message-Id munging within notmuch-mutt is the way to go, especially
> since Message-Id fields that intersect with notmuch features are rare.

The code that does removal of spaces and () delited sequences goes back
to Carl's 2009 implementation of manual header parsing (which we have
mostly, but not entirely replaced with calls to gmime).

It would probably be reasonable to disable that code (or maybe migrate
to gmime parsing of message-ids?), but the cost/benefit analysis is not
too clear to me.

I had to look this up, but apparently parens mark a comment in RFC822 /
RFC5322. If I understand RFC5322 correctly then comments are only
permitted in the obsolete syntax, but I guess it makes sense to accept
those. I don't know how many messages in the wild use this syntax;
of the .01% in in my mail store, most don't put the comment inside the actual <>
delimited message-id id, and those that do look like errors of various kinds
- some unbalanced parens
- some (null) indicating printf-ing null pointers