[PATCH] Teach applymbox to keep the Subject: line.

This is a companion patch to the previous format-patch fix.
With "-k", format-patch can be told not to remove the [PATCH] in
the original commit, nor to add the [PATCH] on its own.

However, applymbox toolchain has a code to remove [PATCH] (among
other things) from the Subject: line, which is the right thing
to do when dealing with real e-mailed patches, but it interferes
with the "format-patch to applymbox" cherry-picking.  The -k
flag disables this.

Signed-off-by: Junio C Hamano <redacted>
---

 tools/git-applymbox  |   11 ++++++++---
 tools/git-applypatch |    5 ++++-
 tools/mailinfo.c     |   15 +++++++++++++++
 3 files changed, 27 insertions(+), 4 deletions(-)

6bff6a60680ef402f614abae8189c2cb198cfa49

diff --git a/tools/git-applymbox b/tools/git-applymbox
--- a/tools/git-applymbox
+++ b/tools/git-applymbox

@@ -9,7 +9,7 @@
 ## You give it a mbox-format collection of emails, and it will try to
 ## apply them to the kernel using "applypatch"
 ##
-## applymbox [ -q ] (-c .dotest/msg-number | mail_archive) [Signoff_file]"
+## applymbox [ -k ] [ -q ] (-c .dotest/msg-number | mail_archive) [Signoff_file]"
 ##
 ## The patch application may fail in the middle.  In which case:
 ## (1) look at .dotest/patch and fix it up to apply

@@ -18,10 +18,11 @@
 ## use a Signoff_file, because applypatch wants to append the sign-off
 ## message to msg-clean every time it is run.
 
-query_apply= continue= resume=t
+keep_subject= query_apply= continue= resume=t
 while case "$#" in 0) break ;; esac
 do
 	case "$1" in
+	-k)	keep_subject=-k ;;
 	-q)	query_apply=t ;;
 	-c)	continue="$2"; resume=f; shift ;;
 	-*)	usage ;;

@@ -41,6 +42,9 @@ esac
 case "$query_apply" in
 t)	touch .dotest/.query_apply
 esac
+case "$keep_subject" in
+-k)	: >.dotest/.keep_subject
+esac
 
 signoff="$1"
 set x .dotest/0*

@@ -52,7 +56,8 @@ do
     f,$i)	resume=t;;
     f,*)	continue;;
     *)
-	    git-mailinfo .dotest/msg .dotest/patch <$i >.dotest/info || exit 1
+	    git-mailinfo $keep_subject \
+		.dotest/msg .dotest/patch <$i >.dotest/info || exit 1
 	    git-stripspace < .dotest/msg > .dotest/msg-clean
 	    ;;
     esac

diff --git a/tools/git-applypatch b/tools/git-applypatch
--- a/tools/git-applypatch
+++ b/tools/git-applypatch

@@ -16,6 +16,7 @@ final=.dotest/final-commit
 ## If this file exists, we ask before applying
 ##
 query_apply=.dotest/.query_apply
+keep_subject=.dotest/.keep_subject
 MSGFILE=$1
 PATCHFILE=$2
 INFO=$3

@@ -30,8 +31,10 @@ export SUBJECT="$(sed -n '/^Subject/ s/S
 if [ -n "$signoff" -a -f "$signoff" ]; then
 	cat $signoff >> $MSGFILE
 fi
+patch_header=
+test -f "$keep_subject" || patch_header='[PATCH] '
 
-(echo "[PATCH] $SUBJECT" ; if [ -s $MSGFILE ]; then echo ; cat $MSGFILE; fi ) > $final
+(echo "$patch_header$SUBJECT" ; if [ -s $MSGFILE ]; then echo ; cat $MSGFILE; fi ) > $final
 
 f=0
 [ -f $query_apply ] || f=1

diff --git a/tools/mailinfo.c b/tools/mailinfo.c
--- a/tools/mailinfo.c
+++ b/tools/mailinfo.c

@@ -9,6 +9,7 @@
 
 static FILE *cmitmsg, *patchfile;
 
+static int keep_subject = 0;
 static char line[1000];
 static char date[1000];
 static char name[1000];

