threading replies fixes v3

classic Classic list List threaded Threaded
23 messages Options
12
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

threading replies fixes v3

This obsoletes the series at

     id:[hidden email]

An annotated interdiff is at the end. At high level this represents a
cleanup and light re-organization of the previous series. I also
dropped the test for multiple thread terms, as it is not relevant to
these changes.

There are two main changes: patches 1 to 10 introduce the use of
references to parent messages. Patches 11 to 15 conditionally revert
to trusting In-Reply-To (when it looks like a single message-id).

[PATCH 04/15] lib/thread: sort sibling messages by date

This has the WIP patch of a few days ago squashed into it, along with
memory allocation fixes.

[PATCH 06/15] lib/thread: refactor in_reply_to test
[PATCH 07/15] lib/thread: initial use of references as for fallback

Patch 07 is split out of patch 06 in the previous series.

There are style fixes throughout from uncrustify.

######################################################################
# Bug fix: we need to make sure the lazy reading is done.

diff --git a/lib/message.cc b/lib/message.cc
index f329be20..5e17029a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -625,6 +625,7 @@ _notmuch_message_label_depths (notmuch_message_t *message,
 const notmuch_string_list_t *
 _notmuch_message_get_references (notmuch_message_t *message)
 {
+    _notmuch_message_ensure_metadata (message, message->reference_list);
     return message->reference_list;
 }
 
@@ -640,15 +641,18 @@ _cmpmsg (const void *pa, const void *pb)
 }

######################################################################
# Memory management fixes for sort_subtrees

 notmuch_message_list_t *
-_notmuch_message_sort_subtrees (notmuch_message_list_t *list) {
+_notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list)
+{
 
     size_t count = 0;
     size_t capacity = 16;
 
-    if (!list)
+    if (! list)
  return list;
 
-    notmuch_message_t **message_array = talloc_zero_array (list, notmuch_message_t *, capacity);
+    void *local = talloc_new (NULL);
+    notmuch_message_list_t *new_list = _notmuch_message_list_create (ctx);
+    notmuch_message_t **message_array = talloc_zero_array (local, notmuch_message_t *, capacity);
 
     for (notmuch_messages_t *messages = _notmuch_messages_create (list);
  notmuch_messages_valid (messages);
@@ -656,19 +660,19 @@ _notmuch_message_sort_subtrees (notmuch_message_list_t *list) {
  notmuch_message_t *root = notmuch_messages_get (messages);
  if (count >= capacity) {
     capacity *= 2;
-    message_array = talloc_realloc (root, message_array, notmuch_message_t *, capacity);
+    message_array = talloc_realloc (local, message_array, notmuch_message_t *, capacity);
  }
  message_array[count++] = root;
- root->replies = _notmuch_message_sort_subtrees (root->replies);
+ root->replies = _notmuch_message_sort_subtrees (root, root->replies);
     }
 
-    notmuch_message_list_t *new_list = _notmuch_message_list_create (list);
-
     qsort (message_array, count, sizeof (notmuch_message_t *), _cmpmsg);
-    for (size_t i=0; i<count; i++){
+    for (size_t i = 0; i < count; i++) {
  _notmuch_message_list_add_message (new_list, message_array[i]);
     }
-    talloc_free (message_array);
+
+    talloc_free (local);
+    talloc_free (list);
     return new_list;
 }
 
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 64f4e982..5bbaa292 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -560,7 +560,7 @@ _notmuch_message_label_depths (notmuch_message_t *message,
        size_t depth);
 
 notmuch_message_list_t *
-_notmuch_message_sort_subtrees (notmuch_message_list_t *list);
+_notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list);
 
 /* sha1.c */

######################################################################
# update comments

diff --git a/lib/thread.cc b/lib/thread.cc
index e9e15a5c..e4ab4319 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -498,15 +498,17 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
  notmuch_messages_valid (roots);
  notmuch_messages_move_to_next (roots)) {
  notmuch_message_t *message = notmuch_messages_get (roots);
- /* XXX add _notmuch_messages_count */
+ /* XXX TODO: cleaner way to test iterator for last, list for emptiness  */
  if (roots->iterator->next || thread->toplevel_list->head)
     _parent_or_toplevel (thread, message);
  else
     _notmuch_message_list_add_message (thread->toplevel_list, message);
     }
 
-    /* XXX this creates garbage */
-    thread->toplevel_list = _notmuch_message_sort_subtrees (thread->toplevel_list);
+    /* XXX this could be made conditional on messages being inserted
+     * (out of order) in later passes
+     */
+    thread->toplevel_list = _notmuch_message_sort_subtrees (thread, thread->toplevel_list);
 
     talloc_free (local);
 }

######################################################################
# drop the last patch in the series

diff --git a/test/.gitignore b/test/.gitignore
index 71bbd7ed..73fe7e24 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -8,5 +8,4 @@
 /make-db-version
 /test-results
 /ghost-report
-/term-report
 /tmp.*
diff --git a/test/Makefile.local b/test/Makefile.local
index c39feace..1cf09778 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -44,9 +44,6 @@ $(dir)/make-db-version: $(dir)/make-db-version.o
 $(dir)/ghost-report: $(dir)/ghost-report.o
  $(call quiet,CXX) $^ -o $@ $(LDFLAGS) $(XAPIAN_LDFLAGS)
 
-$(dir)/term-report: $(dir)/term-report.o
- $(call quiet,CXX) $^ -o $@ $(LDFLAGS) $(XAPIAN_LDFLAGS)
-
 .PHONY: test check
 
 test_main_srcs=$(dir)/arg-test.c \
@@ -57,9 +54,7 @@ test_main_srcs=$(dir)/arg-test.c \
       $(dir)/symbol-test.cc \
       $(dir)/make-db-version.cc \
       $(dir)/ghost-report.cc \
-      $(dir)/message-id-parse.c \
-      $(dir)/term-report.cc
-
+      $(dir)/message-id-parse.c
 
 test_srcs=$(test_main_srcs) $(dir)/database-test.c
 
diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index f5ee81fe..4688c0ab 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -189,6 +189,26 @@ End of search results.
 EOF
 test_expect_equal_file EXPECTED OUTPUT


######################################################################
# New test cases, based on Gregor's anonymized data

