[PATCHv2 0/4] Using git-mailsplit in mixed line ending environment

[PATCHv2 0/4] Using git-mailsplit in mixed line ending environment

[Stefan-W. Hahn]

Hello,

I'm using git in environments with files having dos or unix line
ending. I apply patches using 'git format-patch ... | git am ...'.
A change in git-mailsplit in commit c2ca1d79 introduced a change in
the default behaviour of git-mailsplit when splitting mbox patches. It
makes dos line endings to unix line endings. With this behaviour it is
impossible to apply patches.

The following patches introduce the '--kepp-cr' parameter to git-am an
an additional possibility to set '--keep-cr' via configuration for
git-mailsplit. Also I added missing description for '--keep-cr' of
git-mailsplit.

Second round:
I changed 'mailsplit.keep-cr' to 'mailsplit.keepcr' as suggested by Jakub
and a comment in the testcase.

Stefan-W. Hahn (4):
  git-mailsplit: Show parameter '--keep-cr' in usage and documentation
  git-mailsplit: add `mailsplit.keepcr` configuration variable.
  git-am: Add command line parameter `--keep-cr` passing it to
    git-mailsplit.
  Adding test for `--keep-cr` for git-mailsplit and git-am.

 Documentation/config.txt        |    4 ++
 Documentation/git-mailsplit.txt |    5 ++-
 builtin-mailsplit.c             |   12 ++++++-
 git-am.sh                       |   18 +++++++--
 t/t5101-mailinfo-dos.sh         |   74 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 107 insertions(+), 6 deletions(-)
 create mode 100644 t/t5101-mailinfo-dos.sh

[PATCH 1/4] git-mailsplit: Show parameter '--keep-cr' in usage and documentation

[Stefan-W. Hahn]

Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
 Documentation/git-mailsplit.txt |    5 ++++-
 builtin-mailsplit.c             |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-mailsplit.txt b/Documentation/git-mailsplit.txt
index 5cc94ec..a634485 100644
--- a/Documentation/git-mailsplit.txt
+++ b/Documentation/git-mailsplit.txt
@@ -7,7 +7,7 @@ git-mailsplit - Simple UNIX mbox splitter program
 
 SYNOPSIS
 --------
-'git mailsplit' [-b] [-f<nn>] [-d<prec>] -o<directory> [--] [<mbox>|<Maildir>...]
+'git mailsplit' [-b] [-f<nn>] [-d<prec>] [--keep-cr] -o<directory> [--] [<mbox>|<Maildir>...]
 
 DESCRIPTION
 -----------
@@ -43,6 +43,9 @@ OPTIONS
 	Skip the first <nn> numbers, for example if -f3 is specified,
 	start the numbering with 0004.
 
+--keep-cr::
+	Do not remove `\r` from lines ending with `\r\n`.
+
 Author
 ------
 Written by Linus Torvalds <torvalds@osdl.org>
diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index 207e358..cdfc1b7 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c
@@ -10,7 +10,7 @@
 #include "strbuf.h"
 
 static const char git_mailsplit_usage[] =
-"git mailsplit [-d<prec>] [-f<n>] [-b] -o<directory> [<mbox>|<Maildir>...]";
+"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [<mbox>|<Maildir>...]";
 
 static int is_from_line(const char *line, int len)
 {
-- 
1.7.0.rc2.17.g39385

[PATCH 2/4] git-mailsplit: add `mailsplit.keepcr` configuration variable.

[Stefan-W. Hahn]

If using git-mailsplit in environments where files with dos and unix
line ending stay in one repository mbox patches must be split with
additional parameter '--keep-cr', because the behaviour of
git-mailsplit have been changed in commit c2ca1d79.

With this patch the behaviour of git-mailsplit can be set via
configuration file.

Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
 Documentation/config.txt |    4 ++++
 builtin-mailsplit.c      |   10 ++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4c36aa9..37a4968 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1246,6 +1246,10 @@ mailmap.file::
 	subdirectory, or somewhere outside of the repository itself.
 	See linkgit:git-shortlog[1] and linkgit:git-blame[1].
 
+mailsplit.keepcr::
+	If true git-mailsplit will not remove `\r` from lines ending
+	with `\r\n`. See linkgit:git-mailsplit[1].
+
 man.viewer::
 	Specify the programs that may be used to display help in the
 	'man' format. See linkgit:git-help[1].
diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c
index cdfc1b7..e76974d 100644
--- a/builtin-mailsplit.c
+++ b/builtin-mailsplit.c
@@ -210,6 +210,15 @@ out:
 	return ret;
 }
 
