threading replies fixes v3

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

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


This series looks pretty good to me (comments sent separately), meaning
that I did not see any obvious problems there. I did not run the tests
(at least yet).

Tomi


On Thu, Aug 30 2018, David Bremner wrote:

> 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
_______________________________________________
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
|

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

In reply to this post by Tomi Ollila-2
Tomi Ollila <[hidden email]> writes:
> 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 )?

I guess that version could just go in notmuch-private.h, since it isn't
(currently) used in the CLI.

Or even directly in thread.cc, since that's the only place it's used
currently.

d
_______________________________________________
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
|

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

In reply to this post by Tomi Ollila-2
Tomi Ollila <[hidden email]> writes:

>> +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 ?
>

The return type is defined as int by the qsort prototype.

What we care about here is the sign. I think it's better to just avoid
difftime

-    return (int) difftime (time_a, time_b);
+    if (time_a == time_b)
+       return 0;
+    else if (time_a < time_b)
+       return -1;
+    else
+       return 1;
 }


_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
12