+test_begin_subtest "reply to ghost (RT)"
+notmuch show --entire-thread=true id:[hidden email] | grep ^Subject: | head -1  > OUTPUT
+cat <<EOF > EXPECTED
+Subject: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "reply to ghost (RT/tree view)"
+test_emacs '(notmuch-tree "id:[hidden email]")
+    (notmuch-test-wait)
+    (test-output)
+    (delete-other-windows)'
+cat <<EOF > EXPECTED
+  2016-06-19  Gregor Zattler       ┬┬►FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx                (inbox unread)
+  2016-06-19   via RT              │╰─►[support.xxxxxxxxxxx-xxxxxxxxx-xxxxxxxxx.de #33575] AutoReply: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx (inbox unread)
+  2016-06-26   via RT              ╰─►[support.xxxxxxxxxxx-xxxxxxxxx-xxxxxxxxx.de #33575] Resolved: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx (inbox unread)
+End of search results.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "trusting reply-to (tree view)"
 test_emacs '(notmuch-tree "id:[hidden email]")
     (notmuch-test-wait)

######################################################################
# drop last patch

diff --git a/test/T720-database-schema.sh b/test/T720-database-schema.sh
deleted file mode 100755
index a6a99239..00000000

######################################################################
# new test data, see patches for content

diff --git a/test/corpora/threading/ghost-root/1529425589.M615261P21663.len:2,S b/test/corpora/threading/ghost-root/1529425589.M615261P21663.len:2,S
new file mode 100644
diff --git a/test/corpora/threading/ghost-root/1532672447.R3166642290392477575.len:2,S b/test/corpora/threading/ghost-root/1532672447.R3166642290392477575.len:2,S
new file mode 100644
diff --git a/test/corpora/threading/ghost-root/1532672447.R6968667928580738175.len:2,S b/test/corpora/threading/ghost-root/1532672447.R6968667928580738175.len:2,S
new file mode 100644

######################################################################
# drop last patch

diff --git a/test/corpora/threading/mutant-ref/file1 b/test/corpora/threading/mutant-ref/file1
deleted file mode 100644
index 97f8db58..00000000
diff --git a/test/corpora/threading/mutant-ref/file2 b/test/corpora/threading/mutant-ref/file2
deleted file mode 100644
index 2b2ccd1d..00000000
diff --git a/test/corpora/threading/mutant-ref/file3 b/test/corpora/threading/mutant-ref/file3
deleted file mode 100644
index a8e705bc..00000000
diff --git a/test/corpora/threading/mutant-ref/file4 b/test/corpora/threading/mutant-ref/file4
deleted file mode 100644
index 3a0a5a13..00000000
diff --git a/test/corpora/threading/mutant-ref/file5 b/test/corpora/threading/mutant-ref/file5
deleted file mode 100644
index 8f525d63..00000000

######################################################################
# uncrustify

diff --git a/test/message-id-parse.c b/test/message-id-parse.c
index 25decc9b..752eb1fd 100644
--- a/test/message-id-parse.c
+++ b/test/message-id-parse.c
@@ -2,22 +2,24 @@
 #include <talloc.h>
 #include "notmuch-private.h"
 
-int main(unused (int argc), unused (char **argv)){
+int
+main (unused (int argc), unused (char **argv))
+{
     char *line = NULL;
     size_t len = 0;
     ssize_t nread;
     void *local = talloc_new (NULL);
 
-    while ((nread = getline(&line, &len, stdin)) != -1) {
- int last = strlen(line) - 1;
+    while ((nread = getline (&line, &len, stdin)) != -1) {
+ int last = strlen (line) - 1;
  if (line[last] == '\n')
     line[last] = '\0';
 
  char *mid = _notmuch_message_id_parse_strict (local, line);
  if (mid)
-    printf("GOOD: %s\n", mid);
+    printf ("GOOD: %s\n", mid);
  else
-    printf("BAD: %s\n", line);
+    printf ("BAD: %s\n", line);
     }
 
     talloc_free (local);
diff --git a/test/term-report.cc b/test/term-report.cc
deleted file mode 100644
index 88cd1bf5..00000000

######################################################################
# put back copyright header
diff --git a/util/debug_print.c b/util/debug_print.c
index 9f00f69b..8b4bc73d 100644
--- a/util/debug_print.c
+++ b/util/debug_print.c
@@ -1,3 +1,24 @@
+/* error_util.c - internal error utilities for notmuch.
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2018 David Bremner
[snip]
+ *
+ * Author: Carl Worth <[hidden email]>
+ */
+
 #include "debug_print.h"

######################################################################
# whitespace fix
 void
@@ -8,7 +29,6 @@ _debug_printf (const char *format, ...)
     va_start (va_args, format);
     vfprintf (stderr, format, va_args);
     va_end (va_args);
-    
 }
 
 void

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 01/15] util: add DEBUG_PRINTF, rename error_util.h -> debug_print.h

It seemed silly to have two headers that were almost copies.
The new macro is intended for debugging, and probably should not be
left in released code.
---
 command-line-arguments.c             |  2 +-
 lib/notmuch-private.h                |  2 +-
 util/Makefile.local                  |  4 ++--
 util/{error_util.c => debug_print.c} | 16 +++++++++++-----
 util/{error_util.h => debug_print.h} | 22 ++++++++++++++++++----
 util/hex-escape.c                    |  2 +-
 util/util.c                          |  2 +-
 util/xutil.c                         |  2 +-
 8 files changed, 36 insertions(+), 16 deletions(-)
 rename util/{error_util.c => debug_print.c} (81%)
 rename util/{error_util.h => debug_print.h} (75%)

diff --git a/command-line-arguments.c b/command-line-arguments.c
index d64aa85b..90d69453 100644
--- a/command-line-arguments.c
+++ b/command-line-arguments.c
@@ -1,7 +1,7 @@
 #include <assert.h>
 #include <string.h>
 #include <stdio.h>
-#include "error_util.h"
+#include "debug_print.h"
 #include "command-line-arguments.h"
 
 typedef enum {
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 3764a6a9..499d73d4 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -50,7 +50,7 @@ NOTMUCH_BEGIN_DECLS
 #include "gmime-extra.h"
 
 #include "xutil.h"
-#include "error_util.h"
+#include "debug_print.h"
 #include "string-util.h"
 #include "crypto.h"
 
diff --git a/util/Makefile.local b/util/Makefile.local
index ba03230e..ccf1bd79 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -3,9 +3,9 @@
 dir := util
 extra_cflags += -I$(srcdir)/$(dir)
 
-libnotmuch_util_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
+libnotmuch_util_c_srcs := $(dir)/xutil.c $(dir)/hex-escape.c \
   $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c \
- $(dir)/util.c $(dir)/gmime-extra.c $(dir)/crypto.c
+ $(dir)/util.c $(dir)/gmime-extra.c $(dir)/crypto.c $(dir)/debug_print.c
 
 libnotmuch_util_modules := $(libnotmuch_util_c_srcs:.c=.o)
 
diff --git a/util/error_util.c b/util/debug_print.c
similarity index 81%
rename from util/error_util.c
rename to util/debug_print.c
index e64162c7..8b4bc73d 100644
--- a/util/error_util.c
+++ b/util/debug_print.c
@@ -1,6 +1,7 @@
 /* error_util.c - internal error utilities for notmuch.
  *
  * Copyright © 2009 Carl Worth
+ * Copyright © 2018 David Bremner
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -18,11 +19,17 @@
  * Author: Carl Worth <[hidden email]>
  */
 
-#include <stdlib.h>
-#include <stdarg.h>
-#include <stdio.h>
+#include "debug_print.h"
 
-#include "error_util.h"
+void
+_debug_printf (const char *format, ...)
+{
+    va_list va_args;
+
+    va_start (va_args, format);
+    vfprintf (stderr, format, va_args);
+    va_end (va_args);
+}
 
 void
 _internal_error (const char *format, ...)
@@ -37,4 +44,3 @@ _internal_error (const char *format, ...)
     va_end (va_args);
     exit (1);
 }
-
diff --git a/util/error_util.h b/util/debug_print.h
similarity index 75%
rename from util/error_util.h
rename to util/debug_print.h
index 4bb338a2..4a096945 100644
--- a/util/error_util.h
+++ b/util/debug_print.h
@@ -1,6 +1,7 @@
-/* error_util.h - Provide the INTERNAL_ERROR macro
+/* debug_print.h - Provide the INTERNAL_ERROR and DEBUG_PRINTF macros
  *
  * Copyright © 2009 Carl Worth
+ * Copyright © 2018 David Bremner
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -18,13 +19,26 @@
  * Author: Carl Worth <[hidden email]>
  */
 
-#ifndef ERROR_UTIL_H
-#define ERROR_UTIL_H
+#ifndef DEBUG_PRINT_H
+#define DEBUG_PRINT_H
 
 #include <talloc.h>
-
 #include "function-attributes.h"
 
+void
+_debug_printf (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
+
+/*
+ * Provide a quick way to silence debugging output.
+ */
+
+#ifdef DEBUG_PRINT
+#define DEBUG_PRINTF(format, ...) _debug_printf(format " (%s).\n", \
+ ##__VA_ARGS__, __location__)
+#else
+#define DEBUG_PRINTF(format, ...) /* ignored */
+#endif
+
 /* There's no point in continuing when we've detected that we've done
  * something wrong internally (as opposed to the user passing in a
  * bogus value).
diff --git a/util/hex-escape.c b/util/hex-escape.c
index 8883ff90..ff20c702 100644
--- a/util/hex-escape.c
+++ b/util/hex-escape.c
@@ -22,7 +22,7 @@
 #include <string.h>
 #include <talloc.h>
 #include <ctype.h>
-#include "error_util.h"
+#include "debug_print.h"
 #include "hex-escape.h"
 
 static const char *output_charset =
diff --git a/util/util.c b/util/util.c
index 06659b35..75df0ead 100644
--- a/util/util.c
+++ b/util/util.c
@@ -1,5 +1,5 @@
 #include "util.h"
-#include "error_util.h"
+#include "debug_print.h"
 #include <string.h>
 #include <errno.h>
 
diff --git a/util/xutil.c b/util/xutil.c
index f211eaaa..11042c9e 100644
--- a/util/xutil.c
+++ b/util/xutil.c
@@ -22,7 +22,7 @@
 #include <string.h>
 
 #include "xutil.h"
-#include "error_util.h"
+#include "debug_print.h"
 
 void *
 xcalloc (size_t nmemb, size_t size)
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 02/15] test: start threading test corpus

In reply to this post by David Bremner-2
There are 3 threads here, two synthetic, and one anonymized one using
data from Gregor. They test various aspects of thread
ordering/construction in the presence of replies to ghost messages.
---
 .../1529425589.M615261P21663.len:2,S           |  9 +++++++++
 .../1532672447.R3166642290392477575.len:2,S    | 17 +++++++++++++++++
 .../1532672447.R6968667928580738175.len:2,S    | 18 ++++++++++++++++++
 test/corpora/threading/ghost-root/child        |  9 +++++++++
 test/corpora/threading/ghost-root/fake-root    |  9 +++++++++
 test/corpora/threading/ghost-root/grand-child  |  9 +++++++++
 test/corpora/threading/ghost-root/grand-child2 |  9 +++++++++
 .../threading/ghost-root/great-grand-child     |  9 +++++++++
 test/corpora/threading/ghost-root/real-root    |  7 +++++++
 9 files changed, 96 insertions(+)
 create mode 100644 test/corpora/threading/ghost-root/1529425589.M615261P21663.len:2,S
 create mode 100644 test/corpora/threading/ghost-root/1532672447.R3166642290392477575.len:2,S
 create mode 100644 test/corpora/threading/ghost-root/1532672447.R6968667928580738175.len:2,S
 create mode 100644 test/corpora/threading/ghost-root/child
 create mode 100644 test/corpora/threading/ghost-root/fake-root
 create mode 100644 test/corpora/threading/ghost-root/grand-child
 create mode 100644 test/corpora/threading/ghost-root/grand-child2
 create mode 100644 test/corpora/threading/ghost-root/great-grand-child
 create mode 100644 test/corpora/threading/ghost-root/real-root

diff --git a/test/corpora/threading/ghost-root/1529425589.M615261P21663.len:2,S b/test/corpora/threading/ghost-root/1529425589.M615261P21663.len:2,S
new file mode 100644
index 00000000..62bf98db
--- /dev/null
+++ b/test/corpora/threading/ghost-root/1529425589.M615261P21663.len:2,S
@@ -0,0 +1,9 @@
+From: Gregor Zattler <[hidden email]>
+To: xxx request tracker <[hidden email]>
+Subject: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
+Date: Tue, 19 Jun 2016 18:26:26 +0200
+Message-ID: <[hidden email]>
+MIME-Version: 1.0
+Content-Type: text/plain; charset=utf-8
+Content-Transfer-Encoding: quoted-printable
+
diff --git a/test/corpora/threading/ghost-root/1532672447.R3166642290392477575.len:2,S b/test/corpora/threading/ghost-root/1532672447.R3166642290392477575.len:2,S
new file mode 100644
index 00000000..b79eaf7a
--- /dev/null
+++ b/test/corpora/threading/ghost-root/1532672447.R3166642290392477575.len:2,S
@@ -0,0 +1,17 @@
+Return-Path: <prvs=701fd58e1=[hidden email]>
+Subject: [support.xxxxxxxxxxx-xxxxxxxxx-xxxxxxxxx.de #33575] AutoReply: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
+From: " via RT" <[hidden email]>
+Reply-To: [hidden email]
+In-Reply-To: <[hidden email]>
+References: <[hidden email]>
+ <[hidden email]>
+Message-ID: <[hidden email]>
+To: [hidden email]
+Content-Type: text/plain; charset="utf-8"
+Date: Tue, 19 Jun 2016 18:26:36 +0200
+MIME-Version: 1.0
+Content-Transfer-Encoding: 8bit
+
+
+
+
diff --git a/test/corpora/threading/ghost-root/1532672447.R6968667928580738175.len:2,S b/test/corpora/threading/ghost-root/1532672447.R6968667928580738175.len:2,S
new file mode 100644
index 00000000..343a855e
--- /dev/null
+++ b/test/corpora/threading/ghost-root/1532672447.R6968667928580738175.len:2,S
@@ -0,0 +1,18 @@
+Return-Path: <prvs=708ebe06b=[hidden email]>
+Subject: [support.xxxxxxxxxxx-xxxxxxxxx-xxxxxxxxx.de #33575] Resolved: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
+From: " via RT" <[hidden email]>
+Reply-To: [hidden email]
+References: <[hidden email]>
+Message-ID: <[hidden email]>
+To: [hidden email]
+Content-Type: text/plain; charset="utf-8"
+Date: Tue, 26 Jun 2016 14:44:24 +0200
+MIME-Version: 1.0
+Content-Transfer-Encoding: 8bit
+
+
+
+
+According to our records, your request has been resolved. If you have any
+further questions or concerns, please respond to this message.
+
diff --git a/test/corpora/threading/ghost-root/child b/test/corpora/threading/ghost-root/child
new file mode 100644
index 00000000..4c36af95
--- /dev/null
+++ b/test/corpora/threading/ghost-root/child
@@ -0,0 +1,9 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: child message
+Message-ID: <[hidden email]>
+In-Reply-To: <[hidden email]>
+References:  <[hidden email]>
+Date: Fri, 17 Jun 2016 22:14:41 -0400
+
+
diff --git a/test/corpora/threading/ghost-root/fake-root b/test/corpora/threading/ghost-root/fake-root
new file mode 100644
index 00000000..a698185d
--- /dev/null
+++ b/test/corpora/threading/ghost-root/fake-root
@@ -0,0 +1,9 @@
+From: Mallory <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: fake root message
+Message-ID: <[hidden email]>
+In-Reply-to: <[hidden email]>
+References: <[hidden email]> <[hidden email]> <[hidden email]>
+Date: Thu, 16 Jun 2016 22:14:41 -0400
+
+This message has an in-reply-to pointing to a non-existent message
diff --git a/test/corpora/threading/ghost-root/grand-child b/test/corpora/threading/ghost-root/grand-child
new file mode 100644
index 00000000..5f77ac36
--- /dev/null
+++ b/test/corpora/threading/ghost-root/grand-child
@@ -0,0 +1,9 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: grand-child message
+Message-ID: <[hidden email]>
+In-Reply-To: <[hidden email]>
+References:  <[hidden email]> <[hidden email]>
+Date: Fri, 17 Jun 2016 22:24:41 -0400
+
+
diff --git a/test/corpora/threading/ghost-root/grand-child2 b/test/corpora/threading/ghost-root/grand-child2
new file mode 100644
index 00000000..59682a95
--- /dev/null
+++ b/test/corpora/threading/ghost-root/grand-child2
@@ -0,0 +1,9 @@
+From: Daniel <[hidden email]>
+To: Alice <[hidden email]>
+Subject: grand-child message 2
+Message-ID: <[hidden email]>
+In-Reply-To: <[hidden email]>
+References:  <[hidden email]> <[hidden email]>
+Date: Fri, 17 Jun 2016 22:34:41 -0400
+
+
diff --git a/test/corpora/threading/ghost-root/great-grand-child b/test/corpora/threading/ghost-root/great-grand-child
new file mode 100644
index 00000000..287a8954
--- /dev/null
+++ b/test/corpora/threading/ghost-root/great-grand-child
@@ -0,0 +1,9 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: great grand-child message
+Message-ID: <[hidden email]>
+In-Reply-To: <[hidden email]>
+References:  <[hidden email]> <[hidden email]>
+Date: Fri, 17 Jun 2016 22:44:41 -0400
+
+
diff --git a/test/corpora/threading/ghost-root/real-root b/test/corpora/threading/ghost-root/real-root
new file mode 100644
index 00000000..f1b16a0c
--- /dev/null
+++ b/test/corpora/threading/ghost-root/real-root
@@ -0,0 +1,7 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: root message
+Message-ID: <[hidden email]>
+Date: Thu, 16 Jun 2016 22:14:41 -0400
+
+This message has no in-reply-to
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 03/15] test: add known broken tests for "ghost roots"

In reply to this post by David Bremner-2
This documents the bug discussed at

     id:[hidden email]

The underlying issue is that the reply to a ghost (missing) message is
falsely classified as a root message in _resolve_thread_relationships.

There are two pairs of tests; in each case the the first test is
simpler / more robust, but also easier to fool.
---
 test/T510-thread-replies.sh | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 6837ff17..753c7ae1 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -164,5 +164,53 @@ expected='[[[{"id": "XXXXX", "match": true, "excluded": false,
 expected=`echo "$expected" | notmuch_json_show_sanitize`
 test_expect_equal_json "$output" "$expected"
 
+add_email_corpus threading
+
+test_begin_subtest "reply to ghost"
+test_subtest_known_broken
+notmuch show --entire-thread=true id:[hidden email] | grep ^Subject: | head -1  > OUTPUT
+cat <<EOF > EXPECTED
+Subject: root message
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "reply to ghost (tree view)"
+test_subtest_known_broken
+test_emacs '(notmuch-tree "id:[hidden email]")
+    (notmuch-test-wait)
+    (test-output)
+    (delete-other-windows)'
+cat <<EOF > EXPECTED
+  2016-06-17  Alice                 ┬►root message                                        (inbox unread)
+  2016-06-18  Alice                 ╰┬►child message                                      (inbox unread)
+  2016-06-17  Mallory                ├─►fake root message                                 (inbox unread)
+  2016-06-18  Alice                  ├┬►grand-child message                               (inbox unread)
+  2016-06-18  Alice                  │╰─►great grand-child message                        (inbox unread)
+  2016-06-18  Daniel                 ╰─►grand-child message 2                             (inbox unread)
+End of search results.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "reply to ghost (RT)"
+test_subtest_known_broken
+notmuch show --entire-thread=true id:[hidden email] | grep ^Subject: | head -1  > OUTPUT
+cat <<EOF > EXPECTED
+Subject: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "reply to ghost (RT/tree view)"
+test_subtest_known_broken
+test_emacs '(notmuch-tree "id:[hidden email]")
+    (notmuch-test-wait)
+    (test-output)
+    (delete-other-windows)'
+cat <<EOF > EXPECTED
+  2016-06-19  Gregor Zattler       ┬┬►FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx                (inbox unread)
+  2016-06-19   via RT              │╰─►[support.xxxxxxxxxxx-xxxxxxxxx-xxxxxxxxx.de #33575] AutoReply: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx (inbox unread)
+  2016-06-26   via RT              ╰─►[support.xxxxxxxxxxx-xxxxxxxxx-xxxxxxxxx.de #33575] Resolved: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx (inbox unread)
+End of search results.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
 
 test_done
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 04/15] lib/thread: sort sibling messages by date

In reply to this post by David Bremner-2
For non-root messages, this should not should anything currently, as
the messages are already added in date order. In the future we will
add some non-root messages in a second pass out of order and the
sorting will be useful. It does fix the order of multiple
root-messages (although it is overkill for that).
---
 lib/message.cc              | 47 +++++++++++++++++++++++++++++++++++++
 lib/notmuch-private.h       |  3 +++
 lib/thread.cc               |  6 +++++
 test/T510-thread-replies.sh |  2 --
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 153e4bed..59e42d36 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -588,6 +588,53 @@ _notmuch_message_add_reply (notmuch_message_t *message,
     _notmuch_message_list_add_message (message->replies, reply);
 }
 
+static int
+_cmpmsg (const void *pa, const void *pb)
+{
+    notmuch_message_t **a = (notmuch_message_t **) pa;
+    notmuch_message_t **b = (notmuch_message_t **) pb;
+    time_t time_a = notmuch_message_get_date (*a);
+    time_t time_b = notmuch_message_get_date (*b);
+
+    return (int) difftime (time_a, time_b);
+}
+
+notmuch_message_list_t *
+_notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list)
+{
+
+    size_t count = 0;
+    size_t capacity = 16;
+
+    if (! list)
+ return list;
+
+    void *local = talloc_new (NULL);
+    notmuch_message_list_t *new_list = _notmuch_message_list_create (ctx);
+    notmuch_message_t **message_array = talloc_zero_array (local, notmuch_message_t *, capacity);
+
+    for (notmuch_messages_t *messages = _notmuch_messages_create (list);
+ notmuch_messages_valid (messages);
+ notmuch_messages_move_to_next (messages)) {
+ notmuch_message_t *root = notmuch_messages_get (messages);
+ if (count >= capacity) {
+    capacity *= 2;
+    message_array = talloc_realloc (local, message_array, notmuch_message_t *, capacity);
+ }
+ message_array[count++] = root;
+ root->replies = _notmuch_message_sort_subtrees (root, root->replies);
+    }
+
+    qsort (message_array, count, sizeof (notmuch_message_t *), _cmpmsg);
+    for (size_t i = 0; i < count; i++) {
+ _notmuch_message_list_add_message (new_list, message_array[i]);
+    }
+
+    talloc_free (local);
+    talloc_free (list);
+    return new_list;
+}
+
 notmuch_messages_t *
 notmuch_message_get_replies (notmuch_message_t *message)
 {
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 499d73d4..725f9a7e 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -539,6 +539,9 @@ _notmuch_message_remove_unprefixed_terms (notmuch_message_t *message);
 const char *
 _notmuch_message_get_thread_id_only(notmuch_message_t *message);
 
+notmuch_message_list_t *
+_notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list);
+
 /* sha1.c */
 
 char *
diff --git a/lib/thread.cc b/lib/thread.cc
index e961c76b..b599a97d 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -429,6 +429,12 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
     _notmuch_message_list_add_message (thread->toplevel_list, message);
     }
 
+    /* XXX this could be made conditional on messages being inserted
+     * (out of order) in later passes
+     */
+    thread->toplevel_list = _notmuch_message_sort_subtrees (thread, thread->toplevel_list);
+
+
     /* XXX: After scanning through the entire list looking for parents
      * via "In-Reply-To", we should do a second pass that looks at the
      * list of messages IDs in the "References" header instead. (And
diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 753c7ae1..72af50df 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -192,7 +192,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "reply to ghost (RT)"
-test_subtest_known_broken
 notmuch show --entire-thread=true id:[hidden email] | grep ^Subject: | head -1  > OUTPUT
 cat <<EOF > EXPECTED
 Subject: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
@@ -200,7 +199,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "reply to ghost (RT/tree view)"
-test_subtest_known_broken
 test_emacs '(notmuch-tree "id:[hidden email]")
     (notmuch-test-wait)
     (test-output)
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 05/15] lib: read reference terms into message struct.

In reply to this post by David Bremner-2
The plan is to use these in resolving threads.
---
 lib/message.cc        | 18 ++++++++++++++++++
 lib/notmuch-private.h |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/lib/message.cc b/lib/message.cc
index 59e42d36..d00da0ad 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -41,6 +41,7 @@ struct _notmuch_message {
     notmuch_message_file_t *message_file;
     notmuch_string_list_t *property_term_list;
     notmuch_string_map_t *property_map;
+    notmuch_string_list_t *reference_list;
     notmuch_message_list_t *replies;
     unsigned long flags;
     /* For flags that are initialized on-demand, lazy_flags indicates
@@ -129,6 +130,7 @@ _notmuch_message_create_for_document (const void *talloc_owner,
     message->author = NULL;
     message->property_term_list = NULL;
     message->property_map = NULL;
+    message->reference_list = NULL;
 
     message->replies = _notmuch_message_list_create (message);
     if (unlikely (message->replies == NULL)) {
@@ -349,6 +351,7 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message, void *field)
  *type_prefix = _find_prefix ("type"),
  *filename_prefix = _find_prefix ("file-direntry"),
  *property_prefix = _find_prefix ("property"),
+ *reference_prefix = _find_prefix ("reference"),
  *replyto_prefix = _find_prefix ("replyto");
 
     /* We do this all in a single pass because Xapian decompresses the
@@ -413,6 +416,14 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message, void *field)
     _notmuch_database_get_terms_with_prefix (message, i, end,
  property_prefix);
 
+    /* get references */
+    assert (strcmp (property_prefix, reference_prefix) < 0);
+    if (!message->reference_list) {
+ message->reference_list =
+    _notmuch_database_get_terms_with_prefix (message, i, end,
+     reference_prefix);
+    }
+
     /* Get reply to */
     assert (strcmp (property_prefix, replyto_prefix) < 0);
     if (!message->in_reply_to)
@@ -588,6 +599,13 @@ _notmuch_message_add_reply (notmuch_message_t *message,
     _notmuch_message_list_add_message (message->replies, reply);
 }
 
+const notmuch_string_list_t *
+_notmuch_message_get_references (notmuch_message_t *message)
+{
+    _notmuch_message_ensure_metadata (message, message->reference_list);
+    return message->reference_list;
+}
+
 static int
 _cmpmsg (const void *pa, const void *pb)
 {
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 725f9a7e..8f1b3238 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -583,6 +583,9 @@ _notmuch_string_list_append (notmuch_string_list_t *list,
 void
 _notmuch_string_list_sort (notmuch_string_list_t *list);
 
+const notmuch_string_list_t *
+_notmuch_message_get_references(notmuch_message_t *message);
+
 /* string-map.c */
 typedef struct _notmuch_string_map  notmuch_string_map_t;
 typedef struct _notmuch_string_map_iterator notmuch_string_map_iterator_t;
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 06/15] lib/thread: refactor in_reply_to test

In reply to this post by David Bremner-2
This is not a complete win in code-size, but it makes the code (which
is about to get more complicated) easier to follow.
---
 lib/thread.cc | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index b599a97d..8074b625 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -387,12 +387,31 @@ _thread_add_matched_message (notmuch_thread_t *thread,
     _thread_add_matched_author (thread, _notmuch_message_get_author (hashed_message));
 }
 
+static bool
+_parent_via_in_reply_to (notmuch_thread_t *thread, notmuch_message_t *message) {
+    notmuch_message_t *parent;
+    const char *in_reply_to;
+
+    in_reply_to = _notmuch_message_get_in_reply_to (message);
+    DEBUG_PRINTF("checking message = %s in_reply_to=%s\n",
+ notmuch_message_get_message_id (message), in_reply_to);
+
+    if (in_reply_to && strlen (in_reply_to) &&
+ g_hash_table_lookup_extended (thread->message_hash,
+      in_reply_to, NULL,
+      (void **) &parent)) {
+ _notmuch_message_add_reply (parent, message);
+ return true;
+    } else {
+ return false;
+    }
+}
+
 static void
 _resolve_thread_relationships (notmuch_thread_t *thread)
 {
     notmuch_message_node_t *node, *first_node;
-    notmuch_message_t *message, *parent;
-    const char *in_reply_to;
+    notmuch_message_t *message;
 
     first_node = thread->message_list->head;
     if (! first_node)
@@ -400,13 +419,7 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
 
     for (node = first_node->next; node; node = node->next) {
  message = node->message;
- in_reply_to = _notmuch_message_get_in_reply_to (message);
- if (in_reply_to && strlen (in_reply_to) &&
-    g_hash_table_lookup_extended (thread->message_hash,
-  in_reply_to, NULL,
-  (void **) &parent))
-    _notmuch_message_add_reply (parent, message);
- else
+ if (! _parent_via_in_reply_to (thread, message))
     _notmuch_message_list_add_message (thread->toplevel_list, message);
     }
 
@@ -418,15 +431,10 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
      */
     if (first_node) {
  message = first_node->message;
- in_reply_to = _notmuch_message_get_in_reply_to (message);
- if (thread->toplevel_list->head &&
-    in_reply_to && strlen (in_reply_to) &&
-    g_hash_table_lookup_extended (thread->message_hash,
-  in_reply_to, NULL,
-  (void **) &parent))
-    _notmuch_message_add_reply (parent, message);
- else
+ if (! thread->toplevel_list->head ||
+    ! _parent_via_in_reply_to (thread, message)) {
     _notmuch_message_list_add_message (thread->toplevel_list, message);
+ }
     }
 
     /* XXX this could be made conditional on messages being inserted
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 07/15] lib/thread: initial use of references as for fallback parenting

In reply to this post by David Bremner-2
This is mainly to lay out the structure of the final code. The problem
isn't really solved yet, although some very simple cases are
better (hence the fixed test). We need two passes through the messages
because we need to be careful not to re-parent too many messages and
end up without any toplevel messages.
---
 lib/thread.cc               | 43 +++++++++++++++++++++++++++++++++++--
 test/T510-thread-replies.sh |  1 -
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index 8074b625..72c447ef 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -407,22 +407,52 @@ _parent_via_in_reply_to (notmuch_thread_t *thread, notmuch_message_t *message) {
     }
 }
 
+static void
+_parent_or_toplevel (notmuch_thread_t *thread, notmuch_message_t *message)
+{
+    bool found = false;
+    notmuch_message_t *parent = NULL;
+    const notmuch_string_list_t *references =
+ _notmuch_message_get_references (message);
+    for (notmuch_string_node_t *ref_node = references->head;
+ ! found && ref_node; ref_node = ref_node->next) {
+ if ((found = g_hash_table_lookup_extended (thread->message_hash,
+   ref_node->string, NULL,
+   (void **) &parent))) {
+    _notmuch_message_add_reply (parent, message);
+ }
+    }
+    if (! found)
+ _notmuch_message_list_add_message (thread->toplevel_list, message);
+}
+
 static void
 _resolve_thread_relationships (notmuch_thread_t *thread)
 {
     notmuch_message_node_t *node, *first_node;
     notmuch_message_t *message;
+    void *local;
+    notmuch_message_list_t *maybe_toplevel_list;
 
     first_node = thread->message_list->head;
     if (! first_node)
  return;
 
+    local = talloc_new (thread);
+    maybe_toplevel_list = _notmuch_message_list_create (local);
+
     for (node = first_node->next; node; node = node->next) {
  message = node->message;
  if (! _parent_via_in_reply_to (thread, message))
-    _notmuch_message_list_add_message (thread->toplevel_list, message);
+    _notmuch_message_list_add_message (maybe_toplevel_list, message);
     }
 
+    for (notmuch_messages_t *roots = _notmuch_messages_create (maybe_toplevel_list);
+ notmuch_messages_valid (roots);
+ notmuch_messages_move_to_next (roots)) {
+ notmuch_message_t *message = notmuch_messages_get (roots);
+ _parent_or_toplevel (thread, message);
+    }
     /*
      * if we reach the end of the list without finding a top-level
      * message, that means the thread is a cycle (or set of cycles)
@@ -433,7 +463,15 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
  message = first_node->message;
  if (! thread->toplevel_list->head ||
     ! _parent_via_in_reply_to (thread, message)) {
-    _notmuch_message_list_add_message (thread->toplevel_list, message);
+    /*
+     * If the oldest message happens to be in-reply-to a
+     * missing message, we only check for references if there
+     * is some other candidate for root message.
+     */
+    if (thread->toplevel_list->head)
+ _parent_or_toplevel (thread, message);
+    else
+ _notmuch_message_list_add_message (thread->toplevel_list, message);
  }
     }
 
@@ -453,6 +491,7 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
      * correctly in the thread even when an intermediate message is
      * missing from the thread.
      */
+    talloc_free (local);
 }
 
 /* Create a new notmuch_thread_t object by finding the thread
diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 72af50df..c244054a 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -167,7 +167,6 @@ test_expect_equal_json "$output" "$expected"
 add_email_corpus threading
 
 test_begin_subtest "reply to ghost"
-test_subtest_known_broken
 notmuch show --entire-thread=true id:[hidden email] | grep ^Subject: | head -1  > OUTPUT
 cat <<EOF > EXPECTED
 Subject: root message
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 08/15] lib: calculate message depth in thread

In reply to this post by David Bremner-2
This will be used in reparenting messages without useful in-reply-to,
but with useful references
---
 lib/message.cc        | 23 +++++++++++++++++++++++
 lib/notmuch-private.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/lib/message.cc b/lib/message.cc
index d00da0ad..5e17029a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -32,6 +32,7 @@ struct _notmuch_message {
     int frozen;
     char *message_id;
     char *thread_id;
+    size_t thread_depth;
     char *in_reply_to;
     notmuch_string_list_t *tag_list;
     notmuch_string_list_t *filename_term_list;
@@ -118,6 +119,9 @@ _notmuch_message_create_for_document (const void *talloc_owner,
     /* the message is initially not synchronized with Xapian */
     message->last_view = 0;
 
+    /* Calculated after the thread structure is computed */
+    message->thread_depth = 0;
+
     /* Each of these will be lazily created as needed. */
     message->message_id = NULL;
     message->thread_id = NULL;
@@ -599,6 +603,25 @@ _notmuch_message_add_reply (notmuch_message_t *message,
     _notmuch_message_list_add_message (message->replies, reply);
 }
 
+size_t
+_notmuch_message_get_thread_depth (notmuch_message_t *message) {
+    return message->thread_depth;
+}
+
+void
+_notmuch_message_label_depths (notmuch_message_t *message,
+       size_t depth)
+{
+    message->thread_depth = depth;
+
+    for (notmuch_messages_t *messages = _notmuch_messages_create (message->replies);
+ notmuch_messages_valid (messages);
+ notmuch_messages_move_to_next (messages)) {
+ notmuch_message_t *child = notmuch_messages_get (messages);
+ _notmuch_message_label_depths (child, depth+1);
+    }
+}
+
 const notmuch_string_list_t *
 _notmuch_message_get_references (notmuch_message_t *message)
 {
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 8f1b3238..fd0d251b 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -539,6 +539,12 @@ _notmuch_message_remove_unprefixed_terms (notmuch_message_t *message);
 const char *
 _notmuch_message_get_thread_id_only(notmuch_message_t *message);
 
+size_t _notmuch_message_get_thread_depth (notmuch_message_t *message);
+
+void
+_notmuch_message_label_depths (notmuch_message_t *message,
+       size_t depth);
+
 notmuch_message_list_t *
 _notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list);
 
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 09/15] lib/thread: rewrite _parent_or_toplevel to use depths

In reply to this post by David Bremner-2
This is part 1/2 of changing the reparenting of alleged toplevel
messages to use a "deep" reference rather than just the first one
found.
---
 lib/thread.cc | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index 72c447ef..85ca00ae 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -410,20 +410,40 @@ _parent_via_in_reply_to (notmuch_thread_t *thread, notmuch_message_t *message) {
 static void
 _parent_or_toplevel (notmuch_thread_t *thread, notmuch_message_t *message)
 {
-    bool found = false;
+    size_t max_depth = 0;
+    notmuch_message_t *new_parent;
     notmuch_message_t *parent = NULL;
     const notmuch_string_list_t *references =
  _notmuch_message_get_references (message);
+
+    DEBUG_PRINTF("trying to reparent via references: %s\n",
+     notmuch_message_get_message_id (message));
+
     for (notmuch_string_node_t *ref_node = references->head;
- ! found && ref_node; ref_node = ref_node->next) {
- if ((found = g_hash_table_lookup_extended (thread->message_hash,
-   ref_node->string, NULL,
-   (void **) &parent))) {
-    _notmuch_message_add_reply (parent, message);
+ ref_node; ref_node = ref_node->next) {
+ DEBUG_PRINTF("checking reference=%s\n", ref_node->string);
+ if ((g_hash_table_lookup_extended (thread->message_hash,
+   ref_node->string, NULL,
+   (void **) &new_parent))) {
+    size_t new_depth = _notmuch_message_get_thread_depth (new_parent);
+    DEBUG_PRINTF("got depth %lu\n", new_depth);
+    if (new_depth > max_depth || !parent) {
+ DEBUG_PRINTF("adding at depth %lu parent=%s\n", new_depth, ref_node->string);
+ max_depth = new_depth;
+ parent = new_parent;
+    }
  }
     }
-    if (! found)
+    if (parent) {
+ DEBUG_PRINTF("adding reply %s to parent=%s\n",
+ notmuch_message_get_message_id (message),
+ notmuch_message_get_message_id (parent));
+ _notmuch_message_add_reply (parent, message);
+    } else {
+ DEBUG_PRINTF("adding as toplevel %s\n",
+ notmuch_message_get_message_id (message));
  _notmuch_message_list_add_message (thread->toplevel_list, message);
+    }
 }
 
 static void
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 10/15] lib/thread: change _resolve_thread_relationships to use depths

In reply to this post by David Bremner-2
We (finally) implement the XXX comment. It requires a bit of care not
to reparent all of the possible toplevel messages. There is some
slightly naughty low level access to the list structures here that can
be cleaned up in a later commit.
---
 lib/thread.cc               | 53 ++++++++++++++++++-------------------
 test/T510-thread-replies.sh |  1 -
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index 85ca00ae..e4ab4319 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -467,12 +467,6 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
     _notmuch_message_list_add_message (maybe_toplevel_list, message);
     }
 
-    for (notmuch_messages_t *roots = _notmuch_messages_create (maybe_toplevel_list);
- notmuch_messages_valid (roots);
- notmuch_messages_move_to_next (roots)) {
- notmuch_message_t *message = notmuch_messages_get (roots);
- _parent_or_toplevel (thread, message);
-    }
     /*
      * if we reach the end of the list without finding a top-level
      * message, that means the thread is a cycle (or set of cycles)
@@ -481,36 +475,41 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
      */
     if (first_node) {
  message = first_node->message;
- if (! thread->toplevel_list->head ||
+ DEBUG_PRINTF("checking first message  %s\n",
+     notmuch_message_get_message_id (message));
+
+ if (! maybe_toplevel_list->head ||
     ! _parent_via_in_reply_to (thread, message)) {
-    /*
-     * If the oldest message happens to be in-reply-to a
-     * missing message, we only check for references if there
-     * is some other candidate for root message.
-     */
-    if (thread->toplevel_list->head)
- _parent_or_toplevel (thread, message);
-    else
- _notmuch_message_list_add_message (thread->toplevel_list, message);
+    DEBUG_PRINTF("adding first message as toplevel = %s\n",
+     notmuch_message_get_message_id (message));
+    _notmuch_message_list_add_message (maybe_toplevel_list, message);
  }
     }
 
+    for (notmuch_messages_t *messages = _notmuch_messages_create (maybe_toplevel_list);
+ notmuch_messages_valid (messages);
+ notmuch_messages_move_to_next (messages))
+    {
+ notmuch_message_t *message = notmuch_messages_get (messages);
+ _notmuch_message_label_depths (message, 0);
+    }
+
+    for (notmuch_messages_t *roots = _notmuch_messages_create (maybe_toplevel_list);
+ notmuch_messages_valid (roots);
+ notmuch_messages_move_to_next (roots)) {
+ notmuch_message_t *message = notmuch_messages_get (roots);
+ /* XXX TODO: cleaner way to test iterator for last, list for emptiness  */
+ if (roots->iterator->next || thread->toplevel_list->head)
+    _parent_or_toplevel (thread, message);
+ else
+    _notmuch_message_list_add_message (thread->toplevel_list, message);
+    }
+
     /* XXX this could be made conditional on messages being inserted
      * (out of order) in later passes
      */
     thread->toplevel_list = _notmuch_message_sort_subtrees (thread, thread->toplevel_list);
 
-
-    /* XXX: After scanning through the entire list looking for parents
-     * via "In-Reply-To", we should do a second pass that looks at the
-     * list of messages IDs in the "References" header instead. (And
-     * for this the parent would be the "deepest" message of all the
-     * messages found in the "References" list.)
-     *
-     * Doing this will allow messages and sub-threads to be positioned
-     * correctly in the thread even when an intermediate message is
-     * missing from the thread.
-     */
     talloc_free (local);
 }
 
diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index c244054a..915e68ef 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -174,7 +174,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "reply to ghost (tree view)"
-test_subtest_known_broken
 test_emacs '(notmuch-tree "id:[hidden email]")
     (notmuch-test-wait)
     (test-output)
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 11/15] test: add known broken test for good In-Reply-To / bad References

