Re: [PATCH 04/10] git-submodule.sh: dispatch "foreach" to helper

From: Glen Choo <chooglen@google.com>
Date: Oct 20, 2022, 21:14 UTC
Message-ID: <kl6ly1taxkn4.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-04.10-db6a09ee42a-20221017T115544Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 5 quoted lines

> Dispatch the "git submodule foreach" command directly to "git
> submodule--helper foreach". This case requires the addition of the
> PARSE_OPT_STOP_AT_NON_OPTION flag, since the shellscript was
> unconditionally adding "--" to the "git submodule--helper"
> command-line.

The commands in the previous patch also unconditionally add "--", so this wasn't clear to me on first read.

It might be clearer to include some extra context, which is that "git submodule foreach" is different because everything after "foreach" should be treated like a single comand to run even if they are passed as separate strings, e.g. from t/t7407-submodule-foreach.sh.21, these two should be equivalent:

  git submodule foreach "echo be --quiet"

vs

  git submodule foreach echo be --quiet

So PARSE_OPT_STOP_AT_NON_OPTION is necessary in order to stop us from intepreting option-like args as args to "git submodule foreach" instead of as part of the command to run. Without PARSE_OPT_STOP_AT_NON_OPTION,

  git submodule foreach echo be --quiet

becomes more like

  git submodule foreach --quiet "echo be"

There could have been a subtle change in behavior, since "git submodule foreach -- --foo" will be parsed as if "--foo" is the command, but PARSE_OPT_STOP_AT_NON_OPTION will happily accept "--foo" as an option. But we could never get into this situation anyway since we used to emit usage on the first option-like arg...

Show 20 quoted lines

> -# Execute an arbitrary command sequence in each checked out
> -# submodule
> -#
> -# $@ = command to execute
> -#
> -cmd_foreach()
> -{
> -	# parse $args after "submodule ... foreach".
> -	while test $# -ne 0
> -	do
> -		case "$1" in
> -		-q|--quiet)
> -			quiet=1
> -			;;
> -		--recursive)
> -			recursive=1
> -			;;
> -		-*)
> -			usage
> -			;;

i.e. here.

So this patch looks safe.