format-patch: escape "From " lines recognized by mailsplit

builtin/mailsplit.c +1 -31

index 3068168..bb8f9c9 100644
@@ -8,41 +8,11 @@
 #include "builtin.h"
 #include "string-list.h"
 #include "strbuf.h"
+#include "mailinfo.h"
 
 static const char git_mailsplit_usage[] =
 "git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]";
 
-static int is_from_line(const char *line, int len)
-{
-	const char *colon;
-
-	if (len < 20 || memcmp("From ", line, 5))
-		return 0;
-
-	colon = line + len - 2;
-	line += 5;
-	for (;;) {
-		if (colon < line)
-			return 0;
-		if (*--colon == ':')
-			break;
-	}
-
-	if (!isdigit(colon[-4]) ||
-	    !isdigit(colon[-2]) ||
-	    !isdigit(colon[-1]) ||
-	    !isdigit(colon[ 1]) ||
-	    !isdigit(colon[ 2]))
-		return 0;
-
-	/* year */
-	if (strtol(colon+3, NULL, 10) <= 90)
-		return 0;
-
-	/* Ok, close enough */
-	return 1;
-}
-
 static struct strbuf buf = STRBUF_INIT;
 static int keep_cr;
 static int mboxrd;

mailinfo.c +31 -0

index 9f19ca1..0ebd953 100644
@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
 
 	strbuf_release(&mi->log_message);
 }
+
+int is_from_line(const char *line, int len)
+{
+	const char *colon;
+
+	if (len < 20 || memcmp("From ", line, 5))
+		return 0;
+
+	colon = line + len - 2;
+	line += 5;
+	for (;;) {
+		if (colon < line)
+			return 0;
+		if (*--colon == ':')
+			break;
+	}
+
+	if (!isdigit(colon[-4]) ||
+	    !isdigit(colon[-2]) ||
+	    !isdigit(colon[-1]) ||
+	    !isdigit(colon[ 1]) ||
+	    !isdigit(colon[ 2]))
+		return 0;
+
+	/* year */
+	if (strtol(colon+3, NULL, 10) <= 90)
+		return 0;
+
+	/* Ok, close enough */
+	return 1;
+}

mailinfo.h +0 -0

index 93776a7..c1430a0 100644

I think that this change:

deserves to live in a separate patch... It would make the real change stick out better.

mailinfo.c +21 -0

index 9f19ca1..0ebd953 100644
@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
 
 	strbuf_release(&mi->log_message);
 }
+
+int is_from_line(const char *line, int len)
+{
+	const char *colon;
+
+	if (len < 20 || memcmp("From ", line, 5))
+		return 0;
+
+	colon = line + len - 2;
+	line += 5;
+	for (;;) {
+		if (colon < line)
+			return 0;
+		if (*--colon == ':')
+			break;
+	}
+
+	if (!isdigit(colon[-4]) ||
+	    !isdigit(colon[-2]) ||
+	    !isdigit(colon[-1]) ||
+	    !isdigit(colon[ 1]) ||

Quoted 34 lines · Eric Wong

mailinfo.c +26 -0

index 9f19ca1..0ebd953 100644
@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
 
 	strbuf_release(&mi->log_message);
 }
+
+int is_from_line(const char *line, int len)
+{
+	const char *colon;
+
+	if (len < 20 || memcmp("From ", line, 5))
+		return 0;
+
+	colon = line + len - 2;
+	line += 5;
+	for (;;) {
+		if (colon < line)
+			return 0;
+		if (*--colon == ':')
+			break;
+	}
+
+	if (!isdigit(colon[-4]) ||
+	    !isdigit(colon[-2]) ||
+	    !isdigit(colon[-1]) ||
+	    !isdigit(colon[ 1]) ||
+	    !isdigit(colon[ 2]))
+		return 0;
+
+	/* year */
+	if (strtol(colon+3, NULL, 10) <= 90)

mailinfo.c void clear_mailinfo(struct mailinfo *mi) · line 1068

Quoted 5 lines

mailinfo.c +5 -0

+		return 0;
+
+	/* Ok, close enough */
+	return 1;
+}

Should this be made more strict, like by checking for a space before the year?

Hi Andreas,

Quoted 36 lines · Andreas Schwab

Quoted 34 lines · Eric Wong

mailinfo.c +26 -0

index 9f19ca1..0ebd953 100644
@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
 
 	strbuf_release(&mi->log_message);
 }
+
+int is_from_line(const char *line, int len)
+{
+	const char *colon;
+
+	if (len < 20 || memcmp("From ", line, 5))
+		return 0;
+
+	colon = line + len - 2;
+	line += 5;
+	for (;;) {
+		if (colon < line)
+			return 0;
+		if (*--colon == ':')
+			break;
+	}
+
+	if (!isdigit(colon[-4]) ||
+	    !isdigit(colon[-2]) ||
+	    !isdigit(colon[-1]) ||
+	    !isdigit(colon[ 1]) ||
+	    !isdigit(colon[ 2]))
+		return 0;
+
+	/* year */
+	if (strtol(colon+3, NULL, 10) <= 90)

Quoted 8 lines

mailinfo.c void clear_mailinfo(struct mailinfo *mi) · line 1068

Quoted 5 lines

mailinfo.c +5 -0

+		return 0;
+
+	/* Ok, close enough */
+	return 1;
+}

Should this be made more strict, like by checking for a space before the year?

This patch only moves the function, so it would be inappropriate to change it.

If you want to make it stricter, you will have to submit a separate patch.

Quoted 18 lines

mailinfo.c +18 -0

+	colon = line + len - 2;
+	line += 5;
+	for (;;) {
+		if (colon < line)
+			return 0;
+		if (*--colon == ':')
+			break;
+	}
+
+	if (!isdigit(colon[-4]) ||
+	    !isdigit(colon[-2]) ||
+	    !isdigit(colon[-1]) ||
+	    !isdigit(colon[ 1]) ||
+	    !isdigit(colon[ 2]))
+		return 0;
+
+	/* year */
+	if (strtol(colon+3, NULL, 10) <= 90)

mailinfo.c void clear_mailinfo(struct mailinfo *mi) · line 1068

Quoted 5 lines

mailinfo.c +5 -0

+		return 0;
+
+	/* Ok, close enough */
+	return 1;
+}

Should this be made more strict, like by checking for a space before the year?

pretty.c +2 -3

@@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
 			strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
 					     line, linelen);
 		else {
-			if (pp->fmt == CMIT_FMT_MBOXRD &&
-					is_mboxrd_from(line, linelen))
-				strbuf_addch(sb, '>');
+			switch (pp->fmt) {
+			case CMIT_FMT_EMAIL:

t/t4014-format-patch.sh +0 -0

index 1206c48..8fa3982 100755
@@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '
 	test_cmp expect actual

t/t4014-format-patch.sh +5 -0

+	git grep -h --no-index \
+		">From 0000000000000000000000000000000000000000 " \
+		patch
+'
+
 test_done

Signature · 1 line

EW

Quoted 9 lines · Eric Wong

pretty.c +2 -3

@@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
 			strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
 					     line, linelen);
 		else {
-			if (pp->fmt == CMIT_FMT_MBOXRD &&
-					is_mboxrd_from(line, linelen))
-				strbuf_addch(sb, '>');
+			switch (pp->fmt) {
+			case CMIT_FMT_EMAIL:

pretty.c void pp_remainder(struct pretty_print_context *pp, · line 1760

Quoted 10 lines

pretty.c +10 -0

+				if (is_from_line(line, linelen))
+					strbuf_addch(sb, '>');
+				break;
+			case CMIT_FMT_MBOXRD:
+				if (is_mboxrd_from(line, linelen))
+					strbuf_addch(sb, '>');
+				break;
+			default:
+				break;
+			}

Sorry to be nitpicking once again; I think this would be conciser (and easier to read at least for me) as:

Users have mistakenly copied "From " lines into commit messages in the past, and will certainly make the same mistakes in the future. Since not everyone uses mboxrd, yet, we should at least prevent miss-split mails by always escaping "From " lines based on the check used by mailsplit.

mailsplit will not perform unescaping by default, yet, as it could cause further invocations of format-patch from old versions of git to generate bad output. Propagating the mboxo escaping is preferable to miss-split patches. Unescaping may still be performed via "--mboxrd".

As a tool to produce mbox file, quoting like this in format-patch output may make sense, I would think, but shouldn't send-email undo this when sending individual patches?

Quoted 11 lines · Eric Wong

Users have mistakenly copied "From " lines into commit messages in the past, and will certainly make the same mistakes in the future. Since not everyone uses mboxrd, yet, we should at least prevent miss-split mails by always escaping "From " lines based on the check used by mailsplit.

mailsplit will not perform unescaping by default, yet, as it could cause further invocations of format-patch from old versions of git to generate bad output. Propagating the mboxo escaping is preferable to miss-split patches. Unescaping may still be performed via "--mboxrd".

As a tool to produce mbox file, quoting like this in format-patch output may make sense, I would think, but shouldn't send-email undo this when sending individual patches?

Quoted 14 lines · Junio C Hamano

Quoted 11 lines · Eric Wong

Users have mistakenly copied "From " lines into commit messages in the past, and will certainly make the same mistakes in the future. Since not everyone uses mboxrd, yet, we should at least prevent miss-split mails by always escaping "From " lines based on the check used by mailsplit.

mailsplit will not perform unescaping by default, yet, as it could cause further invocations of format-patch from old versions of git to generate bad output. Propagating the mboxo escaping is preferable to miss-split patches. Unescaping may still be performed via "--mboxrd".

Quoted 3 lines

As a tool to produce mbox file, quoting like this in format-patch output may make sense, I would think, but shouldn't send-email undo this when sending individual patches?

Also, doesn't it break "git rebase" (non-interactive), or anything that internally runs format-patch to individual files and then runs am on each of them, anything that knows that each output file from format-patch corresponds to a single change and there is no need to split, badly if we do this unconditionally?

IOW, shouldn't this be an optional feature to format-patch that is triggered by passing a new command line option that currently nobody is passing?

Quoted 5 lines

Also, doesn't it break "git rebase" (non-interactive), or anything that internally runs format-patch to individual files and then runs am on each of them, anything that knows that each output file from format-patch corresponds to a single change and there is no need to split, badly if we do this unconditionally?

Yes, rebase should probably unescape is_from_line matches.

Quoted 3 lines

IOW, shouldn't this be an optional feature to format-patch that is triggered by passing a new command line option that currently nobody is passing?

Quoted 6 lines · Eric Wong

Quoted 5 lines

Also, doesn't it break "git rebase" (non-interactive), or anything that internally runs format-patch to individual files and then runs am on each of them, anything that knows that each output file from format-patch corresponds to a single change and there is no need to split, badly if we do this unconditionally?

Quoted 1 line

Yes, rebase should probably unescape is_from_line matches.

This shouldn't matter for "git rebase", as it only reads from the mbox "From <commit object name> <datestamp>" line to learn the original commit and extract the log message directly from there.

But a third-party script that wants to read format-patch output would be forced to upgrade, which is a pain if we make this change unconditionally.

Quoted 3 lines

Quoted 3 lines

IOW, shouldn't this be an optional feature to format-patch that is triggered by passing a new command line option that currently nobody is passing?