[PATCH v2 00/14] reply refactor, fixes

[PATCH v2 00/14] reply refactor, fixes

[Jani Nikula]

Hi all, this lengthy series fixes replies to messages with multiple Cc:
headers. Daniel reported the problem in
id:87d1ngv95p.fsf@alice.fifthhorseman.net.

Lots of refactoring is required to switch to using more GMime functions
in the reply code.

This is v2 of the series in id:cover.1466284726.git.jani@nikula.org.

BR,
Jani.


Jani Nikula (14):
  test: add known broken test for reply to message with multiple Cc
    headers
  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
  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    | 435 +++++++++++++++++++++--------------------------------
 test/T220-reply.sh |  24 +++
 2 files changed, 192 insertions(+), 267 deletions(-)

-- 
2.1.4

[PATCH v2 01/14] test: add known broken test for reply to message with multiple Cc headers

[Jani Nikula]

As Daniel Kahn Gillmor <dkg@fifthhorseman.net> reports in
id:87d1ngv95p.fsf@alice.fifthhorseman.net, notmuch show combines
multiple Cc: fields into one, while notmuch reply does not. While such
messages are in violation of RFC 5322, it would be reasonable to
expect notmuch to be consistent. Add a known broken test to document
this expectation.

Details:

The original message is formatted using the message printing in
notmuch-show.c. For Cc:, it uses g_mime_message_get_recipients(),
which apparently combines all Cc: fields into one internally.

The addresses in the reply headers, OTOH, are based on headers queried
through libnotmuch. It boils down to g_mime_object_get_header() in
lib/message-file.c, which returns only the first occurence of header.
---
 test/T220-reply.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/test/T220-reply.sh b/test/T220-reply.sh
index 30b78f679d97..d6f3a839ca48 100755
--- a/test/T220-reply.sh
+++ b/test/T220-reply.sh
@@ -253,5 +253,30 @@ test_expect_equal_json "$output" '
     }
 }'
 
+test_begin_subtest "Reply to a message with multiple Cc headers"
+test_subtest_known_broken
+cat > "${MAIL_DIR}"/broken_cc <<EOF
+From: Alice <alice@example.org>
+To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+Cc: Bob <bob@example.org>
+Subject: wowsers!
+cc: Charles <charles@example.org>
+Message-Id: <abc123@example.org>
+Date: Thu, 16 Jun 2016 22:14:41 -0400
+
+Note the Cc: and cc: headers.
+EOF
+notmuch new >/dev/null
+output=$(notmuch reply id:abc123@example.org)
+notmuch reply id:abc123@example.com
+test_expect_equal "$output" "From: Notmuch Test Suite <test_suite@notmuchmail.org>
+Subject: Re: wowsers!
+To: Alice <alice@example.org>, Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+Cc: Bob <bob@example.org>, Charles <charles@example.org>
+In-Reply-To: <abc123@example.org>
+References: <abc123@example.org>
+
+On Thu, 16 Jun 2016 22:14:41 -0400, Alice <alice@example.org> wrote:
+> Note the Cc: and cc: headers."
 
 test_done
-- 
2.1.4

Re: [PATCH v2 01/14] test: add known broken test for reply to message with multiple Cc headers

[David Bremner]

Jani Nikula <jani@nikula.org> writes:


> +output=$(notmuch reply id:abc123@example.org)
> +notmuch reply id:abc123@example.com
> +test_expect_equal "$output" "From: Notmuch Test Suite <test_suite@notmuchmail.org>
> +Subject: Re: wowsers!
> +To: Alice <alice@example.org>, Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> +Cc: Bob <bob@example.org>, Charles <charles@example.org>
> +In-Reply-To: <abc123@example.org>
> +References: <abc123@example.org>
> +
> +On Thu, 16 Jun 2016 22:14:41 -0400, Alice <alice@example.org> wrote:
> +> Note the Cc: and cc: headers."

I'm not sure it's rational, but I think I prefer files rather than
variables/strings for this kind of test.

Re: [PATCH v2 01/14] test: add known broken test for reply to message with multiple Cc headers

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

>  
> +test_begin_subtest "Reply to a message with multiple Cc headers"
> +test_subtest_known_broken
> +cat > "${MAIL_DIR}"/broken_cc <<EOF
> +From: Alice <alice@example.org>
> +To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> +Cc: Bob <bob@example.org>
> +Subject: wowsers!
> +cc: Charles <charles@example.org>
> +Message-Id: <abc123@example.org>
> +Date: Thu, 16 Jun 2016 22:14:41 -0400

As another minor point, It's probably not a great idea to use real
addresses when making up test messages. Probably dkg has reached maximum
spam, but still.

d

Re: [PATCH v2 01/14] test: add known broken test for reply to message with multiple Cc headers

[Jani Nikula]

