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? >> + 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. > 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.