patchgit 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 <andersk@mit.edu>
---
 git-submodule.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

On Thu, Sep 26, 2013 at 10:10 PM, Anders Kaseorg <andersk@mit.edu> wrote:

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

‘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@mit.edu>
---

On Fri, 27 Sep 2013, Johan Herland wrote:

Test added.

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(-)

On Fri, Sep 27, 2013 at 12:23 PM, Anders Kaseorg <andersk@mit.edu> wrote:

Acked-by: Johan Herland <johan@herland.net>

Ok. I have nothing to add.

...Johan

Hey folks,

On Thu, Sep 26, 2013 at 04:10:15PM -0400, Anders Kaseorg wrote:

It seems this patch has broken the use of $name, $path, etc. inside the command ran by foreach (when it contains more than one argument):

matthijs@grubby:~/test$ git --version git version 1.9.0 matthijs@grubby:~/test$ git submodule foreach echo '$name' Entering 'test' $name

But it works on the single-argument version:

matthijs@grubby:~/test$ git submodule foreach 'echo $name' Entering 'test' test

And it used to work in older versions:

matthijs@login:~/test$ git --version git version 1.7.5.4 matthijs@login:~/test$ git submodule foreach 'echo $name' Entering 'test' test matthijs@login:~/test$ git submodule foreach echo '$name' Entering 'test' test

I'm not sure how to fix this exactly. Adding "export" for the variables in git-submodule.sh seems obvious but doesn't seem to be a complete solution. This makes the variables available in the environment of any commands called (so git submodule sh -c 'echo $name') works, but the git submodule foreach echo '$name' above still doesn't work, since the "$@" used does not do any substitution, it just executes $@ as a commandline unmodified. Ideally, you would do variable substitution, but not word splitting, but I'm not sure how to do that. Also, you'd still need one more layer of backslash escapes, which is probably what this commit wanted to prevent...

Note that saying "you should use the single argument version if you need those variables" doesn't seem possible in all cases. In particular, I'm creating an alias that calls git submodule foreach, where the alias contains part of the command and the rest of command comes from arguments to the alias, meaning we always have at least two arguments...

Finally, the new behaviour (e.g., eval with one argument, directly execute with multiple) is not documented in the manpage, but it seems relevant enough to need documentation?

Gr.

Matthijs

On Tue, Mar 4, 2014 at 2:51 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:

jherland@beta ~/test$ echo '$name' $name

What would you expect echo '$name' to do? What happens if you use double instead of single quotes?

...Johan

On Tue, Mar 04, 2014 at 03:53:24PM +0100, Johan Herland wrote:

If I run git submodule foreach each '$name', then my shell eats the single quotes (which are only to prevent my shell from interpreting $name). git submodule will see $name, so it will run echo $name, not echo '$name'.

Then my shell eats up the double quotes _and_ replaces $name with nothing, so I can't expect git submodule to replace it with the submodule name then :-)

Does that help to clarify what I mean?

Gr.

Matthijs

On Tue, Mar 4, 2014 at 3:57 PM, Matthijs Kooijman <matthijs@stdin.nl> wrote:

Ok, so IINM, Anders' original commit was about making "git submodule foreach <command>" behave more like "<command>" (from a naive user's perspective), while you rather expect to insert quotes/escapes to finely control exactly when shell interpretation happens. Aren't these POVs mutually incompatible? Is the only 'real' solution to forbid multitple arguments, and force everybody to quote the entire command?

I don't particularly care which way it goes, as long as (a) the common case behaves as most users would expect, (b) the uncommon/complicated case is still _possible_ (though not necessarily simple), and (c) we don't break a sizable number of existing users.

...Johan

Hey Johan,

Ok, that makes sense.

Well, I mostly expect that the $name and $path that git submodule makes available to each command invocation can actually be used by the command.

Yes, I think you're right that they're mutually exclusive. Specifically, if you expect git submodule foreach <command> to behave like <command>, that means you expect the (interactive) shell to do all the interpolation, word-splitting, etc. If so, you can't then later still do interpolation (of course, you could do sed magic to just replace $name and $path, etc., but that's broken).

Well, if you call submodule directly, you can now just put everything in a single command and get $name interpolation.

As I mentioned, I couldn't do this because I was using a git alias. However, a bit of fiddling showed a solution to that using a shell function:

[alias]
	each = "!f(){ git submodule foreach --quiet \"echo \\$name $*\";}; f"

This uses a shell function to collect all alias arguments and then uses $* to expand them again into the single submodule foreach argument. Note that $* is expanded when evaluating the alias, while \\$name is expanded later inside submodule.

This suggests that with the current code, the more complicated cases are still possible. There is one catch in this approach, in that the original word splitting is not preserved ($* expands to just the unquoted arguments as a single word). I'm not sure if this is fixable ($@ expands to multiple quoted words, but then foreach sees multiple arguments and doesn't do the eval). One would need to escape the output of $@ somehow (e.g., add \ before ", but that would become terribly complicated I expect...).

Perhaps an explicit --eval switch to git submodule makes sense for complete control? If it has a correspondning --no-eval, you can even pass a single-argument command without evalling, while still keeping the current "least surprise" approach as the default?

Whatever behaviour is settled for, it should be documented in the submodule manpage (which I think is not the case now).

Gr.

Matthijs