[PATCH 0/2] Fix interactive rebase when the editor saves with CR/LF

[PATCH 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Chad Boles reported that `git rebase -I` recently started producing
errors when the editor saves files with DOS line endings. The symptom
is:

	Warning: the command isn't recognized in the following line:
	 -

	You can fix this with 'git rebase --edit-todo'.
	Or you can abort the rebase with 'git rebase --abort'.

The real bummer is that simply calling `git rebase --continue` "fixes"
it.

Turns out that we now check whether a single Carriage Return is a valid
command. This new check was introduced recently (1db168ee9, ironically
named "rebase-i: loosen over-eager check_bad_cmd check").

The proposed fix is to teach *all* shell scripts in Git to accept CR as
a field separator. Since LF is already specified as such, it should be
an uncontentious change.


Johannes Schindelin (2):
  Demonstrate rebase fails when the editor saves with CR/LF
  sh-setup: explicitly mark CR as a field separator

 git-sh-setup.sh               |  2 +-
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.1.4

[PATCH 1/2] Demonstrate rebase fails when the editor saves with CR/LF

[Johannes Schindelin]

Based on a bug report by Chad Boles.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3de0b1d..5dfa16a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,4 +1261,16 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_failure 'editor saves as CR/LF' '
+	git checkout -b with-crlf &&
+	write_script add-crs.sh <<-\EOF &&
+	sed -e "s/\$/Q/" <"$1" | tr Q "" >"$1".new &&
+	mv -f "$1".new "$1"
+	EOF
+	(
+		test_set_editor "$(pwd)/add-crs.sh" &&
+		git rebase -i HEAD^
+	)
+'
+
 test_done
-- 
2.1.4

[PATCH 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

This is the correct thing to do, really: we already specify LF as
field separator.

Incidentally, this fixes the problem interactive rebase has when the
editor wants to save text with CR/LF line endings, as WordPad does
in Windows 10.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-sh-setup.sh               | 2 +-
 t/t3404-rebase-interactive.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 4691fbc..94dfe04 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -13,7 +13,7 @@ unset CDPATH
 # do not equate an unset IFS with IFS with the default, so here is
 # an explicit SP HT LF.
 IFS=' 	
-'
+'"$(printf '')"
 
 git_broken_path_fix () {
 	case ":$PATH:" in
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5dfa16a..98eb49a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-test_expect_failure 'editor saves as CR/LF' '
+test_expect_success 'editor saves as CR/LF' '
 	git checkout -b with-crlf &&
 	write_script add-crs.sh <<-\EOF &&
 	sed -e "s/\$/Q/" <"$1" | tr Q "" >"$1".new &&
-- 
2.1.4

Re: [PATCH 2/2] sh-setup: explicitly mark CR as a field separator

[Philip Oakley]

From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
> This is the correct thing to do, really: we already specify LF as
> field separator.
>
> Incidentally, this fixes the problem interactive rebase has when the
> editor wants to save text with CR/LF line endings, as WordPad does
> in Windows 10.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> git-sh-setup.sh               | 2 +-
> t/t3404-rebase-interactive.sh | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 4691fbc..94dfe04 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -13,7 +13,7 @@ unset CDPATH
> # do not equate an unset IFS with IFS with the default, so here is
> # an explicit SP HT LF.

Doesn't this comment also need an update, given the patch title?

> IFS='
> -'
> +'"$(printf '')"
>
> git_broken_path_fix () {
>  case ":$PATH:" in
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 5dfa16a..98eb49a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
>  test E = $(git cat-file commit HEAD | sed -ne \$p)
> '
>
> -test_expect_failure 'editor saves as CR/LF' '
> +test_expect_success 'editor saves as CR/LF' '
>  git checkout -b with-crlf &&
>  write_script add-crs.sh <<-\EOF &&
>  sed -e "s/\$/Q/" <"$1" | tr Q "" >"$1".new &&
> -- 
> 2.1.4
--
Philip 

Re: [PATCH 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

Hi Brian,

re-Cc:ing the Git mailing list.

On Sun, 25 Oct 2015, brian m. carlson wrote:

> On Sun, Oct 25, 2015 at 01:50:32PM +0100, Johannes Schindelin wrote:
> > This is the correct thing to do, really: we already specify LF as
> > field separator.
> > 
> > Incidentally, this fixes the problem interactive rebase has when the
> > editor wants to save text with CR/LF line endings, as WordPad does
> > in Windows 10.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  git-sh-setup.sh               | 2 +-
> >  t/t3404-rebase-interactive.sh | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> > index 4691fbc..94dfe04 100644
> > --- a/git-sh-setup.sh
> > +++ b/git-sh-setup.sh
> > @@ -13,7 +13,7 @@ unset CDPATH
> >  # do not equate an unset IFS with IFS with the default, so here is
> >  # an explicit SP HT LF.
> >  IFS=' 	
> > -'
> > +'"$(printf '')"
> 
> On Linux, printf '' produces no output.  From my understanding of the
> POSIX spec, this is the correct behavior.   Does it behave differently
> on Windows?  If so, it might be nice to explain that in the commit
> messgae.

Hrm. This is apparently a bug in the script I tried to write for three
days now (because the mailing list-based code submission is really *so*
much more tedious than the Pull Request-based process I got so used to).

The '' should really read '\r'.

Ciao,
Johannes

Re: [PATCH 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

Hi Philip,

On Sun, 25 Oct 2015, Philip Oakley wrote:

> From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
> > @@ -13,7 +13,7 @@ unset CDPATH
> > # do not equate an unset IFS with IFS with the default, so here is
> > # an explicit SP HT LF.
> 
> Doesn't this comment also need an update, given the patch title?

Yes, you are right.

Ciao,
Dscho

shell scripting woes, was Re: [PATCH 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

Hi Brian,

On Sun, 25 Oct 2015, Johannes Schindelin wrote:

> On Sun, 25 Oct 2015, brian m. carlson wrote:
> 
> > On Sun, Oct 25, 2015 at 01:50:32PM +0100, Johannes Schindelin wrote:
> > > This is the correct thing to do, really: we already specify LF as
> > > field separator.
> > > 
> > > Incidentally, this fixes the problem interactive rebase has when the
> > > editor wants to save text with CR/LF line endings, as WordPad does
> > > in Windows 10.
> > > 
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >  git-sh-setup.sh               | 2 +-
> > >  t/t3404-rebase-interactive.sh | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> > > index 4691fbc..94dfe04 100644
> > > --- a/git-sh-setup.sh
> > > +++ b/git-sh-setup.sh
> > > @@ -13,7 +13,7 @@ unset CDPATH
> > >  # do not equate an unset IFS with IFS with the default, so here is
> > >  # an explicit SP HT LF.
> > >  IFS=' 	
> > > -'
> > > +'"$(printf '')"
> > 
> > On Linux, printf '' produces no output.  From my understanding of the
> > POSIX spec, this is the correct behavior.   Does it behave differently
> > on Windows?  If so, it might be nice to explain that in the commit
> > messgae.
> 
> Hrm. This is apparently a bug in the script I tried to write for three
> days now (because the mailing list-based code submission is really *so*
> much more tedious than the Pull Request-based process I got so used to).
> 
> The '' should really read '\r'.

Aargh. It is 'dash'. Which is the default /bin/sh on Ubuntu. Try to run
the following script through dash and bash, compare the outputs, and weep:

-- snip --
x="$(echo "printf '\\n'")"
echo "$x"
-- snap --

Ciao,
Johannes

P.S.: At least here, the output differs as following:

Bash:
printf '\n'

Dash:
printf '
'

(!!!)

[PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Chad Boles reported that `git rebase -i` recently started producing
errors when the editor saves files with DOS line endings. The symptom
is:

	Warning: the command isn't recognized in the following line:
	 -

	You can fix this with 'git rebase --edit-todo'.
	Or you can abort the rebase with 'git rebase --abort'.

The real bummer is that simply calling `git rebase --continue` "fixes"
it.

Turns out that we now check whether a single Carriage Return is a valid
command. This new check was introduced recently (1db168ee9, ironically
named "rebase-i: loosen over-eager check_bad_cmd check").

The proposed fix is to teach *all* shell scripts in Git to accept CR as
a field separator. Since LF is already specified as such, it should be
an uncontentious change.


Johannes Schindelin (2):
  Demonstrate rebase fails when the editor saves with CR/LF
  sh-setup: explicitly mark CR as a field separator

 git-sh-setup.sh               |  4 ++--
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

Interdiff vs v1:

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 94dfe04..e34673d 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -11,7 +11,7 @@ unset CDPATH
 
 # Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
 # do not equate an unset IFS with IFS with the default, so here is
-# an explicit SP HT LF.
+# an explicit SP HT LF CR.
 IFS=' 	
-'"$(printf '
')"
+'"$(printf '\r')"
 

-- 
2.1.4

[PATCH v2 1/2] Demonstrate rebase fails when the editor saves with CR/LF

[Johannes Schindelin]

Based on a bug report by Chad Boles.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3de0b1d..5dfa16a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,4 +1261,16 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_failure 'editor saves as CR/LF' '
+	git checkout -b with-crlf &&
+	write_script add-crs.sh <<-\EOF &&
+	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
+	mv -f "$1".new "$1"
+	EOF
+	(
+		test_set_editor "$(pwd)/add-crs.sh" &&
+		git rebase -i HEAD^
+	)
+'
+
 test_done
-- 
2.1.4

[PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

This is the correct thing to do, really: we already specify LF as
field separator.

Incidentally, this fixes the problem interactive rebase has when the
editor wants to save text with CR/LF line endings, as WordPad does
in Windows 10.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-sh-setup.sh               | 4 ++--
 t/t3404-rebase-interactive.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 4691fbc..e34673d 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -11,9 +11,9 @@ unset CDPATH
 
 # Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
 # do not equate an unset IFS with IFS with the default, so here is
-# an explicit SP HT LF.
+# an explicit SP HT LF CR.
 IFS=' 	
-'
+'"$(printf '\r')"
 
 git_broken_path_fix () {
 	case ":$PATH:" in
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5dfa16a..98eb49a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-test_expect_failure 'editor saves as CR/LF' '
+test_expect_success 'editor saves as CR/LF' '
 	git checkout -b with-crlf &&
 	write_script add-crs.sh <<-\EOF &&
 	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
-- 
2.1.4

Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Chad Boles reported that `git rebase -i` recently started producing
> errors when the editor saves files with DOS line endings. The symptom
> is:
>
> 	Warning: the command isn't recognized in the following line:
> 	 -
>
> 	You can fix this with 'git rebase --edit-todo'.
> 	Or you can abort the rebase with 'git rebase --abort'.
>
> The real bummer is that simply calling `git rebase --continue` "fixes"
> it.

Two questions.

 * What does the DOS editor do to a file with ^M in the middle of a
   line, e.g. "A^MB^M^J"?

 * Is your shell ported correctly to the platform?

The latter may need a bit of elaboration.  "read a b c" is supposed
to read one line of text (where the definition of line is platform
dependent, your platform may use CRLF to signal the end of an line),
split the characters on the line (i.e. LF vs CRLF no longer matters
at this point) at $IFS characters and give them to $a $b and $c. If
the platform accepts CRLF as the EOL signal, should the program still
see CR at the end of $c?

A solution that mucks with IFS smells like fixing a wrong problem
without addressing the real cause.

Also IFS is used not only by "read", so munging it globally doubly
feels wrong.

In addition, you do not want to split at CR; what you want is to
treat CRLF (i.e. not a lone CR) as the end-of-line separator.
Adding CR to IFS feels triply wrong.

By the way, saying "This is right, really" makes you sound as if you
do not have a real argument.

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Matthieu Moy]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> This is the correct thing to do, really: we already specify LF as
> field separator.

I'm almost convinced that this is the right thing to do in the long run
("almost" because I'm not sure, not because I have arguments against). I
agree with Junio that the commit message should be more convincing, but
indeed, accepting LF and not CR is strange.

However, is this the right thing to do in the maintainance branch? It
does fix the issue, but does so in a rather intrusive way, so I'd need
more arguments to be convinced that this is safe to merge in maint. Or
have a local fix for rebase to be merged in maint, and apply this in
master for the next feature release.

Sorry for being negative, and especially sorry since I'm partly guilty
for the breakage. I just want to be sure that we don't break anything
while repairing it (we already introduced this breakage while repairing
another one...).

>  # Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
>  # do not equate an unset IFS with IFS with the default, so here is
> -# an explicit SP HT LF.
> +# an explicit SP HT LF CR.
>  IFS=' 	
> -'
> +'"$(printf '\r')"

While we're there, it may be better to have a single "printf ' \t\n\r'"
to avoid the whitespace magic in the source code.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Hi Junio,

On Sun, 25 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Chad Boles reported that `git rebase -i` recently started producing
> > errors when the editor saves files with DOS line endings. The symptom
> > is:
> >
> > 	Warning: the command isn't recognized in the following line:
> > 	 -
> >
> > 	You can fix this with 'git rebase --edit-todo'.
> > 	Or you can abort the rebase with 'git rebase --abort'.
> >
> > The real bummer is that simply calling `git rebase --continue` "fixes"
> > it.
> 
> Two questions.
> 
>  * What does the DOS editor do to a file with ^M in the middle of a
>    line, e.g. "A^MB^M^J"?

You mean mixed line endings? At least with this Windows 10 WordPad, *all*
line endings are normalized to CR/LF. That is, if you edit a file that
contains a stray CR, it is presented as a line break.

>  * Is your shell ported correctly to the platform?

I would think so. It is an MSys2 program, therefore it uses all the POSIX
niceties, of course.

A simple test with CR/LF line endings in a script reveals that it is
pretty solid:

	x=a
	case "$x" in a) echo b;; esac

prints out 'b', as expected.

Please also note that things apparently worked alright before the patch
removing the stripspace call was applied.

> The latter may need a bit of elaboration.  "read a b c" is supposed
> to read one line of text (where the definition of line is platform
> dependent, your platform may use CRLF to signal the end of an line),
> split the characters on the line (i.e. LF vs CRLF no longer matters
> at this point) at $IFS characters and give them to $a $b and $c. If
> the platform accepts CRLF as the EOL signal, should the program still
> see CR at the end of $c?
> 
> A solution that mucks with IFS smells like fixing a wrong problem
> without addressing the real cause.

Please note that it is a bit unsettling if you use funny language like
"smells" here and then accuse me of not having an argument when I point
that the same rationale applies to having CR in IFS as applies to having
LF in IFS. Yes, that was an implicit argument, but it is a strong one, so
I do not think you are well served ignoring it.

> Also IFS is used not only by "read", so munging it globally doubly
> feels wrong.
> 
> In addition, you do not want to split at CR; what you want is to
> treat CRLF (i.e. not a lone CR) as the end-of-line separator.
> Adding CR to IFS feels triply wrong.
> 
> By the way, saying "This is right, really" makes you sound as if you
> do not have a real argument.

Again. If CR has no place in IFS, why does LF have a place in IFS? It
makes *no* sense to argue for one and against the other.

In any case, I am not interested in arguing for arguing's sake. Tell me
what you want instead of the patch to IFS, I will implement it and test
whether it fixes the bug and we will move on.

Ciao,
Johannes

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> This is the correct thing to do, really: we already specify LF as
>> field separator.
>
> I'm almost convinced that this is the right thing to do in the long run
> ("almost" because I'm not sure, not because I have arguments against). I
> agree with Junio that the commit message should be more convincing, but
> indeed, accepting LF and not CR is strange.

If there were a single character that denotes CRLF, I'd say that
including such a character in IFS would make sense on a system with
CRLF EOL convention.  But that is not the case.

On a platform with LF EOL convention, having LF in IFS makes sense,
due to two reasons.

 * read is not the only user of IFS.  Expressing "list of things"
   (pre bashism "shell array" days) by concatenating elements into a
   single string variable, separated with LF, and later iterating
   over them is a very common use case, e.g.

	LF='
        '
	list="$thing1"
	list="$list$LF$thing2"
	list="$list$LF$thing3"
        ...

	IFS=$LF
	for thing in $list
        do
        	...

   And including LF by default in IFS, especially when "things" can
   contain SP/HT, is handy.

 * It does not hurt on a platform with LF EOL convention to include
   LF in IFS, because you cannot have a "lone LF" in the middle of a
   line.  Presence of a single LF alone will terminate the current
   line and the bytes that follow it will be a next line.

No similarity argument can be made for a lone CR on a platform that
uses CRLF EOL convention.  A "lone CR" can appear in the middle of a
line without terminating the current line, as only a CR that is
immediately followed by a LF is the end of line, so you cannot make
the "It does not hurt" argument for including CR to IFS.  If you
have a variable in which "A^MB" is there, "set $variable" would
split them into two and assign B to $2, which is not what the
scripts would expect.

> However, is this the right thing to do in the maintainance branch? It
> does fix the issue, but does so in a rather intrusive way, so I'd need
> more arguments to be convinced that this is safe to merge in maint.

If this were the "right" thing in general for shell scripts on
systems with CRLF EOL convention, the implementation of the shell
language on such a platform would be doing that upon startup (or
upon "unset IFS"), and we wouldn't be having this discussion.  Don't
you think it is strange that individual applications like Git have
to set IFS to include CR?  I see it as a strong sign that this is
not a correct thing to do.

Intrusive does not begin to describe it.

I think the "right" thing for a shell implementation on a CRLF
platform to do is twofold.

 * Read
   http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html
   and notice that the operation is defined as a two-step process.
   It is supposed to read an input line and then "The terminating
   <newline> (if any) shall be removed from the input".  And then
   "the result shall be split into fields" (at IFS boundaries).

   As POSIX does not say <newline> has to be a single octet with
   value 10 (the same way you are allowed to show CRLF when you call
   printf("\n")), a shell implementation would read CRLF terminated
   lines, and remove the terminating CRLF before doing the field
   splitting using IFS.

   This is why I said in my message to Dscho that IFS does not get
   into the picture.

 * Implement the "field splitting" (at IFS boundaries) with a small
   twist.  When the script/user included '\n' in IFS, split at LF
   but remove CR if there is one that immediately precedes the LF.

The latter would help if you did the previous "read is not the only
user of IFS" example like this instead:

        LF=$(printf '\n.')
        LF=${LF%.}
	list="$thing1"
	list="$list$LF$thing2"
	list="$list$LF$thing3"
        ...

	IFS=$LF
	for thing in $list
        do
        	...

and the platform 'printf' gave CRLF for "\n" (newline), resulting
the list to be concatenated with CRLF, not LF.

And this is why I asked Dscho if the shell is done correctly _for_
the platform.

> Sorry for being negative, and especially sorry since I'm partly guilty
> for the breakage. I just want to be sure that we don't break anything
> while repairing it (we already introduced this breakage while repairing
> another one...).

Something along the line of the following would be tolerable, even
though I think in the longer term, not just in Git land but in the
larger ecosystem to use POSIXy tools on Windows, the best solution
is to fix the shell so that it matches the expectation of the users
of its platform.

I say "something along the line of" here because I do not know how
the problematic shell behaves on the assignment command that stuffs
a lone CR into a variable.  It _might_ need a similar protection
against the shell feature "the last EOL is removed from the result
of command expansion", as I did in the above example, depending on
how incoherent the implementation is.  The implementation seems to
accept CRLF and LF in shell scripts proper just fine, but apparently
its implementation of "read" does not honor such platform EOL
convention, which caused this issue, and I don't know what it does
in the codepath that implements command expansion.

 git-rebase--interactive.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c42ba34..8f13a35 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -881,6 +881,7 @@ check_commit_sha () {
 check_bad_cmd_and_sha () {
 	retval=0
 	lineno=0
+	cr=$(printf "%c" 13)
 	while read -r command rest
 	do
 		lineno=$(( $lineno + 1 ))
@@ -888,6 +889,10 @@ check_bad_cmd_and_sha () {
 		"$comment_char"*|''|noop|x|exec)
 			# Doesn't expect a SHA-1
 			;;
+		"$cr")
+			# Windows port of shell not stripping the newline
+			# at the end of an empty line correctly.
+			;;
 		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then

Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> A simple test with CR/LF line endings in a script reveals that it is
> pretty solid:
>
> 	x=a
> 	case "$x" in a) echo b;; esac
>
> prints out 'b', as expected.

I do not see what this has to do with anything.

The shell language parser when parsing a script may do the right
thing, but the bug I was alluding to was that your 'read' does not
seem to be removing the terminating <newline> (which is CRLF on your
platform) after reading a line before splitting the contents on the
line at IFS boundaries.

> Again. If CR has no place in IFS, why does LF have a place in IFS? It
> makes *no* sense to argue for one and against the other.

See my message to Matthieu.

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Junio C Hamano]

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

> Something along the line of the following would be tolerable, even
> though I think in the longer term, not just in Git land but in the
> larger ecosystem to use POSIXy tools on Windows, the best solution
> is to fix the shell so that it matches the expectation of the users
> of its platform.
>
> I say "something along the line of" here because I do not know how
> the problematic shell behaves on the assignment command that stuffs
> a lone CR into a variable.  It _might_ need a similar protection
> against the shell feature "the last EOL is removed from the result
> of command expansion", as I did in the above example, depending on
> how incoherent the implementation is.  The implementation seems to
> accept CRLF and LF in shell scripts proper just fine, but apparently
> its implementation of "read" does not honor such platform EOL
> convention, which caused this issue, and I don't know what it does
> in the codepath that implements command expansion.

I still have to say "something along the lines of" as I do not have
(and I do not wish to have, to be honest) an access to test this
with the problematic shell implementation, but here is an updated
patch.

-- >8 --
Subject: [PATCH] rebase-i: work around Windows CRLF line endings

Editors on Windows can and do save text files with CRLF line
endings, which is the convention on the platform.  We are seeing
reports that the "read" command in a port of bash to the environment
however does not strip the CRLF at the end, not adjusting for the
same convention on the platform.

This breaks the recently added sanity checks for the insn sheet fed
to "rebase -i"; instead of an empty line (hence nothing in $command),
the script was getting a lone CR in there.

Special case a lone CR and treat it the same way as an empty line to
work this around.

The test was stolen from Dscho.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase--interactive.sh    | 13 +++++++++++++
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c42ba34..3911711 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,6 +77,10 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+# Work around a Windows port of shell that does not strip
+# the newline at the end of a line correctly.
+cr=$(printf "\015")
+
 strategy_args=
 if test -n "$do_merge"
 then
@@ -518,6 +522,11 @@ do_next () {
 	"$comment_char"*|''|noop|drop|d)
 		mark_action_done
 		;;
+	"$cr")
+		# Windows port of shell not stripping the newline
+		# at the end of an empty line correctly.
+		mark_action_done
+		;;
 	pick|p)
 		comment_for_reflog pick
 
@@ -888,6 +897,10 @@ check_bad_cmd_and_sha () {
 		"$comment_char"*|''|noop|x|exec)
 			# Doesn't expect a SHA-1
 			;;
+		"$cr")
+			# Windows port of shell not stripping the newline
+			# at the end of an empty line correctly.
+			;;
 		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 88d7d53..2f97a3d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1240,4 +1240,16 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_success 'editor saves as CR/LF' '
+	git checkout -b with-crlf &&
+	write_script add-crs.sh <<-\EOF &&
+	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
+	mv -f "$1".new "$1"
+	EOF
+	(
+		test_set_editor "$(pwd)/add-crs.sh" &&
+		git rebase -i HEAD^
+	)
+'
+
 test_done
-- 
2.6.2-405-g6877da6

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

Hi Junio,

On Mon, 26 Oct 2015, Junio C Hamano wrote:

> I asked Dscho if the shell is done correctly _for_ the platform.

This assumes that the platform is either CR/LF or LF. That is incorrect.
Windows does *not* dictate the line endings to be CR/LF. Certain
applications do.

The shell is an MSys2 shell, and MSys2 convention is to use LF. Hence the
shell is done correctly for the platform.

Yet on Windows (and in cross-platform projects also on Linux and MacOSX),
you have to deal with the fact that text files produced outside the cozy
little shell can have different line endings.

As I said, I am uninterested in arguing for arguing's sake. Otherwise I
would shoot down your argument that LF in IFS "does not hurt" but CR
somehow magically does. I am sure it would be a helluva discussion and
fun. But I do not have the time.

What I really needed you to do indicate the way forward.

The patch you provided in the mail I am replying to fails to fix the bug.

Ciao,
Johannes

Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Hi Junio,

On Mon, 26 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > A simple test with CR/LF line endings in a script reveals that it is
> > pretty solid:
> >
> > 	x=a
> > 	case "$x" in a) echo b;; esac
> >
> > prints out 'b', as expected.
> 
> I do not see what this has to do with anything.

You probably missed the context. Brian pointed out that my patch said
"printf ''" when it should have said "printf '\n'". I responded that my
commit says the right thing, but somewhere along the lines of transforming
beautiful Git commits into mails it must have been lost.

The explanation you quoted is the essence of the problem of my script to
prepare mails for submission to the mailing list: the script works in
Bash, but fails in Dash. And as my script has the shebang "#!/bin/sh" and
as Ubuntu defaults to using the Dash as its /bin/sh, we now have the full
explanation why my first mail showed an incorrect patch.

Ciao,
Johannes

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Johannes Schindelin]

Hi Junio,

On Mon, 26 Oct 2015, Junio C Hamano wrote:

> Subject: [PATCH] rebase-i: work around Windows CRLF line endings
> 
> Editors on Windows can and do save text files with CRLF line
> endings, which is the convention on the platform.  We are seeing
> reports that the "read" command in a port of bash to the environment
> however does not strip the CRLF at the end, not adjusting for the
> same convention on the platform.
> 
> This breaks the recently added sanity checks for the insn sheet fed
> to "rebase -i"; instead of an empty line (hence nothing in $command),
> the script was getting a lone CR in there.
> 
> Special case a lone CR and treat it the same way as an empty line to
> work this around.

You do not need me to tell you that it is likely that the same issue will
arise in other places, but if you are content with this work-around, so be
it.


> The test was stolen from Dscho.

There was no need to steal this, as I made a specific effort to introduce
the test in a separate commit. There was also no need to remove the
attribution for finding the bug.

Let's not do that.

I will submit v3 in a second, including your work-around in favor of the
patch you hate so much.

Ciao,
Johannes

[PATCH v3 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Chad Boles reported that `git rebase -i` recently started producing
errors when the editor saves files with DOS line endings. The symptom
is:

	Warning: the command isn't recognized in the following line:
	 -

	You can fix this with 'git rebase --edit-todo'.
	Or you can abort the rebase with 'git rebase --abort'.

The real bummer is that simply calling `git rebase --continue` "fixes"
it.

Turns out that we now check whether a single Carriage Return is a valid
command. This new check was introduced recently (1db168ee9, ironically
named "rebase-i: loosen over-eager check_bad_cmd check").

The proposed fix is to teach *all* shell scripts in Git to accept CR as
a field separator. Since LF is already specified as such, it should be
an uncontentious change.


Johannes Schindelin (1):
  Demonstrate rebase fails when the editor saves with CR/LF

Junio C Hamano (1):
  rebase-i: work around Windows CRLF line endings

 git-rebase--interactive.sh    | 13 +++++++++++++
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 25 insertions(+)

Interdiff vs v2:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..daadf2d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,6 +77,10 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+# Work around a Windows port of shell that does not strip
+# the newline at the end of a line correctly.
+cr=$(printf "\015")
+
 strategy_args=
 if test -n "$do_merge"
 then
@@ -518,6 +522,11 @@ do_next () {
 	"$comment_char"*|''|noop|drop|d)
 		mark_action_done
 		;;
+	"$cr")
+		# Windows port of shell not stripping the newline
+		# at the end of an empty line correctly.
+		mark_action_done
+		;;
 	pick|p)
 		comment_for_reflog pick
 
@@ -896,6 +905,10 @@ check_bad_cmd_and_sha () {
 		"$comment_char"*|''|noop|x|exec)
 			# Doesn't expect a SHA-1
 			;;
+		"$cr")
+			# Windows port of shell not stripping the newline
+			# at the end of an empty line correctly.
+			;;
 		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index e34673d..4691fbc 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -11,9 +11,9 @@ unset CDPATH
 
 # Similarly for IFS, but some shells (e.g. FreeBSD 7.2) are buggy and
 # do not equate an unset IFS with IFS with the default, so here is
-# an explicit SP HT LF CR.
+# an explicit SP HT LF.
 IFS=' 	
-'"$(printf '\r')"
+'
 
 git_broken_path_fix () {
 	case ":$PATH:" in

-- 
2.1.4

[PATCH v3 1/2] Demonstrate rebase fails when the editor saves with CR/LF

[Johannes Schindelin]

Based on a bug report by Chad Boles.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3de0b1d..5dfa16a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,4 +1261,16 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_failure 'editor saves as CR/LF' '
+	git checkout -b with-crlf &&
+	write_script add-crs.sh <<-\EOF &&
+	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
+	mv -f "$1".new "$1"
+	EOF
+	(
+		test_set_editor "$(pwd)/add-crs.sh" &&
+		git rebase -i HEAD^
+	)
+'
+
 test_done
-- 
2.1.4

[PATCH v3 2/2] rebase-i: work around Windows CRLF line endings

[Johannes Schindelin]

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

Editors on Windows can and do save text files with CRLF line
endings, which is the convention on the platform.  We are seeing
reports that the "read" command in a port of bash to the environment
however does not strip the CRLF at the end, not adjusting for the
same convention on the platform.

This breaks the recently added sanity checks for the insn sheet fed
to "rebase -i"; instead of an empty line (hence nothing in $command),
the script was getting a lone CR in there.

Special case a lone CR and treat it the same way as an empty line to
work this around.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    | 13 +++++++++++++
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..daadf2d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,6 +77,10 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+# Work around a Windows port of shell that does not strip
+# the newline at the end of a line correctly.
+cr=$(printf "\015")
+
 strategy_args=
 if test -n "$do_merge"
 then
@@ -518,6 +522,11 @@ do_next () {
 	"$comment_char"*|''|noop|drop|d)
 		mark_action_done
 		;;
+	"$cr")
+		# Windows port of shell not stripping the newline
+		# at the end of an empty line correctly.
+		mark_action_done
+		;;
 	pick|p)
 		comment_for_reflog pick
 
@@ -896,6 +905,10 @@ check_bad_cmd_and_sha () {
 		"$comment_char"*|''|noop|x|exec)
 			# Doesn't expect a SHA-1
 			;;
+		"$cr")
+			# Windows port of shell not stripping the newline
+			# at the end of an empty line correctly.
+			;;
 		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5dfa16a..98eb49a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-test_expect_failure 'editor saves as CR/LF' '