In reply to this post by David Bremner-2
The current scheme of choosing the replyto (i.e. the default parent
for threading purposes) does not work well for mailers that put
the oldest Reference last.
---
 test/T510-thread-replies.sh                       | 15 +++++++++++++++
 test/corpora/threading/parent-priority/cur/child  | 11 +++++++++++
 .../threading/parent-priority/cur/grand-child     | 10 ++++++++++
 test/corpora/threading/parent-priority/cur/root   |  7 +++++++
 4 files changed, 43 insertions(+)
 create mode 100644 test/corpora/threading/parent-priority/cur/child
 create mode 100644 test/corpora/threading/parent-priority/cur/grand-child
 create mode 100644 test/corpora/threading/parent-priority/cur/root

diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 915e68ef..14d6ee26 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -209,4 +209,19 @@ End of search results.
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "trusting reply-to (tree view)"
+test_subtest_known_broken
+test_emacs '(notmuch-tree "id:[hidden email]")
+    (notmuch-test-wait)
+    (test-output)
+    (delete-other-windows)'
+cat <<EOF > EXPECTED
+  2016-06-17  Alice                 ┬►root message                                        (inbox unread)
+  2016-06-18  Alice                 ╰┬►child message                                      (inbox unread)
+  2016-06-18  Alice                  ╰─►grand-child message                               (inbox unread)
+End of search results.
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+
 test_done
