From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 2BE95431FB6 for ; Fri, 17 Feb 2012 19:25:43 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id R4oOjfJOOD7l for ; Fri, 17 Feb 2012 19:25:42 -0800 (PST) Received: from dmz-mailsec-scanner-8.mit.edu (DMZ-MAILSEC-SCANNER-8.MIT.EDU [18.7.68.37]) by olra.theworths.org (Postfix) with ESMTP id 7F3CF431FAE for ; Fri, 17 Feb 2012 19:25:42 -0800 (PST) X-AuditID: 12074425-b7f4a6d0000008e0-c5-4f3f1a34da45 Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) by dmz-mailsec-scanner-8.mit.edu (Symantec Messaging Gateway) with SMTP id 9A.11.02272.43A1F3F4; Fri, 17 Feb 2012 22:25:40 -0500 (EST) Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q1I3PdCr014641; Fri, 17 Feb 2012 22:25:39 -0500 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) (authenticated bits=0) (User authenticated as amdragon@ATHENA.MIT.EDU) by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q1I3PbY0017290 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); Fri, 17 Feb 2012 22:25:38 -0500 (EST) Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) (envelope-from ) id 1Ryats-0004Bi-3T; Fri, 17 Feb 2012 22:23:52 -0500 Date: Fri, 17 Feb 2012 22:23:52 -0500 From: Austin Clements To: Adam Wolfe Gordon Subject: Re: [PATCH v5.2 3/7] reply: Add a JSON reply format. Message-ID: <20120218032352.GI5991@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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDKsWRmVeSWpSXmKPExsUixG6nomsiZe9v8PIXp8WRPbPYLa7fnMns wOTxbNUtZo+mH4tZA5iiuGxSUnMyy1KL9O0SuDI+zW5hLOiVqZh79Th7A+MBsS5GTg4JAROJ Q4fXM0PYYhIX7q1n62Lk4hAS2McosfPFJXYIZwOjxOUJ/6AyJ5kktj2fwQLhLGGUuH2xB8jh 4GARUJW4eNcRZBSbgIbEtv3LGUFsEQEtiR/rv7KC2MwC0hLffjczgdjCArYSzz5dAlvNK6At cerqDGaImU8YJU6cWMsCkRCUODnzCQtEs47Ezq132EB2gQxa/o8DIiwv0bx1NtgcToFAib2H 1oLNFxVQkZhychvbBEbhWUgmzUIyaRbCpFlIJi1gZFnFKJuSW6Wbm5iZU5yarFucnJiXl1qk a6GXm1mil5pSuokRHAkuqjsYJxxSOsQowMGoxMP7qtPOX4g1say4MvcQoyQHk5Io7xkxe38h vqT8lMqMxOKM+KLSnNTiQ4wSHMxKIrxed4DKeVMSK6tSi/JhUtIcLErivJpa7/yEBNITS1Kz U1MLUotgsjIcHEoSvNmSQEMFi1LTUyvSMnNKENJMHJwgw3mAhneB1PAWFyTmFmemQ+RPMSpK ifPmgyQEQBIZpXlwvbBE9YpRHOgVYd5WkCoeYJKD634FNJgJaDCvEMjVxSWJCCmpBsZdR6cI m81cGXXG9TxXQ8+jRZZTdFvaGvWknuw5+0Bjn4im1P4dx+/Gpbi1e+yX3y/84rHGuTLhuMJ5 bGvLlZ88ubvoTut+2cZrO3I3XnW7YDvF/9i7KlsmU/GQwu+OMbsn9a9qaWXh3Wx4RrLKIGn6 Bu6wy2Iq5bpr599c/aJsLqON0EWN35JKLMUZiYZazEXFiQA0LyOPLwMAAA== Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Feb 2012 03:25:43 -0000 Quoth Adam Wolfe Gordon on Feb 17 at 7:06 pm: > On Fri, Feb 17, 2012 at 10:04, Austin Clements 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.