+static int git_mailsplit_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "mailsplit.keepcr")) {
+		keep_cr = git_config_bool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value, cb);
+}
+
 int cmd_mailsplit(int argc, const char **argv, const char *prefix)
 {
 	int nr = 0, nr_prec = 4, num = 0;
@@ -218,6 +227,7 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)
 	const char **argp;
 	static const char *stdin_only[] = { "-", NULL };
 
+	git_config(git_mailsplit_config, NULL);
 	for (argp = argv+1; *argp; argp++) {
 		const char *arg = *argp;
 
-- 
1.7.0.rc2.17.g39385

Re: [PATCH 2/4] git-mailsplit: add `mailsplit.keepcr` configuration variable.

[Junio C Hamano]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> If using git-mailsplit in environments where files with dos and unix
> line ending stay in one repository mbox patches must be split with
> additional parameter '--keep-cr', because the behaviour of
> git-mailsplit have been changed in commit c2ca1d79.
>
> With this patch the behaviour of git-mailsplit can be set via
> configuration file.

This probably is a bad change.  mailsplit is designed to be usable without
being inside any git repository.  With this change, doesn't the command
start failing to run when run outside a git repository?

Re: [PATCH 2/4] git-mailsplit: add `mailsplit.keepcr` configuration variable.

[Stefan-W. Hahn]

Also sprach Junio C Hamano am Thu, 11 Feb 2010 at 15:21:52 -0800:

> > With this patch the behaviour of git-mailsplit can be set via
> > configuration file.
> 
> This probably is a bad change.  mailsplit is designed to be usable without
> being inside any git repository.  With this change, doesn't the command
> start failing to run when run outside a git repository?

Ouch, I didn't mind this.

But perhaps it will be a better aproach to make such a configuration
available for git-am. I will make a patch for this soon.

Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			

Please note that according to the German law on data retention,
information on every electronic information exchange with me is
retained for a period of six months.
Bundesgesetzblatt:
http://www.bgblportal.de/BGBL/bgbl1f/bgbl107s3198.pdf

Re: [PATCH 2/4] git-mailsplit: add `mailsplit.keepcr` configuration variable.

[Junio C Hamano]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> But perhaps it will be a better aproach to make such a configuration
> available for git-am. I will make a patch for this soon.

If a configuration is desired, I think that is probably a saner place to
put it.

Regardless of where the configuration goes between am and mailsplit, you
need to think about the interaction between rebase and am.  You might set
the configuration for am---should that automatically and always kick in
when you run rebase?  Do we need some way to override it per invocation
when running rebase?

Re: [PATCH 2/4] git-mailsplit: add `mailsplit.keepcr` configuration variable.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
>
>> But perhaps it will be a better aproach to make such a configuration
>> available for git-am. I will make a patch for this soon.
>
> If a configuration is desired, I think that is probably a saner place to
> put it.
>
> Regardless of where the configuration goes between am and mailsplit, you
> need to think about the interaction between rebase and am.  You might set
> the configuration for am---should that automatically and always kick in
> when you run rebase?  Do we need some way to override it per invocation
> when running rebase?

Nah, I wasn't thinking straight.  There is no reason to turn it off while
rebase is running.  That means under --rebasing mode, am should ignore the
configuration and always do keep-cr, even when am.keepcr is set to false.

Sorry about the noise.

Re: [PATCH 2/4] git-mailsplit: add `mailsplit.keepcr` configuration variable.

[Stefan-W. Hahn]

Also sprach Junio C Hamano am Fri, 12 Feb 2010 at 13:49:12 -0800:
> Regardless of where the configuration goes between am and mailsplit, you
> need to think about the interaction between rebase and am.  You might set
> the configuration for am---should that automatically and always kick in
> when you run rebase?  Do we need some way to override it per invocation
> when running rebase?

I did not have had any problem with git-rebase in mixed line ending
environment. If used via git-rebase, git-mailsplit should called
with --keep-cr set, independent from configuration for this.

Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			

Please note that according to the German law on data retention,
information on every electronic information exchange with me is
retained for a period of six months.
Bundesgesetzblatt:
http://www.bgblportal.de/BGBL/bgbl1f/bgbl107s3198.pdf

[PATCH 3/4] git-am: Add command line parameter `--keep-cr` passing it to git-mailsplit.

[Stefan-W. Hahn]

If applying patches with the following command sequence

   git format-patch --stdout ... | git am ...

in repositories having files with dos and unix line endings
git-mailsplit, which is called from git-am must be called with
`--keep-cr` parameter since commit c2ca1d79.

This patch adds the command line parameter `--keep-cr` for git-am.

Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
 git-am.sh |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c8b9cbb..c3101ae 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -15,6 +15,7 @@ q,quiet         be quiet
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
+e,keep-cr       pass --keep-cr flag to git-mailsplit for mbox format
 c,scissors      strip everything before a scissors line
 whitespace=     pass it through git-apply
 ignore-space-change pass it through git-apply
@@ -216,10 +217,12 @@ check_patch_format () {
 split_patches () {
 	case "$patch_format" in
 	mbox)
-		case "$rebasing" in
-		'')
+		case "$rebasing,$keepcr" in
+		'','')
 			keep_cr= ;;
