[PATCH] rebase -p: avoid grep on potentailly non-ASCII data

The included test case, which uses rebase -p with non-ASCII commit
messages, was failing as follows:

  Warning: the command isn't recognized in the following line:
   - Binary file (standard input) matches

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

Possibly related to recent GNU grep changes, as with commit
316336379cf7937c2ecf122c7197cfe5da6b2061.  Avoid the issue by using sed
instead.

Signed-off-by: Anders Kaseorg <redacted>
---
 git-rebase--interactive.sh        |  2 +-
 t/t3409-rebase-preserve-merges.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c0cfe88..0efc65c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -1241,7 +1241,7 @@ then
 			# be rebasing on top of it
 			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
 			sha1=$(git rev-list -1 $rev)
-			sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+			sed "/^[a-z][a-z]* $sha1/d" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
 			rm "$rewritten"/$rev
 		fi
 	done

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 8c251c5..1f01b29 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh

@@ -119,4 +119,25 @@ test_expect_success 'rebase -p ignores merge.log config' '
 	)
 '
 
+test_expect_success 'rebase -p works with non-ASCII commit message' '
+	(
+	mkdir non-ascii &&
+	cd non-ascii &&
+	git init &&
+	echo a > a &&
+	git add a &&
+	git commit -m a &&
+	echo b > b &&
+	git add b &&
+	git commit -m b &&
+	git branch foo &&
+	git reset --hard HEAD^ &&
+	git cherry-pick -x foo &&
+	echo c > c &&
+	git add c &&
+	git commit -m "$(printf "I \\342\\231\\245 Unicode")" &&
+	git rebase -p foo
+	)
+'
+
 test_done

-- 
2.8.0.rc0

On 03/08/2016 08:59 AM, Anders Kaseorg wrote:

#The cd should be done in a subshell:
(

+	cd non-ascii &&
+	git init &&
+	echo a > a &&
+	git add a &&
+	git commit -m a &&
+	echo b > b &&

#Style: No space after ">" (and even above and below)

echo b >b

+	git add b &&
+	git commit -m b &&
+	git branch foo &&
+	git reset --hard HEAD^ &&
+	git cherry-pick -x foo &&
+	echo c > c &&
+	git add c &&
+	git commit -m "$(printf "I \\342\\231\\245 Unicode")" &&
+	git rebase -p foo
+	)
+

#end of subshell
)

'
+
  test_done

Torsten Bögershausen venit, vidit, dixit 08.03.2016 13:25:

On 03/08/2016 08:59 AM, Anders Kaseorg wrote:

quoted

The included test case, which uses rebase -p with non-ASCII commit
messages, was failing as follows:

   Warning: the command isn't recognized in the following line:
    - Binary file (standard input) matches

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

Possibly related to recent GNU grep changes, as with commit
316336379cf7937c2ecf122c7197cfe5da6b2061.  Avoid the issue by using sed
instead.

Signed-off-by: Anders Kaseorg <redacted>
---
  git-rebase--interactive.sh        |  2 +-
  t/t3409-rebase-preserve-merges.sh | 21 +++++++++++++++++++++
  2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c0cfe88..0efc65c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -1241,7 +1241,7 @@ then
  			# be rebasing on top of it
  			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
  			sha1=$(git rev-list -1 $rev)
-			sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+			sed "/^[a-z][a-z]* $sha1/d" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
  			rm "$rewritten"/$rev
  		fi
  	done

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 8c251c5..1f01b29 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh

@@ -119,4 +119,25 @@ test_expect_success 'rebase -p ignores merge.log config' '
  	)
  '
  
+test_expect_success 'rebase -p works with non-ASCII commit message' '
+	(
+	mkdir non-ascii &&

#The cd should be done in a subshell:
(

quoted

+	cd non-ascii &&
+	git init &&
+	echo a > a &&
+	git add a &&
+	git commit -m a &&
+	echo b > b &&

#Style: No space after ">" (and even above and below)

And also on the sane_grep/sed line.

echo b >b

quoted

+	git add b &&
+	git commit -m b &&
+	git branch foo &&
+	git reset --hard HEAD^ &&
+	git cherry-pick -x foo &&
+	echo c > c &&
+	git add c &&
+	git commit -m "$(printf "I \\342\\231\\245 Unicode")" &&
+	git rebase -p foo
+	)
+

#end of subshell
)

The whole test is in a subshell already, although that is easy to miss
(missing indentation).

quoted

'
+
  test_done

It may be worth noting whether other occurrences of "sane_grep" are safe
from binary input.

After all, one my question the degree of sanity of our sane_grep, or
whether we need asane_grep and bisane_grep in our shell library - "make
our grep sane again".

Michael

On Tue, Mar 08, 2016 at 02:45:20PM +0100, Michael J Gruber wrote:

It may be worth noting whether other occurrences of "sane_grep" are safe
from binary input.

After all, one my question the degree of sanity of our sane_grep, or
whether we need asane_grep and bisane_grep in our shell library - "make
our grep sane again".

I actually wonder if we should have a build-time knob to put "grep -a"
into sane_grep(). We do not ever plan to feed it binary data, so that
will do what, provided the system grep handles "-a". And on those that
do not know about "-a", one imagines that they do not suffer from this
problem in the first place (which is really limited to recent versions
of GNU grep).

-Peff