From: Michal Sojka <sojkam1@fel.cvut.cz>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>, cworth@cworth.org
In-Reply-To:
 <1268238686-13605-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
References: <87zl2hic7d.fsf@yoom.home.cworth.org>
	<1268238686-13605-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
Date: Thu, 11 Mar 2010 13:45:45 +0100
Message-ID: <87r5nrm3gm.fsf@steelpick.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: "Aneesh Kumar K.V" <aneesh.kumar@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [notmuch] [PATCH -V3 1/2] notmuch-reply: Add support for
	replying only to sender
Precedence: list

Hi,

On Wed, 10 Mar 2010, Aneesh Kumar K.V wrote:
> From: Aneesh Kumar K.V <aneesh.kumar@gmail.com>
> 
> This patch add --recipient=all|sender option
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@gmail.com>
> ---
>  notmuch-client.h |    2 +
>  notmuch-reply.c  |   55 ++++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index c80b39c..26fdb4a 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -70,6 +70,8 @@
>  #define STRNCMP_LITERAL(var, literal) \
>      strncmp ((var), (literal), sizeof (literal) - 1)
>  
> +#define NOTMUCH_REPLY_ALL   0x1
> +#define NOTMUCH_REPLY_SENDER_ONLY 0x2

Why not to define this as enum? When I see a definition like this
it reminds me bit flags which can be used together e.g.
  (NOTMUCH_REPLY_ALL | NOTMUCH_REPLY_SENDER_ONLY).

This has obviously no sense here.

>  static inline void
>  chomp_newline (char *str)
>  {
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 6c15536..e8a0820 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -232,20 +232,37 @@ 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_options)
>  {
> -    struct {
> +    struct reply_to_map {
>  	const char *header;
>  	const char *fallback;
>  	GMimeRecipientType recipient_type;
> -    } reply_to_map[] = {
> +    } ;
> +    const char *from_addr = NULL;
> +    unsigned int i;
> +    struct reply_to_map *reply_to_map;
> +
> +    struct reply_to_map reply_to_map_all[] = {
>  	{ "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 }
> +	{ "bcc",        NULL, GMIME_RECIPIENT_TYPE_BCC },
> +	{  NULL,        NULL, 0 }
>      };
> -    const char *from_addr = NULL;
> -    unsigned int i;
> +
> +    /* we try from first and then reply-to */
> +    struct reply_to_map reply_to_map_sender[] = {
> +	{ "from", "reply-to", GMIME_RECIPIENT_TYPE_TO  },
> +	{  NULL,        NULL, 0 }
> +    };

I'm not sure whether an e-mail without From: is a valid e-mail. If not,
you would never use "reply-to" header. Also, given the link below
(http://www.unicom.com/pw/reply-to-harmful.html), this will not behave
as Carl likes :). Finally, don't forget that reply_to_map can be
modified in add_recipients_from_message().

I missed your original patch so I implement this for myself in a less
intrusive way (see
id:1267464588-21050-1-git-send-email-sojkam1@fel.cvut.cz). What I like
at your approach is that --recipient option can have several values. I
think it would be useful to have three possible values of this option:
"all", "sender" and something like "reply-to-or-sender". The latter
option would cause using of Reply-to: header event if it is found as
redundant by reply_to_header_is_redundant(). I find this useful for
several private mailing lists I use.

-Michal
> +
> +    if (reply_options == NOTMUCH_REPLY_SENDER_ONLY) {
> +	reply_to_map = reply_to_map_sender;
> +    } else {
> +	reply_to_map = reply_to_map_all;
> +    }
>  
>      /* Some mailing lists munge the Reply-To header despite it being A Bad
>       * Thing, see http://www.unicom.com/pw/reply-to-harmful.html