Re: [PATCH] submodule--helper.c: add only-active to foreach

[Junio C Hamano <gitster@pobox.com>]

"Guillaume G. via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
>
> On repository with some submodule not active, it could be needed to run
> a command only for active submodule. Today it can be achived with the
> command:
>
> git submodule foreach 'git -C $toplevel submodule--helper is-active \
> $sm_path && pwd || :'

"it could be needed" is being too modest.

	To iterate only on active submodules, we can do ...

	 ... << the above command >> ...

	but it is inefficient to ask about each and every submodule.

may be convincing enough.  

If iterating over only active ones is useful, surely it would also
be useful to be able to iterate over only inactive ones, right? 

So, before getting married too much to the use-case of "only active
ones" and getting our eyes clouded from seeing a larger picture,
let's see what other "traits" of submodules we can use to pick which
ones to act on.

Are there attributes other than "is-active" that we may want to and
can check about submodules?  There is is_submodule_populated() next
to is_submodule_active(), which might be a candidate. IOW, what I am
wondering is if it makes sense to extend this to

	git submodule foreach --trait=is-active ...
	git submodule foreach --trait=!is-active ...
	git submodule foreach --trait=is-populated ...

to allow iterating only on submodules with/without given trait (I am
not suggesting the actual option name, but merely making sure that
'is-active' is not anything special but one of the possibilities
that can be used to limit the iteration using the same mechanism).

>  builtin/submodule--helper.c  |  8 +++++++-
>  t/t7407-submodule-foreach.sh | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c882..1a275403764 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -450,6 +450,7 @@ struct foreach_cb {
>  	const char *prefix;
>  	int quiet;
>  	int recursive;
> +	int only_active;

And I tend to agree with Eric downthread that active_only would be a
more natural name if we want to have this field.

> @@ -464,6 +465,9 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	char *displaypath;
>  
> +	if (info->only_active && !is_submodule_active(the_repository, path))
> +		return;
> +
>  	displaypath = get_submodule_displaypath(path, info->prefix);
>  
>  	sub = submodule_from_path(the_repository, &null_oid, path);
> @@ -565,11 +569,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
>  		OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
>  		OPT_BOOL(0, "recursive", &info.recursive,
>  			 N_("Recurse into nested submodules")),
> +		OPT_BOOL(0, "only-active", &info.only_active,
> +			 N_("Call command only for active submodules")),
>  		OPT_END()
>  	};
>  
>  	const char *const git_submodule_helper_usage[] = {
> -		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
> +		N_("git submodule--helper foreach [--quiet] [--recursive] [--only-active] [--] <command>"),
>  		NULL
>  	};
>  
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 6b2aa917e11..f90a16e3e67 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -80,6 +80,22 @@ test_expect_success 'test basic "submodule foreach" usage' '
>  	test_i18ncmp expect actual
>  '
>  
> +sub3sha1=$(cd super/sub3 && git rev-parse HEAD)
> +cat > expect <<EOF
> +Entering 'sub3'
> +$pwd/clone-foo3-sub3-$sub3sha1
> +EOF
> +
> +test_expect_success 'test "submodule--helper foreach --only-active" usage' '
> +	test_when_finished "git -C clone config --unset submodule.foo1.active" &&
> +	(
> +		cd clone &&
> +		git config --bool submodule.foo1.active "false" &&
> +		git submodule--helper foreach --only-active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
> +	) &&
> +	test_i18ncmp expect actual
> +'
> +
>  cat >expect <<EOF
>  Entering '../sub1'
>  $pwd/clone-foo1-sub1-../sub1-$sub1sha1
>
> base-commit: b994622632154fc3b17fb40a38819ad954a5fb88