@@ -101,6 +102,8 @@ static void check_line(char *line, int l
 
 static char * cleanup_subject(char *subject)
 {
+	if (keep_subject)
+		return subject;
 	for (;;) {
 		char *p;
 		int len, remove;

@@ -242,8 +245,20 @@ static void usage(void)
 	exit(1);
 }
 
+static const char mailinfo_usage[] =
+"git-mailinfo [-k] msg patch <mail >info";
 int main(int argc, char ** argv)
 {
+	while (1 < argc && argv[1][0] == '-') {
+		if (!strcmp(argv[1], "-k"))
+			keep_subject = 1;
+		else {
+			fprintf(stderr, "usage: %s\n", mailinfo_usage);
+			exit(1);
+		}
+		argc--; argv++;
+	}
+
 	if (argc != 3)
 		usage();
 	cmitmsg = fopen(argv[1], "w");

On Tue, 16 Aug 2005, Junio C Hamano wrote:

This is a companion patch to the previous format-patch fix.
With "-k", format-patch can be told not to remove the [PATCH] in
the original commit, nor to add the [PATCH] on its own.

I think this might be the point in time to just make the "[PATCH]" prefix 
go away.

It made much more sense with BK than it does with git: since git keeps 
track of "author" and "committer" separately, you can always see when the 
committer wasn't the author of the change, which is what the "[PATCH]" 
thing was all about. 

In other words, at least for the kernel, [PATCH] was a marker that said 
"the author didn't commit this directly". Git already has that information 
explicitly in the git data.

(Also, with proper "Signed-off-by:" lines it's also always clear that 
there were other people involved, and that the author of the patch is 
different from the person who applied it).

So I would personally not mind if we just made the "[PATCH]" prefix go 
away entirely, or perhaps had a separate flag to "git-applymbox" that told 
it to prepend a specific prefix to the Subject: line (which might not be 
"[PATCH] " at all) defaulting to "no prefix".

			Linus

PS. Another historical reason for marking patches explicitly was that
people were worried that introducing BK would somehow make the old
patch-based submissions be "second-class citizens". It was easy to make
statistics and show that at least half the real changes (as opposed to
merges) were still patch-based. So again, the "PATCH" marker had some 
historical relevance, but I don't think it matters any more.

If someone is thus motivated, I have two requests in this area:

1) Fix applymbox such that it understands RFC822-valid Subject lines 
which wrap across multiple text lines.

2) Teach it to understand MIME, and not treat the MIME headers like part 
of the message.

Jeff Garzik [off-list ref] writes:

1) Fix applymbox such that it understands RFC822-valid Subject lines 
which wrap across multiple text lines.

I thought I did this in mailinfo (read_one_header() function)
some time ago.  I'd appreciate it if you could point me at a
sample message that fails to get processed correctly.

2) Teach it to understand MIME, and not treat the MIME headers like part 
of the message.

I share the same gripe; I always end up running applymbox -q and
hand-fixing things for this reason.

On Wed, 17 Aug 2005, Jeff Garzik wrote:

1) Fix applymbox such that it understands RFC822-valid Subject lines 
which wrap across multiple text lines.

It already should do this.

2) Teach it to understand MIME, and not treat the MIME headers like part 
of the message.

But this one I really realyl disagree with.

The fact is, anybody who doesn't edit the emails that come in is BROKEN. 
There are two kinds of emails:

 - the nicely formatted ones where the author follows all the rules

   This kind of email doesn't need MIME decoding anyway.

 - the others

   This kind might need MIME decoding, but since it _also_ needs 
   hand-editing to remove all the "Hi, please apply this patch" etc crud
   that inevitably go along with this kind of patch, trying to handle it 
   automatically is WRONG WRONG WRONG.

   And if it's mime-encoded you often have trouble editing it anyway.

Ergo: if somebody sends you mime-encoded patches, hit them with a baseball 
bat (politely) and teach them not to do that. "Fixing" the tools really 
will just make things worse if it means that you apply raw emails without 
having edited them.

		Linus

Linus Torvalds [off-list ref] writes:

The fact is, anybody who doesn't edit the emails that come in is BROKEN. 
There are two kinds of emails:

 - the nicely formatted ones where the author follows all the rules

   This kind of email doesn't need MIME decoding anyway.

That depends on what the rules are, but I consider detecting B
encodings in the header fields and transliterating it into UTF-8
a good idea (although that sometimes is a lossy conversion
depending on the original charset).

Remember this one that prompted me to do the header folding?

    From: YOSHIFUJI Hideaki / =?iso-2022-jp?B?GyRCNUhGIzFRTEAbKEI=?= 
            [off-list ref]

 - the others

   ..., trying to handle it 
   automatically is WRONG WRONG WRONG.
   And if it's mime-encoded you often have trouble editing it anyway.

Ergo: if somebody sends you mime-encoded patches, hit them with a baseball 
bat (politely) and teach them not to do that. "Fixing" the tools really 
will just make things worse if it means that you apply raw emails without 
having edited them.

I agree with you in principle and that is why I always run
applymbox with the -q flag.  Maybe I should start trying the
baseball bat approach to see what happens.

Hi,

On Wed, 17 Aug 2005, Linus Torvalds wrote:

On Wed, 17 Aug 2005, Jeff Garzik wrote:

quoted

2) Teach it to understand MIME, and not treat the MIME headers like part 
of the message.

[...]

