git-mailinfo doesn't stop parsing at the end of the header
Hello,
today, after working on a topic branch and trying to rebase it on top of the updated master, the rebase failed, complaining about an invalid email address.
Some investigating revealed an interesting quirk in git-mailinfo which seems to be a bit too eager to extract author information: Instead of just looking at the From:-Line in a mails header (git-rebase seems to use git-am which in turn uses git-mailinfo), it searches for "from:" *anywhere* in the mail and uses the last found information as the source for the author information.
In this case, git-format-patch has generated a file that looks something like this:
--------------8<---------------
From d28f21ea8ca64681ba7756417799ceea81ad6873 Mon Sep 17 00:00:00 2001
From: Foo Bar <[email protected]>
Date: Tue, 17 Nov 2009 15:27:25 +0100
Subject: blah, blah, blah
from:
- this is a
- list for stuff
--- list/of/changed/files | 1 - list/of/changed/files2 | 1 - 2 files changed, 0 insertions(+), 2 deletions(-)
the actual diff down here
--------------8<---------------
And when you feed this into mailinfo, this is what you get:
pilif@celes ~/git % git mailinfo /dev/null /dev/null < somepatch.patch Author:
Email:
Subject: blah, blah, blah
Date: Tue, 17 Nov 2009 15:27:25 +0100
pilif@celes ~/git %
and consequently, anything that depends on the correct author being extracted then fails.
While I know it's rude to have a line beginning with "from:" (and it's even ruder to have a line beginning with "from "), IMHO the header ends at the first blank line and I see no reason to extract author information past the header.
And if this is in fact intended behavior, it should probably not be permitted to create a commit that later on can't be rebased or applied using git-am.
I had a look at the source of git-mailinfo to fix it myself, but this thing does too much for my minimal knowledge in C.
Philip
Signature · 4 lines
Sensational AG Giesshübelstrasse 62c, Postfach 1966, 8021 Zürich Tel. +41 43 544 09 60, Mobile +41 79 341 01 99 [email protected], http://www.sensational.ch
Quoted 6 lines · Philip Hofstetter
Some investigating revealed an interesting quirk in git-mailinfo which seems to be a bit too eager to extract author information: Instead of just looking at the From:-Line in a mails header (git-rebase seems to use git-am which in turn uses git-mailinfo), it searches for "from:" *anywhere* in the mail and uses the last found information as the source for the author information.
It is not quite "anywhere"; extra headers are respected at the very top of the message body. This is intentional, to allow one to indicate that a patch you are sending was authored by somebody else.
So the problem is slightly less severe; the body of your commit message has to _start_ with "From:". Still, it is awfully ugly to hit a parsing ambiguity like this when you are trying to do something as simple as rebase.
Some solutions I can think of are:
1. Improve the header-finding heuristic to actually look for something more sane, like "From:.*<.*@.*>" (I don't recall off the top of my head which other headers we handle in this position. Probably Date, too).
2. Give mailinfo a "--strict" mode to indicate that it is directly parsing the output of format-patch, and not some random email. Use --strict when invoking "git am" via "git rebase".
Quoted 4 lines
While I know it's rude to have a line beginning with "from:" (and it's even ruder to have a line beginning with "from "), IMHO the header ends at the first blank line and I see no reason to extract author information past the header.
As I explained above, there is a reason, but I don't think it's rude to have either of those lines. You were, after all, writing a commit message, not an email (and even if you were, it is a failure of the storage format if it can't represent your data correctly). So I think git is to blame here.
-Peff
Quoted 7 lines · Jeff King
So the problem is slightly less severe; the body of your commit message has to _start_ with "From:". Still, it is awfully ugly to hit a parsing ambiguity like this when you are trying to do something as simple as rebase.
Some solutions I can think of are:
Quoted 8 lines
1. Improve the header-finding heuristic to actually look for something more sane, like "From:.*<.*@.*>" (I don't recall off the top of my head which other headers we handle in this position. Probably Date, too).
2. Give mailinfo a "--strict" mode to indicate that it is directly parsing the output of format-patch, and not some random email. Use --strict when invoking "git am" via "git rebase".
Solution (2) seemed like a lot of work, so here is the relatively small solution (1). I think looking for <.*@.*> is too restrictive, as people may be using:
From: [email protected]
which should remain valid. I just look for an '@' instead.
Note that this validation also applies to actual headers. Should we turn it off for them? As it is, it breaks t3400, which uses a bogus email address. I suppose we should probably preserve such bogosities if they are in the official headers.
builtin-mailinfo.c +15 -2
@@ -275,6 +275,17 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr) line->buf[len] ==':' &&isspace (line->buf[len +1 ]); } +static int validate_header (const char *header,const struct strbuf *data) +{ +if (!strcmp (header,"From" )) +return !!strchr (data->buf,'@' ); +if (!strcmp (header,"Date" )) { +char buf[50 ]; +return parse_date (data->buf, buf,sizeof (buf)) >=0 ; + } +return 1 ; +} +static int check_header (const struct strbuf *line,struct strbuf *hdr_data[],int overwrite) {
@@ -289,8 +300,10 @@ static int check_header(const struct strbuf *line,*/ strbuf_add (&sb, line->buf + len +2 , line->len - len -2 );decode_header (&sb); -handle_header (&hdr_data[i], &sb); - ret =1 ; +if (validate_header (header[i], &sb)) { + ret =1 ; +handle_header (&hdr_data[i], &sb); + } goto check_header_out; } }
t/t5100-mailinfo.sh +1 -1
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \ 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last && last=`cat last` && echo total is $last && - test `cat last` = 14' + test `cat last` = 16' check_mailinfo () { mail=$1 opt=$2
t/t5100/info0015 new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/info0016 new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/msg0015 new file +3 -0
@@ -0,0 +1,3 @@ +From: bogosity + - a list + - of stuff
t/t5100/msg0016 new file +4 -0
@@ -0,0 +1,4 @@ +Date: bogus + +and some content +
t/t5100/patch0015 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0016 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/sample.mbox +33 -0
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644 convert_to_utf8(line, charset.buf); -- 1.6.4.1 +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +From: bogosity + - a list + - of stuff +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content + +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +Date: bogus + +and some content + +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
PATCH 1/1 git am/mailinfo: Don't look at in-body headers when rebasing
When we are rebasing we know that the header lines in the patch are good and that we don't need to pick up any headers from the body of the patch.
This makes it possible to rebase commits whose commit message start with "From" or "Date".
Test vectors by Jeff King.
---
Quoted 7 lines · Jeff King
Quoted 7 lines
Some solutions I can think of are:
1. Improve the header-finding heuristic to actually look for something more sane, like "From:.*<.*@.*>" (I don't recall off the top of my head which other headers we handle in this position. Probably Date, too).
Quoted 8 lines
Quoted 3 lines
2. Give mailinfo a "--strict" mode to indicate that it is directly parsing the output of format-patch, and not some random email. Use --strict when invoking "git am" via "git rebase".
Solution (2) seemed like a lot of work, so here is the relatively small solution (1). I think looking for <.*@.*> is too restrictive, as people may be using:
This is an implementation of solution (2). Not much work, but I might have missed something. git-mailinfo usally breaks when I touch it, but the testsuite passes with this patch, including the extra test vectors from Jeff.
The actual change is that mailinfo doesn't look for in-body headers at all if --no-inbody-headers is passed. git-am now passes this option to mailinfo when rebasing.
This won't handle the case when a "bad" patch is passed to git-am from somewhere else than git rebase.
/Lukas
builtin-mailinfo.c | 5 +++++ git-am.sh | 13 ++++++++++--- t/t5100-mailinfo.sh | 6 +++++- t/t5100/info0015 | 5 +++++ t/t5100/info0015--no-inbody-headers | 5 +++++ t/t5100/info0016 | 5 +++++ t/t5100/info0016--no-inbody-headers | 5 +++++ t/t5100/msg0015 | 2 ++ t/t5100/msg0015--no-inbody-headers | 3 +++ t/t5100/msg0016 | 2 ++ t/t5100/msg0016--no-inbody-headers | 4 ++++ t/t5100/patch0015 | 8 ++++++++ t/t5100/patch0015--no-inbody-headers | 8 ++++++++ t/t5100/patch0016 | 8 ++++++++ t/t5100/patch0016--no-inbody-headers | 8 ++++++++ t/t5100/sample.mbox | 33 +++++++++++++++++++++++++++++++++ 16 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 t/t5100/info0015 create mode 100644 t/t5100/info0015--no-inbody-headers create mode 100644 t/t5100/info0016 create mode 100644 t/t5100/info0016--no-inbody-headers create mode 100644 t/t5100/msg0015 create mode 100644 t/t5100/msg0015--no-inbody-headers create mode 100644 t/t5100/msg0016 create mode 100644 t/t5100/msg0016--no-inbody-headers create mode 100644 t/t5100/patch0015 create mode 100644 t/t5100/patch0015--no-inbody-headers create mode 100644 t/t5100/patch0016 create mode 100644 t/t5100/patch0016--no-inbody-headers
builtin-mailinfo.c +3 -0
@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;static int patch_lines;static struct strbuf **p_hdr_data, **s_hdr_data;static int use_scissors; +static int use_inbody_headers =1 ;#define MAX_HDR_PARSED 10#define MAX_BOUNDARIES 5
@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)return 0 ;if (still_looking) { +if (!use_inbody_headers) + still_looking =0 ;strbuf_ltrim (line);if (!line->len)return 0 ;
@@ -1033,6 +1036,8 @@ int cmd_mailinfo(int argc, const char **argv,
const char *prefix)
use_scissors = 1;
else if (!strcmp(argv[1], "--no-scissors"))
use_scissors = 0;
+ else if (!strcmp(argv[1], "--no-inbody-headers"))
+ use_inbody_headers = 0;
else
usage(mailinfo_usage);
argc--; argv++;
git-am.sh +10 -3
@@ -289,7 +289,7 @@ split_patches () { prec=4 dotest="$GIT_DIR/rebase-apply" sign= utf8=t keep= skip= interactive= resolved= rebasing= abort= -resolvemsg= resume= scissors= +resolvemsg= resume= scissors= no_inbody_headers= git_apply_opt= committer_date_is_author_date= ignore_date=
@@ -322,7 +322,7 @@ do --abort) abort=t ;; --rebasing) - rebasing=t threeway=t keep=t scissors=f ;; + rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;; -d|--dotest) die "-d option is no longer supported. Do not use." ;;
@@ -448,6 +448,7 @@ else echo "$utf8" >"$dotest/utf8" echo "$keep" >"$dotest/keep" echo "$scissors" >"$dotest/scissors" + echo "$no_inbody_headers" >"$dotest/no_inbody_headers" echo "$GIT_QUIET" >"$dotest/quiet" echo 1 >"$dotest/next" if test -n "$rebasing"
@@ -495,6 +496,12 @@ t) f) scissors=--no-scissors ;; esac +if test "$(cat "$dotest/no_inbody_headers")" = t +then + no_inbody_headers=--no-inbody-headers +else + no_inbody_headers= +fi if test "$(cat "$dotest/quiet")" = t then GIT_QUIET=t
@@ -549,7 +556,7 @@ do # by the user, or the user can tell us to do so by --resolved flag. case "$resume" in '') - git mailinfo $keep $scissors $utf8 "$dotest/msg" "$dotest/patch" \ + git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \ <"$dotest/$msgnum" >"$dotest/info" || stop_here $this
t/t5100-mailinfo.sh +5 -1
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \ 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last && last=`cat last` && echo total is $last && - test `cat last` = 14' + test `cat last` = 16' check_mailinfo () { mail=$1 opt=$2
@@ -30,6 +30,10 @@ do if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors then check_mailinfo $mail --scissors + fi && + if test -f "$TEST_DIRECTORY"/t5100/msg$mail--use-first-header + then + check_mailinfo $mail --no-inbody-headers fi ' done
t/t5100/info0015 new file +5 -0
@@ -0,0 +1,5 @@ +Author: +Email: +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
(quoted diff) +0 -0
b/t/t5100/info0015--no-inbody-headers
new file mode 100644
index 0000000..c4d8d77
t/t5100/info0015--no-inbody-headers new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/info0016 new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: bogus +
(quoted diff) +0 -0
b/t/t5100/info0016--no-inbody-headers
new file mode 100644
index 0000000..f4857d4
t/t5100/info0016--no-inbody-headers new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/msg0015 new file +2 -0
@@ -0,0 +1,2 @@ +- a list + - of stuff
(quoted diff) +0 -0
b/t/t5100/msg0015--no-inbody-headers
new file mode 100644
index 0000000..be5115b
t/t5100/msg0015--no-inbody-headers new file +3 -0
@@ -0,0 +1,3 @@ +From: bogosity + - a list + - of stuff
t/t5100/msg0016 new file +2 -0
@@ -0,0 +1,2 @@ +and some content +
(quoted diff) +0 -0
b/t/t5100/msg0016--no-inbody-headers
new file mode 100644
index 0000000..1063f51
t/t5100/msg0016--no-inbody-headers new file +4 -0
@@ -0,0 +1,4 @@ +Date: bogus + +and some content +
t/t5100/patch0015 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
(quoted diff) +0 -0
b/t/t5100/patch0015--no-inbody-headers
new file mode 100644
index 0000000..ad64848
t/t5100/patch0015--no-inbody-headers new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0016 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
(quoted diff) +0 -0
b/t/t5100/patch0016--no-inbody-headers
new file mode 100644
index 0000000..ad64848
t/t5100/patch0016--no-inbody-headers new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/sample.mbox +33 -0
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644 convert_to_utf8(line, charset.buf); -- 1.6.4.1 +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +From: bogosity + - a list + - of stuff +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content + +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +Date: bogus + +and some content + +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
Signature · 1 line
1.6.4.4
Re: [PATCH] git am/mailinfo: Don't look at in-body headers when rebasing
Hi,
Quoted 3 lines · Lukas Sandström
The actual change is that mailinfo doesn't look for in-body headers at all if --no-inbody-headers is passed. git-am now passes this option to mailinfo when rebasing.
after all the earlier discussion and a lot of thinking, I have to say, that IMHO, this is the best option as it doesn't rely on heuristics and now that you chose a descriptive command line switch, even the small problem of "why exactly is this switch here?" seems to go away.
As I have no experience in git's codebase at all, I'll leave the commenting on the patch itself to the people with clue, but conceptionally, this feels much better than the method 1
Quoted 2 lines
This won't handle the case when a "bad" patch is passed to git-am from somewhere else than git rebase.
of course, that leaves the question what "somewhere else" can contain. If it's just manual calls to git-am, this is a non-issue as it's easily fixed by the caller. If it's being called from other higher-level operations though, you might run into the same issue again.
Here too, I can't really provide any meaningful input though as I just don't know well enough what really makes git tick.
Just my two cents :-)
Philip
PATCH v2 1/1 git am/mailinfo: Don't look at in-body headers when rebasing
When we are rebasing we know that the header lines in the patch are good and that we don't need to pick up any headers from the body of the patch.
This makes it possible to rebase commits whose commit message start with "From" or "Date".
Test vectors by Jeff King.
---
Argh. I just realized that the change to t5100 in the previous patch doesn't actually test the new option, since I forgot to change the
+ if test -f "$TEST_DIRECTORY"/t5100/msg$mail--use-first-header
line to "--no-inbody-headers", after my first attempt at an option name.
This time I checked that the tests actually fail when the test is broken. Still passes, just one line changed since the previous version.
/Lukas
builtin-mailinfo.c | 5 +++++ git-am.sh | 13 ++++++++++--- t/t5100-mailinfo.sh | 6 +++++- t/t5100/info0015 | 5 +++++ t/t5100/info0015--no-inbody-headers | 5 +++++ t/t5100/info0016 | 5 +++++ t/t5100/info0016--no-inbody-headers | 5 +++++ t/t5100/msg0015 | 2 ++ t/t5100/msg0015--no-inbody-headers | 3 +++ t/t5100/msg0016 | 2 ++ t/t5100/msg0016--no-inbody-headers | 4 ++++ t/t5100/patch0015 | 8 ++++++++ t/t5100/patch0015--no-inbody-headers | 8 ++++++++ t/t5100/patch0016 | 8 ++++++++ t/t5100/patch0016--no-inbody-headers | 8 ++++++++ t/t5100/sample.mbox | 33 +++++++++++++++++++++++++++++++++ 16 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 t/t5100/info0015 create mode 100644 t/t5100/info0015--no-inbody-headers create mode 100644 t/t5100/info0016 create mode 100644 t/t5100/info0016--no-inbody-headers create mode 100644 t/t5100/msg0015 create mode 100644 t/t5100/msg0015--no-inbody-headers create mode 100644 t/t5100/msg0016 create mode 100644 t/t5100/msg0016--no-inbody-headers create mode 100644 t/t5100/patch0015 create mode 100644 t/t5100/patch0015--no-inbody-headers create mode 100644 t/t5100/patch0016 create mode 100644 t/t5100/patch0016--no-inbody-headers
builtin-mailinfo.c +5 -0
@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;static int patch_lines;static struct strbuf **p_hdr_data, **s_hdr_data;static int use_scissors; +static int use_inbody_headers =1 ;#define MAX_HDR_PARSED 10#define MAX_BOUNDARIES 5
@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)return 0 ;if (still_looking) { +if (!use_inbody_headers) + still_looking =0 ;strbuf_ltrim (line);if (!line->len)return 0 ;
@@ -1033,6 +1036,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) use_scissors =1 ; else if (!strcmp(argv[1], "--no-scissors")) use_scissors =0 ; + else if (!strcmp(argv[1], "--no-inbody-headers")) + use_inbody_headers =0 ; elseusage (mailinfo_usage ); argc--; argv++;
git-am.sh +10 -3
@@ -289,7 +289,7 @@ split_patches () { prec=4 dotest="$GIT_DIR/rebase-apply" sign= utf8=t keep= skip= interactive= resolved= rebasing= abort= -resolvemsg= resume= scissors= +resolvemsg= resume= scissors= no_inbody_headers= git_apply_opt= committer_date_is_author_date= ignore_date=
@@ -322,7 +322,7 @@ do --abort) abort=t ;; --rebasing) - rebasing=t threeway=t keep=t scissors=f ;; + rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;; -d|--dotest) die "-d option is no longer supported. Do not use." ;;
@@ -448,6 +448,7 @@ else echo "$utf8" >"$dotest/utf8" echo "$keep" >"$dotest/keep" echo "$scissors" >"$dotest/scissors" + echo "$no_inbody_headers" >"$dotest/no_inbody_headers" echo "$GIT_QUIET" >"$dotest/quiet" echo 1 >"$dotest/next" if test -n "$rebasing"
@@ -495,6 +496,12 @@ t) f) scissors=--no-scissors ;; esac +if test "$(cat "$dotest/no_inbody_headers")" = t +then + no_inbody_headers=--no-inbody-headers +else + no_inbody_headers= +fi if test "$(cat "$dotest/quiet")" = t then GIT_QUIET=t
@@ -549,7 +556,7 @@ do # by the user, or the user can tell us to do so by --resolved flag. case "$resume" in '') - git mailinfo $keep $scissors $utf8 "$dotest/msg" "$dotest/patch" \ + git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \ <"$dotest/$msgnum" >"$dotest/info" || stop_here $this
t/t5100-mailinfo.sh +5 -1
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \ 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last && last=`cat last` && echo total is $last && - test `cat last` = 14' + test `cat last` = 16' check_mailinfo () { mail=$1 opt=$2
@@ -30,6 +30,10 @@ do if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors then check_mailinfo $mail --scissors + fi && + if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers + then + check_mailinfo $mail --no-inbody-headers fi ' done
t/t5100/info0015 new file +5 -0
@@ -0,0 +1,5 @@ +Author: +Email: +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/info0015--no-inbody-headers new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/info0016 new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: bogus +
t/t5100/info0016--no-inbody-headers new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/msg0015 new file +2 -0
@@ -0,0 +1,2 @@ +- a list + - of stuff
t/t5100/msg0015--no-inbody-headers new file +3 -0
@@ -0,0 +1,3 @@ +From: bogosity + - a list + - of stuff
t/t5100/msg0016 new file +2 -0
@@ -0,0 +1,2 @@ +and some content +
t/t5100/msg0016--no-inbody-headers new file +4 -0
@@ -0,0 +1,4 @@ +Date: bogus + +and some content +
t/t5100/patch0015 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0015--no-inbody-headers new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0016 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0016--no-inbody-headers new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/sample.mbox +33 -0
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644 convert_to_utf8(line, charset.buf); -- 1.6.4.1 +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +From: bogosity + - a list + - of stuff +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content + +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +Date: bogus + +and some content + +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
Signature · 1 line
1.6.4.4
Re: [PATCH v2] git am/mailinfo: Don't look at in-body headers when rebasing
Quoted 8 lines · Lukas Sandström
When we are rebasing we know that the header lines in the patch are good and that we don't need to pick up any headers from the body of the patch.
This makes it possible to rebase commits whose commit message start with "From" or "Date".
Test vectors by Jeff King.
Thanks, it did end up being a pretty small change. Though I think we may be better off with _both_ patches. Your patch protects the message absolutely during rebasing, and my patch improves the heuristic when applying non-rebase patches.
builtin-mailinfo.c static int handle_commit_msg(struct strbuf *line) · line 779
Quoted 9 lines
builtin-mailinfo.c +2
-0@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)return 0 ;if (still_looking) { +if (!use_inbody_headers) + still_looking =0 ;strbuf_ltrim (line);if (!line->len)return 0 ;
Hmm. But we still end up in this conditional for the very first line. Which I guess happens to work because the first line we feed is presumably the empty blank line (but I didn't check). Still, wouldn't it be more clear as:
if (use_inbody_headers && still_looking) {
...in which case still_looking simply becomes irrelevant when the feature is disabled?
t/t5100/sample.mbox index b0b5d8f..461c47e 100644 · line 661
Quoted 9 lines
t/t5100/sample.mbox +9
-0+From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +From: bogosity + - a list + - of stuff +---
Since your feature is meant to prevent us looking at inbody headers no matter if they are valid-looking or not, wouldn't a better test be to actually have:
From: Other Author <[email protected]>
Otherwise, you don't know if it is your feature blocking it, or my patch (if it gets applied on top).
t/t5100/sample.mbox index b0b5d8f..461c47e 100644 · line 677
Quoted 9 lines
t/t5100/sample.mbox +9
-0+From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +Date: bogus + +and some content +
And ditto for the Date here.
-Peff
PATCH 1/1 git am/mailinfo: Don't look at in-body headers when rebasing
When we are rebasing we know that the header lines in the patch are good and that we don't need to pick up any headers from the body of the patch.
This makes it possible to rebase commits whose commit message start with "From" or "Date".
Test vectors by Jeff King.
---
Quoted 6 lines · Jeff King
On Thu, Nov 19, 2009 at 09:51:36AM +0100, Lukas Sandström wrote:
Thanks, it did end up being a pretty small change. Though I think we may be better off with _both_ patches. Your patch protects the message absolutely during rebasing, and my patch improves the heuristic when applying non-rebase patches.
I looked a bit at using your extra safety check, but it would be a change in behavior to only accept valid email adresses, and mailinfo is riddled with corner cases. If this changed is made someone will propably complain, saying that they rely on invalid dates in their commit messages.
I don't know when to apply your stricter check whithout breaking any tests.
Quoted 15 lines
Review static int handle_commit_msg(struct strbuf *line) · line 779
Quoted 9 lines
(quoted diff) +2
-0@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line) return 0; if (still_looking) { + if (!use_inbody_headers) + still_looking = 0; strbuf_ltrim(line); if (!line->len) return 0;Hmm. But we still end up in this conditional for the very first line. Which I guess happens to work because the first line we feed is presumably the empty blank line (but I didn't check). Still, wouldn't it be more clear as:
Quoted 5 lines
if (use_inbody_headers && still_looking) { ...in which case
still_lookingsimply becomes irrelevant when the feature is disabled?
When rebasing the first line passed to handle_commit_msg will be the blank line between the headers and commit message. This should be removed. I rewrote this part to make it a bit more obvious.
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +From: bogosity + - a list + - of stuff +---Since your feature is meant to prevent us looking at inbody headers no matter if they are valid-looking or not, wouldn't a better test be to actually have:
Quoted 5 lines
From: Other Author <[email protected]>
Otherwise, you don't know if it is your feature blocking it, or my patch (if it gets applied on top).
I kept the tests as is for now, since they show the problem originally reported.
/Lukas
builtin-mailinfo.c | 12 +++++++++++- git-am.sh | 13 ++++++++++--- t/t5100-mailinfo.sh | 6 +++++- t/t5100/info0015 | 5 +++++ t/t5100/info0015--no-inbody-headers | 5 +++++ t/t5100/info0016 | 5 +++++ t/t5100/info0016--no-inbody-headers | 5 +++++ t/t5100/msg0015 | 2 ++ t/t5100/msg0015--no-inbody-headers | 3 +++ t/t5100/msg0016 | 2 ++ t/t5100/msg0016--no-inbody-headers | 4 ++++ t/t5100/patch0015 | 8 ++++++++ t/t5100/patch0015--no-inbody-headers | 8 ++++++++ t/t5100/patch0016 | 8 ++++++++ t/t5100/patch0016--no-inbody-headers | 8 ++++++++ t/t5100/sample.mbox | 33 +++++++++++++++++++++++++++++++++ 16 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 t/t5100/info0015 create mode 100644 t/t5100/info0015--no-inbody-headers create mode 100644 t/t5100/info0016 create mode 100644 t/t5100/info0016--no-inbody-headers create mode 100644 t/t5100/msg0015 create mode 100644 t/t5100/msg0015--no-inbody-headers create mode 100644 t/t5100/msg0016 create mode 100644 t/t5100/msg0016--no-inbody-headers create mode 100644 t/t5100/patch0015 create mode 100644 t/t5100/patch0015--no-inbody-headers create mode 100644 t/t5100/patch0016 create mode 100644 t/t5100/patch0016--no-inbody-headers
builtin-mailinfo.c +11 -1
@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;static int patch_lines;static struct strbuf **p_hdr_data, **s_hdr_data;static int use_scissors; +static int use_inbody_headers =1 ;#define MAX_HDR_PARSED 10#define MAX_BOUNDARIES 5
@@ -774,10 +775,17 @@ static int handle_commit_msg(struct strbuf *line)strbuf_ltrim (line);if (!line->len)return 0 ; + } + +if (use_inbody_headers && still_looking) { still_looking =check_header (line, s_hdr_data,0 );if (still_looking)return 0 ; - } + }else +/* Only trim the first (blank) line of the commit message +* when ignoring in-body headers. +*/ + still_looking =0 ;/* normalize the log message to UTF-8. */ if (metainfo_charset)
@@ -1033,6 +1041,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) use_scissors =1 ; else if (!strcmp(argv[1], "--no-scissors")) use_scissors =0 ; + else if (!strcmp(argv[1], "--no-inbody-headers")) + use_inbody_headers =0 ; elseusage (mailinfo_usage ); argc--; argv++;
git-am.sh +10 -3
@@ -289,7 +289,7 @@ split_patches () { prec=4 dotest="$GIT_DIR/rebase-apply" sign= utf8=t keep= skip= interactive= resolved= rebasing= abort= -resolvemsg= resume= scissors= +resolvemsg= resume= scissors= no_inbody_headers= git_apply_opt= committer_date_is_author_date= ignore_date=
@@ -322,7 +322,7 @@ do --abort) abort=t ;; --rebasing) - rebasing=t threeway=t keep=t scissors=f ;; + rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;; -d|--dotest) die "-d option is no longer supported. Do not use." ;;
@@ -448,6 +448,7 @@ else echo "$utf8" >"$dotest/utf8" echo "$keep" >"$dotest/keep" echo "$scissors" >"$dotest/scissors" + echo "$no_inbody_headers" >"$dotest/no_inbody_headers" echo "$GIT_QUIET" >"$dotest/quiet" echo 1 >"$dotest/next" if test -n "$rebasing"
@@ -495,6 +496,12 @@ t) f) scissors=--no-scissors ;; esac +if test "$(cat "$dotest/no_inbody_headers")" = t +then + no_inbody_headers=--no-inbody-headers +else + no_inbody_headers= +fi if test "$(cat "$dotest/quiet")" = t then GIT_QUIET=t
@@ -549,7 +556,7 @@ do # by the user, or the user can tell us to do so by --resolved flag. case "$resume" in '') - git mailinfo $keep $scissors $utf8 "$dotest/msg" "$dotest/patch" \ + git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \ <"$dotest/$msgnum" >"$dotest/info" || stop_here $this
t/t5100-mailinfo.sh +5 -1
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \ 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last && last=`cat last` && echo total is $last && - test `cat last` = 14' + test `cat last` = 16' check_mailinfo () { mail=$1 opt=$2
@@ -30,6 +30,10 @@ do if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors then check_mailinfo $mail --scissors + fi && + if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers + then + check_mailinfo $mail --no-inbody-headers fi ' done
t/t5100/info0015 new file +5 -0
@@ -0,0 +1,5 @@ +Author: +Email: +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/info0015--no-inbody-headers new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/info0016 new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: bogus +
t/t5100/info0016--no-inbody-headers new file +5 -0
@@ -0,0 +1,5 @@ +Author: A U Thor +Email: [email protected] +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 +
t/t5100/msg0015 new file +2 -0
@@ -0,0 +1,2 @@ +- a list + - of stuff
t/t5100/msg0015--no-inbody-headers new file +3 -0
@@ -0,0 +1,3 @@ +From: bogosity + - a list + - of stuff
t/t5100/msg0016 new file +2 -0
@@ -0,0 +1,2 @@ +and some content +
t/t5100/msg0016--no-inbody-headers new file +4 -0
@@ -0,0 +1,4 @@ +Date: bogus + +and some content +
t/t5100/patch0015 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0015--no-inbody-headers new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0016 new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/patch0016--no-inbody-headers new file +8 -0
@@ -0,0 +1,8 @@ +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
t/t5100/sample.mbox +33 -0
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644 convert_to_utf8(line, charset.buf); -- 1.6.4.1 +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (from) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +From: bogosity + - a list + - of stuff +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content + +From nobody Mon Sep 17 00:00:00 2001 +From: A U Thor <[email protected]> +Subject: check bogus body header (date) +Date: Fri, 9 Jun 2006 00:44:16 -0700 + +Date: bogus + +and some content + +--- +diff --git a/foo b/foo +index e69de29..d95f3ad 100644 +--- a/foo ++++ b/foo +@@ -0,0 +1 @@ ++content +
Signature · 1 line
1.6.4.4
Hello,
Quoted 6 lines · Jeff King
On Wed, Nov 18, 2009 at 03:20:48PM +0100, Philip Hofstetter wrote:
1. Improve the header-finding heuristic to actually look for something more sane, like "From:.*<.*@.*>" (I don't recall off the top of my head which other headers we handle in this position. Probably Date, too).
or at least don't prefer obviously invalid data over valid data that has already been seen.
Quoted 3 lines
2. Give mailinfo a "--strict" mode to indicate that it is directly parsing the output of format-patch, and not some random email. Use --strict when invoking "git am" via "git rebase".
That would solve the problem too, though it feels like adding yet another switch to guard against one specific issue. The purpose behind options like this tends to get forgotten over time.
Quoted 5 lines
As I explained above, there is a reason, but I don't think it's rude to have either of those lines. You were, after all, writing a commit message, not an email (and even if you were, it is a failure of the storage format if it can't represent your data correctly). So I think git is to blame here.
IMHO, another workable solution would be to reject a commit that later can't be handled. That way the current attempts at getting an email address can remain intact and the (much more) unlikely case that somebody begins the commit message with from: will be caught before damage is done.
So, just check that from-line for a valid email address at commit time. If it is, ok. If not, treat it as an error and inform the user that an invalid email address was given in the commit message.
Also, the error message by rebase (which is actually the message printed by am) could have been a bit more helpful. If am fails during a rebase, rebase could explicitly tell which commit am failed at. The output I got made me suspect the problem to be in the first commit (as that was the last one printed) when in fact it was in the second one (which was not printed).
But that's just nit-picking.
Philip
Quoted 11 lines · Philip Hofstetter
Quoted 5 lines
As I explained above, there is a reason, but I don't think it's rude to have either of those lines. You were, after all, writing a commit message, not an email (and even if you were, it is a failure of the storage format if it can't represent your data correctly). So I think git is to blame here.
IMHO, another workable solution would be to reject a commit that later can't be handled. That way the current attempts at getting an email address can remain intact and the (much more) unlikely case that somebody begins the commit message with from: will be caught before damage is done.
I'm not sure I like that solution for a few reasons:
1. It creates a bad user experience. You are not unreasonable for wanting to put some specific text in your commit message. Having git come back and say "oops, I might get confused by this later" just seems like an annoyance to the user.
2. Mailinfo has to deal with data created by older versions of git. So in your case, the rebase was a bomb waiting to go off. If we can fix it so that an existing bomb doesn't go off, rather than not creating the bomb in the first place, then we are better off.
3. Commit has to know about rules for mailinfo, even versions of mailinfo that will exist in the future. Probably the rules aren't going to change much, but it is a weakness.
4. Commit messages can come from other places than "git commit". What should we do with a commit message like this that is imported from SVN? Reject the import? Munge the message?
Of course all of that presupposes that we can correctly handle the existing data after the fact. Even with my patch, you still can't write "From: [email protected]" as the first line of your commit body. But that is, IMHO, getting even more unlikely than your "From:" (which already seems fairly unlikely).
I also think "git commit" would not be the right time for such a feature. The problem is not that you have this text in your commit message. The problem is that the "format-patch | am" transport is lossy. You would do better to have format-patch say "Ah, this is going to create a bogus email address" and somehow quote it appropriately.
-Peff
Hello,
Quoted 4 lines · Jeff King
1. It creates a bad user experience. You are not unreasonable for wanting to put some specific text in your commit message. Having git come back and say "oops, I might get confused by this later" just seems like an annoyance to the user.
agreed, though it's not that bad: when learning git, you will be confronted with the fact that the commit message has a few things that are special (well. it's doesn't break git, but the first line should be < 56 chars in length for example).
Not being able to have From: lines in them that are not describing an author would then just be one of them.
Quoted 4 lines
2. Mailinfo has to deal with data created by older versions of git. So in your case, the rebase was a bomb waiting to go off. If we can fix it so that an existing bomb doesn't go off, rather than not creating the bomb in the first place, then we are better off.
This is a very good point. I didn't quite think about that.
Quoted 3 lines
4. Commit messages can come from other places than "git commit". What should we do with a commit message like this that is imported from SVN? Reject the import? Munge the message?
I would leave that to the tool that does the import. Probably it would have to munge it. Yes.
I DO see though that implementing the check at commit time would lead to problems popping up at other places.
Quoted 3 lines
Of course all of that presupposes that we can correctly handle the existing data after the fact. Even with my patch, you still can't write "From: [email protected]" as the first line of your commit body. But that
can't you? IMHO it would just attribute the commit to [email protected] which can be an equally bad, if not worse thing (I'm saying that without the needed knowledge about git internals to really be sure, so take this with a grain of salt)
I just have a bad feeling about trying out heuristics to see whether thing thing after from: is an email address or not as email addresses are notoriously hard to detect.
Typing a commit message and applying a patch from an email should be separate things and should be handled separately. Currently they are not and this is what's causing the problem in the first place.
Maybe that --strict thing is actually a good thing in the long run, even though I don't quite like it either :-)
Interesting problem to have though.
Philip
Quoted 5 lines · Jeff King
I also think "git commit" would not be the right time for such a feature. The problem is not that you have this text in your commit message. The problem is that the "format-patch | am" transport is lossy. You would do better to have format-patch say "Ah, this is going to create a bogus email address" and somehow quote it appropriately.
Doesn't mbox format have some way of escaping "From:" (or is it "From ")? If I remember correctly it uses ">From " or something for that. git-format-patch could do this also (perhaps only with --rebasing option).
P.S. As git-format-patch / git-am have hidden --rebasing option, perhaps git-mailinfo should have it as well (even if it is called --strict).
Signature · 3 lines
Jakub Narebski Poland ShadeHawk on #git
Quoted 10 lines · Jakub Narebski
Quoted 5 lines
I also think "git commit" would not be the right time for such a feature. The problem is not that you have this text in your commit message. The problem is that the "format-patch | am" transport is lossy. You would do better to have format-patch say "Ah, this is going to create a bogus email address" and somehow quote it appropriately.
Doesn't mbox format have some way of escaping "From:" (or is it "From ")? If I remember correctly it uses ">From " or something for that. git-format-patch could do this also (perhaps only with --rebasing option).
It's for "From " lines, which are the mbox separator. This would be somewhat different. It has nothing at all to do with the mail format itself, but rather is about git treating the body of the message specially. I don't think a quoting mechanism currently exists to handle this.
-Peff