[PATCH 0/7] cli/reply: refactoring

[PATCH 0/7] cli/reply: refactoring

[Jani Nikula]

Hi all -

I started looking at how to use GMimeMessage for the headers in notmuch
reply, based on [1]. Turns out we *already* parse the message twice in
most cases, once in the lib and once in mime_node_open(). However,
notmuch reply is a mess, and passing that GMIME_MESSAGE (node->part)
from the node to add_recipients_from_message() gets tricky. Here's a
bunch of refactoring to make the future work easier. I like the diffstat
already.

Oh, as a bonus, if we ever try to get that "reply to multiple messages
at once" feature back, this makes it easier too.

BR,
Jani.

[1] id:87bn30tdb0.fsf@nikula.org

Jani Nikula (7):
  cli/reply: push notmuch reply format abstraction lower in the stack
  cli/reply: reuse show_reply_headers() in headers-only format
  cli/reply: unify reply format functions
  cli/reply: reorganize create_reply_message()
  cli/reply: make references header creation easier to follow
  cli/reply: reuse create_reply_message() also for headers-only format
  cli/reply: reduce the reply format abstractions

 notmuch-reply.c | 266 +++++++++++++++++---------------------------------------
 1 file changed, 79 insertions(+), 187 deletions(-)

-- 
2.1.4

[PATCH 1/7] cli/reply: push notmuch reply format abstraction lower in the stack

[Jani Nikula]

There's quite a bit of duplication, and some consequent deviation,
between the various notmuch reply format code paths. Perform the query
and message iteration in common code, and make the format specific
functions operate on single messages.

There should be no functional changes.
---
 notmuch-reply.c | 216 ++++++++++++++++++++++++++------------------------------
 1 file changed, 101 insertions(+), 115 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 49513732e620..847e306f94d2 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -598,82 +598,42 @@ create_reply_message(void *ctx,
 static int
 notmuch_reply_format_default(void *ctx,
 			     notmuch_config_t *config,
-			     notmuch_query_t *query,
+			     notmuch_message_t *message,
 			     notmuch_show_params_t *params,
 			     notmuch_bool_t reply_all,
 			     unused (sprinter_t *sp))
 {
     GMimeMessage *reply;
-    notmuch_messages_t *messages;
-    notmuch_message_t *message;
     mime_node_t *root;
-    notmuch_status_t status;
 
-    status = notmuch_query_search_messages_st (query, &messages);
-    if (print_status_query ("notmuch reply", query, status))
+    reply = create_reply_message (ctx, config, message, reply_all);
+    if (!reply)
 	return 1;
 
-    for (;
-	 notmuch_messages_valid (messages);
-	 notmuch_messages_move_to_next (messages))
-    {
-	message = notmuch_messages_get (messages);
+    show_reply_headers (reply);
 
-	reply = create_reply_message (ctx, config, message, reply_all);
-
-	/* If reply creation failed, we're out of memory, so don't
-	 * bother trying any more messages.
-	 */
-	if (!reply) {
-	    notmuch_message_destroy (message);
-	    return 1;
-	}
-
-	show_reply_headers (reply);
-
-	g_object_unref (G_OBJECT (reply));
-	reply = NULL;
-
-	if (mime_node_open (ctx, message, &(params->crypto), &root) == NOTMUCH_STATUS_SUCCESS) {
-	    format_part_reply (root);
-	    talloc_free (root);
-	}
+    g_object_unref (G_OBJECT (reply));
 
-	notmuch_message_destroy (message);
+    if (mime_node_open (ctx, message, &params->crypto, &root) == NOTMUCH_STATUS_SUCCESS) {
+	format_part_reply (root);
+	talloc_free (root);
     }
+
     return 0;
 }
 
 static int
 notmuch_reply_format_sprinter(void *ctx,
 			      notmuch_config_t *config,
-			      notmuch_query_t *query,
+			      notmuch_message_t *message,
 			      notmuch_show_params_t *params,
 			      notmuch_bool_t reply_all,
 			      sprinter_t *sp)
 {
     GMimeMessage *reply;
-    notmuch_messages_t *messages;
-    notmuch_message_t *message;
     mime_node_t *node;
-    unsigned count;
-    notmuch_status_t status;
-
-    status = notmuch_query_count_messages_st (query, &count);
-    if (print_status_query ("notmuch reply", query, status))
-	return 1;
-
-    if (count != 1) {
-	fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
-	return 1;
-    }
 
-    status = notmuch_query_search_messages_st (query, &messages);
-    if (print_status_query ("notmuch reply", query, status))
-	return 1;
-
-    message = notmuch_messages_get (messages);
-    if (mime_node_open (ctx, message, &(params->crypto), &node) != NOTMUCH_STATUS_SUCCESS)
+    if (mime_node_open (ctx, message, &params->crypto, &node) != NOTMUCH_STATUS_SUCCESS)
 	return 1;
 
     reply = create_reply_message (ctx, config, message, reply_all);
@@ -686,7 +646,6 @@ notmuch_reply_format_sprinter(void *ctx,
     sp->map_key (sp, "reply-headers");
     format_headers_sprinter (sp, reply, TRUE);
     g_object_unref (G_OBJECT (reply));
-    reply = NULL;
 
     /* Start the original */
     sp->map_key (sp, "original");
@@ -694,7 +653,6 @@ notmuch_reply_format_sprinter(void *ctx,
 
     /* End */
     sp->end (sp);
-    notmuch_message_destroy (message);
 
     return 0;
 }
@@ -703,65 +661,48 @@ notmuch_reply_format_sprinter(void *ctx,
 static int
 notmuch_reply_format_headers_only(void *ctx,
 				  notmuch_config_t *config,
-				  notmuch_query_t *query,
+				  notmuch_message_t *message,
 				  unused (notmuch_show_params_t *params),
 				  notmuch_bool_t reply_all,
 				  unused (sprinter_t *sp))
 {
     GMimeMessage *reply;
-    notmuch_messages_t *messages;
-    notmuch_message_t *message;
     const char *in_reply_to, *orig_references, *references;
     char *reply_headers;
-    notmuch_status_t status;
 
-    status = notmuch_query_search_messages_st (query, &messages);
-    if (print_status_query ("notmuch reply", query, status))
+    /* The 0 means we do not want headers in a "pretty" order. */
+    reply = g_mime_message_new (0);
+    if (reply == NULL) {
+	fprintf (stderr, "Out of memory\n");
 	return 1;
+    }
 
-    for (;
-	 notmuch_messages_valid (messages);
-	 notmuch_messages_move_to_next (messages))
-    {
-	message = notmuch_messages_get (messages);
-
-	/* The 0 means we do not want headers in a "pretty" order. */
-	reply = g_mime_message_new (0);
-	if (reply == NULL) {
-	    fprintf (stderr, "Out of memory\n");
-	    return 1;
-	}
-
-	in_reply_to = talloc_asprintf (ctx, "<%s>",
-			     notmuch_message_get_message_id (message));
-
-        g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "In-Reply-To", in_reply_to);
+    in_reply_to = talloc_asprintf (ctx, "<%s>",
+				   notmuch_message_get_message_id (message));
 
+    g_mime_object_set_header (GMIME_OBJECT (reply), "In-Reply-To", in_reply_to);
 
-	orig_references = notmuch_message_get_header (message, "references");
+    orig_references = notmuch_message_get_header (message, "references");
 
-	/* We print In-Reply-To followed by References because git format-patch treats them
-         * specially.  Git does not interpret the other headers specially
-	 */
-	references = talloc_asprintf (ctx, "%s%s%s",
-				      orig_references ? orig_references : "",
-				      orig_references ? " " : "",
-				      in_reply_to);
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "References", references);
+    /*
+     * We print In-Reply-To followed by References because git
+     * format-patch treats them specially. Git does not interpret the
+     * other headers specially.
+     */
+    references = talloc_asprintf (ctx, "%s%s%s",
+				  orig_references ? orig_references : "",
+				  orig_references ? " " : "",
+				  in_reply_to);
+    g_mime_object_set_header (GMIME_OBJECT (reply), "References", references);
 
-	(void)add_recipients_from_message (reply, config, message, reply_all);
+    (void)add_recipients_from_message (reply, config, message, reply_all);
 
-	reply_headers = g_mime_object_to_string (GMIME_OBJECT (reply));
-	printf ("%s", reply_headers);
-	free (reply_headers);
+    reply_headers = g_mime_object_to_string (GMIME_OBJECT (reply));
+    printf ("%s", reply_headers);
+    free (reply_headers);
 
-	g_object_unref (G_OBJECT (reply));
-	reply = NULL;
+    g_object_unref (G_OBJECT (reply));
 
-	notmuch_message_destroy (message);
-    }
     return 0;
 }
 
@@ -772,6 +713,70 @@ enum {
     FORMAT_HEADERS_ONLY,
 };
 
+static int do_reply(notmuch_config_t *config,
+		    notmuch_query_t *query,
+		    notmuch_show_params_t *params,
+		    int format,
+		    notmuch_bool_t reply_all)
+{
+    notmuch_messages_t *messages;
+    notmuch_message_t *message;
+    notmuch_status_t status;
+    struct sprinter *sp = NULL;
+    int ret = 0;
+    int (*reply_format_func) (void *ctx,
+			      notmuch_config_t *config,
+			      notmuch_message_t *message,
+			      notmuch_show_params_t *params,
+			      notmuch_bool_t reply_all,
+			      struct sprinter *sp);
+
+    if (format == FORMAT_JSON || format == FORMAT_SEXP) {
+	unsigned count;
+
+	status = notmuch_query_count_messages_st (query, &count);
+	if (print_status_query ("notmuch reply", query, status))
+	    return 1;
+
+	if (count != 1) {
+	    fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
+	    return 1;
+	}
+    }
+
+    if (format == FORMAT_HEADERS_ONLY) {
+	reply_format_func = notmuch_reply_format_headers_only;
+    } else if (format == FORMAT_JSON) {
+	reply_format_func = notmuch_reply_format_sprinter;
+	sp = sprinter_json_create (config, stdout);
+    } else if (format == FORMAT_SEXP) {
+	reply_format_func = notmuch_reply_format_sprinter;
+	sp = sprinter_sexp_create (config, stdout);
+    } else {
+	reply_format_func = notmuch_reply_format_default;
+    }
+
+    status = notmuch_query_search_messages_st (query, &messages);
+    if (print_status_query ("notmuch reply", query, status))
+	return 1;
+
+    for (;
+	 notmuch_messages_valid (messages);
+	 notmuch_messages_move_to_next (messages))
+    {
+	message = notmuch_messages_get (messages);
+
+	ret = reply_format_func(config, config, message, params, reply_all, sp);
+
+	notmuch_message_destroy (message);
+
+	if (ret)
+	    break;
+    }
+
+    return ret;
+}
+
 int
 notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 {
@@ -779,12 +784,6 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_query_t *query;
     char *query_string;
     int opt_index;
-    int (*reply_format_func) (void *ctx,
-			      notmuch_config_t *config,
-			      notmuch_query_t *query,
-			      notmuch_show_params_t *params,
-			      notmuch_bool_t reply_all,
-			      struct sprinter *sp);
     notmuch_show_params_t params = {
 	.part = -1,
 	.crypto = {
@@ -795,7 +794,6 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     };
     int format = FORMAT_DEFAULT;
     int reply_all = TRUE;
-    struct sprinter *sp = NULL;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
@@ -820,18 +818,6 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_process_shared_options (argv[0]);
 
-    if (format == FORMAT_HEADERS_ONLY) {
-	reply_format_func = notmuch_reply_format_headers_only;
-    } else if (format == FORMAT_JSON) {
-	reply_format_func = notmuch_reply_format_sprinter;
-	sp = sprinter_json_create (config, stdout);
-    } else if (format == FORMAT_SEXP) {
-	reply_format_func = notmuch_reply_format_sprinter;
-	sp = sprinter_sexp_create (config, stdout);
-    } else {
-	reply_format_func = notmuch_reply_format_default;
-    }
-
     notmuch_exit_if_unsupported_format ();
 
     query_string = query_string_from_args (config, argc-opt_index, argv+opt_index);
@@ -859,7 +845,7 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-    if (reply_format_func (config, config, query, &params, reply_all, sp) != 0)
+    if (do_reply (config, query, &params, format, reply_all) != 0)
 	return EXIT_FAILURE;
 
     notmuch_crypto_cleanup (&params.crypto);
-- 
2.1.4

[PATCH 2/7] cli/reply: reuse show_reply_headers() in headers-only format

[Jani Nikula]

Align the code with default format reply. There should be no changes
in output.
---
 notmuch-reply.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 847e306f94d2..5adbab624ad7 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -668,7 +668,6 @@ notmuch_reply_format_headers_only(void *ctx,
 {
     GMimeMessage *reply;
     const char *in_reply_to, *orig_references, *references;
-    char *reply_headers;
 
     /* The 0 means we do not want headers in a "pretty" order. */
     reply = g_mime_message_new (0);
@@ -697,9 +696,7 @@ notmuch_reply_format_headers_only(void *ctx,
 
     (void)add_recipients_from_message (reply, config, message, reply_all);
 
-    reply_headers = g_mime_object_to_string (GMIME_OBJECT (reply));
-    printf ("%s", reply_headers);
-    free (reply_headers);
+    show_reply_headers (reply);
 
     g_object_unref (G_OBJECT (reply));
 
-- 
2.1.4

[PATCH 3/7] cli/reply: unify reply format functions

[Jani Nikula]

Prepare for further future unification by making the code similar. The
only functional change is that errors in mime_node_open() also break
execution in default reply format.
---
 notmuch-reply.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 5adbab624ad7..4b97ffa4f096 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -604,20 +604,20 @@ notmuch_reply_format_default(void *ctx,
 			     unused (sprinter_t *sp))
 {
     GMimeMessage *reply;
-    mime_node_t *root;
+    mime_node_t *node;
+
+    if (mime_node_open (ctx, message, &params->crypto, &node))
+	return 1;
 
     reply = create_reply_message (ctx, config, message, reply_all);
     if (!reply)
 	return 1;
 
     show_reply_headers (reply);
+    format_part_reply (node);
 
     g_object_unref (G_OBJECT (reply));
-
-    if (mime_node_open (ctx, message, &params->crypto, &root) == NOTMUCH_STATUS_SUCCESS) {
-	format_part_reply (root);
-	talloc_free (root);
-    }
+    talloc_free (node);
 
     return 0;
 }
@@ -633,7 +633,7 @@ notmuch_reply_format_sprinter(void *ctx,
     GMimeMessage *reply;
     mime_node_t *node;
 
-    if (mime_node_open (ctx, message, &params->crypto, &node) != NOTMUCH_STATUS_SUCCESS)
+    if (mime_node_open (ctx, message, &params->crypto, &node))
 	return 1;
 
     reply = create_reply_message (ctx, config, message, reply_all);
@@ -645,7 +645,6 @@ notmuch_reply_format_sprinter(void *ctx,
     /* The headers of the reply message we've created */
     sp->map_key (sp, "reply-headers");
     format_headers_sprinter (sp, reply, TRUE);
-    g_object_unref (G_OBJECT (reply));
 
     /* Start the original */
     sp->map_key (sp, "original");
@@ -654,6 +653,9 @@ notmuch_reply_format_sprinter(void *ctx,
     /* End */
     sp->end (sp);
 
+    g_object_unref (G_OBJECT (reply));
+    talloc_free (node);
+
     return 0;
 }
 
-- 
2.1.4

[PATCH 4/7] cli/reply: reorganize create_reply_message()

[Jani Nikula]

Again, in preparation for later unification, reorganize
create_reply_message() to be more similar to the open coded version in
the headers-only format reply code. Due to "pretty" header ordering,
there should be no change in output. There should be no functional
changes.
---
 notmuch-reply.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 4b97ffa4f096..eb07405591fd 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -532,12 +532,20 @@ create_reply_message(void *ctx,
 	return NULL;
     }
 
-    subject = notmuch_message_get_header (message, "subject");
-    if (subject) {
-	if (strncasecmp (subject, "Re:", 3))
-	    subject = talloc_asprintf (ctx, "Re: %s", subject);
-	g_mime_message_set_subject (reply, subject);
-    }
+    in_reply_to = talloc_asprintf (ctx, "<%s>",
+				   notmuch_message_get_message_id (message));
+
+    g_mime_object_set_header (GMIME_OBJECT (reply), "In-Reply-To", in_reply_to);
+
+    orig_references = notmuch_message_get_header (message, "references");
+    if (!orig_references)
+	/* Treat errors like missing References headers. */
+	orig_references = "";
+    references = talloc_asprintf (ctx, "%s%s%s",
+				  *orig_references ? orig_references : "",
+				  *orig_references ? " " : "",
+				  in_reply_to);
+    g_mime_object_set_header (GMIME_OBJECT (reply), "References", references);
 
     from_addr = add_recipients_from_message (reply, config,
 					     message, reply_all);
@@ -572,25 +580,14 @@ create_reply_message(void *ctx,
     from_addr = talloc_asprintf (ctx, "%s <%s>",
 				 notmuch_config_get_user_name (config),
 				 from_addr);
-    g_mime_object_set_header (GMIME_OBJECT (reply),
-			      "From", from_addr);
-
-    in_reply_to = talloc_asprintf (ctx, "<%s>",
-				   notmuch_message_get_message_id (message));
-
-    g_mime_object_set_header (GMIME_OBJECT (reply),
-			      "In-Reply-To", in_reply_to);
+    g_mime_object_set_header (GMIME_OBJECT (reply), "From", from_addr);
 
-    orig_references = notmuch_message_get_header (message, "references");
-    if (!orig_references)
-	/* Treat errors like missing References headers. */
-	orig_references = "";
-    references = talloc_asprintf (ctx, "%s%s%s",
-				  *orig_references ? orig_references : "",
-				  *orig_references ? " " : "",
-				  in_reply_to);
-    g_mime_object_set_header (GMIME_OBJECT (reply),
-			      "References", references);
+    subject = notmuch_message_get_header (message, "subject");
+    if (subject) {
+	if (strncasecmp (subject, "Re:", 3))
+	    subject = talloc_asprintf (ctx, "Re: %s", subject);
+	g_mime_message_set_subject (reply, subject);
+    }
 
     return reply;
 }
-- 
2.1.4

[PATCH 5/7] cli/reply: make references header creation easier to follow

[Jani Nikula]

Just use strdup when original references is not available, instead of
trying to cram everything into a monster asprintf. There should be no
functional changes.
---
 notmuch-reply.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index eb07405591fd..c2d7402d40ae 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -538,13 +538,12 @@ create_reply_message(void *ctx,
     g_mime_object_set_header (GMIME_OBJECT (reply), "In-Reply-To", in_reply_to);
 
     orig_references = notmuch_message_get_header (message, "references");
-    if (!orig_references)
-	/* Treat errors like missing References headers. */
-	orig_references = "";
-    references = talloc_asprintf (ctx, "%s%s%s",
-				  *orig_references ? orig_references : "",
-				  *orig_references ? " " : "",
-				  in_reply_to);
+    if (orig_references && *orig_references)
+	references = talloc_asprintf (ctx, "%s %s", orig_references,
+				      in_reply_to);
+    else
+	references = talloc_strdup (ctx, in_reply_to);
+
     g_mime_object_set_header (GMIME_OBJECT (reply), "References", references);
 
     from_addr = add_recipients_from_message (reply, config,
-- 
2.1.4

[PATCH 6/7] cli/reply: reuse create_reply_message() also for headers-only format

[Jani Nikula]

Add an option for "limited" headers for the (slightly misleadingly
named) headers-only format. There should be no functional changes.
---
 notmuch-reply.c | 46 +++++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index c2d7402d40ae..daad453efb09 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -520,13 +520,17 @@ static GMimeMessage *
 create_reply_message(void *ctx,
 		     notmuch_config_t *config,
 		     notmuch_message_t *message,
-		     notmuch_bool_t reply_all)
+		     notmuch_bool_t reply_all,
+		     notmuch_bool_t limited)
 {
     const char *subject, *from_addr = NULL;
     const char *in_reply_to, *orig_references, *references;
 
-    /* The 1 means we want headers in a "pretty" order. */
-    GMimeMessage *reply = g_mime_message_new (1);
+    /*
+     * Use the below header order for limited headers, "pretty" order
+     * otherwise.
+     */
+    GMimeMessage *reply = g_mime_message_new (limited ? 0 : 1);
     if (reply == NULL) {
 	fprintf (stderr, "Out of memory\n");
 	return NULL;
@@ -549,6 +553,10 @@ create_reply_message(void *ctx,
     from_addr = add_recipients_from_message (reply, config,
 					     message, reply_all);
 
+    /* The above is all that is needed for limited headers. */
+    if (limited)
+	return reply;
+
     /*
      * Sadly, there is no standard way to find out to which email
      * address a mail was delivered - what is in the headers depends
@@ -605,7 +613,7 @@ notmuch_reply_format_default(void *ctx,
     if (mime_node_open (ctx, message, &params->crypto, &node))
 	return 1;
 
-    reply = create_reply_message (ctx, config, message, reply_all);
+    reply = create_reply_message (ctx, config, message, reply_all, FALSE);
     if (!reply)
 	return 1;
 
@@ -632,7 +640,7 @@ notmuch_reply_format_sprinter(void *ctx,
     if (mime_node_open (ctx, message, &params->crypto, &node))
 	return 1;
 
-    reply = create_reply_message (ctx, config, message, reply_all);
+    reply = create_reply_message (ctx, config, message, reply_all, FALSE);
     if (!reply)
 	return 1;
 
@@ -665,34 +673,10 @@ notmuch_reply_format_headers_only(void *ctx,
 				  unused (sprinter_t *sp))
 {
     GMimeMessage *reply;
-    const char *in_reply_to, *orig_references, *references;
 
-    /* The 0 means we do not want headers in a "pretty" order. */
-    reply = g_mime_message_new (0);
-    if (reply == NULL) {
-	fprintf (stderr, "Out of memory\n");
+    reply = create_reply_message (ctx, config, message, reply_all, TRUE);
+    if (!reply)
 	return 1;
-    }
-
-    in_reply_to = talloc_asprintf (ctx, "<%s>",
-				   notmuch_message_get_message_id (message));
-
-    g_mime_object_set_header (GMIME_OBJECT (reply), "In-Reply-To", in_reply_to);
-
-    orig_references = notmuch_message_get_header (message, "references");
-
-    /*
-     * We print In-Reply-To followed by References because git
-     * format-patch treats them specially. Git does not interpret the
-     * other headers specially.
-     */
-    references = talloc_asprintf (ctx, "%s%s%s",
-				  orig_references ? orig_references : "",
-				  orig_references ? " " : "",
-				  in_reply_to);
-    g_mime_object_set_header (GMIME_OBJECT (reply), "References", references);
-
-    (void)add_recipients_from_message (reply, config, message, reply_all);
 
     show_reply_headers (reply);
 
-- 
2.1.4

[PATCH 7/7] cli/reply: reduce the reply format abstractions

[Jani Nikula]

Now that we've made the various reply formats quite similar to each
other, there's no point in keeping the abstractions. They are now
close enough to be put in one function.

For now, a mime node will be uselessly created for the headers-only
case, but this is insignificant, and may change in the future.
---
 notmuch-reply.c | 145 ++++++++++++++------------------------------------------
 1 file changed, 36 insertions(+), 109 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index daad453efb09..b380678e7204 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -599,92 +599,6 @@ create_reply_message(void *ctx,
     return reply;
 }
 
-static int
-notmuch_reply_format_default(void *ctx,
-			     notmuch_config_t *config,
-			     notmuch_message_t *message,
-			     notmuch_show_params_t *params,
-			     notmuch_bool_t reply_all,
-			     unused (sprinter_t *sp))
-{
-    GMimeMessage *reply;
-    mime_node_t *node;
-
-    if (mime_node_open (ctx, message, &params->crypto, &node))
-	return 1;
-
-    reply = create_reply_message (ctx, config, message, reply_all, FALSE);
-    if (!reply)
-	return 1;
-
-    show_reply_headers (reply);
-    format_part_reply (node);
-
-    g_object_unref (G_OBJECT (reply));
-    talloc_free (node);
-
-    return 0;
-}
-
-static int
-notmuch_reply_format_sprinter(void *ctx,
-			      notmuch_config_t *config,
-			      notmuch_message_t *message,
-			      notmuch_show_params_t *params,
-			      notmuch_bool_t reply_all,
-			      sprinter_t *sp)
-{
-    GMimeMessage *reply;
-    mime_node_t *node;
-
-    if (mime_node_open (ctx, message, &params->crypto, &node))
-	return 1;
-
-    reply = create_reply_message (ctx, config, message, reply_all, FALSE);
-    if (!reply)
-	return 1;
-
-    sp->begin_map (sp);
-
-    /* The headers of the reply message we've created */
-    sp->map_key (sp, "reply-headers");
-    format_headers_sprinter (sp, reply, TRUE);
-
-    /* Start the original */
-    sp->map_key (sp, "original");
-    format_part_sprinter (ctx, sp, node, TRUE, TRUE, FALSE);
-
-    /* End */
-    sp->end (sp);
-
-    g_object_unref (G_OBJECT (reply));
-    talloc_free (node);
-
-    return 0;
-}
-
-/* This format is currently tuned for a git send-email --notmuch hook */
-static int
-notmuch_reply_format_headers_only(void *ctx,
-				  notmuch_config_t *config,
-				  notmuch_message_t *message,
-				  unused (notmuch_show_params_t *params),
-				  notmuch_bool_t reply_all,
-				  unused (sprinter_t *sp))
-{
-    GMimeMessage *reply;
-
-    reply = create_reply_message (ctx, config, message, reply_all, TRUE);
-    if (!reply)
-	return 1;
-
-    show_reply_headers (reply);
-
-    g_object_unref (G_OBJECT (reply));
-
-    return 0;
-}
-
 enum {
     FORMAT_DEFAULT,
     FORMAT_JSON,
@@ -698,17 +612,12 @@ static int do_reply(notmuch_config_t *config,
 		    int format,
 		    notmuch_bool_t reply_all)
 {
+    GMimeMessage *reply;
+    mime_node_t *node;
     notmuch_messages_t *messages;
     notmuch_message_t *message;
     notmuch_status_t status;
     struct sprinter *sp = NULL;
-    int ret = 0;
-    int (*reply_format_func) (void *ctx,
-			      notmuch_config_t *config,
-			      notmuch_message_t *message,
-			      notmuch_show_params_t *params,
-			      notmuch_bool_t reply_all,
-			      struct sprinter *sp);
 
     if (format == FORMAT_JSON || format == FORMAT_SEXP) {
 	unsigned count;
@@ -721,18 +630,11 @@ static int do_reply(notmuch_config_t *config,
 	    fprintf (stderr, "Error: search term did not match precisely one message (matched %d messages).\n", count);
 	    return 1;
 	}
-    }
 
-    if (format == FORMAT_HEADERS_ONLY) {
-	reply_format_func = notmuch_reply_format_headers_only;
-    } else if (format == FORMAT_JSON) {
-	reply_format_func = notmuch_reply_format_sprinter;
-	sp = sprinter_json_create (config, stdout);
-    } else if (format == FORMAT_SEXP) {
-	reply_format_func = notmuch_reply_format_sprinter;
-	sp = sprinter_sexp_create (config, stdout);
-    } else {
-	reply_format_func = notmuch_reply_format_default;
+	if (format == FORMAT_JSON)
+	    sp = sprinter_json_create (config, stdout);
+	else
+	    sp = sprinter_sexp_create (config, stdout);
     }
 
     status = notmuch_query_search_messages_st (query, &messages);
@@ -745,15 +647,40 @@ static int do_reply(notmuch_config_t *config,
     {
 	message = notmuch_messages_get (messages);
 
-	ret = reply_format_func(config, config, message, params, reply_all, sp);
+	if (mime_node_open (config, message, &params->crypto, &node))
+	    return 1;
 
-	notmuch_message_destroy (message);
+	reply = create_reply_message (config, config, message, reply_all,
+				      format == FORMAT_HEADERS_ONLY);
+	if (!reply)
+	    return 1;
 
-	if (ret)
-	    break;
+	if (format == FORMAT_JSON || format == FORMAT_SEXP) {
+	    sp->begin_map (sp);
+
+	    /* The headers of the reply message we've created */
+	    sp->map_key (sp, "reply-headers");
+	    format_headers_sprinter (sp, reply, TRUE);
+
+	    /* Start the original */
+	    sp->map_key (sp, "original");
+	    format_part_sprinter (config, sp, node, TRUE, TRUE, FALSE);
+
+	    /* End */
+	    sp->end (sp);
+	} else {
+	    show_reply_headers (reply);
+	    if (format == FORMAT_DEFAULT)
+		format_part_reply (node);
+	}
+
+	g_object_unref (G_OBJECT (reply));
+	talloc_free (node);
+
+	notmuch_message_destroy (message);
     }
 
-    return ret;
+    return 0;
 }
 
 int
-- 
2.1.4

[RFC PATCH 0/6] cli/reply: refactoring part 2

[Jani Nikula]

This one expands on [1] to use GMimeMessage more. There are some less
than perfect intermediate steps in order to split the stuff into
reasonable sized patches, but it gets better in the end. There's still
more to do, but I ran out of steam a bit (thus RFC), and it's better to
get the first series merged first anyway.

This should fix the issue Daniel saw [2], but I haven't really tested
that, as there are no dedicated tests for it. Daniel, care to try this
on the problematic message? Reindexing is not needed.

BR,
Jani.

[1] id:cover.1466284726.git.jani@nikula.org
[2] id:87d1ngv95p.fsf@alice.fifthhorseman.net


Jani Nikula (6):
  cli/reply: use dedicated functions for reply to mapping
  cli/reply: check for NULL list first in scan_address_list()
  cli/reply: return internet address list from get header funcs
  cli/reply: pass internet address list to munge detect
  cli/reply: pass gmime message to munge detection
  cli/reply: only pass gmime message to add recipients to reply message

 notmuch-reply.c | 171 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 90 insertions(+), 81 deletions(-)

-- 
2.1.4

[RFC PATCH 1/6] cli/reply: use dedicated functions for reply to mapping

[Jani Nikula]

The main motivation here is to move the special casing around
reply-to/from handling into a function of its own, clarifying the main
logic.
---
 notmuch-reply.c | 82 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index b380678e7204..9b78ea2c2b20 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -256,17 +256,13 @@ scan_address_string (const char *recipients,
  * in either the 'To' or 'Cc' header of the message?
  */
 static int
-reply_to_header_is_redundant (notmuch_message_t *message)
+reply_to_header_is_redundant (notmuch_message_t *message, const char *reply_to)
 {
-    const char *reply_to, *to, *cc, *addr;
+    const char *to, *cc, *addr;
     InternetAddressList *list;
     InternetAddress *address;
     InternetAddressMailbox *mailbox;
 
-    reply_to = notmuch_message_get_header (message, "reply-to");
-    if (reply_to == NULL || *reply_to == '\0')
-	return 0;
-
     list = internet_address_list_parse_string (reply_to);
 
     if (internet_address_list_length (list) != 1)
@@ -291,6 +287,47 @@ reply_to_header_is_redundant (notmuch_message_t *message)
     return 0;
 }
 
+static const char *get_sender(notmuch_message_t *message)
+{
+    const char *reply_to;
+
+    reply_to = notmuch_message_get_header (message, "reply-to");
+    if (reply_to && *reply_to) {
+        /*
+	 * Some mailing lists munge the Reply-To header despite it
+	 * being A Bad Thing, see
+	 * http://marc.merlins.org/netrants/reply-to-harmful.html
+	 *
+	 * The munging is easy to detect, because it results in a
+	 * redundant reply-to header, (with an address that already
+	 * exists in either To or Cc). So in this case, we ignore the
+	 * Reply-To field and use the From header. This ensures the
+	 * original sender will get the reply even if not subscribed
+	 * to the list. Note that the address in the Reply-To header
+	 * will always appear in the reply if reply_all is true.
+	 */
+	if (! reply_to_header_is_redundant (message, reply_to))
+	    return reply_to;
+    }
+
+    return notmuch_message_get_header (message, "from");
+}
+
+static const char *get_to(notmuch_message_t *message)
+{
+    return notmuch_message_get_header (message, "to");
+}
+
+static const char *get_cc(notmuch_message_t *message)
+{
+    return notmuch_message_get_header (message, "cc");
+}
+
+static const char *get_bcc(notmuch_message_t *message)
+{
+    return notmuch_message_get_header (message, "bcc");
+}
+
 /* Augment the recipients of 'reply' from the "Reply-to:", "From:",
  * "To:", "Cc:", and "Bcc:" headers of 'message'.
  *
@@ -310,43 +347,22 @@ add_recipients_from_message (GMimeMessage *reply,
 			     notmuch_bool_t reply_all)
 {
     struct {
-	const char *header;
-	const char *fallback;
+	const char * (*get_header)(notmuch_message_t *message);
 	GMimeRecipientType recipient_type;
     } reply_to_map[] = {
-	{ "reply-to", "from", GMIME_RECIPIENT_TYPE_TO  },
-	{ "to",         NULL, GMIME_RECIPIENT_TYPE_TO  },
-	{ "cc",         NULL, GMIME_RECIPIENT_TYPE_CC  },
-	{ "bcc",        NULL, GMIME_RECIPIENT_TYPE_BCC }
+	{ get_sender,	GMIME_RECIPIENT_TYPE_TO },
+	{ get_to,	GMIME_RECIPIENT_TYPE_TO },
+	{ get_cc,	GMIME_RECIPIENT_TYPE_CC },
+	{ get_bcc,	GMIME_RECIPIENT_TYPE_BCC },
     };
     const char *from_addr = NULL;
     unsigned int i;
     unsigned int n = 0;
 
-    /* Some mailing lists munge the Reply-To header despite it being A Bad
-     * Thing, see http://marc.merlins.org/netrants/reply-to-harmful.html
-     *
-     * The munging is easy to detect, because it results in a
-     * redundant reply-to header, (with an address that already exists
-     * in either To or Cc). So in this case, we ignore the Reply-To
-     * field and use the From header. This ensures the original sender
-     * will get the reply even if not subscribed to the list. Note
-     * that the address in the Reply-To header will always appear in
-     * the reply if reply_all is true.
-     */
-    if (reply_to_header_is_redundant (message)) {
-	reply_to_map[0].header = "from";
-	reply_to_map[0].fallback = NULL;
-    }
-
     for (i = 0; i < ARRAY_SIZE (reply_to_map); i++) {
 	const char *recipients;
 
-	recipients = notmuch_message_get_header (message,
-						 reply_to_map[i].header);
-	if ((recipients == NULL || recipients[0] == '\0') && reply_to_map[i].fallback)
-	    recipients = notmuch_message_get_header (message,
-						     reply_to_map[i].fallback);
+	recipients = reply_to_map[i].get_header (message);
 
 	n += scan_address_string (recipients, config, reply,
 				  reply_to_map[i].recipient_type, &from_addr);
-- 
2.1.4

[RFC PATCH 2/6] cli/reply: check for NULL list first in scan_address_list()

[Jani Nikula]

Support passing NULL list later on. Also use it to simplify the
recursion.
---
 notmuch-reply.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 9b78ea2c2b20..d90f46f9bed3 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -192,6 +192,9 @@ scan_address_list (InternetAddressList *list,
     int i;
     unsigned int n = 0;
 
+    if (list == NULL)
+	return 0;
+
     for (i = 0; i < internet_address_list_length (list); i++) {
 	address = internet_address_list_get_address (list, i);
 	if (INTERNET_ADDRESS_IS_GROUP (address)) {
@@ -200,9 +203,6 @@ scan_address_list (InternetAddressList *list,
 
 	    group = INTERNET_ADDRESS_GROUP (address);
 	    group_list = internet_address_group_get_members (group);
-	    if (group_list == NULL)
-		continue;
-
 	    n += scan_address_list (group_list, config, message, type, user_from);
 	} else {
 	    InternetAddressMailbox *mailbox;
-- 
2.1.4

[RFC PATCH 3/6] cli/reply: return internet address list from get header funcs

[Jani Nikula]

Pass in GMimeMessage to simplify To/Cc/Bcc headers. We'll eventually
remove the notmuch message passing altogether, but keep both for now
to not make too big changes at once.

Get rid of an intermediate function.

The small annoyance is the ownership differences in the address lists.
---
 notmuch-reply.c | 73 ++++++++++++++++++++++++---------------------------------
 1 file changed, 30 insertions(+), 43 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index d90f46f9bed3..98034485c546 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -227,31 +227,6 @@ scan_address_list (InternetAddressList *list,
     return n;
 }
 
-/* Scan addresses in 'recipients'.
- *
- * See the documentation of scan_address_list() above. This function
- * does exactly the same, but converts 'recipients' to an
- * InternetAddressList first.
- */
-static unsigned int
-scan_address_string (const char *recipients,
-		     notmuch_config_t *config,
-		     GMimeMessage *message,
-		     GMimeRecipientType type,
-		     const char **user_from)
-{
-    InternetAddressList *list;
-
-    if (recipients == NULL)
-	return 0;
-
-    list = internet_address_list_parse_string (recipients);
-    if (list == NULL)
-	return 0;
-
-    return scan_address_list (list, config, message, type, user_from);
-}
-
 /* Does the address in the Reply-To header of 'message' already appear
  * in either the 'To' or 'Cc' header of the message?
  */
@@ -287,11 +262,12 @@ reply_to_header_is_redundant (notmuch_message_t *message, const char *reply_to)
     return 0;
 }
 
-static const char *get_sender(notmuch_message_t *message)
+static InternetAddressList *get_sender(notmuch_message_t *message,
+				       GMimeMessage *mime_message)
 {
     const char *reply_to;
 
-    reply_to = notmuch_message_get_header (message, "reply-to");
+    reply_to = g_mime_message_get_reply_to (mime_message);
     if (reply_to && *reply_to) {
         /*
 	 * Some mailing lists munge the Reply-To header despite it
@@ -307,25 +283,32 @@ static const char *get_sender(notmuch_message_t *message)
 	 * will always appear in the reply if reply_all is true.
 	 */
 	if (! reply_to_header_is_redundant (message, reply_to))
-	    return reply_to;
+	    return internet_address_list_parse_string (reply_to);
     }
 
-    return notmuch_message_get_header (message, "from");
+    return internet_address_list_parse_string (
+	g_mime_message_get_sender (mime_message));
 }
 
-static const char *get_to(notmuch_message_t *message)
+static InternetAddressList *get_to(unused(notmuch_message_t *message),
+				   GMimeMessage *mime_message)
 {
-    return notmuch_message_get_header (message, "to");
+    return g_mime_message_get_recipients (mime_message,
+					  GMIME_RECIPIENT_TYPE_TO);
 }
 
-static const char *get_cc(notmuch_message_t *message)
+static InternetAddressList *get_cc(unused(notmuch_message_t *message),
+				   GMimeMessage *mime_message)
 {
-    return notmuch_message_get_header (message, "cc");
+    return g_mime_message_get_recipients (mime_message,
+					  GMIME_RECIPIENT_TYPE_CC);
 }
 
-static const char *get_bcc(notmuch_message_t *message)
+static InternetAddressList *get_bcc(unused(notmuch_message_t *message),
+				    GMimeMessage *mime_message)
 {
-    return notmuch_message_get_header (message, "bcc");
+    return g_mime_message_get_recipients (mime_message,
+					  GMIME_RECIPIENT_TYPE_BCC);
 }
 
 /* Augment the recipients of 'reply' from the "Reply-to:", "From:",
@@ -344,10 +327,12 @@ static const char *
 add_recipients_from_message (GMimeMessage *reply,
 			     notmuch_config_t *config,
 			     notmuch_message_t *message,
+			     GMimeMessage *mime_message,
 			     notmuch_bool_t reply_all)
 {
     struct {
-	const char * (*get_header)(notmuch_message_t *message);
+	InternetAddressList * (*get_header)(notmuch_message_t *message,
+					    GMimeMessage *mime_message);
 	GMimeRecipientType recipient_type;
     } reply_to_map[] = {
 	{ get_sender,	GMIME_RECIPIENT_TYPE_TO },
@@ -360,12 +345,12 @@ add_recipients_from_message (GMimeMessage *reply,
     unsigned int n = 0;
 
     for (i = 0; i < ARRAY_SIZE (reply_to_map); i++) {
-	const char *recipients;
+	InternetAddressList *recipients;
 
-	recipients = reply_to_map[i].get_header (message);
+	recipients = reply_to_map[i].get_header (message, mime_message);
 
-	n += scan_address_string (recipients, config, reply,
-				  reply_to_map[i].recipient_type, &from_addr);
+	n += scan_address_list (recipients, config, reply,
+				reply_to_map[i].recipient_type, &from_addr);
 
 	if (!reply_all && n) {
 	    /* Stop adding new recipients in reply-to-sender mode if
@@ -536,6 +521,7 @@ static GMimeMessage *
 create_reply_message(void *ctx,
 		     notmuch_config_t *config,
 		     notmuch_message_t *message,
+		     GMimeMessage *mime_message,
 		     notmuch_bool_t reply_all,
 		     notmuch_bool_t limited)
 {
@@ -566,8 +552,8 @@ create_reply_message(void *ctx,
 
     g_mime_object_set_header (GMIME_OBJECT (reply), "References", references);
 
-    from_addr = add_recipients_from_message (reply, config,
-					     message, reply_all);
+    from_addr = add_recipients_from_message (reply, config, message,
+					     mime_message, reply_all);
 
     /* The above is all that is needed for limited headers. */
     if (limited)
@@ -666,7 +652,8 @@ static int do_reply(notmuch_config_t *config,
 	if (mime_node_open (config, message, &params->crypto, &node))
 	    return 1;
 
-	reply = create_reply_message (config, config, message, reply_all,
+	reply = create_reply_message (config, config, message,
+				      GMIME_MESSAGE (node->part), reply_all,
 				      format == FORMAT_HEADERS_ONLY);
 	if (!reply)
 	    return 1;
-- 
2.1.4

[RFC PATCH 4/6] cli/reply: pass internet address list to munge detect

[Jani Nikula]

---
 notmuch-reply.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 98034485c546..cf4248bd6794 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -231,19 +231,18 @@ scan_address_list (InternetAddressList *list,
  * in either the 'To' or 'Cc' header of the message?
  */
 static int
-reply_to_header_is_redundant (notmuch_message_t *message, const char *reply_to)
+reply_to_header_is_redundant (notmuch_message_t *message,
+			      InternetAddressList *reply_to_list)
 {
     const char *to, *cc, *addr;
-    InternetAddressList *list;
     InternetAddress *address;
     InternetAddressMailbox *mailbox;
 
-    list = internet_address_list_parse_string (reply_to);
-
-    if (internet_address_list_length (list) != 1)
+    if (reply_to_list == NULL ||
+	internet_address_list_length (reply_to_list) != 1)
 	return 0;
 
-    address = internet_address_list_get_address (list, 0);
+    address = internet_address_list_get_address (reply_to_list, 0);
     if (INTERNET_ADDRESS_IS_GROUP (address))
 	return 0;
 
@@ -269,6 +268,8 @@ static InternetAddressList *get_sender(notmuch_message_t *message,
 
     reply_to = g_mime_message_get_reply_to (mime_message);
     if (reply_to && *reply_to) {
+	InternetAddressList *reply_to_list;
+
         /*
 	 * Some mailing lists munge the Reply-To header despite it
 	 * being A Bad Thing, see
@@ -282,8 +283,11 @@ static InternetAddressList *get_sender(notmuch_message_t *message,
 	 * to the list. Note that the address in the Reply-To header
 	 * will always appear in the reply if reply_all is true.
 	 */
-	if (! reply_to_header_is_redundant (message, reply_to))
-	    return internet_address_list_parse_string (reply_to);
+	reply_to_list = internet_address_list_parse_string (reply_to);
+	if (! reply_to_header_is_redundant (message, reply_to_list))
+	    return reply_to_list;
+
+	g_object_unref (G_OBJECT (reply_to_list));
     }
 
     return internet_address_list_parse_string (
-- 
2.1.4

[RFC PATCH 5/6] cli/reply: pass gmime message to munge detection

[Jani Nikula]

Improves the accuracy in many ways.
---
 notmuch-reply.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index cf4248bd6794..abf3a6c1824c 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -230,13 +230,16 @@ scan_address_list (InternetAddressList *list,
 /* Does the address in the Reply-To header of 'message' already appear
  * in either the 'To' or 'Cc' header of the message?
  */
-static int
-reply_to_header_is_redundant (notmuch_message_t *message,
+static notmuch_bool_t
+reply_to_header_is_redundant (GMimeMessage *message,
 			      InternetAddressList *reply_to_list)
 {
-    const char *to, *cc, *addr;
+    const char *addr, *reply_to;
     InternetAddress *address;
     InternetAddressMailbox *mailbox;
+    InternetAddressList *recipients;
+    notmuch_bool_t ret = FALSE;
+    int i;
 
     if (reply_to_list == NULL ||
 	internet_address_list_length (reply_to_list) != 1)
@@ -247,18 +250,26 @@ reply_to_header_is_redundant (notmuch_message_t *message,
 	return 0;
 
     mailbox = INTERNET_ADDRESS_MAILBOX (address);
-    addr = internet_address_mailbox_get_addr (mailbox);
+    reply_to = internet_address_mailbox_get_addr (mailbox);
 
-    to = notmuch_message_get_header (message, "to");
-    cc = notmuch_message_get_header (message, "cc");
+    recipients = g_mime_message_get_all_recipients (message);
 
-    if ((to && strstr (to, addr) != 0) ||
-	(cc && strstr (cc, addr) != 0))
-    {
-	return 1;
+    for (i = 0; i < internet_address_list_length (recipients); i++) {
+	address = internet_address_list_get_address (recipients, i);
+	if (INTERNET_ADDRESS_IS_GROUP (address))
+	    continue;
+
+	mailbox = INTERNET_ADDRESS_MAILBOX (address);
+	addr = internet_address_mailbox_get_addr (mailbox);
+	if (strcmp (addr, reply_to) == 0) {
+	    ret = TRUE;
+	    break;
+	}
     }
 
-    return 0;
+    g_object_unref (G_OBJECT (recipients));
+
+    return ret;
 }
 
 static InternetAddressList *get_sender(notmuch_message_t *message,
@@ -284,7 +295,7 @@ static InternetAddressList *get_sender(notmuch_message_t *message,
 	 * will always appear in the reply if reply_all is true.
 	 */
 	reply_to_list = internet_address_list_parse_string (reply_to);
-	if (! reply_to_header_is_redundant (message, reply_to_list))
+	if (! reply_to_header_is_redundant (mime_message, reply_to_list))
 	    return reply_to_list;
 
 	g_object_unref (G_OBJECT (reply_to_list));
-- 
2.1.4

[RFC PATCH 6/6] cli/reply: only pass gmime message to add recipients to reply message

[Jani Nikula]

The notmuch message is no longer needed. Simplify.
---
 notmuch-reply.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index abf3a6c1824c..8c894974485d 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -272,12 +272,11 @@ reply_to_header_is_redundant (GMimeMessage *message,
     return ret;
 }
 
-static InternetAddressList *get_sender(notmuch_message_t *message,
-				       GMimeMessage *mime_message)
+static InternetAddressList *get_sender(GMimeMessage *message)
 {
     const char *reply_to;
 
-    reply_to = g_mime_message_get_reply_to (mime_message);
+    reply_to = g_mime_message_get_reply_to (message);
     if (reply_to && *reply_to) {
 	InternetAddressList *reply_to_list;
 
@@ -295,35 +294,29 @@ static InternetAddressList *get_sender(notmuch_message_t *message,
 	 * will always appear in the reply if reply_all is true.
 	 */
 	reply_to_list = internet_address_list_parse_string (reply_to);
-	if (! reply_to_header_is_redundant (mime_message, reply_to_list))
+	if (! reply_to_header_is_redundant (message, reply_to_list))
 	    return reply_to_list;
 
 	g_object_unref (G_OBJECT (reply_to_list));
     }
 
     return internet_address_list_parse_string (
-	g_mime_message_get_sender (mime_message));
+	g_mime_message_get_sender (message));
 }
 
-static InternetAddressList *get_to(unused(notmuch_message_t *message),
-				   GMimeMessage *mime_message)
+static InternetAddressList *get_to(GMimeMessage *message)
 {
-    return g_mime_message_get_recipients (mime_message,
-					  GMIME_RECIPIENT_TYPE_TO);
+    return g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_TO);
 }
 
-static InternetAddressList *get_cc(unused(notmuch_message_t *message),
-				   GMimeMessage *mime_message)
+static InternetAddressList *get_cc(GMimeMessage *message)
 {
-    return g_mime_message_get_recipients (mime_message,
-					  GMIME_RECIPIENT_TYPE_CC);
+    return g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_CC);
 }
 
-static InternetAddressList *get_bcc(unused(notmuch_message_t *message),
-				    GMimeMessage *mime_message)
+static InternetAddressList *get_bcc(GMimeMessage *message)
 {
-    return g_mime_message_get_recipients (mime_message,
-					  GMIME_RECIPIENT_TYPE_BCC);
+    return g_mime_message_get_recipients (message, GMIME_RECIPIENT_TYPE_BCC);
 }
 
 /* Augment the recipients of 'reply' from the "Reply-to:", "From:",
@@ -341,13 +334,11 @@ static InternetAddressList *get_bcc(unused(notmuch_message_t *message),
 static const char *
 add_recipients_from_message (GMimeMessage *reply,
 			     notmuch_config_t *config,
-			     notmuch_message_t *message,
-			     GMimeMessage *mime_message,
+			     GMimeMessage *message,
 			     notmuch_bool_t reply_all)
 {
     struct {
-	InternetAddressList * (*get_header)(notmuch_message_t *message,
-					    GMimeMessage *mime_message);
+	InternetAddressList * (*get_header)(GMimeMessage *message);
 	GMimeRecipientType recipient_type;
     } reply_to_map[] = {
 	{ get_sender,	GMIME_RECIPIENT_TYPE_TO },
@@ -362,7 +353,7 @@ add_recipients_from_message (GMimeMessage *reply,
     for (i = 0; i < ARRAY_SIZE (reply_to_map); i++) {
 	InternetAddressList *recipients;
 
-	recipients = reply_to_map[i].get_header (message, mime_message);
+	recipients = reply_to_map[i].get_header (message);
 
 	n += scan_address_list (recipients, config, reply,
 				reply_to_map[i].recipient_type, &from_addr);
@@ -567,7 +558,7 @@ create_reply_message(void *ctx,
 
     g_mime_object_set_header (GMIME_OBJECT (reply), "References", references);
 
-    from_addr = add_recipients_from_message (reply, config, message,
+    from_addr = add_recipients_from_message (reply, config,
 					     mime_message, reply_all);
 
     /* The above is all that is needed for limited headers. */
-- 
2.1.4