From: Jani Nikula <jani@nikula.org>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org,
	david@tethera.net
Subject: Re: [PATCH 1/4] Add the option "--reply-to" to notmuch reply.
In-Reply-To: <1325856857-15969-1-git-send-email-markwalters1009@gmail.com>
References: <8739btdkir.fsf@qmul.ac.uk>
	<1325856857-15969-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.10.2+182~g93862a2 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Sun, 08 Jan 2012 14:47:33 +0200
Message-ID: <871urafjiy.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Fri,  6 Jan 2012 13:34:14 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
>     Possible values for this option are "sender" which replies just to
>     sender and "all" (the default).
> 
>     More precisely reply to sender follows these rules:
>     reply only to sender unless it was the user
>     reply only to all people on the "to" line unless they were all the user
>     reply to all people on the "cc" line
> 
>     Implementation details
> 
>     We continue parsing addresses beyond the ones we reply to because
>     we want to make sure the from address is correct. (At the very least it
>     is the same as it would be if we replied to all.)
> 
>     We overload the message variable in add_recipients_for_address_list so
>     if it is NULL we parse the address (looking for the users address)
>     but do not add to the message recipients list
> 
>     We add the variable reply_to_all to the function chain to keep track
>     of whether we should reply to everyone.
> ---
>  notmuch-reply.c |   48 +++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index f8d5f64..9a77fe6 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -212,7 +212,8 @@ add_recipients_for_address_list (GMimeMessage *message,
>  		if (ret == NULL)
>  		    ret = addr;
>  	    } else {
> -		g_mime_message_add_recipient (message, type, name, addr);
> +		 if (message)
> +		      g_mime_message_add_recipient (message, type, name, addr);
>  	    }
>  	}
>      }
> @@ -292,7 +293,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,
> +			     int reply_to_all)
>  {
>      struct {
>  	const char *header;
> @@ -332,9 +334,20 @@ add_recipients_from_message (GMimeMessage *reply,
>  	    recipients = notmuch_message_get_header (message,
>  						     reply_to_map[i].fallback);
>  
> -	addr = add_recipients_for_string (reply, config,
> -					  reply_to_map[i].recipient_type,
> -					  recipients);
> +
> +	/* We add the addresses if we are replying to all or we have not yet found
> +	 * a non-user address. We have to keep parsing to make sure we do find the
> +	 * correct from address for the user, but we pass a NULL message
> +	 */
> +	if ((reply_to_all) || (g_mime_message_get_all_recipients (reply) == NULL))

Looking into this, it occurred to me g_mime_message_get_all_recipients()
allocates a new InternetAddressList when the return value is
non-NULL. Thus this leaks memory. OTOH allocating and deallocating for
this purpose seems suboptimal. I'll think this over.

BR,
Jani.



> +	    addr = add_recipients_for_string (reply, config,
> +					      reply_to_map[i].recipient_type,
> +					      recipients);
> +	else
> +	     addr = add_recipients_for_string (NULL, config,
> +					       reply_to_map[i].recipient_type,
> +					       recipients);
> +
>  	if (from_addr == NULL)
>  	    from_addr = addr;
>      }
> @@ -480,7 +493,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,
> +			     int reply_to_all)
>  {
>      GMimeMessage *reply;
>      notmuch_messages_t *messages;
> @@ -509,7 +523,7 @@ 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_to_all);
>  
>  	if (from_addr == NULL)
>  	    from_addr = guess_from_received_header (config, message);
> @@ -558,7 +572,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),
> +				  int reply_to_all)
>  {
>      GMimeMessage *reply;
>      notmuch_messages_t *messages;
> @@ -598,7 +613,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_to_all);
>  
>  	reply_headers = g_mime_object_to_string (GMIME_OBJECT (reply));
>  	printf ("%s", reply_headers);
> @@ -620,7 +635,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      notmuch_query_t *query;
>      char *opt, *query_string;
>      int i, ret = 0;
> -    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_show_params_t *params);
> +    int reply_to_all = 1;
> +    int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_show_params_t *params, int reply_to_all);
>      notmuch_show_params_t params;
>  
>      reply_format_func = notmuch_reply_format_default;
> @@ -654,6 +670,16 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  		g_object_unref (session);
>  		session = NULL;
>  	    }
> +	} else if (STRNCMP_LITERAL (argv[i], "--reply-to=") == 0) {
> +	    opt = argv[i] + sizeof ("--reply-to=") - 1;
> +	    if (strcmp (opt, "sender") == 0) {
> +		 reply_to_all = 0;
> +	    } else if (strcmp (opt, "all") == 0) {
> +		 reply_to_all = 1;
> +	    } else {
> +		 fprintf (stderr, "Invalid value for --reply-to: %s\n", opt);
> +		 return 1;
> +	    }
>  	} else {
>  	    fprintf (stderr, "Unrecognized option: %s\n", argv[i]);
>  	    return 1;
> @@ -689,7 +715,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_to_all) != 0)
>  	return 1;
>  
>      notmuch_query_destroy (query);
> -- 
> 1.7.2.3
>