[PATCH v2] git submodule foreach: Skip eval for more than one argument

‘eval "$@"’ created an extra layer of shell interpretation, which was
probably not expected by a user who passed multiple arguments to git
submodule foreach:

$ git grep "'"
[searches for single quotes]
$ git submodule foreach git grep "'"
Entering '[submodule]'
/usr/lib/git-core/git-submodule: 1: eval: Syntax error: Unterminated quoted string
Stopping at '[submodule]'; script returned non-zero status.

To fix this, if the user passed more than one argument, just execute
"$@" directly instead of passing it to eval.

Signed-off-by: Anders Kaseorg <redacted>
---

On Fri, 27 Sep 2013, Johan Herland wrote:

1. Please add the use case you mention above as a new test case, so
that we can easily catch future regressions.

Test added.

2. If we are unlucky there might be existing users that work around the 
existing behavior by adding an extra level of quoting (i.e. doing the 
equivalent of git submodule foreach git grep "\'" in your example 
above). Will their workaround break as a result of your change? Is that 
acceptable?

Anyone adding an extra level of quoting ought to realize that they should 
be passing a single argument to submodule foreach, so that the reason for 
the extra quoting is clear:
  git submodule foreach "git grep \'"
will not break.  If someone is actually doing
  git submodule foreach git grep "\'"
then this will change in behavior.  I think this change is important.

(One could even imagine someone feeding untrusted input to
  git submodule foreach git grep "$variable"
which, without my patch, results in a nonobvious shell code injection 
vulnerability.)

I considered an alternative fix where the first argument is always 
shell-evaulated and any others are not (i.e. cmd=$1 && shift && eval 
"$cmd \"\$@\""), which is potentially more useful in case the command 
needs to use $path.  But that may be too confusing, and this way has some 
precedent (e.g. perl’s system()).

Anders


 git-submodule.sh             | 7 ++++++-
 t/t7407-submodule-foreach.sh | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c17bef1..3381864 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -545,7 +545,12 @@ cmd_foreach()
 				sm_path=$(relative_path "$sm_path") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
-				eval "$@" &&
+				if [ $# -eq 1 ]
+				then
+					eval "$1"
+				else
+					"$@"
+				fi &&
 				if test -n "$recursive"
 				then
 					cmd_foreach "--recursive" "$@"

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index be93f10..6b2fd39 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh

@@ -329,4 +329,13 @@ test_expect_success 'command passed to foreach --recursive retains notion of std
 	test_cmp expected actual
 '
 
+test_expect_success 'multi-argument command passed to foreach is not shell-evaluated twice' '
+	(
+		cd super &&
+		git submodule foreach "echo \\\"quoted\\\"" > ../expected &&
+		git submodule foreach echo \"quoted\" > ../actual
+	) &&
+	test_cmp expected actual
+'
+
 test_done

-- 
1.8.4