Received-SPF: pass (google.com: domain of awg@xvx.ca designates 10.112.102.68
	as permitted sender) client-ip=10.112.102.68; 
MIME-Version: 1.0
Sender: awg@xvx.ca
In-Reply-To: <20120217170457.GE5991@mit.edu>
References: <1329361957-28493-1-git-send-email-awg+notmuch@xvx.ca>
	<1329361957-28493-4-git-send-email-awg+notmuch@xvx.ca>
	<20120217170457.GE5991@mit.edu>
Date: Fri, 17 Feb 2012 19:06:47 -0700
Message-ID:
 <CAMoJFUt_um9wTVaGEBm+9VmRcb1QZoOnbVCOrBQ7bfhjVQM9zg@mail.gmail.com>
Subject: Re: [PATCH v5.2 3/7] reply: Add a JSON reply format.
From: Adam Wolfe Gordon <awg+notmuch@xvx.ca>
To: Austin Clements <amdragon@mit.edu>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Cc: notmuch@notmuchmail.org
Precedence: list

On Fri, Feb 17, 2012 at 10:04, Austin Clements <amdragon@mit.edu> wrote:
> The first two patches LGTM. =A0A 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 fi=
rst);
>> +
>
> This is the wrong place for this declaration, since it is not part of
> the MIME node abstraction. =A0It should go somewhere above the /*
> notmuch-config.c */ comment. =A0Above that, it's a bit of a jumble. =A0I'=
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?

>> + =A0 =A0if (notmuch_query_count_messages (query) !=3D 1) {
>> + =A0 =A0 fprintf (stderr, "Error: search term did not match precisely o=
ne message.\n");
>> + =A0 =A0 return 1;
>> + =A0 =A0}
>
> 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.