Re: [PATCH] sequencer: support folding in rfc2822 footer

[Jeff King]

On Tue, Sep 06, 2016 at 04:30:24PM -0700, Jonathan Tan wrote:

> On 09/06/2016 03:08 PM, Jonathan Tan wrote:
> > On 09/02/2016 07:23 PM, Junio C Hamano wrote:
> > > A slightly related tangent.  An unconditionally good change you
> > > could make is to allow folding of in-body headers.  I.e. you can
> > > have e.g.
> > > 
> > > -- >8 --
> > > Subject: [PATCH] sequencer: support in-body headers that are
> > >  folded according to RFC2822 rules
> > > 
> > > The first paragraph after the above long title begins
> > > here...
> > > 
> > > in the body of the msssage, and I _think_ we do not fold it properly
> > > when applying such a patch.  We should, as that is something that
> > > appears in format-patch output (i.e. something Git itself produces,
> > > unlike the folded "footer").
> > 
> > OK, I'll take a look at this.
> 
> It turns out that Git seems to already do this, at least for Subject.

Right, because "Subject" is actually a real RFC 2822 header in the
generated email message. Not only do we expect things like mail readers
to handle this, but we _have_ to wrap at a certain point to meet the
standard[1].

I don't think any part of Git ever shunts "Subject" to an in-body
header, though I'd guess people do it manually all the time. 

> $ git format-patch HEAD^
> 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch
> $ cat 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch
> 
> Subject: [PATCH] this is a very long subject to test line wrapping this is a
>  very long subject to test line wrapping
> 

So the interesting bit is what happens with:

  git checkout master^
  git am 0001-*

and with:

  perl -lpe '
# Bump subject down to in-body header.
if (/^Subject:/) {
print "Subject: real subject";
print "";
}
  ' 0001-* >patch
  git checkout master^
  git am patch

It looks like we get the first one right, but not the second.

-Peff

[1] A careful reader may note that arbitrarily-long body lines,
including in-body headers and footers, may _also_ run afoul of
the body line-length limits. The "right" solution there is
probably quoted-printable, but it's ugly enough that I wouldn't do
so unless we see a real-world case where the line lengths are a
problem.

Re: [PATCH] sequencer: support folding in rfc2822 footer

[Jonathan Tan]

On 09/06/2016 03:08 PM, Jonathan Tan wrote:

On 09/02/2016 07:23 PM, Junio C Hamano wrote:

A slightly related tangent.  An unconditionally good change you
could make is to allow folding of in-body headers.  I.e. you can
have e.g.

-- >8 --
Subject: [PATCH] sequencer: support in-body headers that are
 folded according to RFC2822 rules

The first paragraph after the above long title begins
here...

in the body of the msssage, and I _think_ we do not fold it properly
when applying such a patch.  We should, as that is something that
appears in format-patch output (i.e. something Git itself produces,
unlike the folded "footer").


OK, I'll take a look at this.


It turns out that Git seems to already do this, at least for Subject. 
Transcript below:


$ echo one > file.txt
$ git add file.txt
$ git commit -m x
[master (root-commit) 2389483] x
 1 file changed, 1 insertion(+)
 create mode 100644 file.txt
$ echo two > file.txt
$ git commit -am 'this is a very long subject to test line wrapping this 
is a very long subject to test line wrapping'
[master ca86792] this is a very long subject to test line wrapping this 
is a very long subject to test line wrapping

 1 file changed, 1 insertion(+), 1 deletion(-)
$ git format-patch HEAD^
0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch
$ cat 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch

Subject: [PATCH] this is a very long subject to test line wrapping this is a
 very long subject to test line wrapping

Re: [PATCH] sequencer: support folding in rfc2822 footer

[Jonathan Tan]

On 09/02/2016 07:23 PM, Junio C Hamano wrote:

Jonathan Tan  writes:


Sample-field: multiple-line field body
 that causes a blank line below


I am not sure this is unconditionally good, or may cause problems to
those with workflows you did not consider when you wrote this patch.

Not being too lenient here historically has been a deliberate
decision to avoid misidentification of non "footers".  Does Git
itself produce some folded footer line?  If Git itself produced such
folded lines, I'd be a lot more receptive to this change, but I do
not think that is the case here.


I don't think Git produces any folded lines, but folded lines do appear 
in footers in projects that use Git. For example, some Android commits 
have multi-line "Test:" fields (example, [1]) and some Linux commits 
have multi-line "Tested-by:" fields (example, [2]).


Taking the Android commit as an example, this would mean that 
cherrypicking that commit would create a whole new footer, and tripping 
up tools (for example, Gerrit, which looks for "Change-Id:" in the last 
paragraph). But this would not happen if "Test:" was single-line instead 
of multi-line - which seems inconsistent.


[1] 
https://android.googlesource.com/platform/frameworks/base/+/4c5281862f750cbc9d7355a07ef1a5545b9b3523
[2] 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/69f92f67b68ab7028ffe15f0eea76b59f8859383



A slightly related tangent.  An unconditionally good change you
could make is to allow folding of in-body headers.  I.e. you can
have e.g.

