Re: [PATCH] generate a valid rfc2047 mail header for multi-line subject.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2011, 20:43 UTC
Message-ID: <7vsjvfby0z.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297670968-28130-1-git-send-email-xiaozhu@gmail.com>

xzer <xiaozhu@gmail.com> writes:

> Subject: Re: [PATCH] generate a valid rfc2047 mail header for multi-line subject.

We prefer to have "[PATCH] subsystem: description without final full-stop" here.

> There is still a problem that git-am will lost the line break.

What does "still" refer to? It is unclear under what condition the command lose "the line break" (nor which line break you are refering to; I am guessing that you have a commit that begins with a multi-line paragraph and you are talking about line breaks between the lines in the first paragraph).

> It's not easy to retain it, but as the first step, we can generate
> a valid rfc2047 header now.

Please describe what is broken (iow, "Given this sample input, we currently generate this output, which is not a valid rfc2047") and what the new output looks like ("Update pp_title_line() to generate this output instead.")

> ---

Missing sign-off with a real name.

Show 16 quoted lines

> diff --git a/pretty.c b/pretty.c
> index 8549934..f18a38d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -249,6 +249,33 @@ needquote:
>  	strbuf_addstr(sb, "?=");
>  }
>  
> +static void add_rfc2047_multiline(struct strbuf *sb, const char *line, int len,
> +                       const char *encoding)
> +{
> +	int first = 1;
> +	char *mline = xmemdupz(line, len);
> +	const char *cline = mline;
> +	int offset = 0, linelen = 0;
> +        for (;;) {

You seem to have indent that uses SPs instead of HT around here...

> +                linelen = get_one_line(cline);

I can see you are trying to be careful not to let get_one_line() overstep past "len" the caller gave you by making a copy first, but is this overhead really necessary? After all we know in this static function that the caller is feeding the contents from a strbuf, which always have a terminating NUL (and that is why it is Ok that get_one_line() is not a counted string interface).

Show 17 quoted lines

> +
> +                cline += linelen;
> +
> +                if (!linelen)
> +                        break;
> +		
> +		if (!first)
> +			strbuf_addf(sb, "\n ");
> +
> +		offset = *(cline -1) == '\n'; 
> +
> +		add_rfc2047(sb, cline-linelen, linelen-offset, encoding);
> +		first = 0;
> +
> +        }
> +	free(mline);
> +}

So the general idea of this change (I am thinking aloud what should be in the updated commit log message as the problem description) is that:

 - We currently give an entire multi-line paragraph string to the
   add_rfc2047() function to be formatted as the title of the commit;

 - The add_rfc2047() functionjust passes "\n" through, without making it a
   folding whitespace followed by a newline, to help callers that want to
   use this function to produce a header line that is rfc 2822 conformant;

 - The patch introduces a new function add_rfc2047_multiline() that splits
   its input and performs line folding for such a caller (namely, the
   pp_title_line() function);

 - Another caller of add_rfc2047(), pp_user_info, is not changed, and it
   won't fold the name of the user that appear on the From: line.

It is unclear if the last point is really the right thing to do, though. It is not a new problem that an author name that has a "\n" in it would break the output, but we probably would want to fix that case too here?