+test_expect_success 'editor saves as CR/LF' '
 	git checkout -b with-crlf &&
 	write_script add-crs.sh <<-\EOF &&
 	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
-- 
2.1.4

Re: [PATCH v3 2/2] rebase-i: work around Windows CRLF line endings

[Johannes Schindelin]

Hi,

On Tue, 27 Oct 2015, Johannes Schindelin wrote:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 5dfa16a..98eb49a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
>  	test E = $(git cat-file commit HEAD | sed -ne \$p)
>  '
>  
> -test_expect_failure 'editor saves as CR/LF' '
> +test_expect_success 'editor saves as CR/LF' '
>  	git checkout -b with-crlf &&
>  	write_script add-crs.sh <<-\EOF &&
>  	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&

My apologies: I forgot to spell out explicitly that this passes in Git for
Windows 2.x' SDK: It does.

Ciao,
Johannes

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> This is the correct thing to do, really: we already specify LF as
>>> field separator.
>>
>> I'm almost convinced that this is the right thing to do in the long run
>> ("almost" because I'm not sure, not because I have arguments against). I
>> agree with Junio that the commit message should be more convincing, but
>> indeed, accepting LF and not CR is strange.
>
> If there were a single character that denotes CRLF, I'd say that
> including such a character in IFS would make sense on a system with
> CRLF EOL convention.  But that is not the case.

It's not, but there are platforms where the newline convention is
CR-only. AFAIK, they are all "old" architectures (like Mac OS < X), but
there are still CR-only files lying around.

>  * read is not the only user of IFS.  Expressing "list of things"
>    (pre bashism "shell array" days) by concatenating elements into a
>    single string variable, separated with LF, and later iterating
>    over them is a very common use case, e.g.
>
> 	LF='
>         '
> 	list="$thing1"
> 	list="$list$LF$thing2"
> 	list="$list$LF$thing3"
>         ...
>
> 	IFS=$LF
> 	for thing in $list
>         do
>         	...
>
>    And including LF by default in IFS, especially when "things" can
>    contain SP/HT, is handy.

I don't get the argument. You're talking about a case where you
explicitly set IFS, and the patch is about changing the default. The
use-case above would work exactly like before if we modify the default
IFS.

> If you
> have a variable in which "A^MB" is there, "set $variable" would
> split them into two and assign B to $2, which is not what the
> scripts would expect.

The same goes if you replace ^M with a tab or a space. Using unquoted
"set $variable" is sane only if you are sure that $variable does not
contain unexpected special characters. I can't imagine a case where one
would accept space, tab or LF as separator and would need to accept CR
as a non-separator.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3 2/2] rebase-i: work around Windows CRLF line endings

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 27 Oct 2015, Johannes Schindelin wrote:
>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 5dfa16a..98eb49a 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
>>  	test E = $(git cat-file commit HEAD | sed -ne \$p)
>>  '
>>  
>> -test_expect_failure 'editor saves as CR/LF' '
>> +test_expect_success 'editor saves as CR/LF' '
>>  	git checkout -b with-crlf &&
>>  	write_script add-crs.sh <<-\EOF &&
>>  	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
>
> My apologies: I forgot to spell out explicitly that this passes in Git for
> Windows 2.x' SDK: It does.

Can you add that to the log message?  Your cover letter may also
want to be updated for those who fish for the last version posted
in the list archive.

Thanks.

Re: [PATCH v2 2/2] sh-setup: explicitly mark CR as a field separator

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This assumes that the platform is either CR/LF or LF. That is incorrect.
> Windows does *not* dictate the line endings to be CR/LF. Certain
> applications do.
>
> The shell is an MSys2 shell, and MSys2 convention is to use LF. Hence the
> shell is done correctly for the platform.
>
> Yet on Windows (and in cross-platform projects also on Linux and MacOSX),
> you have to deal with the fact that text files produced outside the cozy
> little shell can have different line endings.

I know that.  That is why I said this is platform specific.  When
somebody on Ubuntu comes and says "XYZ does not work when I saved my
text file with CRLF", I can say "don't do that then".  I've been
assuming that you cannot say the same to your users, in order to be
nicer to Windows.  You say MSys2 convention is to use LF, but what
do you exactly mean by that?

I am hoping that you do not mean "we treat a file created by a DOS
editor as a text file with CR appended at the end of each line".
I'd instead expect that a file the user wrote ABC on the first line
will give you ABC in $word if the user did this:

	#!/bin/sh
        read word <file

and not "ABC^M".  That is, "we may produce only LF files, we consume
LF files just fine, but we also treat CRLF files as text as their
users intended."

