Date: Thu, 12 Jan 2012 17:02:51 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v4 2/5] cli: add support for replying just to the sender
	in "notmuch reply"
Message-ID: <20120112220251.GG18625@mit.edu>
References: <cover.1325794371.git.jani@nikula.org>
	<cover.1326403905.git.jani@nikula.org>
	<9be66534c17fbffe00b0c1201f378e60fadd11d1.1326403905.git.jani@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <9be66534c17fbffe00b0c1201f378e60fadd11d1.1326403905.git.jani@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

LGTM.  Again, one minor nit below, but not enough to hold up this
patch.

Quoth Jani Nikula on Jan 12 at 11:40 pm:
> Add new option --reply-to=(all|sender) to "notmuch reply" to select whether
> to reply to all (sender and all recipients), or just sender. Reply to all
> remains the default.
> 
> Credits to Mark Walters <markwalters1009@gmail.com> for his similar earlier
> work where I picked up the basic idea of handling reply-to-sender in
> add_recipients_from_message(). All bugs are mine, though.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> 
> ---
> 
> Settled on --reply-to=(all|sender) per Carl's earlier suggestion
> (id:87pqn5cg4g.fsf@yoom.home.cworth.org) and David's approval on IRC.
> ---
>  man/man1/notmuch-reply.1 |   28 ++++++++++++++++++++----
>  notmuch-reply.c          |   52 +++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 65 insertions(+), 15 deletions(-)
> 
> diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
> index db464d8..5160ece 100644
> --- a/man/man1/notmuch-reply.1
> +++ b/man/man1/notmuch-reply.1
> @@ -14,11 +14,13 @@ Constructs a reply template for a set of messages.
>  To make replying to email easier,
>  .B notmuch reply
>  takes an existing set of messages and constructs a suitable mail
> -template. The Reply-to header (if any, otherwise From:) is used for
> -the To: address. Vales from the To: and Cc: headers are copied, but
> -not including any of the current user's email addresses (as configured
> -in primary_mail or other_email in the .notmuch\-config file) in the
> -recipient list
> +template. The Reply-to: header (if any, otherwise From:) is used for
> +the To: address. Unless
> +.BR \-\-reply-to=sender
> +is specified, values from the To: and Cc: headers are copied, but not
> +including any of the current user's email addresses (as configured in
> +primary_mail or other_email in the .notmuch\-config file) in the
> +recipient list.
>  
>  It also builds a suitable new subject, including Re: at the front (if
>  not already present), and adding the message IDs of the messages being
> @@ -45,6 +47,22 @@ Includes subject and quoted message body.
>  Only produces In\-Reply\-To, References, To, Cc, and Bcc headers.
>  .RE
>  .RE
> +.RS
> +.TP 4
> +.BR \-\-reply\-to= ( all | sender )
> +.RS
> +.TP 4
> +.BR all " (default)"
> +Replies to all addresses.
> +.TP 4
> +.BR sender
> +Replies only to the sender. If replying to user's own message
> +(Reply-to: or From: header is one of the user's configured email
> +addresses), try To:, Cc:, and Bcc: headers in this order, and copy
> +values from the first that contains something other than only the
> +user's addresses.
> +.RE
> +.RE
>  
>  See \fBnotmuch-search-terms\fR(7)
>  for details of the supported syntax for <search-terms>.
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 4fae66f..b4c81de 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -289,7 +289,14 @@ reply_to_header_is_redundant (notmuch_message_t *message)
>      return 0;
>  }
>  
> -/* Augments the recipients of reply from the headers of message.
> +/* Augment the recipients of 'reply' from the "Reply-to:", "From:", "To:",
> + * "Cc:", and "Bcc:" headers of 'message'.
> + *
> + * If 'reply_all' is true, use sender and all recipients, otherwise scan the
> + * headers for the first that contains something other than the user's addresses
> + * and add the recipients from this header (typically this would be
> + * reply-to-sender, but also handles reply to user's own message in a sensible
> + * way).

Same comment about 80 column wrapping.

>   *
>   * If any of the user's addresses were found in these headers, the first
>   * of these returned, otherwise NULL is returned.
> @@ -297,7 +304,8 @@ reply_to_header_is_redundant (notmuch_message_t *message)
>  static const char *
>  add_recipients_from_message (GMimeMessage *reply,
>  			     notmuch_config_t *config,
> -			     notmuch_message_t *message)
> +			     notmuch_message_t *message,
> +			     notmuch_bool_t reply_all)
>  {
>      struct {
>  	const char *header;
> @@ -311,6 +319,7 @@ add_recipients_from_message (GMimeMessage *reply,
>      };
>      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://www.unicom.com/pw/reply-to-harmful.html
> @@ -337,8 +346,23 @@ add_recipients_from_message (GMimeMessage *reply,
>  	    recipients = notmuch_message_get_header (message,
>  						     reply_to_map[i].fallback);
>  
> -	scan_address_string (recipients, config, reply,
> -			     reply_to_map[i].recipient_type, &from_addr);
> +	n += scan_address_string (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 we have
> +	     * added some recipient(s) above.
> +	     *
> +	     * This also handles the case of user replying to his own message,
> +	     * where reply-to/from is not a recipient. In this case there may be
> +	     * more than one recipient even if not replying to all.
> +	     */
> +	    reply = NULL;
> +
> +	    /* From address and some recipients are enough, bail out. */
> +	    if (from_addr)
> +		break;

