Re: [PATCH] git-submodule - Add 'foreach' subcommand

[Petr Baudis <pasky@suse.cz>]

  Hi,

On Sun, Aug 10, 2008 at 07:10:04PM -0400, Mark Levedahl wrote:
> submodule foreach <command-list> will execute the list of commands in
> each currently checked out submodule directory. The list of commands
> is arbitrary as long as it is acceptable to sh. The variables '$path'
> and '$sha1' are availble to the command-list, defining the submodule
> path relative to the superproject and the submodules's commitID as
> recorded in the superproject (this may be different than HEAD in the
> submodule).

  in principle, this looks pretty sensible.

> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index bf33b0c..1e7d352 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -14,6 +14,7 @@ SYNOPSIS
>  'git submodule' [--quiet] init [--] [<path>...]
>  'git submodule' [--quiet] update [--init] [--] [<path>...]
>  'git submodule' [--quiet] summary [--summary-limit <n>] [commit] [--] [<path>...]
> +'git submodule' foreach <command-list>
>  
>  
>  DESCRIPTION

  But as visible here, this is a little bit inconsistent. I think
foreach should be by default verbose about which submodules does it
recurse into - this is what you want in case of casual usage on
command-line. In case you want to have full control on the output within
a script, you can always pass the extra --quiet and it's less obnoxious
this way around.

  I also have a problem with the <command-list> - is this one argument?
Multiple arguments? The semantics is ill-defined. If it is supposed to
be a single argument, please drop the -list bit; silent DWIMmery of
using "$@" internally is acceptable, I guess. If it is supposed to be
multiple arguments, you need to

	(i) Specify that as <command>... instead

	(ii) Either have an eternal annoyance about insane behaviour
here, or use something better than eval "$@". Since

	git submodule foreach cp x\ y z

will simply _not_ work properly.

  So I think it's best to just drop the 'list' part. You're just
evaluating a shell expression passed in a parameter.

> @@ -123,6 +124,20 @@ summary::
>  	in the submodule between the given super project commit and the
>  	index or working tree (switched by --cached) are shown.
>  
> +foreach::
> +	Executes an arbitrary list of commands in each checked out submodule.

I think "evaluates" is a better word here, too.

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates