On Thu, Sep 26, 2013 at 10:10 PM, Anders Kaseorg <andersk@xxxxxxx> wrote: > ‘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 <andersk@xxxxxxx> The change looks good, and the existing tests (in t7407) pass. :-) Two comments, however: 1. Please add the use case you mention above as a new test case, so that we can easily catch future regressions. 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? Have fun! :) ...Johan > --- > git-submodule.sh | 7 ++++++- > 1 file changed, 6 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" "$@" > -- > 1.8.4 > -- Johan Herland, <johan@xxxxxxxxxxx> www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html