notmuch_thread_get_authors

notmuch_thread_get_authors

[Ronny Chevalier]

Hi,

I would like to know the reason behind the way we get authors from a
thread, with notmuch_thread_get_authors.

Getting a string formatted as it is right now make it impossible to
parse properly the authors. If there is an author with | or , in its
name (or email address if there is no name), there is no way to
distinguish between another author in the list comma separated or an
author with a comma, and the same with |.

A function like notmuch_thread_get_authors_arrays(notmuch_thread_t
*thread, char ***authors_matched, char ***authors_not_mached) with
authors_matched and authors_not_mached being string arrays
NULL-terminated could deal with this issue.

What do you think? or maybe I am missing something?

Ronny

Re: notmuch_thread_get_authors

[David Bremner]

Ronny Chevalier <chevalier.ronny@gmail.com> writes:

> Hi,
>
> I would like to know the reason behind the way we get authors from a
> thread, with notmuch_thread_get_authors.
>

there is some related patches/discussion at

http://thread.gmane.org/gmane.mail.notmuch.general/19422

d

Re: notmuch_thread_get_authors

[Ronny Chevalier]

On Tue, Apr 21, 2015 at 1:35 AM, David Bremner <david@tethera.net> wrote:
> Ronny Chevalier <chevalier.ronny@gmail.com> writes:
>
>> Hi,
>>
>> I would like to know the reason behind the way we get authors from a
>> thread, with notmuch_thread_get_authors.
>>
>
> there is some related patches/discussion at
>
> http://thread.gmane.org/gmane.mail.notmuch.general/19422

Ok thanks.

Since I just subscribed to the mailing list, I can't reply to this the
thread so I reply here.

Austin Clements wrote:
> And I think there's a fairly easy way to do it in C code that will
> also prevent library interface bloat: instead of introducing new
> library APIs to get at this information, just use the existing
> notmuch_thread_get_messages API and construct the matched and
> non-matched lists in the CLI.  Doing it in the CLI wouldn't require
> the library to export yet another string list structure, which is
> always a huge pain (thanks C!), and wouldn't introduce more "helper"
> functions into the library API.

I disagree with what Austin said. Because this does not solve the
issue at all (or I misunderstood). The issue is with the notmuch API,
if someone is using this library there no way it can parse properly
the authors.
In my case I am not using the CLI but the notmuch library, fixing this
in the CLI is just an hack, and it does not fix the issue for the
library users.

Furthermore, I do not see why providing a string list NULL-terminated
in C is a huge pain?

Otherwise, I agree with Mark Walters comments on the patch.

If no one is working to fix this at the moment, I can send a patch?

Ronny

Re: notmuch_thread_get_authors

[Austin Clements]

On Tue, 21 Apr 2015, Ronny Chevalier <chevalier.ronny@gmail.com> wrote:
> On Tue, Apr 21, 2015 at 1:35 AM, David Bremner <david@tethera.net> wrote:
>> Ronny Chevalier <chevalier.ronny@gmail.com> writes:
> Austin Clements wrote:
>> And I think there's a fairly easy way to do it in C code that will
>> also prevent library interface bloat: instead of introducing new
>> library APIs to get at this information, just use the existing
>> notmuch_thread_get_messages API and construct the matched and
>> non-matched lists in the CLI.  Doing it in the CLI wouldn't require
>> the library to export yet another string list structure, which is
>> always a huge pain (thanks C!), and wouldn't introduce more "helper"
>> functions into the library API.
>
> I disagree with what Austin said. Because this does not solve the
> issue at all (or I misunderstood). The issue is with the notmuch API,
> if someone is using this library there no way it can parse properly
> the authors.
> In my case I am not using the CLI but the notmuch library, fixing this
> in the CLI is just an hack, and it does not fix the issue for the
> library users.

My suggestion was in no way specific to the CLI. That was the context of
the discussion at the time, but for the purposes of this discussion, the
CLI is just another library user.

You're completely right that there's no way to reliably parse the
authors list returned by notmuch_thread_get_authors. So don't do
that. Just use notmuch_thread_get_messages, walk the messages list, and
build your own authors list. There's no need to introduce additional
complexity and surface area into the library API for this specific use
case (IMO, even notmuch_thread_get_authors shouldn't exist, but it's
there for legacy reasons.) Then you can get author lists for matched,
non-matched, matching a specific tag, just the to, just the from, counts
of how many times each author appeared, whatever you want.

> Furthermore, I do not see why providing a string list NULL-terminated
> in C is a huge pain?

See the notmuch_tags_* and notmuch_filesnames_* APIs. Those are just
string lists.

> Otherwise, I agree with Mark Walters comments on the patch.
>
> If no one is working to fix this at the moment, I can send a patch?
>
> Ronny

Re: notmuch_thread_get_authors

[David Bremner]

Austin Clements <aclements@csail.mit.edu> writes:

> You're completely right that there's no way to reliably parse the
> authors list returned by notmuch_thread_get_authors. So don't do
> that. Just use notmuch_thread_get_messages, walk the messages list, and
> build your own authors list. There's no need to introduce additional
> complexity and surface area into the library API for this specific use
> case (IMO, even notmuch_thread_get_authors shouldn't exist, but it's
> there for legacy reasons.)

Ah, so when my DEPRECATED macros get merge, we should deprecate that
function maybe.

d

Re: notmuch_thread_get_authors

[Ronny Chevalier]

