[PATCH] Optimize usage of grep by passing -q

Instead of redirecting all grep output to /dev/null, we can just
pass in -q instead. This preserves the exit code behavior, but is faster.
As grep returns true if it finds at least one match, grep can exit promptly
after finding the first line and doesn't need to find more occurrences
which would be redirected to /dev/null anyways.

This is true for the gnu version of grep. I am not sure if all
versions of grep support this optimization. In case it is not,
we'd revert this patch.

Signed-off-by: Stefan Beller <redacted>
---
 git-bisect.sh              | 5 ++---
 git-rebase--interactive.sh | 2 +-
 git-rebase.sh              | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..b909605 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -519,8 +519,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 
 		cat "$GIT_DIR/BISECT_RUN"
 
-		if sane_grep "first $TERM_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \
-			>/dev/null
+		if sane_grep -q "first $TERM_BAD commit could be any of" "$GIT_DIR/BISECT_RUN"
 		then
 			gettextln "bisect run cannot continue any more" >&2
 			exit $res

@@ -533,7 +532,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 			exit $res
 		fi
 
-		if sane_grep "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null
+		if sane_grep -q "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN"
 		then
 			gettextln "bisect run success"
 			exit 0;

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..f360ac0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -1225,7 +1225,7 @@ 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 && test "$(sane_grep -q "$rev" "$state_dir"/not-cherry-picks)"
 		then
 			# Use -f2 because if rev-list is telling us this commit is
 			# not worthwhile, we don't want to track its multiple heads,

diff --git a/git-rebase.sh b/git-rebase.sh
index af7ba5f..b6a5f73 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -578,7 +578,7 @@ mb=$(git merge-base "$onto" "$orig_head")
 if test "$type" != interactive && test "$upstream" = "$onto" &&
 	test "$mb" = "$onto" && test -z "$restrict_revision" &&
 	# linear history?
-	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
+	! (git rev-list --parents "$onto".."$orig_head" | sane_grep -q " .* ")
 then
 	if test -z "$force_rebase"
 	then

-- 
2.6.3.368.gf34be46

On Mon, Nov 16, 2015 at 10:43 PM, Stefan Beller [off-list ref] wrote:

Instead of redirecting all grep output to /dev/null, we can just
pass in -q instead. This preserves the exit code behavior, but is faster.
As grep returns true if it finds at least one match, grep can exit promptly
after finding the first line and doesn't need to find more occurrences
which would be redirected to /dev/null anyways.

This is true for the gnu version of grep. I am not sure if all
versions of grep support this optimization. In case it is not,
we'd revert this patch.

POSIX specifies -q, so you should be fine.
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html

-- 
Mikael Magnusson

+cc Andrey Rybak, who I credit for finding the reasoning below (he
sent to me privately,
without cc'ing the list)

On Mon, Nov 16, 2015 at 4:59 PM, Mikael Magnusson [off-list ref] wrote:

On Mon, Nov 16, 2015 at 10:43 PM, Stefan Beller [off-list ref] wrote:

quoted

Instead of redirecting all grep output to /dev/null, we can just
pass in -q instead. This preserves the exit code behavior, but is faster.
As grep returns true if it finds at least one match, grep can exit promptly
after finding the first line and doesn't need to find more occurrences
which would be redirected to /dev/null anyways.

This is true for the gnu version of grep. I am not sure if all
versions of grep support this optimization. In case it is not,
we'd revert this patch.

POSIX specifies -q, so you should be fine.
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html

From http://www.gnu.org/software/grep/manual/grep.html :

-q
--quiet
--silent
Quiet; do not write anything to standard output. Exit immediately with
zero status if any match is found, even if an error was detected. Also
see the -s or --no-messages option. (-q is specified by POSIX.)
-s
--no-messages
Suppress error messages about nonexistent or unreadable files.
Portability note: unlike GNU grep, 7th Edition Unix grep did not
conform to POSIX, because it lacked -q and its -s option behaved like
GNU grep's -q option.1
USG-style grep also lacked -q but its -s option behaved like GNU
grep's. Portable shell scripts should avoid both -q and -s and should
redirect standard and error output to /dev/null instead. (-s is
specified by POSIX.)

Reading that in full, I think my patch is a bad idea.

On Mon, Nov 16, 2015 at 05:04:24PM -0800, Stefan Beller wrote:

quoted

quoted

This is true for the gnu version of grep. I am not sure if all
versions of grep support this optimization. In case it is not,
we'd revert this patch.

POSIX specifies -q, so you should be fine.
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html

From http://www.gnu.org/software/grep/manual/grep.html :
[...]
Portability note: unlike GNU grep, 7th Edition Unix grep did not
conform to POSIX, because it lacked -q and its -s option behaved like
GNU grep's -q option.1
USG-style grep also lacked -q but its -s option behaved like GNU
grep's. Portable shell scripts should avoid both -q and -s and should
redirect standard and error output to /dev/null instead. (-s is
specified by POSIX.)

I wonder what the current state of "most" systems is. 7th Edition Unix
is probably old enough for us not to worry about. :)

For the git project, being in POSIX is not an automatic pass for a
feature. We care about real systems. I note that we do have quite a bit
of "grep -q" in the test scripts, but not in the actual git-scripts.

This came up as recently as 2008 (e.g., aadbe44), but I don't recall
anybody complaining recently. Perhaps Solaris grep finally grew a "-q"
option. Or maybe nobody runs the tests there anymore.

Since this is an optimization, I'd be more interested if we had numbers
for the improvement. Are these files really big enough that grepping the
rest of the file is noticeable versus the cost of starting grep in the
first place?

If this is something measurable, we might be able to make it a build
flag (e.g., by wrapping these grep invocations in a shell function in
git-sh-setup.sh, and picking the implementation at build time).

-Peff