> As I said, I am uninterested in arguing for arguing's sake.

I already explained why this particular thing does not have anything
to do with IFS but comes from the way your "read" behaves, which is
not in line with the way users on your platform expects.  Is that so
hard to understand?

An argument you make to support adding stuff to IFS _is_ an argument
for arguing's sake.

> What I really needed you to do indicate the way forward.

Yes, and I already did, didn't I?

I already said that mucking with IFS is a wrong thing to do, and
indicated that that is not the way forward.

I also already said that fixing your read is the only sensible thing
to do in the longer term iff you claim to support those who use
"certain applications" that leave CRLF line terminator.

 * http://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html
   defines the operation as a two-step process.  It is supposed to read
   an input line and then "The terminating <newline> (if any) shall
   be removed from the input".  And then "the result shall be split into
   fields" (at IFS boundaries).

   As POSIX does not say <newline> has to be a single octet with
   value 10 (the same way you are allowed to show CRLF when you call
   printf("\n"), and you do not see CR left at the end when you use
   gets(3)).

   So on a platform that (optionally) accepts CRLF as a line
   terminator, "read" should read up to LF for a line, and if there
   is a CR immediately before that LF, remove that CR, too.  Now you
   got the result of "terminating <newline> (if any) shall be
   removed".  Then look at "the result" and split into fields using
   IFS.  That way, you still process LF terminated files correctly,
   and you would never show a single ^M for an empty line, which
   triggered this thread.

 * Optionally, you may want to implement the "field splitting" (at
   IFS boundaries) with a small twist.  When the script/user
   included LF in IFS, split at LF but remove CR if there is one
   that immediately precedes the LF.