On Wed, Apr 22, 2015 at 3:28 AM, Austin Clements
<aclements@csail.mit.edu> wrote:
> On Tue, 21 Apr 2015, Ronny Chevalier <chevalier.ronny@gmail.com> wrote:
>> On Tue, Apr 21, 2015 at 1:35 AM, David Bremner <david@tethera.net> wrote:
>>> Ronny Chevalier <chevalier.ronny@gmail.com> writes:
>> Austin Clements wrote:
>>> And I think there's a fairly easy way to do it in C code that will
>>> also prevent library interface bloat: instead of introducing new
>>> library APIs to get at this information, just use the existing
>>> notmuch_thread_get_messages API and construct the matched and
>>> non-matched lists in the CLI.  Doing it in the CLI wouldn't require
>>> the library to export yet another string list structure, which is
>>> always a huge pain (thanks C!), and wouldn't introduce more "helper"
>>> functions into the library API.
>>
>> I disagree with what Austin said. Because this does not solve the
>> issue at all (or I misunderstood). The issue is with the notmuch API,
>> if someone is using this library there no way it can parse properly
>> the authors.
>> In my case I am not using the CLI but the notmuch library, fixing this
>> in the CLI is just an hack, and it does not fix the issue for the
>> library users.
>
> My suggestion was in no way specific to the CLI. That was the context of
> the discussion at the time, but for the purposes of this discussion, the
> CLI is just another library user.

Ok, sorry for misunderstanding.

>
> You're completely right that there's no way to reliably parse the
> authors list returned by notmuch_thread_get_authors. So don't do
> that. Just use notmuch_thread_get_messages, walk the messages list, and
> build your own authors list. There's no need to introduce additional
> complexity and surface area into the library API for this specific use
> case (IMO, even notmuch_thread_get_authors shouldn't exist, but it's
> there for legacy reasons.) Then you can get author lists for matched,
> non-matched, matching a specific tag, just the to, just the from, counts
> of how many times each author appeared, whatever you want.
>

Ok thanks!

If I read the code correctly, _notmuch_thread_create in lib/thread.cc
process every message to get information like tags, subject and
authors. Since notmuch_thread_get_authors is here for legacy reasons,
would it be better to generate the list of authors only when requested
with notmuch_thread_get_authors (and cache the result of course)?
Because, new code will not use this and will do this work manually,
the generation of the list in intern consumes resources for nothing.

>> Furthermore, I do not see why providing a string list NULL-terminated
>> in C is a huge pain?
>
> See the notmuch_tags_* and notmuch_filesnames_* APIs. Those are just
> string lists.
>
>> Otherwise, I agree with Mark Walters comments on the patch.
>>
>> If no one is working to fix this at the moment, I can send a patch?
>>
>> Ronny

Re: notmuch_thread_get_authors

[Austin Clements]

Quoth Ronny Chevalier on Apr 22 at  4:01 am:
> On Wed, Apr 22, 2015 at 3:28 AM, Austin Clements
> <aclements@csail.mit.edu> wrote:
> > On Tue, 21 Apr 2015, Ronny Chevalier <chevalier.ronny@gmail.com> wrote:
> >> On Tue, Apr 21, 2015 at 1:35 AM, David Bremner <david@tethera.net> wrote:
> >>> Ronny Chevalier <chevalier.ronny@gmail.com> writes:
> >> Austin Clements wrote:
> >>> And I think there's a fairly easy way to do it in C code that will
> >>> also prevent library interface bloat: instead of introducing new
> >>> library APIs to get at this information, just use the existing
> >>> notmuch_thread_get_messages API and construct the matched and
> >>> non-matched lists in the CLI.  Doing it in the CLI wouldn't require
> >>> the library to export yet another string list structure, which is
> >>> always a huge pain (thanks C!), and wouldn't introduce more "helper"
> >>> functions into the library API.
> >>
> >> I disagree with what Austin said. Because this does not solve the
> >> issue at all (or I misunderstood). The issue is with the notmuch API,
> >> if someone is using this library there no way it can parse properly
> >> the authors.
> >> In my case I am not using the CLI but the notmuch library, fixing this
> >> in the CLI is just an hack, and it does not fix the issue for the
> >> library users.
> >
> > My suggestion was in no way specific to the CLI. That was the context of
> > the discussion at the time, but for the purposes of this discussion, the
> > CLI is just another library user.
> 
> Ok, sorry for misunderstanding.
> 
> >
> > You're completely right that there's no way to reliably parse the
> > authors list returned by notmuch_thread_get_authors. So don't do
> > that. Just use notmuch_thread_get_messages, walk the messages list, and
> > build your own authors list. There's no need to introduce additional
> > complexity and surface area into the library API for this specific use
> > case (IMO, even notmuch_thread_get_authors shouldn't exist, but it's
> > there for legacy reasons.) Then you can get author lists for matched,
> > non-matched, matching a specific tag, just the to, just the from, counts
> > of how many times each author appeared, whatever you want.
> >
> 
> Ok thanks!
> 
> If I read the code correctly, _notmuch_thread_create in lib/thread.cc
> process every message to get information like tags, subject and
> authors. Since notmuch_thread_get_authors is here for legacy reasons,
> would it be better to generate the list of authors only when requested
> with notmuch_thread_get_authors (and cache the result of course)?
> Because, new code will not use this and will do this work manually,
> the generation of the list in intern consumes resources for nothing.

It might be worth making this lazy. I'd be surprised if this has
noticeable CPU or memory cost in the grand scheme of putting together
a thread, but I don't have any numbers to back this up.