threads / discuss / 4472

git-applymbox broken?

Subject: git-applymbox broken?

## tl;dr

13 messages between Jun 11, 2006 and Jun 13, 2006.

replies: 12people: 5as markdown or json

Linus Torvalds· Jun 11, 2006, 22:40 UTC · lore
It looks like something has broken git-applymbox lately.

The "From: authorname" lines are no longer removed from the message, and are duplicated in the commit log. This has resulted in several recent kernel commits looking like this:

	commit c0bbbc73d58f1b774cd987b5687a478a027f137c
	Author: Christoph Lameter <clameter@sgi.com>
	Date:   Sun Jun 11 15:22:26 2006 -0700
	
	    [PATCH] typo in vmscan.c
	    
	    From: Christoph Lameter <clameter@sgi.com>
	    
	    Looks like a comma was left from the conversion from a struct to an
	    assignment.
	    
	    Signed-off-by: Christoph Lameter <clameter@sgi.com>
	    Signed-off-by: Andrew Morton <akpm@osdl.org>
	    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

where that "From:" in the body is totally wrong. I just didn't notice, until now. Arrr!

I _suspect_ that this is the work by Eric Biederman, ie part of the patches that do "Allow in body headers beyond the in body header prefix." and "Refactor commit messge handling."

Eric? Can you please fix this up? Lines from the body of the email that have been used to set authorship should _not_ also show up in the commit message.

		Linus
Eric W. Biederman· Jun 11, 2006, 23:33 UTC · re: Linus Torvalds · lore

Re: git-applymbox broken?

Linus Torvalds <torvalds@osdl.org> writes:
Show 5 quoted lines
> It looks like something has broken git-applymbox lately.
>
> The "From: authorname" lines are no longer removed from the message, and 
> are duplicated in the commit log. This has resulted in several recent 
> kernel commits looking like this:

Agreed. That isn't terribly desirable. Do you have the original email message some place?

There is an odd case where if someone put the From: header in the middle of the text that we now notice and process and I didn't feel right about removing a line from the middle of the text.

I was fixing a nasty corner case that happens if there aren't any mail headers at all passed to git-mailinfo. Where we could drop lines without processing them at all.

This doesn't look like the From: header was in the middle of the message until it was imported into git so it is probably a small logic error that is easily corrected. But I need to see what we are parsing so I can understand what is happening.

Show 25 quoted lines
> 	commit c0bbbc73d58f1b774cd987b5687a478a027f137c
> 	Author: Christoph Lameter <clameter@sgi.com>
> 	Date:   Sun Jun 11 15:22:26 2006 -0700
> 	
> 	    [PATCH] typo in vmscan.c
> 	    
> 	    From: Christoph Lameter <clameter@sgi.com>
> 	    
> 	    Looks like a comma was left from the conversion from a struct to an
> 	    assignment.
> 	    
> 	    Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 	    Signed-off-by: Andrew Morton <akpm@osdl.org>
> 	    Signed-off-by: Linus Torvalds <torvalds@osdl.org>
>
> where that "From:" in the body is totally wrong. I just didn't notice, 
> until now. Arrr!
>
> I _suspect_ that this is the work by Eric Biederman, ie part of the 
> patches that do "Allow in body headers beyond the in body header 
> prefix." and "Refactor commit messge handling."
>
> Eric? Can you please fix this up? Lines from the body of the email that 
> have been used to set authorship should _not_ also show up in the commit 
> message.
Even if the header lines are in the middle of the body?
Eric
Linus Torvalds· Jun 12, 2006, 00:37 UTC · re: Eric W. Biederman · lore

Re: git-applymbox broken?

On Sun, 11 Jun 2006, Eric W. Biederman wrote:
Show 5 quoted lines
> 
> This doesn't look like the From: header was in the middle of the
> message until it was imported into git so it is probably a small
> logic error that is easily corrected.  But I need to see what
> we are parsing so I can understand what is happening.

No, it's at the top of the body, although there might have been an empty line or two (ie whitespace only) before it.

> Even if the header lines are in the middle of the body?
What do you mean by "middle"?

No, it should only look at From: and Subject: lines if they are at the very top, with no other non-whitespace lines above them. But when it looks at them and uses the data from them, it should then remove them from the body - they are "conceptually" just extended header lines that just happened to technically (from an rfc822 standpoint) be in the body of the email.

		Linus
