Date: Sat, 8 Sep 2012 13:23:37 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jameson Graef Rollins <jrollins@finestructure.net>
Subject: Re: [PATCH 00/11] add recipients to search output
Message-ID: <20120908172337.GB11179@mit.edu>
References: <1345427570-26518-1-git-send-email-jrollins@finestructure.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1345427570-26518-1-git-send-email-jrollins@finestructure.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: Notmuch Mail <notmuch@notmuchmail.org>
Precedence: list

Quoth Jameson Graef Rollins on Aug 19 at  6:52 pm:
> This series is an attempt to add thread recipients to the search
> output.
> 
> My personal overall goal of this series is to support the handling of
> drafts in the emacs ui.  For drafts we want to see recipients, instead
> of authors, in the search output.  I can imagine other uses for this
> series as well, though.
> 
> The first four patches generalize the author list handling in thread
> objects to handle any address list.  These patches could be applied
> regardless of if the rest of the series is accepted.
> 
> After that we modify the thread constructor such that it can hold
> thread recipients as well.  Since there is overhead in retrieving
> thread recipients from the message files (recipients are not stored in
> the database) this is handled with a switch.
> 
> Further patches add the new switch to the search CLI that adds thread
> recipients to the structured output formats.  I didn't modify the text
> output format, since there is no way to extend it.  I can imagine
> tweaking the text output such that the author field is instead
> replaced by the recipients (as is done for the emacs UI at the end of
> the series), but that's not done here.

I've gotten up through patch 8.  Overall I really like this series and
the abstractions you're introducing.  However, I don't think you
should replicate the way the authors list is handled in the CLI.  The
authors list is a kludge inherited from the text format, which had to
invent a syntax for lists, that somehow got baked into the library.
JSON, on the other hand, is very good at lists, and should use them
for the recipients (I would love if it used them for the authors list,
but that'll require an incompatible change, so I'd like to implement
my/some JSON versioning scheme first).  After all, "The string is a
stark data structure and everywhere it is passed there is much
duplication of process. It is a perfect vehicle for hiding
information."

I think treating recipients as a first-class list would lead to a much
cleaner and more general API; it would hard-code less in the library,
though it would put more responsibility on the CLI.  What I'm
imagining is just a single new public API function:
notmuch_thread_get_messages, which returns a notmuch_messages_t of the
thread's messages, in the original query's sort order.  It's
remarkable that we *don't* have this API already.  In this case, it
would give the library user (the CLI) the ability to easily construct
the recipients list however it wants.  JSON list or text string?  No
problem.  Just to or to/cc/bcc?  No problem.  Separating matched and
unmatched or merging them together?  No problem.  Also, since this
would naturally construct the recipients list only when needed, we
wouldn't have to worry about telling the library whether or not to pay
the performance cost, and we could trivially add the headers we end up
using to the database later and get an automatic speed boost.

Relative to your current code, this would require either duplicating a
bit of the matched/unmatched hash table code in the CLI (not perfect,
but you wouldn't need the stringifying function, and there isn't much
other code to that) or moving the thread_addresses abstraction into
util.  _thread_cleanup_author would probably also want to move into
util (which is also completely reasonable since it's a pure leaf
function that depends on nothing).

> In the emacs UI, I add a new toggle function that will toggle display
> of thread authors or recipients in the 'authors' field of the search
> output.  It's unfortunate that this ambiguity in that field name
> remains, but I didn't know how to change that cleanly.  I'm working on
> some tests for the new emacs functionality that I'll include in the
> inevitable v2 of this series.
> 
> The last patch is mostly just a tickle to suggest adding the
> recipients to the database.  It would make the --include-recipient
> searches much faster of course, but it might be overhead in the
> database that folks aren't interested in.
> 
> As always, feedback, review, and comments are much appreciated.
> 
> jamie.