Ergo: if somebody sends you mime-encoded patches, hit them with a baseball 
bat (politely) and teach them not to do that. "Fixing" the tools really 
will just make things worse if it means that you apply raw emails without 
having edited them.

I'd say that QP and the MIME/alternate formats are not really the fault of 
the sender, but rather the mailer. So there might be value in supporting 
at least these, to make the life of maintainers easier.

Ciao,
Dscho

Linus Torvalds [off-list ref] writes:

On Wed, 17 Aug 2005, Jeff Garzik wrote:

quoted

1) Fix applymbox such that it understands RFC822-valid Subject lines 
which wrap across multiple text lines.

It already should do this.

quoted

2) Teach it to understand MIME, and not treat the MIME headers like part 
of the message.

But this one I really realyl disagree with.

The fact is, anybody who doesn't edit the emails that come in is BROKEN. 
There are two kinds of emails:

 - the nicely formatted ones where the author follows all the rules

   This kind of email doesn't need MIME decoding anyway.

Unless they want to write something that doesn't fit in ASCII, as
discussed in another thread here.

But maybe you are only talking about MIME attachments, and not about
MIME content encodings?  We probably need to separate the two.

Note that I'm not really talking about your patch handling for Linux;
you are free to disallow my name in Linux patches if you want to.  But
I'd like to see a way to get rid of that limitation for other uses of
git.

-- 
David Kågedal

(Also, with proper "Signed-off-by:" lines it's also always clear that 
there were other people involved, and that the author of the patch is 
different from the person who applied it).

I almost always handedit my mails and I find myself forgetting to add
"Signed-off-by" from time to time.
Is there a simple way to implment a trigger that can check that _I_
signed off the patch before applying it?

I prefer to add it myself rather than to have it added automatically -
but mayve thats you me being a bit mistrusting.

Btw. I'm a Cogito user if that makes a difference.
The only git- command I use today is git-applymbox.

	Sam

On Thu, 18 Aug 2005, Sam Ravnborg wrote:

I almost always handedit my mails and I find myself forgetting to add
"Signed-off-by" from time to time.
Is there a simple way to implment a trigger that can check that _I_
signed off the patch before applying it?

Well, Junio has been talking about adding commit hooks. I don't think
that's been done. The idea being that you could verify that the thing 
you're committing follows certain rules (no bad whitespace added in the 
diff, sign-offs in the messages, whatever).

That said, git-applypatch (which is what git-applymbox ends up calling) 
does not use the general "git commit" script. So it would have to have its 
own hook. The script is pretty easy to read, though: just look at 
git-applypatch, and notice that the last stages are:

	...
	git-apply --index $PATCHFILE || exit 1
	tree=$(git-write-tree) || exit 1
	echo Wrote tree $tree
	commit=$(git-commit-tree $tree -p $(cat .git/HEAD) < $final) || exit 1
	echo Committed: $commit
	echo $commit > .git/HEAD

and that just before this thing you could easily add some sanity checking 
by hand. The commit message at that point is in the "$final" file, and the 
patch is obviously in $PATCHFILE, so you can verify either of those to 
your hearts content.

The only question is what the hook/trigger should look like. just put
something like

	[ -x .git/hooks/applypatch-hook ] &&
	     .git/hooks/applypatch-hook "$tree" "$PATCHFILE" || exit

at the line before that "git-apply" perhaps? Then, you could install your 
own applypatch hook which looks at the message or the patch?

		Linus

Sam Ravnborg [off-list ref] writes:

I prefer to add it myself rather than to have it added automatically -
but mayve thats you me being a bit mistrusting.

The only git- command I use today is git-applymbox.

If you did not have that "add it myself" preference, I would
have recommended the (not counting the flags) second parameter
to git-applymbox.

While we are on the topic of applymbox, currently it takes this
form:

$ applymbox [ -k ] [ -q ] (-c .dotest/msg_num | mail_archive) [Signoff_file]"

It may make more sense to change it to:

    applymbox [-k] [-q] [-s <signoff>] ( -c .dotest/<msg_num> | <mbox>... )

Linus Torvalds [off-list ref] writes:

Well, Junio has been talking about adding commit hooks. I don't think
that's been done.

No, notyet.  But tonight.

The only question is what the hook/trigger should look like. just put
something like

	[ -x .git/hooks/applypatch-hook ] &&
	     .git/hooks/applypatch-hook "$tree" "$PATCHFILE" || exit

at the line before that "git-apply" perhaps? Then, you could install your 
own applypatch hook which looks at the message or the patch?

Sounds sensible.  The hook would probably want a handy access to
the commit object as well to catch:

     - the author name may be spelled wrong or has funky
       B-encodings still left.

     - some people might want to even run spellchecker on the
       commit message.

     - lack of S-O-B line.