Date: Fri, 16 Aug 2013 11:15:25 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v2 2/3] reply: Remove extraneous space from generated
	References
Message-ID: <20130816151525.GH13257@mit.edu>
References: <1376587658-19202-1-git-send-email-amdragon@mit.edu>
	<1376587658-19202-3-git-send-email-amdragon@mit.edu>
	<87k3jl3ehe.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87k3jl3ehe.fsf@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: tomi.ollila@iki.fi, notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Aug 16 at  5:19 pm:
> On Thu, 15 Aug 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > Previously, the References header code seemed to assume
> > notmuch_message_get_header would return NULL if the header was not
> > present, but it actually returns "".  As a result of this, it was
> > inserting an unnecessary space when concatenating an empty or missing
> > original references header with the new reference.
> >
> > This shows up in only two tests because the text reply format later
> > passes the whole reply template through g_mime_filter_headers, which
> > has the side effect of stripping out this extra space.
> > ---
> >  notmuch-reply.c |   14 ++++++++------
> >  test/multipart  |    2 +-
> >  test/reply      |    2 +-
> >  3 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/notmuch-reply.c b/notmuch-reply.c
> > index 3b2b58d..0f3b9cd 100644
> > --- a/notmuch-reply.c
> > +++ b/notmuch-reply.c
> > @@ -537,12 +537,14 @@ create_reply_message(void *ctx,
> >  			      "In-Reply-To", in_reply_to);
> >  
> >      orig_references = notmuch_message_get_header (message, "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);
> > +    if (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);
> > +    }
> 
> If orig_references turned out to be NULL, wouldn't this then fail to add
> a References: header with in_reply_to in it?

That's true.  It's not clear what the best course of action is if
orig_references is NULL, but you're right that failing to produce a
References header altogether is probably not it.  I'll make it treat
errors and missing/empty References headers identically.

> Jani.
> 
> 
> >  
> >      return reply;
> >  }
> > diff --git a/test/multipart b/test/multipart
> > index c974226..2033023 100755
> > --- a/test/multipart
> > +++ b/test/multipart
> > @@ -599,7 +599,7 @@ cat <<EOF >EXPECTED
> >   "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
> >   "To": "Carl Worth <cworth@cworth.org>, cworth@cworth.org",
> >   "In-reply-to": "<87liy5ap00.fsf@yoom.home.cworth.org>",
> > - "References": " <87liy5ap00.fsf@yoom.home.cworth.org>"},
> > + "References": "<87liy5ap00.fsf@yoom.home.cworth.org>"},
> >   "original": {"id": "XXXXX",
> >   "match": false,
> >   "excluded": false,
> > diff --git a/test/reply b/test/reply
> > index c877ffe..a85ebe5 100755
> > --- a/test/reply
> > +++ b/test/reply
> > @@ -242,7 +242,7 @@ test_expect_equal_json "$output" '
> >      "reply-headers": {
> >          "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
> >          "In-reply-to": "<'${gen_msg_id}'>",
> > -        "References": " <'${gen_msg_id}'>",
> > +        "References": "<'${gen_msg_id}'>",
> >          "Subject": "Re: \u00e0\u00df\u00e7",
> >          "To": "\u2603 <snowman@example.com>"
> >      }