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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2016, 23:36 UTC
Message-ID: <xmqqegbkk0ed.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqio0wk151.fsf@gitster.mtv.corp.google.com>

Subject: rebase-i: clarify "is this commit relevant" test

While I was checking all the call sites of sane_grep and sane_egrep, I noticed this one is somewhat strangely written. The lines in the file sane_grep works on all begin with 40-hex object name, so there is no real risk of confusing "test $(...) = ''" by finding something that begins with a dash, but using the status from sane_grep makes it a lot clearer what is going on.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * By the way, if we are going to take that @@SANE_TEXT_GREP@@
   patch, we'd need to add $(SANE_TEXT_GREP) to SCRIPT_DEFINES
   in Makefile to force git-sh-setup to be regenerated when its
   setting changes.

 git-rebase--interactive.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c0cfe88..4cde685 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1233,7 +1233,8 @@ then
 	git rev-list $revisions |
 	while read rev
 	do
-		if test -f "$rewritten"/$rev && test "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = ""
+		if test -f "$rewritten"/$rev &&
+		   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
 		then
 			# Use -f2 because if rev-list is telling us this commit is
 			# not worthwhile, we don't want to track its multiple heads,