diff --git a/test/corpora/threading/parent-priority/cur/child b/test/corpora/threading/parent-priority/cur/child
new file mode 100644
index 00000000..23ee6495
--- /dev/null
+++ b/test/corpora/threading/parent-priority/cur/child
@@ -0,0 +1,11 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: child message
+Message-ID: <[hidden email]>
+In-Reply-To: <[hidden email]>
+References:  <[hidden email]>
+Date: Fri, 17 Jun 2016 22:14:41 -0400
+
+This is a normal-ish reply, and has both a references header and an
+in-reply-to header.
+
diff --git a/test/corpora/threading/parent-priority/cur/grand-child b/test/corpora/threading/parent-priority/cur/grand-child
new file mode 100644
index 00000000..028371d4
--- /dev/null
+++ b/test/corpora/threading/parent-priority/cur/grand-child
@@ -0,0 +1,10 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: grand-child message
+Message-ID: <[hidden email]>
+In-Reply-To: <[hidden email]>
+References:  <[hidden email]> <[hidden email]>
+Date: Fri, 17 Jun 2016 22:24:41 -0400
+
+This has the references headers in the wrong order, with oldest first.
+Debbugs does this.
diff --git a/test/corpora/threading/parent-priority/cur/root b/test/corpora/threading/parent-priority/cur/root
new file mode 100644
index 00000000..3990843d
--- /dev/null
+++ b/test/corpora/threading/parent-priority/cur/root
@@ -0,0 +1,7 @@
+From: Alice <[hidden email]>
+To: Daniel <[hidden email]>
+Subject: root message
+Message-ID: <[hidden email]>
+Date: Thu, 16 Jun 2016 22:14:41 -0400
+
+This message has no reply-to
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 12/15] test/thread-replies: mangle In-Reply-To's

