From: Junio C Hamano <hidden> Date: 2016-06-15 22:50:38
xzer [off-list ref] 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.")
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).
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?
From: Jeff King <hidden> Date: 2016-06-15 22:50:38
On Tue, Feb 22, 2011 at 12:43:40PM -0800, Junio C Hamano wrote:
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?
Yeah, I think the best path forward is:
1. Stop feeding "pre-folded" subject lines to the email formatter.
Give it the regular subject line with no newlines.
2. rfc2047 encoding should encode a literal newline. Which should
generally never happen, but is probably the most sane thing to do
if it does.
3. rfc2047 should fold all lines at some sane length. As it is now, we
may sometimes generate long lines in headers (though in practice, I
doubt this is much of a problem).
I started to work on this, but got stuck on (3). Our existing wrap
functions want NUL-terminated strings, and we are operating on a
substring. I tried converting the wrap functions to handle lengths, but
it got way uglier than I had hoped. I think just strdup'ing the subject
temporarily is probably fine, though. Let me see what I can come up
with.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:38
On Wed, Feb 23, 2011 at 03:08:54AM -0500, Jeff King wrote:
Yeah, I think the best path forward is:
1. Stop feeding "pre-folded" subject lines to the email formatter.
Give it the regular subject line with no newlines.
2. rfc2047 encoding should encode a literal newline. Which should
generally never happen, but is probably the most sane thing to do
if it does.
3. rfc2047 should fold all lines at some sane length. As it is now, we
may sometimes generate long lines in headers (though in practice, I
doubt this is much of a problem).
So here is a series that does this. It still doesn't preserve subject
newlines in "format-patch | am", but I don't think that was ever a goal
of the code. If we want to add it as an optional feature on top (maybe
as part of "-k"?), it should be easy to do (since the rfc2047 encoding
will now preserve embedded newlines).
[1/3]: strbuf: add fixed-length version of add_wrapped_text
[2/3]: format-patch: wrap long header lines
[3/3]: format-patch: rfc2047-encode newlines in headers
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:38
The function strbuf_add_wrapped_text takes a NUL-terminated
string. This makes it annoying to wrap strings we have as a
pointer and a length.
Refactoring strbuf_add_wrapped_text and all of its
sub-functions to handle fixed-length strings turned out to
be really ugly. So this implementation is lame; it just
strdups the text and operates on the NUL-terminated version.
This should be fine as the strings we are wrapping are
generally pretty short. If it becomes a problem, we can
optimize later.
Signed-off-by: Jeff King <redacted>
---
utf8.c | 9 +++++++++
utf8.h | 2 ++
2 files changed, 11 insertions(+), 0 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:50:38
Subject and identity headers may be arbitrarily long. In the
past, we just assumed that single-line headers would be
reasonably short. For multi-line subjects that we squish
into a single line, we just "pre-folded" the data in
pp_title_line by adding a newline and indentation.
There were two problems. One is that, although rare,
single-line messages can actually be longer than the
recommended line-length limits. The second is that the
pre-folding interacted badly with rfc2047 encoding, leading
to malformed headers.
Instead, let's stop pre-folding the subject lines, and just
fold everything based on length in add_rfc2047, whether
it is encoded or not.
Signed-off-by: Jeff King <redacted>
---
Three things to note:
1. We call strbuf_add_wrapped_bytes for the non-encoded case. This
nicely wraps on word and multi-character boundaries. But it will
never do a "hard" wrap if there are no word boundaries, and it
probably should at the 998-character mark which rfc2822 specifies
as a hard limit.
I don't know how much we care. For something like that you'd have
to be maliciously trying to create a bogus patch. If you're mailing
it, you could just create the bogus mail by hand. If you're trying
to buffer overflow somebody's "format-patch | am" script, it won't
do anything, as mailinfo does not have a limit on line length.
2. For the non-quoted case, technically we want to indent less on the
first line than we do on subsequent lines (to account for "Subject:
[PATCH]"). strbuf_add_wrapped_bytes doesn't support that notion. We
could add it, but it probably doesn't matter. We just end up
wrapping the subsequent lines a little tighter than we need to.
3. I used RFC2822's SHOULD value of 78 characters as a line length.
That's probably unnecessarily conservative. In theory wrapping
shouldn't make a difference to the data, but maybe people who
hand-edit the result would prefer not to see wrapping? I dunno. In
that case, we could set it to something higher like 120, which
would still wrap the really ridiculous cases.
pretty.c | 32 +++++++++++++----
t/t4014-format-patch.sh | 84 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 108 insertions(+), 8 deletions(-)
@@ -216,7 +216,15 @@ static int is_rfc2047_special(char ch)staticvoidadd_rfc2047(structstrbuf*sb,constchar*line,intlen,constchar*encoding){-inti,last;+staticconstintmax_length=78;/* per rfc2822 */+inti;+intline_len;++/* How many bytes are already used on the current line? */+for(i=sb->len-1;i>=0;i--)+if(sb->buf[i]=='\n')+break;+line_len=sb->len-(i+1);for(i=0;i<len;i++){intch=line[i];
@@ -225,14 +233,21 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,if((i+1<len)&&(ch=='='&&line[i+1]=='?'))gotoneedquote;}-strbuf_add(sb,line,len);+strbuf_add_wrapped_bytes(sb,line,len,0,1,max_length-line_len);return;needquote:strbuf_grow(sb,len*3+strlen(encoding)+100);strbuf_addf(sb,"=?%s?q?",encoding);-for(i=last=0;i<len;i++){+line_len+=strlen(encoding)+5;/* 5 for =??q? */+for(i=0;i<len;i++){unsignedch=line[i]&0xFF;++if(line_len>=max_length-2){+strbuf_addf(sb,"?=\n =?%s?q?",encoding);+line_len=strlen(encoding)+5+1;/* =??q? plus SP */+}+/**Weencode' 'using'=20'eventhoughrfc2047*allowsusing'_'forreadability.Unfortunately,
From: Jeff King <hidden> Date: 2016-06-15 22:50:38
These should generally never happen, as we already
concatenate multiples in subjects into a single line. But
let's be defensive, since not encoding them means we will
output malformed headers.
Signed-off-by: Jeff King <redacted>
---
pretty.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
On Wed, Feb 23, 2011 at 03:08:54AM -0500, Jeff King wrote:
quoted
Yeah, I think the best path forward is:
1. Stop feeding "pre-folded" subject lines to the email formatter.
Give it the regular subject line with no newlines.
2. rfc2047 encoding should encode a literal newline. Which should
generally never happen, but is probably the most sane thing to do
if it does.
3. rfc2047 should fold all lines at some sane length. As it is now, we
may sometimes generate long lines in headers (though in practice, I
doubt this is much of a problem).
So here is a series that does this. It still doesn't preserve subject
newlines in "format-patch | am", but I don't think that was ever a goal
of the code. If we want to add it as an optional feature on top (maybe
as part of "-k"?), it should be easy to do (since the rfc2047 encoding
will now preserve embedded newlines).
[1/3]: strbuf: add fixed-length version of add_wrapped_text
[2/3]: format-patch: wrap long header lines
[3/3]: format-patch: rfc2047-encode newlines in headers
-Peff
To the first point, I really want to find a way that we can remain the
line breaker
after import a formatted patch. That's why I add a new function to product multi
line header, I want to do something which is special to subject. In my usage,
I told my men every day that don't write too long in the first
paragraph, but there
are always somebody who forgets it, then I will get a patch with a
very long subject
just like a nightmare(yes, I gave them my temporary fix which I submitted here,
so they can write as long as they want).
So I want to know whether we can generate a 2047 compatible header so
that mailer
can catch it correctly and the git-am can import it with line breaker
correctly too.
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.
quoted
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).
Yes, that is what I am refering, the line breaks in the first paragraph.
quoted
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.")
At present we can only concatenate the lines in the first paragraph so
that we can generate a valid rfc2047 mail, but we will lost the line breaks
after import the patch by git-am.
quoted
---
Missing sign-off with a real name.
I am sorry that I didn't find the document of submitting a patch until
yesterday, Thanks for your comment.
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...
quoted
+ 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).
I am not sure that who will call this function in future, I think since there is
a argument as len, so I'd better to obey the function declare.
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?
Your comment is just right for what I tried to do, I explained why I add a new
function for subject specially in the mail which replied to Jeff, I
want to remain
the line breaks after import the patch, so I think I need do something here
in future, it will be compatible with rfc2047 and also can be imported with
line breaks correctly. I don't know how yet, so I just want to left a
possibility.
So I introduce a new function for subject only.
xzer
From: Jeff King <hidden> Date: 2016-06-15 22:50:38
On Thu, Feb 24, 2011 at 12:16:04AM +0900, xzer wrote:
To the first point, I really want to find a way that we can remain the
line breaker
after import a formatted patch. That's why I add a new function to product multi
line header, I want to do something which is special to subject. In my usage,
I told my men every day that don't write too long in the first
paragraph, but there
are always somebody who forgets it, then I will get a patch with a
very long subject
just like a nightmare(yes, I gave them my temporary fix which I submitted here,
so they can write as long as they want).
So I want to know whether we can generate a 2047 compatible header so
that mailer
can catch it correctly and the git-am can import it with line breaker
correctly too.
Yes. With my patches, if you feed a subject with linebreaks to
add_rfc2047, they will be encoded. So you just need an extra patch on
top of mine that will use straight linebreaks (_not_ linebreaks with an
extra space) in pp_title_line. Below is a quick and dirty patch to do
that when "-k" is specified. You will also need to specify "-k" with
applying it with "git am", but other than that it seems to work.
However, I'm still not sure it's a good idea. Other parts of git will
try to treat your paragraph as a single line (e.g., git log --oneline).
Plus this patch is ugly because of the number of layers of abstraction
we have to pass the keep-subject through. I'm not sure there's a good
way around that.
---
@@ -1130,6 +1130,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)die("-n and -k are mutually exclusive.");if(keep_subject&&subject_prefix)die("--subject-prefix and -k are mutually exclusive.");+rev.preserve_subject=keep_subject;argc=setup_revisions(argc,argv,&rev,&s_r_opt);if(argc>1)
@@ -1254,7 +1255,8 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,/* These formats treat the title line specially. */if(fmt==CMIT_FMT_ONELINE||fmt==CMIT_FMT_EMAIL)pp_title_line(fmt,&msg,sb,context->subject,-context->after_subject,encoding,need_8bit_cte);+context->after_subject,encoding,need_8bit_cte,+context->preserve_subject);beginning_of_body=sb->len;if(fmt!=CMIT_FMT_ONELINE)