And also I gave you a more localized (i.e. less likely to negatively
affect unrelated parts of the system) workaround that we could use,
until your "read" is fixed.

Mind you, I am not happy with that "$cr may appear at the end of the
last token on the line" workaround.  Other places in the code (both
inside and outside Git scripted porcelains) that calls "read" to
read from text files can suffer from the same issue, until it is
fixed.  Covering up the bug in "read" by throwing extra characters
to IFS, especially when read is not the only user of IFS, is not a
fix.

[PATCH v4 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Chad Boles reported that `git rebase -i` recently started producing
errors when the editor saves files with DOS line endings. The symptom
is:

	Warning: the command isn't recognized in the following line:
	 -

	You can fix this with 'git rebase --edit-todo'.
	Or you can abort the rebase with 'git rebase --abort'.

The real bummer is that simply calling `git rebase --continue` "fixes"
it.

Turns out that we now check whether a single Carriage Return is a valid
command. This new check was introduced recently (1db168ee9, ironically
named "rebase-i: loosen over-eager check_bad_cmd check").

The fix provided by Junio works around this issue by testing for an
explicit trailing carriage return and handles it like an empty line.

Unfortunately, this is the best we can do for now as there is
disagreement about a more general fix.

This iteration clarifies the comments in git-rebase--interactive,
updates the commit message to state that this has been tested with Git
for Windows, and replaces the description of the proposed fix with a
description of the actual work-around provided by Junio.


Johannes Schindelin (1):
  Demonstrate rebase fails when the editor saves with CR/LF

Junio C Hamano (1):
  rebase-i: work around Windows CRLF line endings

 git-rebase--interactive.sh    | 12 ++++++++++++
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 24 insertions(+)

Interdiff vs v3:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index daadf2d..34cfe66 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,8 +77,7 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
-# Work around a Windows port of shell that does not strip
-# the newline at the end of a line correctly.
+# Work around Git for Windows' Bash that strips only LFs but no CRs.
 cr=$(printf "\015")
 
 strategy_args=
@@ -523,8 +522,8 @@ do_next () {
 		mark_action_done
 		;;
 	"$cr")
-		# Windows port of shell not stripping the newline
-		# at the end of an empty line correctly.
+		# Work around Carriage Returns not being stripped (e.g. with
+		# Git for Windows' Bash).
 		mark_action_done
 		;;
 	pick|p)