-		?*)
+		'',t)
+			keep_cr=--keep-cr ;;
+		?*,t)
 			keep_cr=--keep-cr ;;
 		esac
 		git mailsplit -d"$prec" -o"$dotest" -b $keep_cr -- "$@" > "$dotest/last" ||
@@ -290,7 +293,7 @@ split_patches () {
 
 prec=4
 dotest="$GIT_DIR/rebase-apply"
-sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=
+sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
 resolvemsg= resume= scissors= no_inbody_headers=
 git_apply_opt=
 committer_date_is_author_date=
@@ -347,6 +350,8 @@ do
 		allow_rerere_autoupdate="$1" ;;
 	-q|--quiet)
 		GIT_QUIET=t ;;
+        -e|--keep-cr)
+                keepcr=t ;;
 	--)
 		shift; break ;;
 	*)
@@ -452,6 +457,7 @@ else
 	echo "$sign" >"$dotest/sign"
 	echo "$utf8" >"$dotest/utf8"
 	echo "$keep" >"$dotest/keep"
+	echo "$keepcr" >"$dotest/keepcr"
 	echo "$scissors" >"$dotest/scissors"
 	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
 	echo "$GIT_QUIET" >"$dotest/quiet"
@@ -495,6 +501,10 @@ if test "$(cat "$dotest/keep")" = t
 then
 	keep=-k
 fi
+if test "$(cat "$dotest/keepcr")" = t
+then
+	keepcr=--keep-cr
+fi
 case "$(cat "$dotest/scissors")" in
 t)
 	scissors=--scissors ;;
-- 
1.7.0.rc2.17.g39385

Re: [PATCH 3/4] git-am: Add command line parameter `--keep-cr` passing it to git-mailsplit.

[Junio C Hamano]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> If applying patches with the following command sequence
>
>    git format-patch --stdout ... | git am ...
>
> in repositories having files with dos and unix line endings
> git-mailsplit, which is called from git-am must be called with
> `--keep-cr` parameter since commit c2ca1d79.

Sorry, I cannot parse this.

Perhaps you meant to have a comma between "git-am" and "must be"?

> +e,keep-cr       pass --keep-cr flag to git-mailsplit for mbox format

This short form -e does not make much sense to me.  Why is it -e, and do
we even need a short form in the first place?

I'd say we should drop it.  We may want to use short-and-sweet 'e' for
something more important and common, and we would regret for letting this
option squat on it later.

Another approach _might_ be to let the user to use --rebasing directly; it
currently is documented as "internal use", but as long as we clearly
specify its semantics and give a synonym that is more sensible than the
current name, it might turn out to be a better option.  I dunno; it might
be doing more than what this new use case may want to do.

> @@ -216,10 +217,12 @@ check_patch_format () {
>  split_patches () {
>  	case "$patch_format" in
>  	mbox)
> -		case "$rebasing" in
> -		'')
> +		case "$rebasing,$keepcr" in
> +		'','')
>  			keep_cr= ;;
> -		?*)
> +		'',t)
> +			keep_cr=--keep-cr ;;
> +		?*,t)
>  			keep_cr=--keep-cr ;;

Did you mean to say:

	case "$r$k" in
        '') keep_cr= ;;
        ?*) keep_cr=--keep-cr ;;
        esac

or even:

	if test -n "$r$k"
        then
        	keep_cr=--keep-cr
	else
        	keep_cr=
	fi

Re: [PATCH 3/4] git-am: Add command line parameter `--keep-cr` passing it to git-mailsplit.

[Stefan-W. Hahn]

Also sprach Junio C Hamano am Thu, 11 Feb 2010 at 15:21:32 -0800:
> "Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
> 
> > If applying patches with the following command sequence
> >
> >    git format-patch --stdout ... | git am ...
> >
> > in repositories having files with dos and unix line endings
> > git-mailsplit, which is called from git-am must be called with
> > `--keep-cr` parameter since commit c2ca1d79.
> 
> Sorry, I cannot parse this.

What?

> Perhaps you meant to have a comma between "git-am" and "must be"?
> 
> > +e,keep-cr       pass --keep-cr flag to git-mailsplit for mbox format
> 
> This short form -e does not make much sense to me.  Why is it -e, and do
> we even need a short form in the first place?

Just for shortening the commandline.

