Thanks for the review. New version coming shortly (v4, since the list
ate v3 while everyone was still reading v2).
Quoth Dmitry Kurochkin on Jan 24 at 2:37 am:
> On Sun, 22 Jan 2012 21:31:13 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This callback is the gateway to the new mime_node_t-based formatters.
> > This maintains backwards compatibility so the formatters can be
> > transitioned one at a time. Once all formatters are converted, the
> > formatter structure can be reduced to only message_set_{start,sep,end}
> > and part, most of show_message can be deleted, and all of
> > show-message.c can be deleted.
>
> Few comments below.
>
> > ---
> > notmuch-client.h | 6 ++++++
> > notmuch-reply.c | 2 +-
> > notmuch-show.c | 23 +++++++++++++++++++----
> > 3 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index abfe5d3..59606b4 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -62,8 +62,14 @@
> > #define STRINGIFY(s) STRINGIFY_(s)
> > #define STRINGIFY_(s) #s
> >
> > +struct mime_node;
> > +struct notmuch_show_params;
> > +
> > typedef struct notmuch_show_format {
> > const char *message_set_start;
> > + void (*part) (const void *ctx,
> > + struct mime_node *node, int indent,
> > + struct notmuch_show_params *params);
>
> Can we make the params parameter const?
Apparently we can.
> > const char *message_start;
> > void (*message) (const void *ctx,
> > notmuch_message_t *message,
> > diff --git a/notmuch-reply.c b/notmuch-reply.c
> > index bf67960..f55b1d2 100644
> > --- a/notmuch-reply.c
> > +++ b/notmuch-reply.c
> > @@ -31,7 +31,7 @@ static void
> > reply_part_content (GMimeObject *part);
> >
> > static const notmuch_show_format_t format_reply = {
> > - "",
> > + "", NULL,
> > "", NULL,
> > "", NULL, reply_headers_message_part, ">\n",
> > "",
> > diff --git a/notmuch-show.c b/notmuch-show.c
> > index 682aa71..8185b02 100644
> > --- a/notmuch-show.c
> > +++ b/notmuch-show.c
> > @@ -42,7 +42,7 @@ static void
> > format_part_end_text (GMimeObject *part);
> >
> > static const notmuch_show_format_t format_text = {
> > - "",
> > + "", NULL,
> > "\fmessage{ ", format_message_text,
> > "\fheader{\n", format_headers_text, format_headers_message_part_text, "\fheader}\n",
> > "\fbody{\n",
> > @@ -89,7 +89,7 @@ static void
> > format_part_end_json (GMimeObject *part);
> >
> > static const notmuch_show_format_t format_json = {
> > - "[",
> > + "[", NULL,
> > "{", format_message_json,
> > "\"headers\": {", format_headers_json, format_headers_message_part_json, "}",
> > ", \"body\": [",
> > @@ -110,7 +110,7 @@ format_message_mbox (const void *ctx,
> > unused (int indent));
> >
> > static const notmuch_show_format_t format_mbox = {
> > - "",
> > + "", NULL,
> > "", format_message_mbox,
> > "", NULL, NULL, "",
> > "",
> > @@ -129,7 +129,7 @@ static void
> > format_part_content_raw (GMimeObject *part);
> >
> > static const notmuch_show_format_t format_raw = {
> > - "",
> > + "", NULL,
> > "", NULL,
> > "", NULL, format_headers_message_part_text, "\n",
> > "",
> > @@ -850,6 +850,21 @@ show_message (void *ctx,
> > int indent,
> > notmuch_show_params_t *params)
> > {
> > + if (format->part) {
> > + void *local = talloc_new (ctx);
> > + mime_node_t *root, *part;
> > +
> > + if (mime_node_open (local, message, params->cryptoctx, params->decrypt,
> > + &root) != NOTMUCH_STATUS_SUCCESS)
> > + goto DONE;
> > + part = mime_node_seek_dfs (root, params->part < 0 ? 0 : params->part);
>
> This should be done as a separate patch, but it looks like zero and
> negative params->part is handled in the same way so we can change it to
> always be non-negative.
That's true here, but there are other places where the difference does
matter. (I would certainly *prefer* this not to be asymmetric, but
that's a bigger issue involving show's inconsistent interface.)
> > + if (part)
> > + format->part (local, part, indent, params);
> > + DONE:
> > + talloc_free (local);
> > + return;
>
> Please move part assignment inside the if and remove the goto:
>
> if (mime_node_open() == success && (part = seek()))
> format->part();
Done.
> Regards,
> Dmitry
>
> > + }
> > +
> > if (params->part <= 0) {
> > fputs (format->message_start, stdout);
> > if (format->message)
>