Eric W. Biederman· Jun 12, 2006, 07:35 UTC · re: Linus Torvalds · lore

Re: git-applymbox broken?

Linus Torvalds <torvalds@osdl.org> writes:
Show 9 quoted lines
> On Sun, 11 Jun 2006, Eric W. Biederman wrote:
>> 
>> This doesn't look like the From: header was in the middle of the
>> message until it was imported into git so it is probably a small
>> logic error that is easily corrected.  But I need to see what
>> we are parsing so I can understand what is happening.
>
> No, it's at the top of the body, although there might have been an empty 
> line or two (ie whitespace only) before it.

Ok. I'm not certain why we would not be ignoring blank lines that we used to skip. The untested patch below should ensure we always skip those lines.

Show 10 quoted lines
>> Even if the header lines are in the middle of the body?
>
> What do you mean by "middle"?
>
> No, it should only look at From: and Subject: lines if they are at the 
> very top, with no other non-whitespace lines above them. But when it looks 
> at them and uses the data from them, it should then remove them from the 
> body - they are "conceptually" just extended header lines that just 
> happened to technically (from an rfc822 standpoint) be in the body of the 
> email.

This is a separate conversation and once the problem of not ignoring leading blank lines is fixed I will be happy to address it.

Eric
diff --git a/mailinfo.c b/mailinfo.c
index 5b6c215..72c5454 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -279,6 +279,14 @@ static void handle_inbody_header(int *se
                        return;
                }
        }
+       /* Ignore leading blank lines */
+       if (!(*seen & SEEN_PREFIX)) {
+               char *ch;
+               for (ch = line; isspace(*ch); ch++)
+                       ;
+               if (*ch == '\0')
+                       return;
+       }
        *seen |= SEEN_PREFIX;
 }
Eric W. Biederman· Jun 12, 2006, 18:45 UTC · re: Linus Torvalds · lore

[PATCH] Ignore blank lines among this inbody headers.

This is a fix for a regression introduced in: 8b4525fb3c6d79bd3a64b8f441237a4095db4e22.

When I refactored the inbody header parsing into a state machine I failed to see the logic that skipped multiple leading spaces if they are present. I think I assumed that logic was just there to skip the initial blank line between the mail headers and the body.

This restores that behaviour and since we ignore all leading blank lines
in commit messages now this code removes the special case for the blank
line between the mail headers and the body.
---
 mailinfo.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/mailinfo.c b/mailinfo.c
index 5b6c215..3696d61 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -229,6 +229,14 @@ static int is_multipart_boundary(const c
 	return (!memcmp(line, multipart_boundary, multipart_boundary_len));
 }
 