> or even:
> 
> 	if test -n "$r$k"
>         then
>         	keep_cr=--keep-cr
> 	else
>         	keep_cr=
> 	fi

This one. I will make a patch soon.

Stefan
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			

Please note that according to the German law on data retention,
information on every electronic information exchange with me is
retained for a period of six months.
Bundesgesetzblatt:
http://www.bgblportal.de/BGBL/bgbl1f/bgbl107s3198.pdf

Re: [PATCH 3/4] git-am: Add command line parameter `--keep-cr` passing it to git-mailsplit.

[Junio C Hamano]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> Also sprach Junio C Hamano am Thu, 11 Feb 2010 at 15:21:32 -0800:
>> "Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
>> 
>> > If applying patches with the following command sequence
>> >
>> >    git format-patch --stdout ... | git am ...
>> >
>> > in repositories having files with dos and unix line endings
>> > git-mailsplit, which is called from git-am must be called with
>> > `--keep-cr` parameter since commit c2ca1d79.
>> 
>> Sorry, I cannot parse this.
>
> What?

What I said.  I wasn't able to parse it.  If it were:

    If applying patches with the following command sequence

       git format-patch --stdout ... | git am ...

    in repositories having files with dos and unix line endings,
    git-mailsplit, which is called from git-am, must be called with
    `--keep-cr` parameter since commit c2ca1d79.

iow, two additional commas, I would have been able to, but I was not
certain that is what you meant.

>> > +e,keep-cr       pass --keep-cr flag to git-mailsplit for mbox format
>> 
>> This short form -e does not make much sense to me.  Why is it -e, and do
>> we even need a short form in the first place?
>
> Just for shortening the commandline.

That much I know already ;-) and you probably know that I knew when I
wrote this, which you somehow omitted from your quote:

    ... We may want to use short-and-sweet 'e' for something more
    important and common, and we would regret for letting this option
    squat on it later.

The shorter one can come later after people find --keep-cr is something
they often need to use and deserve a shorter form, but not until then.

[PATCH 4/4] Adding test for `--keep-cr` for git-mailsplit and git-am.

[Stefan-W. Hahn]

This test adds test for git-mailsplit using dos line endings, the
command sequence 'git format-patch ... | git am ...' and the
configuration variable `mailsplit.keepcr`.

Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
 t/t5101-mailinfo-dos.sh |   74 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 t/t5101-mailinfo-dos.sh

diff --git a/t/t5101-mailinfo-dos.sh b/t/t5101-mailinfo-dos.sh
new file mode 100644
index 0000000..c6acd55
--- /dev/null
+++ b/t/t5101-mailinfo-dos.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Stefan-W. Hahn
+#
+
+test_description='git-mailsplit mbox with dos line ending.
+
+'
+. ./test-lib.sh
+
+# Three patches which will be added as files with dos line ending.
+
+cat > file1 <<\EOF
+line 1
+EOF
+
+cat > file2 <<\EOF
+line 1
+line 2
+EOF
+
+cat > file3 <<\EOF
+line 1
+line 2
+line 3
+EOF
+
+test_expect_success 'setup repository with dos files' '
+        append_cr <file1 >file
+        git add file &&
+        git commit -m Initial &&
+        git tag start &&
+        append_cr <file2 >file
+        git commit -a -m Second &&
+        git tag start2 &&
+        append_cr <file3 >file
+        git commit -a -m Third &&
+	git format-patch -k --stdout start.. > format-patch.diff
+'
+
+test_expect_failure 'mailsplit format-patch of dos files' '
+        mkdir split &&
+        git mailsplit -osplit format-patch.diff &&
+        cat split/0001 split/0002 > mailsplit.diff &&
+        test_cmp format-patch.diff mailsplit.diff
+'
+
+test_expect_success 'mailsplit --keep-cr format-patch of dos files' '
+        mkdir split2 &&
+        git mailsplit --keep-cr -osplit2 format-patch.diff &&
+        cat split2/0001 split2/0002 > mailsplit2.diff &&
+        test_cmp format-patch.diff mailsplit2.diff
+'
+
+test_expect_success 'format-patch with dos files --keep-cr' '
+        git checkout -b new start &&
+	git format-patch -k --stdout start..master | git am --keep-cr -k -3 &&
+        git diff master
+'
+
+test_expect_success 'format-patch with dos files -e' '
+        git checkout -b new2 start &&
+	git format-patch -k --stdout start..master | git am -e -k -3 &&
+        git diff master
+'
+
+test_expect_success 'format-patch with dos files config.mailsplit' '
+        git config mailsplit.keepcr 1 &&
+        git checkout -b new3 start &&
+	git format-patch -k --stdout start..master | git am -k -3 &&
+        git diff master
+'
+
+test_done
-- 
1.7.0.rc2.17.g39385