[PATCH] Avoid empty thread names if possible.

[PATCH] Avoid empty thread names if possible.

[Jesse Rosenthal]

Currently the thread is named based on either the oldest or newest
matching message (depending on the search order). If this message has
an empty subject, though, the thread will show up with an empty
subject in the search results. (See the thread starting with
`id:1412371140-21051-1-git-send-email-david@tethera.net` for an
example.)

This patch changes the behavior to name based on the oldest/newest
matching non-empty subject. This is particularly helpful for patchsets.
If the only subjects are empty, the thread subject will still be empty.

Signed-off-by: Jesse Rosenthal <jrosenthal@jhu.edu>
---
 lib/thread.cc | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index 8922403..ea10295 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -348,18 +348,20 @@ _thread_add_matched_message (notmuch_thread_t *thread,
 {
     time_t date;
     notmuch_message_t *hashed_message;
+    const char *cur_subject;
 
     date = notmuch_message_get_date (message);
+    cur_subject = notmuch_thread_get_subject (thread);
 
     if (date < thread->oldest || ! thread->matched_messages) {
 	thread->oldest = date;
-	if (sort == NOTMUCH_SORT_OLDEST_FIRST)
+	if (sort == NOTMUCH_SORT_OLDEST_FIRST || strlen(cur_subject) == 0)
 	    _thread_set_subject_from_message (thread, message);
     }
 
     if (date > thread->newest || ! thread->matched_messages) {
 	thread->newest = date;
-	if (sort != NOTMUCH_SORT_OLDEST_FIRST)
+	if (sort != NOTMUCH_SORT_OLDEST_FIRST || strlen(cur_subject) == 0)
 	    _thread_set_subject_from_message (thread, message);
     }
 
-- 
2.1.2

Re: [PATCH] Avoid empty thread names if possible.

[Sergei Shilovsky]

I would also suggest to drop subjects consisting of only "Re:" and
"Fwd" sequences

Re: [PATCH] Avoid empty thread names if possible.

[Jani Nikula]

On Wed, 08 Oct 2014, Sergei Shilovsky <sshilovsky@gmail.com> wrote:
> I would also suggest to drop subjects consisting of only "Re:" and
> "Fwd" sequences

I think it's okay to avoid empty thread names at the lib level; however
I think any further processing should be done near the user interface.

BR,
Jani.

Re: [PATCH] Avoid empty thread names if possible.

[Tomi Ollila]

On Tue, Oct 07 2014, Jesse Rosenthal wrote:

> Currently the thread is named based on either the oldest or newest
> matching message (depending on the search order). If this message has
> an empty subject, though, the thread will show up with an empty
> subject in the search results. (See the thread starting with
> `id:1412371140-21051-1-git-send-email-david@tethera.net` for an
> example.)
>
> This patch changes the behavior to name based on the oldest/newest
> matching non-empty subject. This is particularly helpful for patchsets.
> If the only subjects are empty, the thread subject will still be empty.
>
> Signed-off-by: Jesse Rosenthal <jrosenthal@jhu.edu>
> ---
>  lib/thread.cc | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index 8922403..ea10295 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -348,18 +348,20 @@ _thread_add_matched_message (notmuch_thread_t *thread,
>  {
>      time_t date;
>      notmuch_message_t *hashed_message;
> +    const char *cur_subject;
>  
>      date = notmuch_message_get_date (message);
> +    cur_subject = notmuch_thread_get_subject (thread);
>  
>      if (date < thread->oldest || ! thread->matched_messages) {
>  	thread->oldest = date;
> -	if (sort == NOTMUCH_SORT_OLDEST_FIRST)
> +	if (sort == NOTMUCH_SORT_OLDEST_FIRST || strlen(cur_subject) == 0)

IMO it is a bit silly to scan through the whole string and use the return
value of 0 (vs != 0) to have effect. we should probably have something like
#define EMPTY_STRING(s) ((s)[0] == '\0') 
and use that instead.

Also, to keep promise, mentioning 'patch' in commit message referring to 
the change introduced does IMO also look silly :D

Tomi

>  	    _thread_set_subject_from_message (thread, message);
>      }
>  
>      if (date > thread->newest || ! thread->matched_messages) {
>  	thread->newest = date;
> -	if (sort != NOTMUCH_SORT_OLDEST_FIRST)
> +	if (sort != NOTMUCH_SORT_OLDEST_FIRST || strlen(cur_subject) == 0)
>  	    _thread_set_subject_from_message (thread, message);
>      }
>  
> -- 
> 2.1.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] Avoid empty thread names if possible.

[Mark Walters]

Hi

On Tue, 07 Oct 2014, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> Currently the thread is named based on either the oldest or newest
> matching message (depending on the search order). If this message has
> an empty subject, though, the thread will show up with an empty
> subject in the search results. (See the thread starting with
> `id:1412371140-21051-1-git-send-email-david@tethera.net` for an
> example.)
>
> This patch changes the behavior to name based on the oldest/newest
> matching non-empty subject. This is particularly helpful for patchsets.
> If the only subjects are empty, the thread subject will still be empty.

I approve of the change in the output but I am unsure about the
implementation. It would be nice to have a clear rule about which
subject is taken. Eg: 

        if sort is oldest first then it is the subject of the oldest
        matching message with a non-empty subject. Similarly if sort
        is newest first.

Also, it would be nice if the implementation did not rely on what order
we call _thread_add_matched_message on the matching messages in the
thread. I think in some ways we already rely on the order (for the order
of the author list), but if you want to rely on the order here I think
it at least deserves a comment.

>
> Signed-off-by: Jesse Rosenthal <jrosenthal@jhu.edu>
> ---
>  lib/thread.cc | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/thread.cc b/lib/thread.cc
> index 8922403..ea10295 100644
> --- a/lib/thread.cc
> +++ b/lib/thread.cc
> @@ -348,18 +348,20 @@ _thread_add_matched_message (notmuch_thread_t *thread,
>  {
>      time_t date;
>      notmuch_message_t *hashed_message;
> +    const char *cur_subject;
>  
>      date = notmuch_message_get_date (message);
> +    cur_subject = notmuch_thread_get_subject (thread);
>  
>      if (date < thread->oldest || ! thread->matched_messages) {
>  	thread->oldest = date;
> -	if (sort == NOTMUCH_SORT_OLDEST_FIRST)
> +	if (sort == NOTMUCH_SORT_OLDEST_FIRST || strlen(cur_subject) == 0)
>  	    _thread_set_subject_from_message (thread, message);
>      }
>  
>      if (date > thread->newest || ! thread->matched_messages) {
>  	thread->newest = date;
> -	if (sort != NOTMUCH_SORT_OLDEST_FIRST)
> +	if (sort != NOTMUCH_SORT_OLDEST_FIRST || strlen(cur_subject) == 0)
>  	    _thread_set_subject_from_message (thread, message);
>      }

So looking at the above I think the oldest first gives the subject in
my suggestion above (since the messages are supplied in oldest first
order). But newest first may not: indeed if the subject starts out as
something and becomes empty then this will set the subject empty and
then leave it empty.

(Note _thread_set_subject_from_message calls notmuch_message_get_header
which returns an empty string "" if the subject line is empty or not
present).

Best wishes

Mark


>  
> -- 
> 2.1.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] Avoid empty thread names if possible.

[Jesse Rosenthal]

Hi,

Thanks for taking a look at this.

Mark Walters <markwalters1009@gmail.com> writes:
> I approve of the change in the output but I am unsure about the
> implementation. It would be nice to have a clear rule about which
> subject is taken. Eg: 
>
>         if sort is oldest first then it is the subject of the oldest
>         matching message with a non-empty subject. Similarly if sort
>         is newest first.

The rule is actually in a four-year-old commit message (4971b85c4), in
almost exactly the same words you used:

    ...name threads based on (a) matches for the query, and (b) the
    search order. If the search order is oldest-first (as in the default
    inbox) it chooses the oldest matching message as the subject. If the
    search order is newest-first it chooses the newest one.
    
    Reply prefixes ("Re: ", "Aw: ", "Sv: ", "Vs: ") are ignored
    (case-insensitively) so a Re: won't change the subject.

So we would, essentially, just need to add "non-empty" to this
phrasing. Where would be the right place to put it? Commit message?
NEWS? `search` man page?

> Also, it would be nice if the implementation did not rely on what order
> we call _thread_add_matched_message on the matching messages in the
> thread. I think in some ways we already rely on the order (for the order
> of the author list), but if you want to rely on the order here I think
> it at least deserves a comment.

That would require a rethinking, I think, of naming -- since it's
traditionally worked in terms of renaming. When a better option comes,
we throw out the old one. So order is pretty essential. (Not saying
that's the best way, just pointing out that it's the way it's been done
since Carl's initial alpha release.)

> So looking at the above I think the oldest first gives the subject in
> my suggestion above (since the messages are supplied in oldest first
> order). But newest first may not: indeed if the subject starts out as
> something and becomes empty then this will set the subject empty and
> then leave it

> (Note b_thread_set_subject_from_message calls notmuch_message_get_header
> which returns an empty string "" if the subject line is empty or not
> present).

Hmmm... I was looking at the following line in
_thread_set_subject_from_message:

    subject = notmuch_message_get_header (message, "subject");
    if (! subject)
	return;

So, I don't think we ever actually change a content-ful string subject
to an empty one, as you describe above? If there's a non-empty string
there, and we get an empty subject, we leave the non-empty string in
place, right?

Best,
Jesse

Re: [PATCH] Avoid empty thread names if possible.

[Jesse Rosenthal]

By the way, this discussion brings up another problem. I wasn't able to
write a test for this (to address the below concerns) because the test
suite for thread-naming supplies some sort of auto-generated subject
for threads with empty subjects. So we can't test behavior for dealing
with empty subjects.

Unfortunately, I don't know the history of the test suite, or why this
auto-subject decisicion was made, so I don't feel comfortable just
changing the behavior.

Re: [PATCH] Avoid empty thread names if possible.

[Jesse Rosenthal]

Thanks for taking a look.

Tomi Ollila <tomi.ollila@iki.fi> writes:

> IMO it is a bit silly to scan through the whole string and use the return
> value of 0 (vs != 0) to have effect. we should probably have something like
> #define EMPTY_STRING(s) ((s)[0] == '\0') 
> and use that instead.

Good point. Will put in the next version.

> Also, to keep promise, mentioning 'patch' in commit message referring to 
> the change introduced does IMO also look silly :D

Ah. Yep, agreed.

Re: [PATCH] Avoid empty thread names if possible.

[Jesse Rosenthal]

Hmm, that's strange -- my inital detailed response to Mark's message
didn't go through to the list. I wonder if it's being filtered or
something. 

Re: [PATCH] Avoid empty thread names if possible.

[Tomi Ollila]

On Wed, Oct 29 2014, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:

> By the way, this discussion brings up another problem. I wasn't able to
> write a test for this (to address the below concerns) because the test
> suite for thread-naming supplies some sort of auto-generated subject
> for threads with empty subjects. So we can't test behavior for dealing
> with empty subjects.
>
> Unfortunately, I don't know the history of the test suite, or why this
> auto-subject decisicion was made, so I don't feel comfortable just
> changing the behavior.

Autogeneration of (unique) headers when some header not given...

Shell could distinguish between unset and empty variable, but that would
proably make most users feel PITA to use it... (*)

... probably better altelnative could be some magic word, like 'none' to
make generate_message() set header in question empty (and first just
implement that "feature" to Subject header.

Tomi

(*) personally I'd be totally comfortable with it, though ;D

Re: [PATCH] Avoid empty thread names if possible.

[Jesse Rosenthal]

[I'm not sure why the below reply did not go to the list. Later replies
did, so I assume there must have been so problem in the sending. Mark,
apologies if you get this twice.]

Hi,

Thanks for taking a look at this.

Mark Walters <markwalters1009@gmail.com> writes:
> I approve of the change in the output but I am unsure about the
> implementation. It would be nice to have a clear rule about which
> subject is taken. Eg: 
>
>         if sort is oldest first then it is the subject of the oldest
>         matching message with a non-empty subject. Similarly if sort
>         is newest first.

The rule is actually in a four-year-old commit message (4971b85c4), in
almost exactly the same words you used:

    ...name threads based on (a) matches for the query, and (b) the
    search order. If the search order is oldest-first (as in the default
    inbox) it chooses the oldest matching message as the subject. If the
    search order is newest-first it chooses the newest one.

    Reply prefixes ("Re: ", "Aw: ", "Sv: ", "Vs: ") are ignored
    (case-insensitively) so a Re: won't change the subject.

So we would, essentially, just need to add "non-empty" to this
phrasing. Where would be the right place to put it? Commit message?
NEWS? `search` man page?

> Also, it would be nice if the implementation did not rely on what order
> we call _thread_add_matched_message on the matching messages in the
> thread. I think in some ways we already rely on the order (for the order
> of the author list), but if you want to rely on the order here I think
> it at least deserves a comment.

That would require a rethinking, I think, of naming -- since it's
traditionally worked in terms of renaming. When a better option comes,
we throw out the old one. So order is pretty essential. (Not saying
that's the best way, just pointing out that it's the way it's been done
since Carl's initial alpha release.)

> So looking at the above I think the oldest first gives the subject in
> my suggestion above (since the messages are supplied in oldest first
[ 4 more citation lines. Click/Enter to show. ]
> order). But newest first may not: indeed if the subject starts out as
> something and becomes empty then this will set the subject empty and
> then leave it

> (Note b_thread_set_subject_from_message calls notmuch_message_get_header
> which returns an empty string "" if the subject line is empty or not
> present).

Hmmm... I was looking at the following line in
_thread_set_subject_from_message:

    subject = notmuch_message_get_header (message, "subject");
    if (! subject)
	return;

So, I don't think we ever actually change a content-ful string subject
to an empty one, as you describe above? If there's a non-empty string
there, and we get an empty subject, we leave the non-empty string in
place, right?

Best,
Jesse

Re: [PATCH] Avoid empty thread names if possible.

[Mark Walters]

Hi

On Wed, 29 Oct 2014, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> [I'm not sure why the below reply did not go to the list. Later replies
> did, so I assume there must have been so problem in the sending. Mark,
> apologies if you get this twice.]
>
> Hi,
>
> Thanks for taking a look at this.
>
> Mark Walters <markwalters1009@gmail.com> writes:
>> I approve of the change in the output but I am unsure about the
>> implementation. It would be nice to have a clear rule about which
>> subject is taken. Eg: 
>>
>>         if sort is oldest first then it is the subject of the oldest
>>         matching message with a non-empty subject. Similarly if sort
>>         is newest first.
>
> The rule is actually in a four-year-old commit message (4971b85c4), in
> almost exactly the same words you used:
>
>     ...name threads based on (a) matches for the query, and (b) the
>     search order. If the search order is oldest-first (as in the default
>     inbox) it chooses the oldest matching message as the subject. If the
>     search order is newest-first it chooses the newest one.
>
>     Reply prefixes ("Re: ", "Aw: ", "Sv: ", "Vs: ") are ignored
>     (case-insensitively) so a Re: won't change the subject.
>
> So we would, essentially, just need to add "non-empty" to this
> phrasing. Where would be the right place to put it? Commit message?
> NEWS? `search` man page?

First I just wanted to check that I knew exactly what behaviour was
intended. Having the new rule in the commit message might well be
sufficient.

>> Also, it would be nice if the implementation did not rely on what order
>> we call _thread_add_matched_message on the matching messages in the
>> thread. I think in some ways we already rely on the order (for the order
>> of the author list), but if you want to rely on the order here I think
>> it at least deserves a comment.
>
> That would require a rethinking, I think, of naming -- since it's
> traditionally worked in terms of renaming. When a better option comes,
> we throw out the old one. So order is pretty essential. (Not saying
> that's the best way, just pointing out that it's the way it's been done
> since Carl's initial alpha release.)

I think that the current code does not depend on the order the messages
are given to _thread_add_matched_message: regardless of the order the
thread will get the subject of the oldest matching message (in
sort=oldest first)

In contrast your code will give different subjects depending in the
order the messages are fed to _thread_add_matched_message.

>> So looking at the above I think the oldest first gives the subject in
>> my suggestion above (since the messages are supplied in oldest first
>> order). But newest first may not: indeed if the subject starts out as
>> something and becomes empty then this will set the subject empty and
>> then leave it
>
>> (Note b_thread_set_subject_from_message calls notmuch_message_get_header
>> which returns an empty string "" if the subject line is empty or not
>> present).
>
> Hmmm... I was looking at the following line in
> _thread_set_subject_from_message:
>
>     subject = notmuch_message_get_header (message, "subject");
>     if (! subject)
> 	return;

but subject="" is not null; subject is only null if
notmuch_message_get_header throws an error. See the documentation for
notmuch_message_get_header.

Best wishes

Mark



>
> So, I don't think we ever actually change a content-ful string subject
> to an empty one, as you describe above? If there's a non-empty string
> there, and we get an empty subject, we leave the non-empty string in
> place, right?
>
> Best,
> Jesse