In reply to this post by David Bremner-2
In a future commit, we will start trusting In-Reply-To's when they
look sane (i.e. a single message-id). Modify these tests so they will
keep passing (i.e. keep choosing References) when that happens.
---
 test/T510-thread-replies.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 14d6ee26..87dae2df 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -45,10 +45,10 @@ expected='[[[{"id": "[hidden email]",
 expected=`echo "$expected" | notmuch_json_show_sanitize`
 test_expect_equal_json "$output" "$expected"
 
-test_begin_subtest "Prefer References to In-Reply-To"
+test_begin_subtest "Prefer References to dodgy In-Reply-To"
 add_message '[id]="[hidden email]"' \
     '[subject]=two'
-add_message '[in-reply-to]="<[hidden email]>"' \
+add_message '[in-reply-to]="Your message of December 31 1999 <[hidden email]>"' \
     '[references]="<[hidden email]>"' \
     '[subject]="Re: two"'
 output=$(notmuch show --format=json 'subject:two' | notmuch_json_show_sanitize)
@@ -101,12 +101,12 @@ expected='[[[{"id": "[hidden email]", "match": true, "excluded": false,
 expected=`echo "$expected" | notmuch_json_show_sanitize`
 test_expect_equal_json "$output" "$expected"
 
-test_begin_subtest "Use last Reference"
+test_begin_subtest "Use last Reference when In-Reply-To is dodgy"
 add_message '[id]="[hidden email]"' \
     '[subject]="four"'
 add_message '[id]="[hidden email]"' \
     '[subject]="not-four"'
-add_message '[in-reply-to]="<[hidden email]>"' \
+add_message '[in-reply-to]="<[hidden email]> (RFC822 4lyfe)"' \
     '[references]="<[hidden email]> <[hidden email]>"' \
     '[subject]="neither"'
 output=$(notmuch show --format=json 'subject:four' | notmuch_json_show_sanitize)
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 13/15] util/string-util: export skip_space

In reply to this post by David Bremner-2
It's only few lines, but we already define the function, so make it
usable elsewhere
---
 util/string-util.c | 2 +-
 util/string-util.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/string-util.c b/util/string-util.c
index b0108811..fc2058e0 100644
--- a/util/string-util.c
+++ b/util/string-util.c
@@ -141,7 +141,7 @@ make_boolean_term (void *ctx, const char *prefix, const char *term,
     return 0;
 }
 
-static const char*
+const char*
 skip_space (const char *str)
 {
     while (*str && isspace ((unsigned char) *str))
diff --git a/util/string-util.h b/util/string-util.h
index 97770614..4c110a20 100644
--- a/util/string-util.h
+++ b/util/string-util.h
@@ -77,6 +77,8 @@ unsigned int strcase_hash (const void *ptr);
 
 void strip_trailing (char *str, char ch);
 
+const char* skip_space (const char *str);
+
 #ifdef __cplusplus
 }
 #endif
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 14/15] lib: add _notmuch_message_id_parse_strict

In reply to this post by David Bremner-2
The idea is that if a message-id parses with this function, the MUA
generating it was probably sane, and in particular it's probably safe
to use the result as a parent from In-Reply-to.
---
 lib/message-id.c        | 32 ++++++++++++++++++
 lib/notmuch-private.h   | 14 ++++++++
 test/Makefile.local     |  6 +++-
 test/T710-message-id.sh | 73 +++++++++++++++++++++++++++++++++++++++++
 test/message-id-parse.c | 26 +++++++++++++++
 5 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100755 test/T710-message-id.sh
 create mode 100644 test/message-id-parse.c