@@ -906,8 +905,8 @@ check_bad_cmd_and_sha () {
 			# Doesn't expect a SHA-1
 			;;
 		"$cr")
-			# Windows port of shell not stripping the newline
-			# at the end of an empty line correctly.
+			# Work around Carriage Returns not being stripped
+			# (e.g. with Git for Windows' Bash).
 			;;
 		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"

-- 
2.1.4

[PATCH v4 1/2] Demonstrate rebase fails when the editor saves with CR/LF

[Johannes Schindelin]

Based on a bug report by Chad Boles.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3de0b1d..5dfa16a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,4 +1261,16 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_failure 'editor saves as CR/LF' '
+	git checkout -b with-crlf &&
+	write_script add-crs.sh <<-\EOF &&
+	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
+	mv -f "$1".new "$1"
+	EOF
+	(
+		test_set_editor "$(pwd)/add-crs.sh" &&
+		git rebase -i HEAD^
+	)
+'
+
 test_done
-- 
2.1.4

[PATCH v4 2/2] rebase-i: work around Windows CRLF line endings

[Johannes Schindelin]

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

Editors on Windows can and do save text files with CRLF line
endings, which is the convention on the platform.  We are seeing
reports that the "read" command in a port of bash to the environment
however does not strip the CRLF at the end, not adjusting for the
same convention on the platform.

