From: Jani Nikula <jani@nikula.org>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH 2/2] cli: clean up user address matching code in
	guess_from_received_header()
In-Reply-To: <20120512154746.GK11804@mit.edu>
References:
 <bae496e6100a4e4d4a598dcf5c73d67b178f4128.1336746187.git.jani@nikula.org>
	<cd73ec6368b3743b0d84ed1696bfa00960124645.1336746187.git.jani@nikula.org>
	<20120512154746.GK11804@mit.edu>
User-Agent: Notmuch/0.12+169~g45438b0 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Sat, 12 May 2012 20:23:16 +0300
Message-ID: <87k40h1fgb.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: notmuch@notmuchmail.org
Precedence: list

On Sat, 12 May 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Series LGTM.  This is a nice cleanup.

Thanks for the review.

> My one comment is that it seems like we should be stricter about
> matching email address lists, since a naive substring match could
> yield strange results.  That's not something that should be changed in
> this patch, though.

Agreed. Especially the Received: header matching should be scrutinized,
as *all* the Received: headers in a message are merged into one by
notmuch_message_file_get_header(). But that should be another patch,
another time.

> Does the context of this patch conflict with
> id:"21a946917c5c8dd63295b7c87b7c2d1ebcb6e71e.1336746160.git.jani@nikula.org"
> ?  It seems like both have context that's changed by the other patch.

Ooops, you're right. The two series do *merge* on top of each other in
either order, but as patches they apply only on master. I wanted to keep
the series separate. Once either gets pushed, I'll rebase the other.

BR,
Jani.

>
> Quoth Jani Nikula on May 11 at  5:33 pm:
>> Get rid of user address matching code duplication in
>> guess_from_received_header() by using the new address matching
>> helpers.
>> 
>> No functional changes.
>> 
>> Signed-off-by: Jani Nikula <jani@nikula.org>
>> ---
>>  notmuch-reply.c |   64 +++++++++++++++++--------------------------------------
>>  1 file changed, 19 insertions(+), 45 deletions(-)
>> 
>> diff --git a/notmuch-reply.c b/notmuch-reply.c
>> index 0c82755..51cb6de 100644
>> --- a/notmuch-reply.c
>> +++ b/notmuch-reply.c
>> @@ -377,20 +377,15 @@ add_recipients_from_message (GMimeMessage *reply,
>>  static const char *
>>  guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message)
>>  {
>> -    const char *received,*primary,*by;
>> -    const char **other;
>> -    char *tohdr;
>> +    const char *addr, *received, *by;
>>      char *mta,*ptr,*token;
>>      char *domain=NULL;
>>      char *tld=NULL;
>>      const char *delim=". \t";
>> -    size_t i,j,other_len;
>> +    size_t i;
>>  
>>      const char *to_headers[] = {"Envelope-to", "X-Original-To"};
>>  
>> -    primary = notmuch_config_get_user_primary_email (config);
>> -    other = notmuch_config_get_user_other_email (config, &other_len);
>> -
>>      /* sadly, there is no standard way to find out to which email
>>       * address a mail was delivered - what is in the headers depends
>>       * on the MTAs used along the way. So we are trying a number of
>> @@ -405,23 +400,13 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message
>>       *    'by' part of Received headers
>>       * If none of these work, we give up and return NULL
>>       */
>> -    for (i = 0; i < sizeof(to_headers)/sizeof(*to_headers); i++) {
>> -	tohdr = xstrdup(notmuch_message_get_header (message, to_headers[i]));
>> -	if (tohdr && *tohdr) {
>> -	    /* tohdr is potentialy a list of email addresses, so here we
>> -	     * check if one of the email addresses is a substring of tohdr
>> -	     */
>> -	    if (strcasestr(tohdr, primary)) {
>> -		free(tohdr);
>> -		return primary;
>> -	    }
>> -	    for (j = 0; j < other_len; j++)
>> -		if (strcasestr (tohdr, other[j])) {
>> -		    free(tohdr);
>> -		    return other[j];
>> -		}
>> -	    free(tohdr);
>> -	}
>> +    for (i = 0; i < ARRAY_SIZE (to_headers); i++) {
>> +	const char *tohdr = notmuch_message_get_header (message, to_headers[i]);
>> +
>> +	/* Note: tohdr potentially contains a list of email addresses. */
>> +	addr = user_address_in_string (tohdr, config);
>> +	if (addr)
>> +	    return addr;
>>      }
>>  
>>      /* We get the concatenated Received: headers and search from the
>> @@ -439,19 +424,12 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message
>>       * header
>>       */
>>      ptr = strstr (received, " for ");
>> -    if (ptr) {
>> -	/* the text following is potentialy a list of email addresses,
>> -	 * so again we check if one of the email addresses is a
>> -	 * substring of ptr
>> -	 */
>> -	if (strcasestr(ptr, primary)) {
>> -	    return primary;
>> -	}
>> -	for (i = 0; i < other_len; i++)
>> -	    if (strcasestr (ptr, other[i])) {
>> -		return other[i];
>> -	    }
>> -    }
>> +
>> +    /* Note: ptr potentially contains a list of email addresses. */
>> +    addr = user_address_in_string (ptr, config);
>> +    if (addr)
>> +	return addr;
>> +
>>      /* Finally, we parse all the " by MTA ..." headers to guess the
>>       * email address that this was originally delivered to.
>>       * We extract just the MTA here by removing leading whitespace and
>> @@ -492,15 +470,11 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message
>>  	     */
>>  	    *(tld-1) = '.';
>>  
>> -	    if (strcasestr(primary, domain)) {
>> -		free(mta);
>> -		return primary;
>> +	    addr = string_in_user_address (domain, config);
>> +	    if (addr) {
>> +		free (mta);
>> +		return addr;
>>  	    }
>> -	    for (i = 0; i < other_len; i++)
>> -		if (strcasestr (other[i],domain)) {
>> -		    free(mta);
>> -		    return other[i];
>> -		}
>>  	}
>>  	free (mta);
>>      }