diff --git a/lib/message-id.c b/lib/message-id.c
index d7541d50..a1dce9c8 100644
--- a/lib/message-id.c
+++ b/lib/message-id.c
@@ -1,4 +1,5 @@
 #include "notmuch-private.h"
+#include "string-util.h"
 
 /* Advance 'str' past any whitespace or RFC 822 comments. A comment is
  * a (potentially nested) parenthesized sequence with '\' used to
@@ -94,3 +95,34 @@ _notmuch_message_id_parse (void *ctx, const char *message_id, const char **next)
 
     return result;
 }
+
+char *
+_notmuch_message_id_parse_strict (void *ctx, const char *message_id)
+{
+    const char *s, *end;
+    char *result;
+
+    if (message_id == NULL || *message_id == '\0')
+ return NULL;
+
+    s = skip_space (message_id);
+    if (*s == '<')
+ s++;
+    else
+ return NULL;
+
+    for (end = s; *end && *end != '>'; end++)
+ if (isspace (*end))
+    return NULL;
+
+    if (*end != '>')
+ return NULL;
+    else {
+ const char *last =  skip_space (end+1);
+ if (*last != '\0')
+    return NULL;
+    }
+
+    result = talloc_strndup (ctx, s, end - s);
+    return result;
+}
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index fd0d251b..5bbaa292 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -526,6 +526,20 @@ _notmuch_query_count_documents (notmuch_query_t *query,
 char *
 _notmuch_message_id_parse (void *ctx, const char *message_id, const char **next);
 
+/* Parse a message-id, discarding leading and trailing whitespace, and
+ * '<' and '>' delimiters.
+ *
+ * Apply a probably-stricter-than RFC definition of what is allowed in
+ * a message-id. In particular, forbid whitespace.
+ *
+ * Returns a newly talloc'ed string belonging to 'ctx'.
+ *
+ * Returns NULL if there is any error parsing the message-id.
+ */
+
+char *
+_notmuch_message_id_parse_strict (void *ctx, const char *message_id);
+
 
 /* message.cc */
 
diff --git a/test/Makefile.local b/test/Makefile.local
index 1a0ab813..1cf09778 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -15,6 +15,9 @@ smtp_dummy_modules = $(smtp_dummy_srcs:.c=.o)
 $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libnotmuch_util.a
  $(call quiet,CC) $^ -o $@ $(LDFLAGS)
 