Nifty.

> +	}
>      }
>  
>      return from_addr;
> @@ -482,7 +506,8 @@ static int
>  notmuch_reply_format_default(void *ctx,
>  			     notmuch_config_t *config,
>  			     notmuch_query_t *query,
> -			     notmuch_show_params_t *params)
> +			     notmuch_show_params_t *params,
> +			     notmuch_bool_t reply_all)
>  {
>      GMimeMessage *reply;
>      notmuch_messages_t *messages;
> @@ -511,7 +536,8 @@ notmuch_reply_format_default(void *ctx,
>  	    g_mime_message_set_subject (reply, subject);
>  	}
>  
> -	from_addr = add_recipients_from_message (reply, config, message);
> +	from_addr = add_recipients_from_message (reply, config, message,
> +						 reply_all);
>  
>  	if (from_addr == NULL)
>  	    from_addr = guess_from_received_header (config, message);
> @@ -560,7 +586,8 @@ static int
>  notmuch_reply_format_headers_only(void *ctx,
>  				  notmuch_config_t *config,
>  				  notmuch_query_t *query,
> -				  unused (notmuch_show_params_t *params))
> +				  unused (notmuch_show_params_t *params),
> +				  notmuch_bool_t reply_all)
>  {
>      GMimeMessage *reply;
>      notmuch_messages_t *messages;
> @@ -600,7 +627,7 @@ notmuch_reply_format_headers_only(void *ctx,
>  	g_mime_object_set_header (GMIME_OBJECT (reply),
>  				  "References", references);
>  
> -	(void)add_recipients_from_message (reply, config, message);
> +	(void)add_recipients_from_message (reply, config, message, reply_all);
>  
>  	reply_headers = g_mime_object_to_string (GMIME_OBJECT (reply));
>  	printf ("%s", reply_headers);
> @@ -627,9 +654,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      notmuch_query_t *query;
>      char *query_string;
>      int opt_index, ret = 0;
> -    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_show_params_t *params);
> +    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_show_params_t *params, notmuch_bool_t reply_all);
>      notmuch_show_params_t params = { .part = -1 };
>      int format = FORMAT_DEFAULT;
> +    int reply_all = TRUE;
>      notmuch_bool_t decrypt = FALSE;
>  
>      notmuch_opt_desc_t options[] = {
> @@ -637,6 +665,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  	  (notmuch_keyword_t []){ { "default", FORMAT_DEFAULT },
>  				  { "headers-only", FORMAT_HEADERS_ONLY },
>  				  { 0, 0 } } },
> +	{ NOTMUCH_OPT_KEYWORD, &reply_all, "reply-to", 'r',
> +	  (notmuch_keyword_t []){ { "all", TRUE },
> +				  { "sender", FALSE },
> +				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_BOOLEAN, &decrypt, "decrypt", 'd', 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
> @@ -690,7 +722,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    if (reply_format_func (ctx, config, query, &params) != 0)
> +    if (reply_format_func (ctx, config, query, &params, reply_all) != 0)
>  	return 1;
>  
>      notmuch_query_destroy (query);