-- >8 --
Subject: [PATCH] sequencer: support in-body headers that are
 folded according to RFC2822 rules

The first paragraph after the above long title begins
here...

in the body of the msssage, and I _think_ we do not fold it properly
when applying such a patch.  We should, as that is something that
appears in format-patch output (i.e. something Git itself produces,
unlike the folded "footer").


OK, I'll take a look at this.

Re: [PATCH] sequencer: support folding in rfc2822 footer

[Junio C Hamano]

Jonathan Tan  writes:

> Sample-field: multiple-line field body
>  that causes a blank line below

I am not sure this is unconditionally good, or may cause problems to
those with workflows you did not consider when you wrote this patch.

Not being too lenient here historically has been a deliberate
decision to avoid misidentification of non "footers".  Does Git
itself produce some folded footer line?  If Git itself produced such
folded lines, I'd be a lot more receptive to this change, but I do
not think that is the case here.

A slightly related tangent.  An unconditionally good change you
could make is to allow folding of in-body headers.  I.e. you can
have e.g.

-- >8 --
Subject: [PATCH] sequencer: support in-body headers that are
 folded according to RFC2822 rules

The first paragraph after the above long title begins
here...

in the body of the msssage, and I _think_ we do not fold it properly
when applying such a patch.  We should, as that is something that
appears in format-patch output (i.e. something Git itself produces,
unlike the folded "footer").

[PATCH] sequencer: support folding in rfc2822 footer

[Jonathan Tan]

When running `git cherry-pick -x`, a blank line would be inserted if a line in
the footer was broken into multiple lines (which is inconsistent with its
behavior if no lines were broken). For example, this command would produce:

---
Sample-field: no blank line below
(cherry picked from commit ...)
---

but would also produce:

---
Sample-field: multiple-line field body
 that causes a blank line below

(cherry picked from commit ...)
---

This, among other things, trips up tools that look for the last paragraph
(including sequencer.c itself).

RFC 2822 allows field bodies to be split into multiple lines, especially (but
not only) to deal with the line-width limitations described in the
specification, referring to this as "folding".

Teach sequencer.c to treat split and unsplit field bodies in the same way (that
is, to not include the blank line).

Signed-off-by: Jonathan Tan 
---
 sequencer.c  | 19 +--
 t/t3511-cherry-pick-x.sh | 19 +++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3804fa9..411dd50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -26,10 +26,20 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
 static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
 static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
 
-static int is_rfc2822_line(const char *buf, int len)
+static int is_rfc2822_line(const char *buf, int len,
+  int allow_folding_continuation)
 {
int i;
 
+   /*
+* Section 2.2.3 of RFC 2822 allows field bodies to continue onto
+* multiple lines, referred to as "folding". Such continuation lines
+* start with whitespace.
+*/
+   if (allow_folding_continuation)
+   if (len && (buf[0] == ' ' || buf[0] == '\t'))
+   return 1;
+
for (i = 0; i < len; i++) {
int ch = buf[i];
if (ch == ':')
@@ -64,6 +74,7 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
int len = sb->len - ignore_footer;
const char *buf = sb->buf;
int found_sob = 0;
+   int allow_folding_continuation = 0;
 
/* footer must end with newline */
if (!len || buf[len - 1] != '\n')
@@ -92,7 +103,11 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
; /* do nothing */
k++;
 
-   found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
+   found_rfc2822 = is_rfc2822_line(buf + i,
+   k - i - 1,
+   allow_folding_continuation);
+   allow_folding_continuation = 1;
+
if (found_rfc2822 && sob &&
!strncmp(buf + i, sob->buf, sob->len))
found_sob = k;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..1a50339 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -36,6 +36,11 @@ mesg_with_cherry_footer="$mesg_with_footer_sob
 (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
 Tested-by: C.U. Thor "
 
+mesg_with_folding_footer="$mesg_no_footer
+
+Field: This is a very long field body
+ that is continued onto another line"
+
 mesg_unclean="$mesg_one_line
 
 
@@ -68,6 +73,8 @@ test_expect_success setup '
git reset --hard initial &&
test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
git reset --hard initial &&
+   test_commit "$mesg_with_folding_footer" foo b mesg-with-folding-footer 
&&
+   git reset --hard initial &&
test_config commit.cleanup verbatim &&
test_commit "$mesg_unclean" foo b mesg-unclean &&
test_unconfig commit.cleanup &&
@@ -234,6 +241,18 @@ test_expect_success 'cherry-pick -x -s treats "(cherry 
picked from..." line as p
test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x does not insert blank line when folding 
footer is found' '
+   pristine_detach initial &&
+   sha1=$(git rev-parse mesg-with-folding-footer^0) &&
+   git cherry-pick -x mesg-with-folding-footer &&
+   cat <<-EOF >expect &&
+   $mesg_with_folding_footer
+   (cherry picked from commit $sha1)
+   EOF
+   git log -1 --pretty=format:%B >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick preserves commit message' '
pristine_detach initial &&
printf "$mesg_unclean" >expect &&
-- 
2.8.0.rc3.226.g39d4020