* Re: [PATCH] Avoid empty thread names if possible.
2014-10-29 8:34 ` Mark Walters
@ 2014-10-29 13:05 ` Jesse Rosenthal
2014-10-29 13:07 ` Jesse Rosenthal
2014-10-29 13:39 ` Jesse Rosenthal
2 siblings, 0 replies; 12+ messages in thread
From: Jesse Rosenthal @ 2014-10-29 13:05 UTC (permalink / raw)
To: Mark Walters, notmuch
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Avoid empty thread names if possible.
2014-10-29 8:34 ` Mark Walters
2014-10-29 13:05 ` Jesse Rosenthal
@ 2014-10-29 13:07 ` Jesse Rosenthal
2014-10-29 13:27 ` Jesse Rosenthal
2014-10-29 13:32 ` Tomi Ollila
2014-10-29 13:39 ` Jesse Rosenthal
2 siblings, 2 replies; 12+ messages in thread
From: Jesse Rosenthal @ 2014-10-29 13:07 UTC (permalink / raw)
To: Mark Walters, notmuch
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Avoid empty thread names if possible.
2014-10-29 13:07 ` Jesse Rosenthal
@ 2014-10-29 13:27 ` Jesse Rosenthal
2014-10-29 13:32 ` Tomi Ollila
1 sibling, 0 replies; 12+ messages in thread
From: Jesse Rosenthal @ 2014-10-29 13:27 UTC (permalink / raw)
To: Mark Walters, notmuch
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Avoid empty thread names if possible.
2014-10-29 13:07 ` Jesse Rosenthal
2014-10-29 13:27 ` Jesse Rosenthal
@ 2014-10-29 13:32 ` Tomi Ollila
1 sibling, 0 replies; 12+ messages in thread
From: Tomi Ollila @ 2014-10-29 13:32 UTC (permalink / raw)
To: Jesse Rosenthal, Mark Walters, notmuch
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Avoid empty thread names if possible.
2014-10-29 8:34 ` Mark Walters
2014-10-29 13:05 ` Jesse Rosenthal
2014-10-29 13:07 ` Jesse Rosenthal
@ 2014-10-29 13:39 ` Jesse Rosenthal
2014-10-29 15:15 ` Mark Walters
2 siblings, 1 reply; 12+ messages in thread
From: Jesse Rosenthal @ 2014-10-29 13:39 UTC (permalink / raw)
To: Mark Walters, notmuch
[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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Avoid empty thread names if possible.
2014-10-29 13:39 ` Jesse Rosenthal
@ 2014-10-29 15:15 ` Mark Walters
0 siblings, 0 replies; 12+ messages in thread
From: Mark Walters @ 2014-10-29 15:15 UTC (permalink / raw)
To: Jesse Rosenthal, notmuch
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
^ permalink raw reply [flat|nested] 12+ messages in thread