[PATCH v5.2 3/7] reply: Add a JSON reply format.

[Austin Clements]

Quoth Adam Wolfe Gordon on Feb 17 at  7:06 pm:
> On Fri, Feb 17, 2012 at 10:04, Austin Clements <amdragon at mit.edu> wrote:
> > The first two patches LGTM.  A few nits in this one.
> 
> Thanks for the review. A couple of points to discuss below; everything
> else I'll change for the next version.
> 
> >> +void
> >> +format_part_json (const void *ctx, mime_node_t *node, notmuch_bool_t first);
> >> +
> >
> > This is the wrong place for this declaration, since it is not part of
> > the MIME node abstraction.  It should go somewhere above the /*
> > notmuch-config.c */ comment.  Above that, it's a bit of a jumble.  I'd
> > probably put it right after notmuch_show_command.
> 
> Agreed. I initially had it earlier in the file (with the other
> show-related functions), but moved it down since it requires the
> mime_node_t declaration. There are a couple of options: put in a
> pre-declaration of mime_node_t early in the file, or move the
> mime_node stuff to a separate header file and include it in
> notmuch-client.h. I lean toward the latter, since notmuch-client.h is
> getting very big as it is. Thoughts?

struct mime_node is already declared at the top of notmuch-client.h.
That should probably just be replaced with
 typedef struct mime_node mime_node_t;
(and notmuch_show_format.part can be updated to take a mime_node_t *).
Alternatively, you could change your declaration to take a struct
mime_node, but it's nicer if the declaration and the definition match
literally and not just logically.  Moving mime_node_t into its own
header isn't a bad idea on its own (in fact, I specifically wrote it
so it could live in util/ if we wanted), but seems like overkill for
this.

> >> +    if (notmuch_query_count_messages (query) != 1) {
> >> +     fprintf (stderr, "Error: search term did not match precisely one message.\n");
> >> +     return 1;
> >> +    }
> >
> > Technically count_messages does not have to be accurate, but since
> > this is the same thing notmuch-show does, it's probably fine for now.
> 
> Ah, I didn't realize this. I just followed the show example.

I think it's fine as is.  Probably as a later, independent patch, we
should update both places to just check the iterator after the call to
notmuch_messages_get.  Or you could update it as an extra minipatch in
your series if you want.

> > Perhaps we should add proper handling of multi-message replies to
> > devel/TODO?
> 
> Probably a good idea, although it means defining what proper handling
> of multi-message replies in the CLI means. Personally, I don't think
> it makes much sense to reply to multiple messages. The only place that
> functionality is actually used (AFAIK) is in notmuch-search.el, which,
> with my patches, throws an error if you try to reply to a thread
> containing multiple messages. In my mind, the correct behavior in that
> specific case is to create a reply to the last message in the thread,
> which is better handled in the emacs code than the CLI anyway.

*Doing* it requires defining what it means, but devel/TODO is a fine
place for arbitrarily fantastical and under-specified desires.  That
said, I don't think defining it is that hard.  We could just do
whatever mutt does.  The body of a multi-message reply in mutt is the
concatenation of the bodies that would be generated for individual
replies and I suspect the headers are the gathered up to/cc addresses,
an in-reply-to that lists all of the replied to message IDs, and a
subject and references header derived from the first message replied
to.