This breaks the recently added sanity checks for the insn sheet fed
to "rebase -i"; instead of an empty line (hence nothing in $command),
the script was getting a lone CR in there.

Special case a lone CR and treat it the same way as an empty line to
work this around.

This patch passes the test with Git for Windows, where the issue was
seen first.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    | 12 ++++++++++++
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..34cfe66 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,6 +77,9 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+# Work around Git for Windows' Bash that strips only LFs but no CRs.
+cr=$(printf "\015")
+
 strategy_args=
 if test -n "$do_merge"
 then
@@ -518,6 +521,11 @@ do_next () {
 	"$comment_char"*|''|noop|drop|d)
 		mark_action_done
 		;;
+	"$cr")
+		# Work around Carriage Returns not being stripped (e.g. with
+		# Git for Windows' Bash).
+		mark_action_done
+		;;
 	pick|p)
 		comment_for_reflog pick
 
@@ -896,6 +904,10 @@ check_bad_cmd_and_sha () {
 		"$comment_char"*|''|noop|x|exec)
 			# Doesn't expect a SHA-1
 			;;
+		"$cr")
+			# Work around Carriage Returns not being stripped
+			# (e.g. with Git for Windows' Bash).
+			;;
 		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5dfa16a..98eb49a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
 	test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-test_expect_failure 'editor saves as CR/LF' '
+test_expect_success 'editor saves as CR/LF' '
 	git checkout -b with-crlf &&
 	write_script add-crs.sh <<-\EOF &&
 	sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
-- 
2.1.4

Re: [PATCH v3 2/2] rebase-i: work around Windows CRLF line endings

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My apologies: I forgot to spell out explicitly that this passes in Git
> > for Windows 2.x' SDK: It does.
> 
> Can you add that to the log message?  Your cover letter may also want to
> be updated for those who fish for the last version posted in the list
> archive.

Done,
Johannes

Re: [PATCH v4 0/2] Fix interactive rebase when the editor saves with CR/LF

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Turns out that we now check whether a single Carriage Return is a valid
> command. This new check was introduced recently (1db168ee9, ironically
> named "rebase-i: loosen over-eager check_bad_cmd check").

Will queue.

The root cause is not really "a new check added recently".  Earlier
the edited result was fed to stripspace and because "stripspace"
does what a dos-to-unix filter that turns CRLF into LF in addition
to cleaning up spaces and comments, we did not see the problematic
behaviour from "read" in this codepath.

Thanks.