On Sun, 11 Sep 2016, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>>  
>> +test_begin_subtest "Reply to a message with multiple Cc headers"
>> +test_subtest_known_broken
>> +cat > "${MAIL_DIR}"/broken_cc <<EOF
>> +From: Alice <alice@example.org>
>> +To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
>> +Cc: Bob <bob@example.org>
>> +Subject: wowsers!
>> +cc: Charles <charles@example.org>
>> +Message-Id: <abc123@example.org>
>> +Date: Thu, 16 Jun 2016 22:14:41 -0400
>
> As another minor point, It's probably not a great idea to use real
> addresses when making up test messages. Probably dkg has reached maximum
> spam, but still.

Agreed. Thoughtless copy-paste from dkg's test case.

BR,
Jani.

[PATCH v2 02/14] 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

Re: [PATCH v2 02/14] cli/reply: push notmuch reply format abstraction lower in the stack

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> -	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);

I'm not sure I should admit this, but I'd have to check a book to make
sure that &params->crypto is the same as &(params->crypto)

Re: [PATCH v2 02/14] cli/reply: push notmuch reply format abstraction lower in the stack

[Jani Nikula]

On Sat, 10 Sep 2016, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> -	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);
>
> I'm not sure I should admit this, but I'd have to check a book to make
> sure that &params->crypto is the same as &(params->crypto)

Nah, you wouldn't have to check a book. If it weren't the same, it
wouldn't compile. :)

BR,
Jani.

[PATCH v2 03/14] 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 v2 04/14] 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

Re: [PATCH v2 04/14] cli/reply: unify reply format functions

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

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

>      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));

I'm not sure if this matters in the long run, but a related change is
that the g_object_unref is skipped in case mime_node_open returns an
error.

d

Re: [PATCH v2 04/14] cli/reply: unify reply format functions

[Jani Nikula]

On Sat, 10 Sep 2016, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> 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.
>
>>      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));
>
> I'm not sure if this matters in the long run, but a related change is
> that the g_object_unref is skipped in case mime_node_open returns an
> error.

The order changes too, we don't yet have an object to g_object_unref if
mime_node_open fails.

BR,
Jani.

[PATCH v2 05/14] 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

Re: [PATCH v2 05/14] cli/reply: reorganize create_reply_message()

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

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

The patch looks ok. The term "open coded" might be a little too jargony
/ technical.

d

[PATCH v2 06/14] 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

Re: [PATCH v2 06/14] cli/reply: make references header creation easier to follow

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> 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);
> +

fine, and probably applicable earlier if the series stalls.

d

[PATCH v2 07/14] 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 v2 08/14] 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

Re: [PATCH v2 08/14] cli/reply: reduce the reply format abstractions

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

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

patches 7,8 LGTM.

d

[PATCH v2 09/14] 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

[PATCH v2 10/14] 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

[PATCH v2 11/14] 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.

Getting the headers from GMimeMessage using GMime functions fixes the
error on duplicate Cc headers reported by Daniel Kahn Gillmor
<dkg@fifthhorseman.net> in id:87d1ngv95p.fsf@alice.fifthhorseman.net.

Get rid of an intermediate function.

The small annoyance is the ownership differences in the address lists.
---
 notmuch-reply.c    | 73 ++++++++++++++++++++++--------------------------------
 test/T220-reply.sh |  1 -
 2 files changed, 30 insertions(+), 44 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;
diff --git a/test/T220-reply.sh b/test/T220-reply.sh
index d6f3a839ca48..47ba5d38e18d 100755
--- a/test/T220-reply.sh
+++ b/test/T220-reply.sh
@@ -254,7 +254,6 @@ test_expect_equal_json "$output" '
 }'
 
 test_begin_subtest "Reply to a message with multiple Cc headers"
-test_subtest_known_broken
 cat > "${MAIL_DIR}"/broken_cc <<EOF
 From: Alice <alice@example.org>
 To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
-- 
2.1.4

Re: [PATCH v2 11/14] cli/reply: return internet address list from get header funcs

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

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

Patches 9-11 LGTM.

d

[PATCH v2 12/14] 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

Re: [PATCH v2 12/14] cli/reply: pass internet address list to munge detect

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

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

The commit message needs a bit more explanation. I think the writer
knows this code too well ;).
- This changes the arguments of reply_to_header_is redundant,
- "munge detect" is really only defined by a careful reading of comments.

[PATCH v2 13/14] 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

Re: [PATCH v2 13/14] cli/reply: pass gmime message to munge detection

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Improves the accuracy in many ways.

Essentially same objections as the last patch

If the unused parameter to get_sender is intentional, that probably
needs a remark as well

[PATCH v2 14/14] 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

Re: [PATCH v2 14/14] cli/reply: only pass gmime message to add recipients to reply message

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> The notmuch message is no longer needed. Simplify.

LGTM. I guess the comment I asked for in the last patch can be something
like "unused parameter will be fixed in the next commit"

d