Quoth Adam Wolfe Gordon on Feb 17 at 7:06 pm: > On Fri, Feb 17, 2012 at 10:04, Austin Clements <amdragon@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.