+$(dir)/message-id-parse: $(dir)/message-id-parse.o lib/libnotmuch.a util/libnotmuch_util.a
+ $(call quiet,CC) $^ -o $@ $(LDFLAGS) $(TALLOC_LDFLAGS)
+
 $(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libnotmuch_util.a
  $(call quiet,CC) $^ -o $@ $(LDFLAGS) $(TALLOC_LDFLAGS)
 
@@ -50,7 +53,8 @@ test_main_srcs=$(dir)/arg-test.c \
       $(dir)/smtp-dummy.c \
       $(dir)/symbol-test.cc \
       $(dir)/make-db-version.cc \
-      $(dir)/ghost-report.cc
+      $(dir)/ghost-report.cc \
+      $(dir)/message-id-parse.c
 
 test_srcs=$(test_main_srcs) $(dir)/database-test.c
 
diff --git a/test/T710-message-id.sh b/test/T710-message-id.sh
new file mode 100755
index 00000000..e73d6ba9
--- /dev/null
+++ b/test/T710-message-id.sh
@@ -0,0 +1,73 @@
+#!/usr/bin/env bash
+test_description="message id parsing"
+
+. $(dirname "$0")/test-lib.sh || exit 1
+
+test_begin_subtest "good message ids"
+${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
+<[hidden email]>
+<[hidden email]>
+<[hidden email]>
+EOF
+cat <<EOF >EXPECTED
+GOOD: [hidden email]
+GOOD: [hidden email]
+GOOD: [hidden email]
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "leading and trailing space is OK"
+${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
+   <[hidden email]>
+<[hidden email]>    
+    <[hidden email]>
+EOF
+cat <<EOF >EXPECTED
+GOOD: [hidden email]
+GOOD: [hidden email]
+GOOD: [hidden email]
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "<> delimeters are required"
+${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
+[hidden email]>
+<[hidden email]
+[hidden email]
+EOF
+cat <<EOF >EXPECTED
+BAD: [hidden email]>
+BAD: <[hidden email]
+BAD: [hidden email]
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "embedded whitespace is forbidden"
+${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
+<018b1a8f2d1df62e804ce88b65401304832dfbbf.1346614915 .[hidden email]>
+<1530507300.raoomurnbf.astroid @strange.none>
+<1258787708-21121- [hidden email]>
+EOF
+cat <<EOF >EXPECTED
+BAD: <018b1a8f2d1df62e804ce88b65401304832dfbbf.1346614915 .[hidden email]>
+BAD: <1530507300.raoomurnbf.astroid @strange.none>
+BAD: <1258787708-21121- [hidden email]>
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+
+test_begin_subtest "folded real life bad In-Reply-To values"
+${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
+<[hidden email]> (Ian Jackson's message of "Mon, 5 Dec 2016 14:41:01 +0000")
+<[hidden email]>  <[hidden email]>
+Your message of Tue, 09 Dec 2014 13:21:11 +0100. <1900758.CgLNVPbY9N@liber>
+EOF
+cat <<EOF >EXPECTED
+BAD: <[hidden email]> (Ian Jackson's message of "Mon, 5 Dec 2016 14:41:01 +0000")
+BAD: <[hidden email]>  <[hidden email]>
+BAD: Your message of Tue, 09 Dec 2014 13:21:11 +0100. <1900758.CgLNVPbY9N@liber>
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+
+test_done
diff --git a/test/message-id-parse.c b/test/message-id-parse.c
new file mode 100644
index 00000000..752eb1fd
--- /dev/null
+++ b/test/message-id-parse.c
@@ -0,0 +1,26 @@
+#include <stdio.h>
+#include <talloc.h>
+#include "notmuch-private.h"
+
+int
+main (unused (int argc), unused (char **argv))
+{
+    char *line = NULL;
+    size_t len = 0;
+    ssize_t nread;
+    void *local = talloc_new (NULL);
+
+    while ((nread = getline (&line, &len, stdin)) != -1) {
+ int last = strlen (line) - 1;
+ if (line[last] == '\n')
+    line[last] = '\0';
+
+ char *mid = _notmuch_message_id_parse_strict (local, line);
+ if (mid)
+    printf ("GOOD: %s\n", mid);
+ else
+    printf ("BAD: %s\n", line);
+    }
+
+    talloc_free (local);
+}
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

[PATCH 15/15] lib: change parent strategy to use In-Reply-To if it looks sane

In reply to this post by David Bremner-2
As reported by Sean Whitton, there are mailers (in particular the
Debian Bug Tracking System) that have sensible In-Reply-To headers,
but un-useful-for-notmuch References (in particular with the BTS, the
oldest reference is last). I looked at a sample of about 200K
messages, and only about 0.5% these had something other than a single
message-id in In-Reply-To. On this basis, if we see a single
message-id in In-Reply-To, consider that as authoritative.
---
 lib/add-message.cc          | 20 +++++++++++++++-----
 test/T510-thread-replies.sh |  1 -
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/lib/add-message.cc b/lib/add-message.cc
index f5fac8be..da37032c 100644
--- a/lib/add-message.cc
+++ b/lib/add-message.cc
@@ -227,7 +227,7 @@ _notmuch_database_link_message_to_parents (notmuch_database_t *notmuch,
    const char **thread_id)
 {
     GHashTable *parents = NULL;
-    const char *refs, *in_reply_to, *in_reply_to_message_id;
+    const char *refs, *in_reply_to, *in_reply_to_message_id, *strict_message_id = NULL;
     const char *last_ref_message_id, *this_message_id;
     GList *l, *keys = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
@@ -242,14 +242,24 @@ _notmuch_database_link_message_to_parents (notmuch_database_t *notmuch,
     parents, refs);
 
     in_reply_to = _notmuch_message_file_get_header (message_file, "in-reply-to");
+    if (in_reply_to)
+ strict_message_id = _notmuch_message_id_parse_strict (message,
+      in_reply_to);
+
     in_reply_to_message_id = parse_references (message,
        this_message_id,
        parents, in_reply_to);
 
-    /* For the parent of this message, use the last message ID of the
-     * References header, if available.  If not, fall back to the
-     * first message ID in the In-Reply-To header. */
-    if (last_ref_message_id) {
+    /* For the parent of this message, use
+     * 1) the In-Reply-To header, if it looks sane, otherwise
+     * 2) the last message ID of the References header, if available.
+     * 3) Otherwise, fall back to the first message ID in
+     * the In-Reply-To header.
+     */
+
+    if (strict_message_id) {
+ _notmuch_message_add_term (message, "replyto", strict_message_id);
+    } else if (last_ref_message_id) {
  _notmuch_message_add_term (message, "replyto",
    last_ref_message_id);
     } else if (in_reply_to_message_id) {
diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
index 87dae2df..4688c0ab 100755
--- a/test/T510-thread-replies.sh
+++ b/test/T510-thread-replies.sh
@@ -210,7 +210,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "trusting reply-to (tree view)"
-test_subtest_known_broken
 test_emacs '(notmuch-tree "id:[hidden email]")
     (notmuch-test-wait)
     (test-output)
--
2.18.0

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 01/15] util: add DEBUG_PRINTF, rename error_util.h -> debug_print.h

In reply to this post by David Bremner-2
On Thu, Aug 30 2018, David Bremner wrote:

> It seemed silly to have two headers that were almost copies.
> The new macro is intended for debugging, and probably should not be
> left in released code.
> ---
>  command-line-arguments.c             |  2 +-
>  lib/notmuch-private.h                |  2 +-
>  util/Makefile.local                  |  4 ++--
>  util/{error_util.c => debug_print.c} | 16 +++++++++++-----
>  util/{error_util.h => debug_print.h} | 22 ++++++++++++++++++----
>  util/hex-escape.c                    |  2 +-
>  util/util.c                          |  2 +-
>  util/xutil.c                         |  2 +-
>  8 files changed, 36 insertions(+), 16 deletions(-)
>  rename util/{error_util.c => debug_print.c} (81%)
>  rename util/{error_util.h => debug_print.h} (75%)
>
> diff --git a/command-line-arguments.c b/command-line-arguments.c
> index d64aa85b..90d69453 100644
> --- a/command-line-arguments.c
> +++ b/command-line-arguments.c
> @@ -1,7 +1,7 @@
>  #include <assert.h>
>  #include <string.h>
>  #include <stdio.h>
> -#include "error_util.h"
> +#include "debug_print.h"
>  #include "command-line-arguments.h"
>  
>  typedef enum {
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index 3764a6a9..499d73d4 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -50,7 +50,7 @@ NOTMUCH_BEGIN_DECLS
>  #include "gmime-extra.h"
>  
>  #include "xutil.h"
> -#include "error_util.h"
> +#include "debug_print.h"
>  #include "string-util.h"
>  #include "crypto.h"
>  
> diff --git a/util/Makefile.local b/util/Makefile.local
> index ba03230e..ccf1bd79 100644
> --- a/util/Makefile.local
> +++ b/util/Makefile.local
> @@ -3,9 +3,9 @@
>  dir := util
>  extra_cflags += -I$(srcdir)/$(dir)
>  
> -libnotmuch_util_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
> +libnotmuch_util_c_srcs := $(dir)/xutil.c $(dir)/hex-escape.c \
>    $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c \
> - $(dir)/util.c $(dir)/gmime-extra.c $(dir)/crypto.c
> + $(dir)/util.c $(dir)/gmime-extra.c $(dir)/crypto.c $(dir)/debug_print.c
>  
>  libnotmuch_util_modules := $(libnotmuch_util_c_srcs:.c=.o)
>  
> diff --git a/util/error_util.c b/util/debug_print.c
> similarity index 81%
> rename from util/error_util.c
> rename to util/debug_print.c
> index e64162c7..8b4bc73d 100644
> --- a/util/error_util.c
> +++ b/util/debug_print.c
> @@ -1,6 +1,7 @@
>  /* error_util.c - internal error utilities for notmuch.
>   *
>   * Copyright © 2009 Carl Worth
> + * Copyright © 2018 David Bremner
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -18,11 +19,17 @@
>   * Author: Carl Worth <[hidden email]>
>   */
>  
> -#include <stdlib.h>
> -#include <stdarg.h>
> -#include <stdio.h>
> +#include "debug_print.h"
>  
> -#include "error_util.h"
> +void
> +_debug_printf (const char *format, ...)
> +{
> +    va_list va_args;
> +
> +    va_start (va_args, format);
> +    vfprintf (stderr, format, va_args);
> +    va_end (va_args);
> +}
>  
>  void
>  _internal_error (const char *format, ...)
> @@ -37,4 +44,3 @@ _internal_error (const char *format, ...)
>      va_end (va_args);
>      exit (1);
>  }
> -
> diff --git a/util/error_util.h b/util/debug_print.h
> similarity index 75%
> rename from util/error_util.h
> rename to util/debug_print.h
> index 4bb338a2..4a096945 100644
> --- a/util/error_util.h
> +++ b/util/debug_print.h
> @@ -1,6 +1,7 @@
> -/* error_util.h - Provide the INTERNAL_ERROR macro
> +/* debug_print.h - Provide the INTERNAL_ERROR and DEBUG_PRINTF macros
>   *
>   * Copyright © 2009 Carl Worth
> + * Copyright © 2018 David Bremner
>   *
>   * This program is free software: you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -18,13 +19,26 @@
>   * Author: Carl Worth <[hidden email]>
>   */
>  
> -#ifndef ERROR_UTIL_H
> -#define ERROR_UTIL_H
> +#ifndef DEBUG_PRINT_H
> +#define DEBUG_PRINT_H
>  
>  #include <talloc.h>
> -
>  #include "function-attributes.h"
>  
> +void
> +_debug_printf (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
> +
> +/*
> + * Provide a quick way to silence debugging output.
> + */
> +
> +#ifdef DEBUG_PRINT
> +#define DEBUG_PRINTF(format, ...) _debug_printf(format " (%s).\n", \
> + ##__VA_ARGS__, __location__)

To me, YAGNI says that we could drop the whole _debug_printf() and
have
#include <stdio.h>
#define DEBUG_PRINTF(format, ...) fprintf(stderr, format " (%s).\n", \
                                          ##__VA_ARGS__, __location__)

(but if there is need, then perhaps the lib code inside #ifdef DEBUG_PRINT
... hmm, but, outside of this context, would this also move
_internal_error() to debug_print ;O )?

> +#else
> +#define DEBUG_PRINTF(format, ...) /* ignored */

perhaps:

#define DEBUG_PRINTF(format, ...) do {} while (0) /* ignored */

> +#endif
> +
>  /* There's no point in continuing when we've detected that we've done
>   * something wrong internally (as opposed to the user passing in a
>   * bogus value).
> diff --git a/util/hex-escape.c b/util/hex-escape.c
> index 8883ff90..ff20c702 100644
> --- a/util/hex-escape.c
> +++ b/util/hex-escape.c
> @@ -22,7 +22,7 @@
>  #include <string.h>
>  #include <talloc.h>
>  #include <ctype.h>
> -#include "error_util.h"
> +#include "debug_print.h"
>  #include "hex-escape.h"
>  
>  static const char *output_charset =
> diff --git a/util/util.c b/util/util.c
> index 06659b35..75df0ead 100644
> --- a/util/util.c
> +++ b/util/util.c
> @@ -1,5 +1,5 @@
>  #include "util.h"
> -#include "error_util.h"
> +#include "debug_print.h"
>  #include <string.h>
>  #include <errno.h>
>  
> diff --git a/util/xutil.c b/util/xutil.c
> index f211eaaa..11042c9e 100644
> --- a/util/xutil.c
> +++ b/util/xutil.c
> @@ -22,7 +22,7 @@
>  #include <string.h>
>  
>  #include "xutil.h"
> -#include "error_util.h"
> +#include "debug_print.h"
>  
>  void *
>  xcalloc (size_t nmemb, size_t size)
> --
> 2.18.0
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 04/15] lib/thread: sort sibling messages by date

In reply to this post by David Bremner-2
On Thu, Aug 30 2018, David Bremner wrote:

> For non-root messages, this should not should anything currently, as
> the messages are already added in date order. In the future we will
> add some non-root messages in a second pass out of order and the
> sorting will be useful. It does fix the order of multiple
> root-messages (although it is overkill for that).
> ---
>  lib/message.cc              | 47 +++++++++++++++++++++++++++++++++++++
>  lib/notmuch-private.h       |  3 +++
>  lib/thread.cc               |  6 +++++
>  test/T510-thread-replies.sh |  2 --
>  4 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index 153e4bed..59e42d36 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -588,6 +588,53 @@ _notmuch_message_add_reply (notmuch_message_t *message,
>      _notmuch_message_list_add_message (message->replies, reply);
>  }
>  
> +static int
> +_cmpmsg (const void *pa, const void *pb)
> +{
> +    notmuch_message_t **a = (notmuch_message_t **) pa;
> +    notmuch_message_t **b = (notmuch_message_t **) pb;
> +    time_t time_a = notmuch_message_get_date (*a);
> +    time_t time_b = notmuch_message_get_date (*b);
> +
> +    return (int) difftime (time_a, time_b);

time_t is often 64 bits now (and I suspect signed). int is often 32-bits
(gcc -dM -E -xc /dev/null | grep -i size to verify), perhaps time_t as
return value ?

> +}
> +
> +notmuch_message_list_t *
> +_notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list)
> +{
> +
> +    size_t count = 0;
> +    size_t capacity = 16;
> +
> +    if (! list)
> + return list;
> +
> +    void *local = talloc_new (NULL);
> +    notmuch_message_list_t *new_list = _notmuch_message_list_create (ctx);
> +    notmuch_message_t **message_array = talloc_zero_array (local, notmuch_message_t *, capacity);
> +
> +    for (notmuch_messages_t *messages = _notmuch_messages_create (list);
> + notmuch_messages_valid (messages);
> + notmuch_messages_move_to_next (messages)) {
> + notmuch_message_t *root = notmuch_messages_get (messages);
> + if (count >= capacity) {
> +    capacity *= 2;
> +    message_array = talloc_realloc (local, message_array, notmuch_message_t *, capacity);
> + }
> + message_array[count++] = root;
> + root->replies = _notmuch_message_sort_subtrees (root, root->replies);
> +    }
> +
> +    qsort (message_array, count, sizeof (notmuch_message_t *), _cmpmsg);
> +    for (size_t i = 0; i < count; i++) {
> + _notmuch_message_list_add_message (new_list, message_array[i]);
> +    }
> +
> +    talloc_free (local);
> +    talloc_free (list);
> +    return new_list;
> +}
> +
>  notmuch_messages_t *
>  notmuch_message_get_replies (notmuch_message_t *message)
>  {
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index 499d73d4..725f9a7e 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -539,6 +539,9 @@ _notmuch_message_remove_unprefixed_terms (notmuch_message_t *message);
>  const char *
>  _notmuch_message_get_thread_id_only(notmuch_message_t *message);
>  
> +notmuch_message_list_t *
> +_notmuch_message_sort_subtrees (void *ctx, notmuch_message_list_t *list);
> +
>  /* sha1.c */
>  
>  char *
> diff --git a/lib/thread.cc b/lib/thread.cc
> index e961c76b..b599a97d 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -429,6 +429,12 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
>      _notmuch_message_list_add_message (thread->toplevel_list, message);
>      }
>  
> +    /* XXX this could be made conditional on messages being inserted
> +     * (out of order) in later passes
> +     */
> +    thread->toplevel_list = _notmuch_message_sort_subtrees (thread, thread->toplevel_list);
> +
> +
>      /* XXX: After scanning through the entire list looking for parents
>       * via "In-Reply-To", we should do a second pass that looks at the
>       * list of messages IDs in the "References" header instead. (And
> diff --git a/test/T510-thread-replies.sh b/test/T510-thread-replies.sh
> index 753c7ae1..72af50df 100755
> --- a/test/T510-thread-replies.sh
> +++ b/test/T510-thread-replies.sh
> @@ -192,7 +192,6 @@ EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
>  test_begin_subtest "reply to ghost (RT)"
> -test_subtest_known_broken
>  notmuch show --entire-thread=true id:[hidden email] | grep ^Subject: | head -1  > OUTPUT
>  cat <<EOF > EXPECTED
>  Subject: FYI: xxxx  xxxxxxx  xxxxxxxxxxxx xxx
> @@ -200,7 +199,6 @@ EOF
>  test_expect_equal_file EXPECTED OUTPUT
>  
>  test_begin_subtest "reply to ghost (RT/tree view)"
> -test_subtest_known_broken
>  test_emacs '(notmuch-tree "id:[hidden email]")
>      (notmuch-test-wait)
>      (test-output)
> --
> 2.18.0
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 06/15] lib/thread: refactor in_reply_to test

In reply to this post by David Bremner-2
On Thu, Aug 30 2018, David Bremner wrote:

> This is not a complete win in code-size, but it makes the code (which
> is about to get more complicated) easier to follow.
> ---
>  lib/thread.cc | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index b599a97d..8074b625 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -387,12 +387,31 @@ _thread_add_matched_message (notmuch_thread_t *thread,
>      _thread_add_matched_author (thread, _notmuch_message_get_author (hashed_message));
>  }
>  
> +static bool
> +_parent_via_in_reply_to (notmuch_thread_t *thread, notmuch_message_t *message) {
> +    notmuch_message_t *parent;
> +    const char *in_reply_to;
> +
> +    in_reply_to = _notmuch_message_get_in_reply_to (message);
> +    DEBUG_PRINTF("checking message = %s in_reply_to=%s\n",
> + notmuch_message_get_message_id (message), in_reply_to);
> +
> +    if (in_reply_to && strlen (in_reply_to) &&

This is a code move, but did we already have a macro/inline function
to check when string is non-empty (or empty) -- expanding to (string[0])
strlen(string_with_million_nonzero_octets) just to check that string
is not empty is not necessarily a smart move...

e.g. something like:

        static inline bool empty_string(const char * string)
        {
                return string[0] == '\0';
        }            

or #define EMPTY_STRING(string) ((string)[0] == '\0')

(or the opposite versions, which I would have presented, but naming is hard)

> + g_hash_table_lookup_extended (thread->message_hash,
> +      in_reply_to, NULL,
> +      (void **) &parent)) {
> + _notmuch_message_add_reply (parent, message);
> + return true;
> +    } else {
> + return false;
> +    }
> +}
> +
>  static void
>  _resolve_thread_relationships (notmuch_thread_t *thread)
>  {
>      notmuch_message_node_t *node, *first_node;
> -    notmuch_message_t *message, *parent;
> -    const char *in_reply_to;
> +    notmuch_message_t *message;
>  
>      first_node = thread->message_list->head;
>      if (! first_node)
> @@ -400,13 +419,7 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
>  
>      for (node = first_node->next; node; node = node->next) {
>   message = node->message;
> - in_reply_to = _notmuch_message_get_in_reply_to (message);
> - if (in_reply_to && strlen (in_reply_to) &&
> -    g_hash_table_lookup_extended (thread->message_hash,
> -  in_reply_to, NULL,
> -  (void **) &parent))
> -    _notmuch_message_add_reply (parent, message);
> - else
> + if (! _parent_via_in_reply_to (thread, message))
>      _notmuch_message_list_add_message (thread->toplevel_list, message);
>      }
>  
> @@ -418,15 +431,10 @@ _resolve_thread_relationships (notmuch_thread_t *thread)
>       */
>      if (first_node) {
>   message = first_node->message;
> - in_reply_to = _notmuch_message_get_in_reply_to (message);
> - if (thread->toplevel_list->head &&
> -    in_reply_to && strlen (in_reply_to) &&
> -    g_hash_table_lookup_extended (thread->message_hash,
> -  in_reply_to, NULL,
> -  (void **) &parent))
> -    _notmuch_message_add_reply (parent, message);
> - else
> + if (! thread->toplevel_list->head ||
> +    ! _parent_via_in_reply_to (thread, message)) {
>      _notmuch_message_list_add_message (thread->toplevel_list, message);
> + }
>      }
>  
>      /* XXX this could be made conditional on messages being inserted
> --
> 2.18.0
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 14/15] lib: add _notmuch_message_id_parse_strict

In reply to this post by David Bremner-2
On Thu, Aug 30 2018, David Bremner wrote:

> The idea is that if a message-id parses with this function, the MUA
> generating it was probably sane, and in particular it's probably safe
> to use the result as a parent from In-Reply-to.
> ---
>  lib/message-id.c        | 32 ++++++++++++++++++
>  lib/notmuch-private.h   | 14 ++++++++
>  test/Makefile.local     |  6 +++-
>  test/T710-message-id.sh | 73 +++++++++++++++++++++++++++++++++++++++++
>  test/message-id-parse.c | 26 +++++++++++++++
>  5 files changed, 150 insertions(+), 1 deletion(-)
>  create mode 100755 test/T710-message-id.sh
>  create mode 100644 test/message-id-parse.c
>
> diff --git a/lib/message-id.c b/lib/message-id.c
> index d7541d50..a1dce9c8 100644
> --- a/lib/message-id.c
> +++ b/lib/message-id.c
> @@ -1,4 +1,5 @@
>  #include "notmuch-private.h"
> +#include "string-util.h"
>  
>  /* Advance 'str' past any whitespace or RFC 822 comments. A comment is
>   * a (potentially nested) parenthesized sequence with '\' used to
> @@ -94,3 +95,34 @@ _notmuch_message_id_parse (void *ctx, const char *message_id, const char **next)
>  
>      return result;
>  }
> +
> +char *
> +_notmuch_message_id_parse_strict (void *ctx, const char *message_id)
> +{
> +    const char *s, *end;
> +    char *result;
> +
> +    if (message_id == NULL || *message_id == '\0')
> + return NULL;
> +
> +    s = skip_space (message_id);
> +    if (*s == '<')
> + s++;
> +    else
> + return NULL;
> +
> +    for (end = s; *end && *end != '>'; end++)
> + if (isspace (*end))
> +    return NULL;
> +
> +    if (*end != '>')
> + return NULL;
> +    else {
> + const char *last =  skip_space (end+1);

this parser looks good to me, but the spacing in above line not
(first 2 spaces, and then missing spaces around '+')

> + if (*last != '\0')
> +    return NULL;
> +    }
> +
> +    result = talloc_strndup (ctx, s, end - s);
> +    return result;
> +}
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index fd0d251b..5bbaa292 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -526,6 +526,20 @@ _notmuch_query_count_documents (notmuch_query_t *query,
>  char *
>  _notmuch_message_id_parse (void *ctx, const char *message_id, const char **next);
>  
> +/* Parse a message-id, discarding leading and trailing whitespace, and
> + * '<' and '>' delimiters.
> + *
> + * Apply a probably-stricter-than RFC definition of what is allowed in
> + * a message-id. In particular, forbid whitespace.
> + *
> + * Returns a newly talloc'ed string belonging to 'ctx'.
> + *
> + * Returns NULL if there is any error parsing the message-id.
> + */
> +
> +char *
> +_notmuch_message_id_parse_strict (void *ctx, const char *message_id);
> +
>  
>  /* message.cc */
>  
> diff --git a/test/Makefile.local b/test/Makefile.local
> index 1a0ab813..1cf09778 100644
> --- a/test/Makefile.local
> +++ b/test/Makefile.local
> @@ -15,6 +15,9 @@ smtp_dummy_modules = $(smtp_dummy_srcs:.c=.o)
>  $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libnotmuch_util.a
>   $(call quiet,CC) $^ -o $@ $(LDFLAGS)
>  
> +$(dir)/message-id-parse: $(dir)/message-id-parse.o lib/libnotmuch.a util/libnotmuch_util.a
> + $(call quiet,CC) $^ -o $@ $(LDFLAGS) $(TALLOC_LDFLAGS)
> +
>  $(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libnotmuch_util.a
>   $(call quiet,CC) $^ -o $@ $(LDFLAGS) $(TALLOC_LDFLAGS)
>  
> @@ -50,7 +53,8 @@ test_main_srcs=$(dir)/arg-test.c \
>        $(dir)/smtp-dummy.c \
>        $(dir)/symbol-test.cc \
>        $(dir)/make-db-version.cc \
> -      $(dir)/ghost-report.cc
> +      $(dir)/ghost-report.cc \
> +      $(dir)/message-id-parse.c
>  
>  test_srcs=$(test_main_srcs) $(dir)/database-test.c
>  
> diff --git a/test/T710-message-id.sh b/test/T710-message-id.sh
> new file mode 100755
> index 00000000..e73d6ba9
> --- /dev/null
> +++ b/test/T710-message-id.sh
> @@ -0,0 +1,73 @@
> +#!/usr/bin/env bash
> +test_description="message id parsing"
> +
> +. $(dirname "$0")/test-lib.sh || exit 1
> +
> +test_begin_subtest "good message ids"
> +${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
> +<[hidden email]>
> +<[hidden email]>
> +<[hidden email]>
> +EOF
> +cat <<EOF >EXPECTED
> +GOOD: [hidden email]
> +GOOD: [hidden email]
> +GOOD: [hidden email]
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +test_begin_subtest "leading and trailing space is OK"
> +${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
> +   <[hidden email]>
> +<[hidden email]>    
> +    <[hidden email]>
> +EOF
> +cat <<EOF >EXPECTED
> +GOOD: [hidden email]
> +GOOD: [hidden email]
> +GOOD: [hidden email]
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +test_begin_subtest "<> delimeters are required"
> +${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
> +[hidden email]>
> +<[hidden email]
> +[hidden email]
> +EOF
> +cat <<EOF >EXPECTED
> +BAD: [hidden email]>
> +BAD: <[hidden email]
> +BAD: [hidden email]
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +test_begin_subtest "embedded whitespace is forbidden"
> +${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
> +<018b1a8f2d1df62e804ce88b65401304832dfbbf.1346614915 .[hidden email]>
> +<1530507300.raoomurnbf.astroid @strange.none>
> +<1258787708-21121- [hidden email]>
> +EOF
> +cat <<EOF >EXPECTED
> +BAD: <018b1a8f2d1df62e804ce88b65401304832dfbbf.1346614915 .[hidden email]>
> +BAD: <1530507300.raoomurnbf.astroid @strange.none>
> +BAD: <1258787708-21121- [hidden email]>
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +
> +test_begin_subtest "folded real life bad In-Reply-To values"
> +${TEST_DIRECTORY}/message-id-parse <<EOF >OUTPUT
> +<[hidden email]> (Ian Jackson's message of "Mon, 5 Dec 2016 14:41:01 +0000")
> +<[hidden email]>  <[hidden email]>
> +Your message of Tue, 09 Dec 2014 13:21:11 +0100. <1900758.CgLNVPbY9N@liber>
> +EOF
> +cat <<EOF >EXPECTED
> +BAD: <[hidden email]> (Ian Jackson's message of "Mon, 5 Dec 2016 14:41:01 +0000")
> +BAD: <[hidden email]>  <[hidden email]>
> +BAD: Your message of Tue, 09 Dec 2014 13:21:11 +0100. <1900758.CgLNVPbY9N@liber>
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +
> +test_done
> diff --git a/test/message-id-parse.c b/test/message-id-parse.c
> new file mode 100644
> index 00000000..752eb1fd
> --- /dev/null
> +++ b/test/message-id-parse.c
> @@ -0,0 +1,26 @@
> +#include <stdio.h>
> +#include <talloc.h>
> +#include "notmuch-private.h"
> +
> +int
> +main (unused (int argc), unused (char **argv))
> +{
> +    char *line = NULL;
> +    size_t len = 0;
> +    ssize_t nread;
> +    void *local = talloc_new (NULL);
> +
> +    while ((nread = getline (&line, &len, stdin)) != -1) {
> + int last = strlen (line) - 1;
> + if (line[last] == '\n')
> +    line[last] = '\0';
> +
> + char *mid = _notmuch_message_id_parse_strict (local, line);
> + if (mid)
> +    printf ("GOOD: %s\n", mid);
> + else
> +    printf ("BAD: %s\n", line);
> +    }
> +
> +    talloc_free (local);
> +}
> --
> 2.18.0
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
12