+static int is_blank(char *line)
+{
+	char *ch;
+	for (ch = line; isspace(*ch); ch++)
+		;
+	return *ch == '\0';
+}
+
 static int eatspace(char *line)
 {
 	int len = strlen(line);
@@ -243,7 +251,7 @@ #define SEEN_SUBJECT 04
 #define SEEN_BOGUS_UNIX_FROM 010
 #define SEEN_PREFIX  020
 
-/* First lines of body can have From:, Date:, and Subject: */
+/* First lines of body can have From:, Date:, and Subject: or be blank */
 static void handle_inbody_header(int *seen, char *line)
 {
 	if (!memcmp(">From", line, 5) && isspace(line[5])) {
@@ -279,6 +287,10 @@ static void handle_inbody_header(int *se
 			return;
 		}
 	}
+	if (isspace(line[0])) {
+		if (!(*seen & SEEN_PREFIX) && is_blank(line))
+			return;
+	}
 	*seen |= SEEN_PREFIX;
 }
 
@@ -420,9 +432,7 @@ static int read_one_header_line(char *li
 		if (fgets(line + ofs, sz - ofs, in) == NULL)
 			break;
 		len = eatspace(line + ofs);
-		if (len == 0)
-			break;
-		if (!is_rfc2822_header(line)) {
+		if ((len == 0) || !is_rfc2822_header(line)) {
 			/* Re-add the newline */
 			line[ofs + len] = '\n';
 			line[ofs + len + 1] = '\0';
@@ -762,10 +772,8 @@ static void handle_body(void)
 {
 	int seen = 0;
 
-	if (line[0] || fgets(line, sizeof(line), stdin) != NULL) {
-		handle_commit_msg(&seen);
-		handle_patch();
-	}
+	handle_commit_msg(&seen);
+	handle_patch();
 	fclose(patchfile);
 	if (!patch_lines) {
 		fprintf(stderr, "No patch found\n");
-- 
1.4.0.rc2.g5e3a6
Eric W. Biederman· Jun 12, 2006, 19:29 UTC · re: Eric W. Biederman · lore

[PATCH] Ignore blank lines among this inbody headers

This is a fix for a regression introduced in: 8b4525fb3c6d79bd3a64b8f441237a4095db4e22.

When I refactored the inbody header parsing into a state machine I failed to see the logic that skipped multiple leading spaces if they are present. I think I assumed that logic was just there to skip the initial blank line between the mail headers and the body.

This restores that behaviour and since we ignore all leading blank lines in commit messages now this code removes the special case for the blank line between the mail headers and the body.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
This is a resend to add my missing Signed-off-by line.
---
 mailinfo.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/mailinfo.c b/mailinfo.c
index 5b6c215..3696d61 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -229,6 +229,14 @@ static int is_multipart_boundary(const c
 	return (!memcmp(line, multipart_boundary, multipart_boundary_len));
 }
 
+static int is_blank(char *line)
+{
+	char *ch;
+	for (ch = line; isspace(*ch); ch++)
+		;
+	return *ch == '\0';
+}
+
 static int eatspace(char *line)
 {
 	int len = strlen(line);
@@ -243,7 +251,7 @@ #define SEEN_SUBJECT 04
 #define SEEN_BOGUS_UNIX_FROM 010
 #define SEEN_PREFIX  020
 
-/* First lines of body can have From:, Date:, and Subject: */
+/* First lines of body can have From:, Date:, and Subject: or be blank */
 static void handle_inbody_header(int *seen, char *line)
 {
 	if (!memcmp(">From", line, 5) && isspace(line[5])) {
@@ -279,6 +287,10 @@ static void handle_inbody_header(int *se
 			return;
 		}
 	}
+	if (isspace(line[0])) {
+		if (!(*seen & SEEN_PREFIX) && is_blank(line))
+			return;
+	}
 	*seen |= SEEN_PREFIX;
 }
 
@@ -420,9 +432,7 @@ static int read_one_header_line(char *li
 		if (fgets(line + ofs, sz - ofs, in) == NULL)
 			break;
 		len = eatspace(line + ofs);
-		if (len == 0)
-			break;
-		if (!is_rfc2822_header(line)) {
+		if ((len == 0) || !is_rfc2822_header(line)) {
 			/* Re-add the newline */
 			line[ofs + len] = '\n';
 			line[ofs + len + 1] = '\0';
@@ -762,10 +772,8 @@ static void handle_body(void)
 {
 	int seen = 0;
 
-	if (line[0] || fgets(line, sizeof(line), stdin) != NULL) {
-		handle_commit_msg(&seen);
-		handle_patch();
-	}
+	handle_commit_msg(&seen);
+	handle_patch();
 	fclose(patchfile);
 	if (!patch_lines) {
 		fprintf(stderr, "No patch found\n");
-- 
1.4.0.g25f48-dirty
Eric W. Biederman· Jun 12, 2006, 18:58 UTC · re: Linus Torvalds · lore

Re: git-applymbox broken?

Linus Torvalds <torvalds@osdl.org> writes:
Show 8 quoted lines
> What do you mean by "middle"?
>
> No, it should only look at From: and Subject: lines if they are at the 
> very top, with no other non-whitespace lines above them. But when it looks 
> at them and uses the data from them, it should then remove them from the 
> body - they are "conceptually" just extended header lines that just 
> happened to technically (from an rfc822 standpoint) be in the body of the 
> email.

Below is an example of the kind of patch that inspired me to relax the rules on parsing in body headers (this comes from Andi Kleen quilt tree).

The first line in this instance is obviously a subject line but there is not really good way to detect that. Then we get a From: line.

Now I doubt any patches ever hit the mail in this format and it probably isn't worth it to track down every variation of patch headers in existence. But if we don't find a From: header in the body prefix it seems to make sense to keep looking for headers in the body, and to use the information if we find it.

--- Kdump i386 nmi event notification fix

From: Vivek Goyal <vgoyal@in.ibm.com>

After a crash we should wait for NMI IPI event and not for external NMI or NMI watchdog tick.

Signed-off-by: Vivek Goyal <vgoyal@in.ibm.com>
Signed-off-by: Andi Kleen <ak@suse.de>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Andi Kleen <ak@suse.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---
 arch/i386/kernel/crash.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Index: linux/arch/i386/kernel/crash.c
===================================================================
--- linux.orig/arch/i386/kernel/crash.c
+++ linux/arch/i386/kernel/crash.c
@@ -102,7 +102,7 @@ static int crash_nmi_callback(struct not
 	struct pt_regs fixed_regs;
 	int cpu;
 
-	if (val != DIE_NMI)
+	if (val != DIE_NMI_IPI)
 		return NOTIFY_OK;
 
 	regs = ((struct die_args *)data)->regs;
@@ -113,7 +113,7 @@ static int crash_nmi_callback(struct not
 	 * an NMI if system was initially booted with nmi_watchdog parameter.
 	 */
 	if (cpu == crashing_cpu)
-		return 1;
+		return NOTIFY_STOP;
 	local_irq_disable();
 
 	if (!user_mode_vm(regs)) {
Linus Torvalds· Jun 12, 2006, 19:10 UTC · re: Eric W. Biederman · lore

Re: git-applymbox broken?

On Mon, 12 Jun 2006, Eric W. Biederman wrote:
> 
> Below is an example of the kind of patch that inspired me to relax the
> rules on parsing in body headers (this comes from Andi Kleen quilt tree).
And this is wrong.

We should _not_ accept crappy patches, and then start guessing at what the person meant.

>From the very beginning of git, I tried to make it extremely clear that 

there is never any guessing going on. We don't use "heuristics" except as a pure optimization: ie a heuristic can have a _performance_ impact, but it must never EVER have semantic impact.

SCM's are not about guessing. They are about saving the _exact_ state that the user asked for. No "let's try to be nice", no gray areas.

If the new git-applymbox just takes random lines from the body of the email, and decides that they may be authorship information, then that is a BUG. The "From: " line in the middle of an email may well be about somebody having _discovered_ the bug, and we're quoting him as part of the explanation. It does NOT mean that it's about authorship.

So we should ONLY check for "From:" (and perhaps "Subject:" and "Date:") at the very top of the email body. NOWHERE ELSE.

The fact that somebody has a crappy quilt tree, and the fact that quilt is very much a "anything goes" kind of laissez faire system does not mean, and should NEVER mean that git becomes the same kind of mess of "let's do a best effort and try to guess what somebody means" kind of thing.

I check and edit my emails before I apply them, and I try to teach the people who send them manners and what the rules are. THAT is the way to handle this, not by having the tool itself become unreliable and random

		Linus
Eric W. Biederman· Jun 12, 2006, 19:48 UTC · re: Linus Torvalds · lore

[PATCH] Don't parse any headers in the real body of an email message.

It was pointed out that the current behaviour might mispart a patch comment so remove this behaviour for now.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 mailinfo.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mailinfo.c b/mailinfo.c
index 3696d61..325c3b2 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -254,6 +254,8 @@ #define SEEN_PREFIX  020
 /* First lines of body can have From:, Date:, and Subject: or be blank */
 static void handle_inbody_header(int *seen, char *line)
 {
+	if (*seen & SEEN_PREFIX)
+		return;
 	if (!memcmp(">From", line, 5) && isspace(line[5])) {
 		if (!(*seen & SEEN_BOGUS_UNIX_FROM)) {
 			*seen |= SEEN_BOGUS_UNIX_FROM;
-- 
1.4.0.g25f48-dirty
Eric W. Biederman· Jun 12, 2006, 20:10 UTC · re: Linus Torvalds · lore

Re: git-applymbox broken?

Linus Torvalds <torvalds@osdl.org> writes:
Show 31 quoted lines
> On Mon, 12 Jun 2006, Eric W. Biederman wrote:
>> 
>> Below is an example of the kind of patch that inspired me to relax the
>> rules on parsing in body headers (this comes from Andi Kleen quilt tree).
>
> And this is wrong.
>
> We should _not_ accept crappy patches, and then start guessing at what the 
> person meant.
>
>>From the very beginning of git, I tried to make it extremely clear that 
> there is never any guessing going on. We don't use "heuristics" except as 
> a pure optimization: ie a heuristic can have a _performance_ impact, but 
> it must never EVER have semantic impact.
>
> SCM's are not about guessing. They are about saving the _exact_ state that 
> the user asked for. No "let's try to be nice", no gray areas.
>
> If the new git-applymbox just takes random lines from the body of the 
> email, and decides that they may be authorship information, then that is a 
> BUG. The "From: " line in the middle of an email may well be about 
> somebody having _discovered_ the bug, and we're quoting him as part of the 
> explanation. It does NOT mean that it's about authorship.
>
> So we should ONLY check for "From:" (and perhaps "Subject:" and "Date:") 
> at the very top of the email body. NOWHERE ELSE.
>
> The fact that somebody has a crappy quilt tree, and the fact that quilt is 
> very much a "anything goes" kind of laissez faire system does not mean, 
> and should NEVER mean that git becomes the same kind of mess of "let's do 
> a best effort and try to guess what somebody means" kind of thing.

Ok. A reasonable position. It would have been nice if you had squawked when I made that change: 2dec02b1ecafc47d4031d0a68a94c775a6a9ff9e

I thought I was explicit when I did it, oh well.

As for quilt being imperfect that among other things is why I am slowly trying to make the tools play together better. So people can use the best tool for the job, which if the integration is tight enough becomes a single tool.

> I check and edit my emails before I apply them, and I try to teach the 
> people who send them manners and what the rules are. THAT is the way to 
> handle this, not by having the tool itself become unreliable and random
What are the rules?

This looks like something that needs to be Documented by more than just the source of git-mailinfo.

The need to skip extra blank lines was a surprise to me. In looking for documentation the best I could quickly find was SubmittingPatches and it only Documents the From: and --- lines.

Eric
Johannes Schindelin· Jun 12, 2006, 22:43 UTC · re: Eric W. Biederman · lore

Re: git-applymbox broken?

Hi,
On Mon, 12 Jun 2006, Eric W. Biederman wrote:
> Index: linux/arch/i386/kernel/crash.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/crash.c
> +++ linux/arch/i386/kernel/crash.c
Tsk, tsk. Not using git, are we?

Ciao, Dscho

Randy.Dunlap· Jun 12, 2006, 23:54 UTC · re: Johannes Schindelin · lore

Re: git-applymbox broken?

On Tue, 13 Jun 2006 00:43:06 +0200 (CEST) Johannes Schindelin wrote:
Show 10 quoted lines
> Hi,
> 
> On Mon, 12 Jun 2006, Eric W. Biederman wrote:
> 
> > Index: linux/arch/i386/kernel/crash.c
> > ===================================================================
> > --- linux.orig/arch/i386/kernel/crash.c
> > +++ linux/arch/i386/kernel/crash.c
> 
> Tsk, tsk. Not using git, are we?

what's your point? Eric clearly identified where the patch came from.

--- ~Randy

Ryan Anderson· Jun 13, 2006, 03:41 UTC · re: Eric W. Biederman · lore

Re: git-applymbox broken?

On Sun, Jun 11, 2006 at 05:33:59PM -0600, Eric W. Biederman wrote:
Show 24 quoted lines
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > It looks like something has broken git-applymbox lately.
> >
> > The "From: authorname" lines are no longer removed from the message, and 
> > are duplicated in the commit log. This has resulted in several recent 
> > kernel commits looking like this:
> 
> Agreed.  That isn't terribly desirable.
> Do you have the original email message some place?
> 
> There is an odd case where if someone put the From: header
> in the middle of the text that we now notice and process and I
> didn't feel right about removing a line from the middle of the
> text.
> 
> I was fixing a nasty corner case that happens if there aren't any
> mail headers at all passed to git-mailinfo.  Where we could drop
> lines without processing them at all.
> 
> This doesn't look like the From: header was in the middle of the
> message until it was imported into git so it is probably a small
> logic error that is easily corrected.  But I need to see what
> we are parsing so I can understand what is happening.

I hate to say this, because I'm bad about it, too, but we should probably have a few tests for applymbox, to cover the various scenarios discussed in this thread.

-- 
Ryan Anderson
  sometimes Pug Majere

← back to recent threads