Re: [PATCH] unset GREP_OPTIONS in test-lib.sh
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 24 quoted lines
>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
>> index c41c2f7..2b2afa6 100755
>> --- a/git-sh-setup.sh
>> +++ b/git-sh-setup.sh
>> @@ -114,6 +114,20 @@ git_editor() {
>> eval "${GIT_EDITOR:=vi}" '"$@"'
>> }
>>
>> +sane_grep () {
>> + GREP_OPTIONS= \
>> + GREP_COLOR= \
>> + GREP_COLORS= \
>> + LC_ALL=C grep "$@"
>> +}
>> +
>> +sane_egrep () {
>> + GREP_OPTIONS= \
>> + GREP_COLOR= \
>> + GREP_COLORS= \
>> + LC_ALL=C egrep "$@"
>> +}
>> +
>
> Ah, yes, much nicer.Actually I am having a second thought after spending some time trying to come up with a commit log message. This leaves the door open for user scripts to honor the environment variables, but it also means that everybody needs to be aware of the insanity. It is really tempting to treat these exactly like CDPATH and unconditionally unset it.
Oh, and unsetting GREP_COLORS/GREP_COLOR was a mistake, I think. As long as we do not pass --color (and unset GREP_OPTIONS to make sure it is not given), their settings should not matter to us.
-- >8 --
Subject: [PATCH] Protect scripted Porcelains from GREP_OPTIONS insanity
If the user has exported the GREP_OPTIONS environment variable, the output from "grep" and "egrep" in scripted Porcelains may be different from what they expect. For example, we may want to count number of matching lines, by "grep" piped to "wc -l", and GREP_OPTIONS=-C3 will break such use.
The approach taken by this change to address this issue is to protect only our own use of grep/egrep. Because we do not unset it at the beginning of our scripts, hook scripts run from the scripted Porcelains are exposed to the same insanity this environment variable causes when grep/egrep is used to implement logic (e.g. "grep | wc -l"), and it is entirely up to the hook scripts to protect themselves.
On the other hand, applypatch-msg hook may want to show offending words in the proposed commit log message using grep to the end user, and the user might want to set GREP_OPTIONS=--color to paint the match more visibly. The approach to protect only our own use without unsetting the environment variable globally will allow this use case.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-sh-setup.sh | 8 ++++++++
git-am.sh | 4 ++--
git-bisect.sh | 4 ++--
git-filter-branch.sh | 2 +-
git-instaweb.sh | 8 ++++----
git-rebase--interactive.sh | 10 +++++-----
git-rebase.sh | 2 +-
git-submodule.sh | 6 +++---
8 files changed, 26 insertions(+), 18 deletions(-)
Show changes to 8 files +26 −18
git-sh-setup.sh, git-am.sh, git-bisect.sh, git-filter-branch.sh, git-instaweb.sh, git-rebase--interactive.sh, git-rebase.sh, git-submodule.sh
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..aa07cc3 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -114,6 +114,14 @@ git_editor() {
eval "${GIT_EDITOR:=vi}" '"$@"'
}
+sane_grep () {
+ GREP_OPTIONS= LC_ALL=C grep "$@"
+}
+
+sane_egrep () {
+ GREP_OPTIONS= LC_ALL=C egrep "$@"
+}
+
is_bare_repository () {
git rev-parse --is-bare-repository
}
diff --git a/git-am.sh b/git-am.sh
index c132f50..b49f26a 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -205,7 +205,7 @@ check_patch_format () {
# and see if it looks like that they all begin with the
# header field names...
sed -n -e '/^$/q' -e '/^[ ]/d' -e p "$1" |
- LC_ALL=C egrep -v '^[!-9;-~]+:' >/dev/null ||
+ sane_egrep -v '^[!-9;-~]+:' >/dev/null ||
patch_format=mbox
fi
} < "$1" || clean_abort
@@ -554,7 +554,7 @@ do
stop_here $this
# skip pine's internal folder data
- grep '^Author: Mail System Internal Data$' \
+ sane_grep '^Author: Mail System Internal Data$' \
<"$dotest"/info >/dev/null &&
go_next && continue
diff --git a/git-bisect.sh b/git-bisect.sh
index 6f6f039..0c422d5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -393,7 +393,7 @@ bisect_run () {
cat "$GIT_DIR/BISECT_RUN"
- if grep "first bad commit could be any of" "$GIT_DIR/BISECT_RUN" \
+ if sane_grep "first bad commit could be any of" "$GIT_DIR/BISECT_RUN" \
> /dev/null; then
echo >&2 "bisect run cannot continue any more"
exit $res
@@ -405,7 +405,7 @@ bisect_run () {
exit $res
fi
- if grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" > /dev/null; then
+ if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" > /dev/null; then
echo "bisect run success"
exit 0;
fi
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index a480d6f..8ef1bde 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -457,7 +457,7 @@ if [ "$filter_tag_name" ]; then
git mktag) ||
die "Could not create new tag object for $ref"
if git cat-file tag "$ref" | \
- grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
+ sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
then
warn "gpg signature stripped from tag object $sha1t"
fi
diff --git a/git-instaweb.sh b/git-instaweb.sh
index d96eddb..84805c6 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -41,7 +41,7 @@ resolve_full_httpd () {
case "$httpd" in
*apache2*|*lighttpd*)
# ensure that the apache2/lighttpd command ends with "-f"
- if ! echo "$httpd" | grep -- '-f *$' >/dev/null 2>&1
+ if ! echo "$httpd" | sane_grep -- '-f *$' >/dev/null 2>&1
then
httpd="$httpd -f"
fi
@@ -297,8 +297,8 @@ EOF
# check to see if Dennis Stosberg's mod_perl compatibility patch
# (<20060621130708.Gcbc6e5c@leonov.stosberg.net>) has been applied
- if test -f "$module_path/mod_perl.so" && grep 'MOD_PERL' \
- "$GIT_DIR/gitweb/gitweb.cgi" >/dev/null
+ if test -f "$module_path/mod_perl.so" &&
+ sane_grep 'MOD_PERL' "$GIT_DIR/gitweb/gitweb.cgi" >/dev/null
then
# favor mod_perl if available
cat >> "$conf" <<EOF
@@ -316,7 +316,7 @@ EOF
# plain-old CGI
resolve_full_httpd
list_mods=$(echo "$full_httpd" | sed "s/-f$/-l/")
- $list_mods | grep 'mod_cgi\.c' >/dev/null 2>&1 || \
+ $list_mods | sane_grep 'mod_cgi\.c' >/dev/null 2>&1 || \
echo "LoadModule cgi_module $module_path/mod_cgi.so" >> "$conf"
cat >> "$conf" <<EOF
AddHandler cgi-script .cgi
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 23ded48..6268e76 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -106,8 +106,8 @@ mark_action_done () {
sed -e 1q < "$TODO" >> "$DONE"
sed -e 1d < "$TODO" >> "$TODO".new
mv -f "$TODO".new "$TODO"
- count=$(grep -c '^[^#]' < "$DONE")
- total=$(($count+$(grep -c '^[^#]' < "$TODO")))
+ count=$(sane_grep -c '^[^#]' < "$DONE")
+ total=$(($count+$(sane_grep -c '^[^#]' < "$TODO")))
if test "$last_count" != "$count"
then
last_count=$count
@@ -147,7 +147,7 @@ die_abort () {
}
has_action () {
- grep '^[^#]' "$1" >/dev/null
+ sane_grep '^[^#]' "$1" >/dev/null
}
pick_one () {
@@ -731,7 +731,7 @@ first and then run 'git rebase --continue' again."
git rev-list $REVISIONS |
while read rev
do
- if test -f "$REWRITTEN"/$rev -a "$(grep "$rev" "$DOTEST"/not-cherry-picks)" = ""
+ if test -f "$REWRITTEN"/$rev -a "$(sane_grep "$rev" "$DOTEST"/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,
@@ -739,7 +739,7 @@ first and then run 'git rebase --continue' again."
# be rebasing on top of it
git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$DROPPED"/$rev
short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
- grep -v "^[a-z][a-z]* $short" <"$TODO" > "${TODO}2" ; mv "${TODO}2" "$TODO"
+ sane_grep -v "^[a-z][a-z]* $short" <"$TODO" > "${TODO}2" ; mv "${TODO}2" "$TODO"
rm "$REWRITTEN"/$rev
fi
done
diff --git a/git-rebase.sh b/git-rebase.sh
index 6ec155c..0ec4355 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -467,7 +467,7 @@ orig_head=$branch
mb=$(git merge-base "$onto" "$branch")
if test "$upstream" = "$onto" && test "$mb" = "$onto" &&
# linear history?
- ! (git rev-list --parents "$onto".."$branch" | grep " .* ") > /dev/null
+ ! (git rev-list --parents "$onto".."$branch" | sane_grep " .* ") > /dev/null
then
if test -z "$force_rebase"
then
diff --git a/git-submodule.sh b/git-submodule.sh
index 0462e52..b7ccd12 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -57,7 +57,7 @@ resolve_relative_url ()
#
module_list()
{
- git ls-files --error-unmatch --stage -- "$@" | grep '^160000 '
+ git ls-files --error-unmatch --stage -- "$@" | sane_grep '^160000 '
}
#
@@ -567,7 +567,7 @@ cmd_summary() {
cd_to_toplevel
# Get modified modules cared by user
modules=$(git $diff_cmd $cached --raw $head -- "$@" |
- egrep '^:([0-7]* )?160000' |
+ sane_egrep '^:([0-7]* )?160000' |
while read mod_src mod_dst sha1_src sha1_dst status name
do
# Always show modules deleted or type-changed (blob<->module)
@@ -581,7 +581,7 @@ cmd_summary() {
test -z "$modules" && return
git $diff_cmd $cached --raw $head -- $modules |
- egrep '^:([0-7]* )?160000' |
+ sane_egrep '^:([0-7]* )?160000' |
cut -c2- |
while read mod_src mod_dst sha1_src sha1_dst status name
do
--
1.6.6.rc0.15.g4fa80.dirty
Bert Wesarg
