{"thread":{"id":"41640","subject":"[PATCH] rebase -p: avoid grep on potentailly non-ASCII data","startedAt":"2016-03-08T07:59:06Z","lastAt":"2016-03-10T17:22:20Z","messageCount":12,"participants":["Anders Kaseorg","Torsten Bögershausen","Michael J Gruber","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280357","messageId":"alpine.DEB.2.10.1603080255030.2674@buzzword-bingo.mit.edu","threadId":"41640","inReplyTo":null,"subject":"[PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-03-08T07:59:06Z","receivedAt":"2016-03-08T07:59:06Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"The included test case, which uses rebase -p with non-ASCII commit\nmessages, was failing as follows:\n\n  Warning: the command isn't recognized in the following line:\n   - Binary file (standard input) matches\n\n  You can fix this with 'git rebase --edit-todo'.\n  Or you can abort the rebase with 'git rebase --abort'.\n\nPossibly related to recent GNU grep changes, as with commit\n316336379cf7937c2ecf122c7197cfe5da6b2061.  Avoid the issue by using sed\ninstead.\n\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n---\n git-rebase--interactive.sh        |  2 +-\n t/t3409-rebase-preserve-merges.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c0cfe88..0efc65c 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -1241,7 +1241,7 @@ then\n \t\t\t# be rebasing on top of it\n \t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n \t\t\tsha1=$(git rev-list -1 $rev)\n-\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n+\t\t\tsed \"/^[a-z][a-z]* $sha1/d\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n \t\t\trm \"$rewritten\"/$rev\n \t\tfi\n \tdone\ndiff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\nindex 8c251c5..1f01b29 100755\n--- a/t/t3409-rebase-preserve-merges.sh\n+++ b/t/t3409-rebase-preserve-merges.sh\n@@ -119,4 +119,25 @@ test_expect_success 'rebase -p ignores merge.log config' '\n \t)\n '\n \n+test_expect_success 'rebase -p works with non-ASCII commit message' '\n+\t(\n+\tmkdir non-ascii &&\n+\tcd non-ascii &&\n+\tgit init &&\n+\techo a > a &&\n+\tgit add a &&\n+\tgit commit -m a &&\n+\techo b > b &&\n+\tgit add b &&\n+\tgit commit -m b &&\n+\tgit branch foo &&\n+\tgit reset --hard HEAD^ &&\n+\tgit cherry-pick -x foo &&\n+\techo c > c &&\n+\tgit add c &&\n+\tgit commit -m \"$(printf \"I \\\\342\\\\231\\\\245 Unicode\")\" &&\n+\tgit rebase -p foo\n+\t)\n+'\n+\n test_done\n-- \n2.8.0.rc0\n"},{"id":"280363","messageId":"56DEC4B4.2000902@web.de","threadId":"41640","inReplyTo":"alpine.DEB.2.10.1603080255030.2674@buzzword-bingo.mit.edu","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-03-08T12:25:24Z","receivedAt":"2016-03-08T12:25:24Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 03/08/2016 08:59 AM, Anders Kaseorg wrote:\n> The included test case, which uses rebase -p with non-ASCII commit\n> messages, was failing as follows:\n>\n>    Warning: the command isn't recognized in the following line:\n>     - Binary file (standard input) matches\n>\n>    You can fix this with 'git rebase --edit-todo'.\n>    Or you can abort the rebase with 'git rebase --abort'.\n>\n> Possibly related to recent GNU grep changes, as with commit\n> 316336379cf7937c2ecf122c7197cfe5da6b2061.  Avoid the issue by using sed\n> instead.\n>\n> Signed-off-by: Anders Kaseorg <andersk@mit.edu>\n> ---\n>   git-rebase--interactive.sh        |  2 +-\n>   t/t3409-rebase-preserve-merges.sh | 21 +++++++++++++++++++++\n>   2 files changed, 22 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index c0cfe88..0efc65c 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -1241,7 +1241,7 @@ then\n>   \t\t\t# be rebasing on top of it\n>   \t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n>   \t\t\tsha1=$(git rev-list -1 $rev)\n> -\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n> +\t\t\tsed \"/^[a-z][a-z]* $sha1/d\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n>   \t\t\trm \"$rewritten\"/$rev\n>   \t\tfi\n>   \tdone\n> diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\n> index 8c251c5..1f01b29 100755\n> --- a/t/t3409-rebase-preserve-merges.sh\n> +++ b/t/t3409-rebase-preserve-merges.sh\n> @@ -119,4 +119,25 @@ test_expect_success 'rebase -p ignores merge.log config' '\n>   \t)\n>   '\n>   \n> +test_expect_success 'rebase -p works with non-ASCII commit message' '\n> +\t(\n> +\tmkdir non-ascii &&\n#The cd should be done in a subshell:\n(\n> +\tcd non-ascii &&\n> +\tgit init &&\n> +\techo a > a &&\n> +\tgit add a &&\n> +\tgit commit -m a &&\n> +\techo b > b &&\n#Style: No space after \">\" (and even above and below)\n\necho b >b\n\n\n> +\tgit add b &&\n> +\tgit commit -m b &&\n> +\tgit branch foo &&\n> +\tgit reset --hard HEAD^ &&\n> +\tgit cherry-pick -x foo &&\n> +\techo c > c &&\n> +\tgit add c &&\n> +\tgit commit -m \"$(printf \"I \\\\342\\\\231\\\\245 Unicode\")\" &&\n> +\tgit rebase -p foo\n> +\t)\n> +\n#end of subshell\n)\n> '\n> +\n>   test_done\n"},{"id":"280369","messageId":"56DED770.4050603@drmicha.warpmail.net","threadId":"41640","inReplyTo":"56DEC4B4.2000902@web.de","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-03-08T13:45:20Z","receivedAt":"2016-03-08T13:45:20Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Torsten Bögershausen venit, vidit, dixit 08.03.2016 13:25:\n> On 03/08/2016 08:59 AM, Anders Kaseorg wrote:\n>> The included test case, which uses rebase -p with non-ASCII commit\n>> messages, was failing as follows:\n>>\n>>    Warning: the command isn't recognized in the following line:\n>>     - Binary file (standard input) matches\n>>\n>>    You can fix this with 'git rebase --edit-todo'.\n>>    Or you can abort the rebase with 'git rebase --abort'.\n>>\n>> Possibly related to recent GNU grep changes, as with commit\n>> 316336379cf7937c2ecf122c7197cfe5da6b2061.  Avoid the issue by using sed\n>> instead.\n>>\n>> Signed-off-by: Anders Kaseorg <andersk@mit.edu>\n>> ---\n>>   git-rebase--interactive.sh        |  2 +-\n>>   t/t3409-rebase-preserve-merges.sh | 21 +++++++++++++++++++++\n>>   2 files changed, 22 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> index c0cfe88..0efc65c 100644\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -1241,7 +1241,7 @@ then\n>>   \t\t\t# be rebasing on top of it\n>>   \t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n>>   \t\t\tsha1=$(git rev-list -1 $rev)\n>> -\t\t\tsane_grep -v \"^[a-z][a-z]* $sha1\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n>> +\t\t\tsed \"/^[a-z][a-z]* $sha1/d\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n>>   \t\t\trm \"$rewritten\"/$rev\n>>   \t\tfi\n>>   \tdone\n>> diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\n>> index 8c251c5..1f01b29 100755\n>> --- a/t/t3409-rebase-preserve-merges.sh\n>> +++ b/t/t3409-rebase-preserve-merges.sh\n>> @@ -119,4 +119,25 @@ test_expect_success 'rebase -p ignores merge.log config' '\n>>   \t)\n>>   '\n>>   \n>> +test_expect_success 'rebase -p works with non-ASCII commit message' '\n>> +\t(\n>> +\tmkdir non-ascii &&\n> #The cd should be done in a subshell:\n> (\n>> +\tcd non-ascii &&\n>> +\tgit init &&\n>> +\techo a > a &&\n>> +\tgit add a &&\n>> +\tgit commit -m a &&\n>> +\techo b > b &&\n> #Style: No space after \">\" (and even above and below)\n\nAnd also on the sane_grep/sed line.\n\n> \n> echo b >b\n> \n> \n>> +\tgit add b &&\n>> +\tgit commit -m b &&\n>> +\tgit branch foo &&\n>> +\tgit reset --hard HEAD^ &&\n>> +\tgit cherry-pick -x foo &&\n>> +\techo c > c &&\n>> +\tgit add c &&\n>> +\tgit commit -m \"$(printf \"I \\\\342\\\\231\\\\245 Unicode\")\" &&\n>> +\tgit rebase -p foo\n>> +\t)\n>> +\n> #end of subshell\n> )\n\nThe whole test is in a subshell already, although that is easy to miss\n(missing indentation).\n\n>> '\n>> +\n>>   test_done\n> \n\nIt may be worth noting whether other occurrences of \"sane_grep\" are safe\nfrom binary input.\n\nAfter all, one my question the degree of sanity of our sane_grep, or\nwhether we need asane_grep and bisane_grep in our shell library - \"make\nour grep sane again\".\n\nMichael\n"},{"id":"280370","messageId":"20160308143556.GA10153@sigill.intra.peff.net","threadId":"41640","inReplyTo":"56DED770.4050603@drmicha.warpmail.net","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-08T14:35:56Z","receivedAt":"2016-03-08T14:35:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 08, 2016 at 02:45:20PM +0100, Michael J Gruber wrote:\n\n> It may be worth noting whether other occurrences of \"sane_grep\" are safe\n> from binary input.\n> \n> After all, one my question the degree of sanity of our sane_grep, or\n> whether we need asane_grep and bisane_grep in our shell library - \"make\n> our grep sane again\".\n\nI actually wonder if we should have a build-time knob to put \"grep -a\"\ninto sane_grep(). We do not ever plan to feed it binary data, so that\nwill do what, provided the system grep handles \"-a\". And on those that\ndo not know about \"-a\", one imagines that they do not suffer from this\nproblem in the first place (which is really limited to recent versions\nof GNU grep).\n\n-Peff\n"},{"id":"280393","messageId":"xmqqio0wk151.fsf@gitster.mtv.corp.google.com","threadId":"41640","inReplyTo":"20160308143556.GA10153@sigill.intra.peff.net","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-08T23:20:26Z","receivedAt":"2016-03-08T23:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I actually wonder if we should have a build-time knob to put \"grep -a\"\n> into sane_grep(). We do not ever plan to feed it binary data, so that\n> will do what, provided the system grep handles \"-a\". And on those that\n> do not know about \"-a\", one imagines that they do not suffer from this\n> problem in the first place (which is really limited to recent versions\n> of GNU grep).\n\nSomething along this line, you mean?  I'll leave it as a\nlow-hanging-fruit to add autoconf support ;-)\n\n Makefile        | 4 ++++\n git-sh-setup.sh | 4 ++--\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 24bef8d..1187498 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -264,6 +264,9 @@ all::\n #\n # Define NO_TCLTK if you do not want Tcl/Tk GUI.\n #\n+# Define SANE_TEXT_GREP to \"-a\" if you use recent versions of GNU grep\n+# and egrep that are picker when their input contains non-ASCII data.\n+#\n # The TCL_PATH variable governs the location of the Tcl interpreter\n # used to optimize git-gui for your system.  Only used if NO_TCLTK\n # is not set.  Defaults to the bare 'tclsh'.\n@@ -1752,6 +1755,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n     -e $(BROKEN_PATH_FIX) \\\n     -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n     -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n+    -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n     $@.sh >$@+\n endef\n \ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 4691fbc..c48139a 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -168,11 +168,11 @@ git_pager() {\n }\n \n sane_grep () {\n-\tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n+\tGREP_OPTIONS= LC_ALL=C grep @@SANE_TEXT_GREP@@ \"$@\"\n }\n \n sane_egrep () {\n-\tGREP_OPTIONS= LC_ALL=C egrep \"$@\"\n+\tGREP_OPTIONS= LC_ALL=C egrep @@SANE_TEXT_GREP@@ \"$@\"\n }\n \n is_bare_repository () {\n"},{"id":"280394","messageId":"xmqqegbkk0ed.fsf@gitster.mtv.corp.google.com","threadId":"41640","inReplyTo":"xmqqio0wk151.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-08T23:36:26Z","receivedAt":"2016-03-08T23:36:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Subject: rebase-i: clarify \"is this commit relevant\" test\n\nWhile I was checking all the call sites of sane_grep and sane_egrep,\nI noticed this one is somewhat strangely written.  The lines in the\nfile sane_grep works on all begin with 40-hex object name, so there\nis no real risk of confusing \"test $(...) = ''\" by finding something\nthat begins with a dash, but using the status from sane_grep makes\nit a lot clearer what is going on.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * By the way, if we are going to take that @@SANE_TEXT_GREP@@\n   patch, we'd need to add $(SANE_TEXT_GREP) to SCRIPT_DEFINES\n   in Makefile to force git-sh-setup to be regenerated when its\n   setting changes.\n\n git-rebase--interactive.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c0cfe88..4cde685 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -1233,7 +1233,8 @@ then\n \tgit rev-list $revisions |\n \twhile read rev\n \tdo\n-\t\tif test -f \"$rewritten\"/$rev && test \"$(sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks)\" = \"\"\n+\t\tif test -f \"$rewritten\"/$rev &&\n+\t\t   ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n \t\tthen\n \t\t\t# Use -f2 because if rev-list is telling us this commit is\n \t\t\t# not worthwhile, we don't want to track its multiple heads,\n"},{"id":"280395","messageId":"20160309001042.GA32669@sigill.intra.peff.net","threadId":"41640","inReplyTo":"xmqqio0wk151.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-09T00:10:43Z","receivedAt":"2016-03-09T00:10:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 08, 2016 at 03:20:26PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I actually wonder if we should have a build-time knob to put \"grep -a\"\n> > into sane_grep(). We do not ever plan to feed it binary data, so that\n> > will do what, provided the system grep handles \"-a\". And on those that\n> > do not know about \"-a\", one imagines that they do not suffer from this\n> > problem in the first place (which is really limited to recent versions\n> > of GNU grep).\n> \n> Something along this line, you mean?  I'll leave it as a\n> low-hanging-fruit to add autoconf support ;-)\n\nYeah, though I think I would probably squash in:\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 723f632..15557c3 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)\n \tHAVE_CLOCK_GETTIME = YesPlease\n \tHAVE_CLOCK_MONOTONIC = YesPlease\n \tHAVE_GETDELIM = YesPlease\n+\tSANE_TEXT_GREP = -a\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n\nIt's not necessary on all Linux platforms yet, but it doesn't hurt, so\nwe can err on the side of including it (and I think we can assume all\nLinux systems have GNU grep or equivalent).\n\n-Peff\n"},{"id":"280396","messageId":"20160309001157.GB32669@sigill.intra.peff.net","threadId":"41640","inReplyTo":"xmqqegbkk0ed.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-09T00:11:57Z","receivedAt":"2016-03-09T00:11:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 08, 2016 at 03:36:26PM -0800, Junio C Hamano wrote:\n\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index c0cfe88..4cde685 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -1233,7 +1233,8 @@ then\n>  \tgit rev-list $revisions |\n>  \twhile read rev\n>  \tdo\n> -\t\tif test -f \"$rewritten\"/$rev && test \"$(sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks)\" = \"\"\n> +\t\tif test -f \"$rewritten\"/$rev &&\n> +\t\t   ! sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks >/dev/null\n\nLooks better. I think \"-q\" might be nicer still (and more efficient,\nthough it almost certainly doesn't matter in practice).\n\n-Peff\n"},{"id":"280406","messageId":"alpine.DEB.2.10.1603082127230.2674@buzzword-bingo.mit.edu","threadId":"41640","inReplyTo":"xmqqio0wk151.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-03-09T02:31:01Z","receivedAt":"2016-03-09T02:31:01Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"The autoconf support you committed as 67f1790a has a small bug (the else \ncause should omit -a):\n\n+if grep -a ascii configure.ac >/dev/null; then\n+  AC_MSG_RESULT([Using 'grep -a' for sane_grep])\n+  SANE_TEXT_GREP=-a\n+else\n+  SANE_TEXT_GREP=-a\n+fi\n+GIT_CONF_SUBST([SANE_TEXT_GREP])\n\nAnders\n"},{"id":"280506","messageId":"xmqqvb4vgzxs.fsf@gitster.mtv.corp.google.com","threadId":"41640","inReplyTo":"alpine.DEB.2.10.1603082127230.2674@buzzword-bingo.mit.edu","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-09T20:26:55Z","receivedAt":"2016-03-09T20:26:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg <andersk@mit.edu> writes:\n\n> The autoconf support you committed as 67f1790a has a small bug (the else \n> cause should omit -a):\n\nThanks.  To summarize the discussion, here is what I ended up with.\n\n-- >8 --\nSubject: [PATCH] sane_grep: pass \"-a\" if grep accepts it\n\nNewer versions of GNU grep is reported to be pickier when we feed a\nnon-ASCII input and break some Porcelain scripts.  As we know we do\nnot feed random binary file to our own sane_grep wrapper, allow us\nto always pass \"-a\" by setting SANE_TEXT_GREP=-a Makefile variable\nto work it around.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile         | 6 +++++-\n config.mak.uname | 1 +\n configure.ac     | 7 +++++++\n git-sh-setup.sh  | 4 ++--\n 4 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 37e2d9e..62d759d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -266,6 +266,9 @@ all::\n #\n # Define NO_TCLTK if you do not want Tcl/Tk GUI.\n #\n+# Define SANE_TEXT_GREP to \"-a\" if you use recent versions of GNU grep\n+# and egrep that are picker when their input contains non-ASCII data.\n+#\n # The TCL_PATH variable governs the location of the Tcl interpreter\n # used to optimize git-gui for your system.  Only used if NO_TCLTK\n # is not set.  Defaults to the bare 'tclsh'.\n@@ -1728,7 +1731,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)\n \n SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n \t$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\\\n-\t$(gitwebdir_SQ):$(PERL_PATH_SQ)\n+\t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP)\n define cmd_munge_script\n $(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n@@ -1740,6 +1743,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n     -e $(BROKEN_PATH_FIX) \\\n     -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \\\n     -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \\\n+    -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \\\n     $@.sh >$@+\n endef\n \ndiff --git a/config.mak.uname b/config.mak.uname\nindex 943c439..a706474 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)\n \tHAVE_CLOCK_GETTIME = YesPlease\n \tHAVE_CLOCK_MONOTONIC = YesPlease\n \tHAVE_GETDELIM = YesPlease\n+\tSANE_TEXT_GREP=-a\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\ndiff --git a/configure.ac b/configure.ac\nindex 1f55009..6fd7b8e 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -471,6 +471,13 @@ if test -n \"$ASCIIDOC\"; then\n \tesac\n fi\n \n+if grep -a ascii configure.ac >/dev/null; then\n+  AC_MSG_RESULT([Using 'grep -a' for sane_grep])\n+  SANE_TEXT_GREP=-a\n+else\n+  SANE_TEXT_GREP=\n+fi\n+GIT_CONF_SUBST([SANE_TEXT_GREP])\n \n ## Checks for libraries.\n AC_MSG_NOTICE([CHECKS for libraries])\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 4691fbc..c48139a 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -168,11 +168,11 @@ git_pager() {\n }\n \n sane_grep () {\n-\tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n+\tGREP_OPTIONS= LC_ALL=C grep @@SANE_TEXT_GREP@@ \"$@\"\n }\n \n sane_egrep () {\n-\tGREP_OPTIONS= LC_ALL=C egrep \"$@\"\n+\tGREP_OPTIONS= LC_ALL=C egrep @@SANE_TEXT_GREP@@ \"$@\"\n }\n \n is_bare_repository () {\n-- \n2.8.0-rc1-141-gbaa22e3\n"},{"id":"280532","messageId":"56E1256B.9030308@web.de","threadId":"41640","inReplyTo":"xmqqvb4vgzxs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-03-10T07:42:35Z","receivedAt":"2016-03-10T07:42:35Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 09.03.16 21:26, Junio C Hamano wrote:\n> Anders Kaseorg <andersk@mit.edu> writes:\n[]\n>  sane_grep () {\n> -\tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n> +\tGREP_OPTIONS= LC_ALL=C grep @@SANE_TEXT_GREP@@ \"$@\"\n>  }\n>  \n>  sane_egrep () {\n> -\tGREP_OPTIONS= LC_ALL=C egrep \"$@\"\n> +\tGREP_OPTIONS= LC_ALL=C egrep @@SANE_TEXT_GREP@@ \"$@\"\n>  }\n>\n\nI always wondered why we do LC_ALL=C.\nIsn't that begging for trouble, when we feed UTF-8, ISO-8895-1\nor other stuff into a program and say LC_ALL=C at the same time ?\n\nOn my Debian Linux system I have\nLANG=en_US.UTF-8\n\nand\n\n$ locale -a\nC\nC.UTF-8\nen_US.utf8\nPOSIX\n--------------\n\nMac OS has LANG unset, and reports\nlocale -a\nen_US\nen_US.ISO8859-1\nen_US.ISO8859-15\nen_US.US-ASCII\nen_US.UTF-8\n#(and a lot more )\nC\nPOSIX\n\n-----\nMy Centos has \nLANG=en_US.UTF-8\n\nand reports e.g.\nen_US\nen_US.iso88591\nen_US.iso885915\nen_US.utf8\n(And many more)\n\nIn t0204 we have\n    LANGUAGE=is LC_ALL=\"$is_IS_locale\" git init repo >actual &&\nwhich is based on\n\t# is_IS.UTF-8 on Solaris and FreeBSD, is_IS.utf8 on Debian\n\tis_IS_locale=$(locale -a 2>/dev/null |\nin \nlib-gettext.sh\n\nIs there something we can steal here ?\n\n\nhttp://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html\n"},{"id":"280560","messageId":"xmqq1t7ifdtf.fsf@gitster.mtv.corp.google.com","threadId":"41640","inReplyTo":"56E1256B.9030308@web.de","subject":"Re: [PATCH] rebase -p: avoid grep on potentailly non-ASCII data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-10T17:22:20Z","receivedAt":"2016-03-10T17:22:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> On 09.03.16 21:26, Junio C Hamano wrote:\n>> Anders Kaseorg <andersk@mit.edu> writes:\n> []\n>>  sane_grep () {\n>> -\tGREP_OPTIONS= LC_ALL=C grep \"$@\"\n>> +\tGREP_OPTIONS= LC_ALL=C grep @@SANE_TEXT_GREP@@ \"$@\"\n>>  }\n>>  \n>>  sane_egrep () {\n>> -\tGREP_OPTIONS= LC_ALL=C egrep \"$@\"\n>> +\tGREP_OPTIONS= LC_ALL=C egrep @@SANE_TEXT_GREP@@ \"$@\"\n>>  }\n>>\n>\n> I always wondered why we do LC_ALL=C.\n\nIt is because, at least back when we started scripted Porcelains of\nGit, that is the only thing we could count on available everywhere,\nand more importantly, that is how you disable all the i18n gotchas\nand ask tools to use the traditional \"a file stores a stream of\nbytes\" semantics.\n\n> Isn't that begging for trouble, when we feed UTF-8, ISO-8895-1\n> or other stuff into a program and say LC_ALL=C at the same time ?\n\nYes and no.  It is true that in \"C\" (\"POSIX\"), behaviour on anything\noutside the portable and control character sets is undefined, so in\ntheory, it is bad that we relied on that undefined behaviour to be\nto treat the input as just a stream of bytes.  But at the same time,\nit is no worse than letting the user's locale take effect or use\nhardcoded en_US.UTF-8 and passing unknown end user data that could\nbe in latin1 and other stuff.  And sensible implementors of \"C\"\nlocale seemed to choose the \"stream of bytes\" back when fa9d3485\n(git-am: force egrep to use correct characters set, 2009-09-25) was\nwritten, which is where LC_ALL=C comes from.  I wasn't around when\nthat patch was accepted, so I cannot quite tell if it was done to\nfix a reported bug (i.e. it would reintroduce the bug if we lost\nLC_ALL=C) or it was done \"just because\".\n\nBack when e1622bfc (Protect scripted Porcelains from GREP_OPTIONS\ninsanity, 2009-11-23) introduced sane_grep, the only caller of it\nthat wanted to use LC_ALL=C was this callsite in \"git am\", and we no\nlonger have that callsite since 783d7e86 (builtin-am: remove\nredirection to git-am.sh, 2015-08-04), so I think whatever fa9d3485\nwanted to do will not be broken if we removed LC_ALL=C from\nsane_grep.  It however is possible that any callsites of sane_grep\nadded after e1622bfc implicitly depended on having LC_ALL=C in the\nimplementation.\n"}]}