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

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

[Guillaume G. via GitGitGadget]

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 || :'

Goal of this change is to make it more accessible by adding the flag
--only-active to the submodule--helper command. Previous example
become:

git submodule--helper foreach --only-active pwd

Signed-off-by: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
---
    submodule--helper.c: add only-active to foreach
    
    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 || :'
    
    Goal of this change is to make it more accessible by adding the flag
    --only-active to the submodule--helper command. Previous example become:
    
    git submodule--helper foreach --only-active pwd
    
    Signed-off-by: Guillaume Galeazzi guillaume.galeazzi@gmail.com
    [guillaume.galeazzi@gmail.com]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-631%2Fgzzi%2Fsubmodule-only-active-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-631/gzzi/submodule-only-active-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/631

 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;
 };
 #define FOREACH_CB_INIT { 0 }
 
@@ -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
-- 
gitgitgadget

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

[Shourya Shukla]

Hey Guillaume,

> 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:

Spelling: achive -> achieve

Maybe we can keep the commit message a bit more imperative?
Something like:
-------------------------
On a repository with some submodules not active, one may need to run a
command only for an active submodule or vice-versa. To achieve this,
one may use:
git submodule foreach 'git -C $toplevel submodule--helper is-active \
$sm_path && pwd || :'

Simplify this expression to make it more readable and easy-to-use by
adding the flag `--is-active` to subcommand `foreach` of `git
submodule`. Thus, simplifying the above command to:
git submodule--helper foreach --is-active pwd
-------------------------
Yes, maybe renaming the flag to `--is-active` would make it a tad bit
simpler? This commit message may not be perfect but it seems like an
improvement over the previous one?

To me this option seems good. It may have some good utility in the
future. Similarly, we may change the struct to:
	struct foreach_cb {
 	const char *prefix;
 	int quiet;
 	int recursive;
	int is_active;
	 };

Therefore, the if-statement becomes:
	if (info->is_active && !is_submodule_active(the_repository, path))
		return;

BTW what do we return here, could you please be more specific?
Again, the change here as well:
		OPT_BOOL(0, "is-active", &info.is_active,

Here, too:
		N_("git submodule--helper foreach [--quiet] [--recursive] [--is-active] [--] <command>"),

And,
	test_expect_success 'test "submodule--helper foreach --is-active" usage' '

Finally,
	git submodule--helper foreach --is-active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual

What do you think?

Regards,
Shourya Shukla

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

[Guillaume Galeazzi]

Hi Shourya

>
> > 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:
>
> Spelling: achive -> achieve
agree

> Maybe we can keep the commit message a bit more imperative?
> Something like:
> -------------------------
> On a repository with some submodules not active, one may need to run a
> command only for an active submodule or vice-versa. To achieve this,
> one may use:
> git submodule foreach 'git -C $toplevel submodule--helper is-active \
> $sm_path && pwd || :'
>
> Simplify this expression to make it more readable and easy-to-use by
> adding the flag `--is-active` to subcommand `foreach` of `git
> submodule`. Thus, simplifying the above command to:
> git submodule--helper foreach --is-active pwd
> -------------------------
Agree with the changes except vice-versa. The original patch support only
iterate the active submodule.

> Yes, maybe renaming the flag to `--is-active` would make it a tad bit
> simpler?
is-active sound more like a question to me but I can change it.

> This commit message may not be perfect but it seems like an
> improvement over the previous one?
yes definitely

> To me this option seems good. It may have some good utility in the
> future. Similarly, we may change the struct to:
>         struct foreach_cb {
>         const char *prefix;
>         int quiet;
>         int recursive;
>         int is_active;
>          };
>
> Therefore, the if-statement becomes:
>         if (info->is_active && !is_submodule_active(the_repository, path))
>                 return;
>
> BTW what do we return here, could you please be more specific?
This is a void function, returning here mean we will not execute the
command. Should I add a
comment like:
                return;  // skip this submodule and go to next one
but maybe it would be more readable to create a intermediate function
which handle only the
filtering part. Is it what you mean?

> Again, the change here as well:
>                 OPT_BOOL(0, "is-active", &info.is_active,
>
> Here, too:
>                 N_("git submodule--helper foreach [--quiet] [--recursive] [--is-active] [--] <command>"),
>
> And,
>         test_expect_success 'test "submodule--helper foreach --is-active" usage' '
>
> Finally,
>         git submodule--helper foreach --is-active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
>
> What do you think?
>
> Regards,
> Shourya Shukla

Now with the vice-versa idea in mind, I think it is maybe better to
change a bit the original patch
to add the option to execute command only on inactive submodule as
well. Could someone need
that in future?

Basically this would mean:

On struct foreach_cb instead of only_active adding field:
        int active;

Defining some macro to hold possible value:
        #define FOREACH_ACTIVE 1
        #define FOREACH_INACTIVE 0
        #define FOREACH_ACTIVE_NOT_SET -1

Changing the FOREACH_CB_INIT to
        #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }

The filter become:
int is_active;
if (FOREACH_ACTIVE_NOT_SET != info->active) {
        is_active = is_submodule_active(the_repository, path);
        if ((is_active && (FOREACH_ACTIVE != info->active)) ||
            (!is_active && (FOREACH_ACTIVE == info->active)))
                return;
}

It need two additionnal function to parse the argument:
static int parse_active(const char *arg)
{
        int active = git_parse_maybe_bool(arg);

        if (active < 0)
                die(_("invalid --active option: %s"), arg);

        return active;
}

static int parse_opt_active_cb(const struct option *opt, const char *arg,
                               int unset)
{
        if (unset)
                *(int *)opt->value = FOREACH_ACTIVE_NOT_SET;
        else if (arg)
                *(int *)opt->value = parse_active(arg);
        else
                *(int *)opt->value = FOREACH_ACTIVE;

        return 0;
}

And the option OPT_BOOL become a OPT_CALLBACK_F:
OPT_CALLBACK_F(0, "active", &info.active, "true|false",
        N_("Call command depending on submodule active state"),
        PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
        parse_opt_active_cb),

The help git_submodule_helper_usage:
N_("git submodule--helper foreach [--quiet] [--recursive]
[--active[=true|false]] [--] <command>"),

the added test change to:
git submodule--helper foreach --active "echo
\$toplevel-\$name-\$path-\$sha1" > ../actual

and adding a test for the inactive submodule:
cat > expect <<EOF
Entering 'sub1'
$pwd/clone-foo1-sub1-$sub1sha1
EOF

test_expect_success 'test "submodule--helper foreach --active=false" 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
git submodule--helper foreach --active=false "echo
\$toplevel-\$name-\$path-\$sha1" > ../actual
) &&
test_i18ncmp expect actual
'

What do you think?

Regards,
Guillaume

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

[Eric Sunshine]

On Sun, May 10, 2020 at 5:53 PM Guillaume Galeazzi
<guillaume.galeazzi@gmail.com> wrote:
> > Yes, maybe renaming the flag to `--is-active` would make it a tad bit
> > simpler?
> is-active sound more like a question to me but I can change it.

I'm not a submodule user nor have I been following this discussion,
but perhaps the name --active-only would be better?

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

[Shourya Shukla]

On 10/05 11:51, Guillaume Galeazzi wrote:

Before I comment on the patch, I want to apologise for the delay in the
reply. I got caught up with some stuff.

> Now with the vice-versa idea in mind, I think it is maybe better to
> change a bit the original patch
> to add the option to execute command only on inactive submodule as
> well. Could someone need
> that in future?
> 
> Basically this would mean:
> 
> On struct foreach_cb instead of only_active adding field:
>         int active;

Yeah, keeping the option name as `active` would be better if we were
to go for the inactive submodules option as well.

> Defining some macro to hold possible value:
>         #define FOREACH_ACTIVE 1
>         #define FOREACH_INACTIVE 0
>         #define FOREACH_ACTIVE_NOT_SET -1
> 
> Changing the FOREACH_CB_INIT to
>         #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }

Do we really need to include the last macro here?

> The filter become:
> int is_active;
> if (FOREACH_ACTIVE_NOT_SET != info->active) {
>         is_active = is_submodule_active(the_repository, path);
>         if ((is_active && (FOREACH_ACTIVE != info->active)) ||
>             (!is_active && (FOREACH_ACTIVE == info->active)))
>                 return;
> }

Is it okay to compare a macro directly? I have not actually seen it
happen so I am a bit skeptical. I am tagging along some people who
will be able to weigh in a solid opinion regarding this.

> It need two additionnal function to parse the argument:
> static int parse_active(const char *arg)
> {
>         int active = git_parse_maybe_bool(arg);
> 
>         if (active < 0)
>                 die(_("invalid --active option: %s"), arg);
> 
>         return active;
> }

Alright, this one is used for parsing out the active submodules right?

> static int parse_opt_active_cb(const struct option *opt, const char *arg,
>                                int unset)
> {
>         if (unset)
>                 *(int *)opt->value = FOREACH_ACTIVE_NOT_SET;
>         else if (arg)
>                 *(int *)opt->value = parse_active(arg);
>         else
>                 *(int *)opt->value = FOREACH_ACTIVE;
> 
>         return 0;
> }
> 
> And the option OPT_BOOL become a OPT_CALLBACK_F:
> OPT_CALLBACK_F(0, "active", &info.active, "true|false",
>         N_("Call command depending on submodule active state"),
>         PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>         parse_opt_active_cb),
> 
> The help git_submodule_helper_usage:
> N_("git submodule--helper foreach [--quiet] [--recursive]
> [--active[=true|false]] [--] <command>"),

What I have inferred right now is that we introduce the `--active`
option which will take a T/F value depending on user input. We have 3
macros to check for the value of `active`, but I don't understand the
significance of changing the FOREACH_CB_INIT macro to accomodate the
third option. And we use a function to parse out the active
submodules.

Instead of the return statement you wrote, won't it be better to call
parse_active() depending on the case? Meaning that we call
parse_active() when `active=true`.

Regards,
Shourya Shukla

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

[Junio C Hamano]

"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

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

[Guillaume Galeazzi]

Hi Junio

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

The idea that other candidate are possible seem good. But then users 
will need combination like is-active && !is-populated. We will end up 
implementing a complex code to filter based on expression which is IMHO 
overkill.

If is-populated is needed, that could be implemented this way:

         git submodule--helper --is-populated[=true|false]

this would allow combination with the is active filter and the
previous example would be

         git submodule--helper --is-active --is-populated=false <cmd>

Regards,
GG

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

[Junio C Hamano]

Guillaume Galeazzi <guillaume.galeazzi@gmail.com> writes:

>> 	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).
>
> The idea that other candidate are possible seem good. But then users
> will need combination like is-active && !is-populated. ...
> ... this would allow combination with the is active filter and the
> previous example would be
>
>         git submodule--helper --is-active --is-populated=false <cmd>

There is no difference between that and

	git submodule--helper --trait=is-active --trait=is-populated

so I fail to see what new you are proposing, sorry.

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

[Guillaume Galeazzi]

>>> 	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).
>>
>> The idea that other candidate are possible seem good. But then users
>> will need combination like is-active && !is-populated. ...
>> ... this would allow combination with the is active filter and the
>> previous example would be
>>
>>          git submodule--helper --is-active --is-populated=false <cmd>
> 
> There is no difference between that and
> 
> 	git submodule--helper --trait=is-active --trait=is-populated
> 
> so I fail to see what new you are proposing, sorry.
> 

The difference is that you repeat twice the same flag. Sometime 
repeating a flag overwrite the previous value, but it depend how it is 
implemented. In current case it should be possible to implement it this 
way, if this is required.

Regarding previous example, it use '!' to negate the value. Not all 
people know the meaning of it. A new proposal would be:

         git submodule--helper foreach [--trait=[not-](active|populated)]

What do you think?

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

[Junio C Hamano]

Guillaume Galeazzi <guillaume.galeazzi@gmail.com> writes:

> The difference is that you repeat twice the same flag. Sometime
> repeating a flag overwrite the previous value,...

I already said I was *not* suggesting a concrete syntax.  The more
important point was to make us realize that we need to think outside
of "active-only" and make sure we can support other kinds of selection
criteria for submodules.

So whatever syntax you would want to use to specify more than one,
combine, and negate, I do not care too deeply, as long as it is in
line with what we use in the other parts of the system ;-).

> Regarding previous example, it use '!' to negate the value. Not all
> people know the meaning of it.

We mark the bottom commit by negating with ^ (e.g. "git log ^maint master"),
we mark "not ignored" entries by prefixing with '!' in .gitignore,
we mark "unset" entries by prefixing with '-' in .gitattributes (the
'!' prefix is used to mark "unspecified" entries), and "rev-list --boundary"
output uses '~' prefix to mean "this is not part of the range".

So there are many one-letter "not" we already use, and there seem to
be no rule to pick which one in what context X-<.

So spelling out "--no-blah" to mean "not with blah" is probably a
good thing to do (especially if readers do not mind being English
centric).

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

[Guillaume Galeazzi]

Le 13.05.2020 à 22:35, Junio C Hamano a écrit :
 > Guillaume Galeazzi <guillaume.galeazzi@gmail.com> writes:
 >
 > I already said I was *not* suggesting a concrete syntax.  The more
 > important point was to make us realize that we need to think outside
 > of "active-only" and make sure we can support other kinds of selection
 > criteria for submodules.

Ok get it now. So after looking a bit, another trait that could be
interesting filtering on is remote tracking branch. A v2 with filtering 
based on active (or not), populated (or not), and tracked remote branch 
is ready on my side. It also include the rename of the struct member 
only_active to active_only. Let me know when I can /submit.

 > So spelling out "--no-blah" to mean "not with blah" is probably a
 > good thing to do (especially if readers do not mind being English
 > centric).
 >

Great, it make it a bit simpler to code, thanks for the tips.

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

[Guillaume Galeazzi]

Le 11.05.2020 à 00:42, Eric Sunshine a écrit :
> On Sun, May 10, 2020 at 5:53 PM Guillaume Galeazzi
> <guillaume.galeazzi@gmail.com> wrote:
>>> Yes, maybe renaming the flag to `--is-active` would make it a tad bit
>>> simpler?
>> is-active sound more like a question to me but I can change it.
> 
> I'm not a submodule user nor have I been following this discussion,
> but perhaps the name --active-only would be better?
> 

To flow up on that topic, the flag can be `--[no-]active`. It simplify 
code and allow the optional negation. On the source, the variable will 
be renamed active_only.

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

[Guillaume Galeazzi]

Le 12.05.2020 à 16:15, Shourya Shukla a écrit :
> On 10/05 11:51, Guillaume Galeazzi wrote:
> 
> 
>> Defining some macro to hold possible value:
>>          #define FOREACH_ACTIVE 1
>>          #define FOREACH_INACTIVE 0
>>          #define FOREACH_ACTIVE_NOT_SET -1
>>
>> Changing the FOREACH_CB_INIT to
>>          #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }
> 
> Do we really need to include the last macro here?

After a cross check, yes it is the correct place to initialise the new 
active_only member of foreach_cb. But it will be changed to use 
designated initializers.

>> The filter become:
>> int is_active;
>> if (FOREACH_ACTIVE_NOT_SET != info->active) {
>>          is_active = is_submodule_active(the_repository, path);
>>          if ((is_active && (FOREACH_ACTIVE != info->active)) ||
>>              (!is_active && (FOREACH_ACTIVE == info->active)))
>>                  return;
>> }
> 
> Is it okay to compare a macro directly? I have not actually seen it
> happen so I am a bit skeptical. I am tagging along some people who
> will be able to weigh in a solid opinion regarding this.

Yes it is okay, a `#define SOMETHING WHATEVER` will just inform the c 
preprocessor to replace the `SOMETHING` by `WHATEVER`. The only thing 
the final c compiler will see is `WHATEVER`. In our case a integer value.

Goal here was to avoid magic number, but after looking to the code it 
seem accepted that true is 1 and false is 0. To comply with that, in 
next version it will be replace it with:

	if (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {
		is_active = is_submodule_active(the_repository, path);
		if (is_active != info->active_only)
			return;
	}

> 
>> It need two additionnal function to parse the argument:
>> static int parse_active(const char *arg)
>> {
>>          int active = git_parse_maybe_bool(arg);
>>
>>          if (active < 0)
>>                  die(_("invalid --active option: %s"), arg);
>>
>>          return active;
>> }
> 
> Alright, this one is used for parsing out the active submodules right
As suggested on other mail of this patch, it will be removed and take 
the shortcut `--no-active`.

>> And the option OPT_BOOL become a OPT_CALLBACK_F:
>> OPT_CALLBACK_F(0, "active", &info.active, "true|false",
>>          N_("Call command depending on submodule active state"),
>>          PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>>          parse_opt_active_cb),
>>
>> The help git_submodule_helper_usage:
>> N_("git submodule--helper foreach [--quiet] [--recursive]
>> [--active[=true|false]] [--] <command>"),
> 
> What I have inferred right now is that we introduce the `--active`
> option which will take a T/F value depending on user input. We have 3
> macros to check for the value of `active`, but I don't understand the
> significance of changing the FOREACH_CB_INIT macro to accomodate the
> third option. And we use a function to parse out the active
> submodules.

The change on `FOREACH_CB_INIT` are to keep original behaviour of the 
command if new flags are not given.

> Instead of the return statement you wrote, won't it be better to call
> parse_active() depending on the case? Meaning that we call
> parse_active() when `active=true`.
> 
> Regards,
> Shourya Shukla
> 

The code to parse command T/F will be removed.

Regards,

Guillaume

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

[Junio C Hamano]

Guillaume Galeazzi <guillaume.galeazzi@gmail.com> writes:

> Goal here was to avoid magic number, but after looking to the code it
> seem accepted that true is 1 and false is 0. To comply with that, in
> next version it will be replace it with:
>
> 	if (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {

It still is unusual to have a constant on the left hand side of the
"!=" or "==" operator, though.  Having a constant on the left hand
side of "<" and "<=" is justifiable, but not for "!=" and "==".

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

[Guillaume Galeazzi]

Le ven. 15 mai 2020 à 19:03, Junio C Hamano <gitster@pobox.com> a écrit :
>
> Guillaume Galeazzi <guillaume.galeazzi@gmail.com> writes:
>
> > Goal here was to avoid magic number, but after looking to the code it
> > seem accepted that true is 1 and false is 0. To comply with that, in
> > next version it will be replace it with:
> >
> >       if (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {
>
> It still is unusual to have a constant on the left hand side of the
> "!=" or "==" operator, though.  Having a constant on the left hand
> side of "<" and "<=" is justifiable, but not for "!=" and "==".

It is call Yoda condition. As it compare with a constant, the compiler
will throw an error if you write only = instead of != or ==.

But after a quick check, this wasn't needed as compiler warn
    builtin/submodule--helper.c:570:2: error: suggest parentheses
around assignment used as truth value [-Werror=parentheses]

And I am not a Yoda condition adept at all. So it will be removed.

[PATCH v2 0/3] submodule--helper.c: add only-active to foreach

[Guillaume G. via GitGitGadget]

On repository with multiple submodules, one may need to run a command based
on submodule trait.

This changes add flags to submodule--helper foreachto fulfill this need by:

Adding the flag --[no-]active to filter submodule based on the active state.
Adding the flag --[no-]populated to filter submodule based on the fact that
it is populated or not. Adding the flag -b|--branch <branch> to filter
submodule based on the tracking branch.

Signed-off-by: Guillaume Galeazzi guillaume.galeazzi@gmail.com
[guillaume.galeazzi@gmail.com]

Guillaume Galeazzi (3):
  submodule--helper.c: add active to foreach
  submodule--helper.c: add populated to foreach
  submodule--helper.c: add branch to foreach

 builtin/submodule--helper.c  | 60 ++++++++++++++++++++++++++++++------
 t/t7407-submodule-foreach.sh | 60 ++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 10 deletions(-)


base-commit: b994622632154fc3b17fb40a38819ad954a5fb88
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-631%2Fgzzi%2Fsubmodule-only-active-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-631/gzzi/submodule-only-active-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/631

Range-diff vs v1:

 1:  77bb291af5d ! 1:  90defeb5a75 submodule--helper.c: add only-active to foreach
     @@ Metadata
      Author: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
      
       ## Commit message ##
     -    submodule--helper.c: add only-active to foreach
     -
     -    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:
     +    submodule--helper.c: add active to foreach
      
     +    On a repository with some submodules not active, one may need to run a
     +    command only for an active submodule or vice-versa. To achieve this,
     +    one may use:
          git submodule foreach 'git -C $toplevel submodule--helper is-active \
          $sm_path && pwd || :'
      
     -    Goal of this change is to make it more accessible by adding the flag
     -    --only-active to the submodule--helper command. Previous example
     -    become:
     -
     -    git submodule--helper foreach --only-active pwd
     +    Simplify this expression to make it more readable and easy-to-use by
     +    adding the flat `--[no-]active` to subcommand `foreach` of `git
     +    submodule`. Thus, simplifying the above command to:
     +    git submodule--helper foreach --active pwd
      
          Signed-off-by: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
      
     @@ builtin/submodule--helper.c: struct foreach_cb {
       	const char *prefix;
       	int quiet;
       	int recursive;
     -+	int only_active;
     ++	int active_only;
       };
     - #define FOREACH_CB_INIT { 0 }
     +-#define FOREACH_CB_INIT { 0 }
     + 
     +-static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
     +-				       void *cb_data)
     ++#define FOREACH_BOOL_FILTER_NOT_SET -1
     ++
     ++#define FOREACH_CB_INIT { .active_only=FOREACH_BOOL_FILTER_NOT_SET }
     ++
     ++static void runcommand_in_submodule(const struct cache_entry *list_item,
     ++				    struct foreach_cb *info)
     + {
     +-	struct foreach_cb *info = cb_data;
     + 	const char *path = list_item->name;
     + 	const struct object_id *ce_oid = &list_item->oid;
       
      @@ builtin/submodule--helper.c: static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
     - 	struct child_process cp = CHILD_PROCESS_INIT;
     - 	char *displaypath;
     + 	free(displaypath);
     + }
       
     -+	if (info->only_active && !is_submodule_active(the_repository, path))
     -+		return;
     ++static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_item,
     ++						void *cb_data)
     ++{
     ++	const char *path = list_item->name;
     ++	struct foreach_cb *info = cb_data;
     ++	int is_active;
      +
     - 	displaypath = get_submodule_displaypath(path, info->prefix);
     - 
     - 	sub = submodule_from_path(the_repository, &null_oid, path);
     ++	if (info->active_only != FOREACH_BOOL_FILTER_NOT_SET) {
     ++		is_active = is_submodule_active(the_repository, path);
     ++		if (is_active != info->active_only)
     ++			return;
     ++	}
     ++
     ++	runcommand_in_submodule(list_item, info);
     ++}
     ++
     + static int module_foreach(int argc, const char **argv, const char *prefix)
     + {
     + 	struct foreach_cb info = FOREACH_CB_INIT;
      @@ builtin/submodule--helper.c: 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_BOOL(0, "active", &info.active_only,
     ++			 N_("Call command depending on submodule active state")),
       		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>"),
     ++		N_("git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--] <command>"),
       		NULL
       	};
       
     +@@ builtin/submodule--helper.c: static int module_foreach(int argc, const char **argv, const char *prefix)
     + 	info.argv = argv;
     + 	info.prefix = prefix;
     + 
     +-	for_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);
     ++	for_each_listed_submodule(&list, runcommand_in_submodule_filtered_cb, &info);
     + 
     + 	return 0;
     + }
      
       ## t/t7407-submodule-foreach.sh ##
      @@ t/t7407-submodule-foreach.sh: 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_expect_success 'test "submodule--helper foreach --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 --active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
     ++	) &&
     ++	test_i18ncmp expect actual
     ++'
     ++
     ++cat > expect <<EOF
     ++Entering 'sub1'
     ++$pwd/clone-foo1-sub1-$sub1sha1
     ++EOF
     ++
     ++test_expect_success 'test "submodule--helper foreach --no-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
     ++		git submodule--helper foreach --no-active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
      +	) &&
      +	test_i18ncmp expect actual
      +'
 -:  ----------- > 2:  f9cbdcdeacf submodule--helper.c: add populated to foreach
 -:  ----------- > 3:  65504cb780a submodule--helper.c: add branch to foreach

-- 
gitgitgadget

[PATCH v2 1/3] submodule--helper.c: add active to foreach

[Guillaume Galeazzi via GitGitGadget]

From: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>

On a repository with some submodules not active, one may need to run a
command only for an active submodule or vice-versa. To achieve this,
one may use:
git submodule foreach 'git -C $toplevel submodule--helper is-active \
$sm_path && pwd || :'

Simplify this expression to make it more readable and easy-to-use by
adding the flat `--[no-]active` to subcommand `foreach` of `git
submodule`. Thus, simplifying the above command to:
git submodule--helper foreach --active pwd

Signed-off-by: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
---
 builtin/submodule--helper.c  | 33 +++++++++++++++++++++++++++------
 t/t7407-submodule-foreach.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c882..4b17e3b3b11 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -450,13 +450,16 @@ struct foreach_cb {
 	const char *prefix;
 	int quiet;
 	int recursive;
+	int active_only;
 };
-#define FOREACH_CB_INIT { 0 }
 
-static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
-				       void *cb_data)
+#define FOREACH_BOOL_FILTER_NOT_SET -1
+
+#define FOREACH_CB_INIT { .active_only=FOREACH_BOOL_FILTER_NOT_SET }
+
+static void runcommand_in_submodule(const struct cache_entry *list_item,
+				    struct foreach_cb *info)
 {
-	struct foreach_cb *info = cb_data;
 	const char *path = list_item->name;
 	const struct object_id *ce_oid = &list_item->oid;
 
@@ -555,6 +558,22 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 	free(displaypath);
 }
 
+static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_item,
+						void *cb_data)
+{
+	const char *path = list_item->name;
+	struct foreach_cb *info = cb_data;
+	int is_active;
+
+	if (info->active_only != FOREACH_BOOL_FILTER_NOT_SET) {
+		is_active = is_submodule_active(the_repository, path);
+		if (is_active != info->active_only)
+			return;
+	}
+
+	runcommand_in_submodule(list_item, info);
+}
+
 static int module_foreach(int argc, const char **argv, const char *prefix)
 {
 	struct foreach_cb info = FOREACH_CB_INIT;
@@ -565,11 +584,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, "active", &info.active_only,
+			 N_("Call command depending on submodule active state")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
+		N_("git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--] <command>"),
 		NULL
 	};
 
@@ -583,7 +604,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	info.argv = argv;
 	info.prefix = prefix;
 
-	for_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);
+	for_each_listed_submodule(&list, runcommand_in_submodule_filtered_cb, &info);
 
 	return 0;
 }
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6b2aa917e11..57c5ae70c8d 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -80,6 +80,36 @@ test_expect_success 'test basic "submodule foreach" usage' '
 	test_i18ncmp expect actual
 '
 
+cat > expect <<EOF
+Entering 'sub3'
+$pwd/clone-foo3-sub3-$sub3sha1
+EOF
+
+test_expect_success 'test "submodule--helper foreach --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 --active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
+cat > expect <<EOF
+Entering 'sub1'
+$pwd/clone-foo1-sub1-$sub1sha1
+EOF
+
+test_expect_success 'test "submodule--helper foreach --no-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 --no-active "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat >expect <<EOF
 Entering '../sub1'
 $pwd/clone-foo1-sub1-../sub1-$sub1sha1
-- 
gitgitgadget

[PATCH v2 2/3] submodule--helper.c: add populated to foreach

[Guillaume Galeazzi via GitGitGadget]

From: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>

On a repository with some submodules not initialised, one may need to
run a command only for not initilaised submodule or vice-versa. This
change, add the flag `--[no-]populated` to subcommand `foreach` of `git
submodule--helper`.

Signed-off-by: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
---
 builtin/submodule--helper.c  | 14 ++++++++------
 t/t7407-submodule-foreach.sh | 15 +++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4b17e3b3b11..eb17e8c5293 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -451,11 +451,12 @@ struct foreach_cb {
 	int quiet;
 	int recursive;
 	int active_only;
+	int populated_only;
 };
 
 #define FOREACH_BOOL_FILTER_NOT_SET -1
 
-#define FOREACH_CB_INIT { .active_only=FOREACH_BOOL_FILTER_NOT_SET }
+#define FOREACH_CB_INIT { .active_only = FOREACH_BOOL_FILTER_NOT_SET, .populated_only = 1 }
 
 static void runcommand_in_submodule(const struct cache_entry *list_item,
 				    struct foreach_cb *info)
@@ -475,9 +476,6 @@ static void runcommand_in_submodule(const struct cache_entry *list_item,
 		die(_("No url found for submodule path '%s' in .gitmodules"),
 			displaypath);
 
-	if (!is_submodule_populated_gently(path, NULL))
-		goto cleanup;
-
 	prepare_submodule_repo_env(&cp.env_array);
 
 	/*
@@ -554,7 +552,6 @@ static void runcommand_in_submodule(const struct cache_entry *list_item,
 				displaypath);
 	}
 
-cleanup:
 	free(displaypath);
 }
 
@@ -571,6 +568,9 @@ static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_i
 			return;
 	}
 
+	if (info->populated_only != is_submodule_populated_gently(path, NULL))
+		return;
+
 	runcommand_in_submodule(list_item, info);
 }
 
@@ -586,11 +586,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 			 N_("Recurse into nested submodules")),
 		OPT_BOOL(0, "active", &info.active_only,
 			 N_("Call command depending on submodule active state")),
+		OPT_BOOL(0, "populated", &info.populated_only,
+			 N_("Call command depending on submodule populated state")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--] <command>"),
+		N_("git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--[no-]populated] [--] <command>"),
 		NULL
 	};
 
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 57c5ae70c8d..be6cd80d464 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -80,6 +80,21 @@ test_expect_success 'test basic "submodule foreach" usage' '
 	test_i18ncmp expect actual
 '
 
+sub2sha1=$(cd super/sub2 && git rev-parse HEAD)
+
+cat > expect <<EOF
+Entering 'sub2'
+$pwd/clone-foo2-sub2-$sub2sha1
+EOF
+
+test_expect_success 'test "submodule--helper foreach --no-populated" usage' '
+	(
+		cd clone &&
+		git submodule--helper foreach --no-populated "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat > expect <<EOF
 Entering 'sub3'
 $pwd/clone-foo3-sub3-$sub3sha1
-- 
gitgitgadget

[PATCH v2 3/3] submodule--helper.c: add branch to foreach

[Guillaume Galeazzi via GitGitGadget]

From: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>

On a repository with some submodules tracking different branch, one
may need to run a command only for submodule tracking one of specified
branch. This change, add flags `-b <branch>` and --branch <branch>` to
subcommand `foreach` of `git submodule--helper`.

Signed-off-by: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
---
 builtin/submodule--helper.c  | 21 +++++++++++++++++++--
 t/t7407-submodule-foreach.sh | 15 +++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index eb17e8c5293..066d1974605 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -452,11 +452,13 @@ struct foreach_cb {
 	int recursive;
 	int active_only;
 	int populated_only;
+	struct string_list remote_branch_filter;
 };
 
 #define FOREACH_BOOL_FILTER_NOT_SET -1
 
-#define FOREACH_CB_INIT { .active_only = FOREACH_BOOL_FILTER_NOT_SET, .populated_only = 1 }
+#define FOREACH_CB_INIT { .active_only = FOREACH_BOOL_FILTER_NOT_SET, \
+	.populated_only = 1, .remote_branch_filter = STRING_LIST_INIT_NODUP }
 
 static void runcommand_in_submodule(const struct cache_entry *list_item,
 				    struct foreach_cb *info)
@@ -555,12 +557,15 @@ static void runcommand_in_submodule(const struct cache_entry *list_item,
 	free(displaypath);
 }
 
+static const char *remote_submodule_branch(const char *path);
+
 static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_item,
 						void *cb_data)
 {
 	const char *path = list_item->name;
 	struct foreach_cb *info = cb_data;
 	int is_active;
+	const char *branch;
 
 	if (info->active_only != FOREACH_BOOL_FILTER_NOT_SET) {
 		is_active = is_submodule_active(the_repository, path);
@@ -571,6 +576,14 @@ static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_i
 	if (info->populated_only != is_submodule_populated_gently(path, NULL))
 		return;
 
+	if (info->remote_branch_filter.nr) {
+		branch = remote_submodule_branch(path);
+		if (!branch)
+			return;
+		if (!unsorted_string_list_has_string(&info->remote_branch_filter, branch))
+			return;
+	}
+
 	runcommand_in_submodule(list_item, info);
 }
 
@@ -588,11 +601,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 			 N_("Call command depending on submodule active state")),
 		OPT_BOOL(0, "populated", &info.populated_only,
 			 N_("Call command depending on submodule populated state")),
+		OPT_STRING_LIST('b', "branch", &info.remote_branch_filter,
+			 N_("branch"), N_("Call command only if submodule remote branch is one of <branch> given")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--[no-]populated] [--] <command>"),
+		N_("git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--[no-]populated] [-b|--branch <branch>] [--] <command>"),
 		NULL
 	};
 
@@ -608,6 +623,8 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 
 	for_each_listed_submodule(&list, runcommand_in_submodule_filtered_cb, &info);
 
+	string_list_clear(&info.remote_branch_filter, 0);
+
 	return 0;
 }
 
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index be6cd80d464..2da19ac54ce 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -125,6 +125,21 @@ test_expect_success 'test "submodule--helper foreach --no-active" usage' '
 	test_i18ncmp expect actual
 '
 
+cat > expect <<EOF
+Entering 'sub1'
+$pwd/clone-foo1-sub1-$sub1sha1
+EOF
+
+test_expect_success 'test "submodule--helper foreach --branch" usage' '
+	test_when_finished "git -C clone config  -f .gitmodules --unset submodule.foo1.branch" &&
+	(
+		cd clone &&
+		git config -f .gitmodules --add submodule.foo1.branch test &&
+		git submodule--helper foreach --branch test "echo \$toplevel-\$name-\$path-\$sha1" > ../actual
+	) &&
+	test_i18ncmp expect actual
+'
+
 cat >expect <<EOF
 Entering '../sub1'
 $pwd/clone-foo1-sub1-../sub1-$sub1sha1
-- 
gitgitgadget

Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach

[Junio C Hamano]

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

> Subject: Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach

Are we still adding 'only-active'?

Does updating the cover letter needs a better support from our
tools, I wonder...

Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach

[Guillaume Galeazzi]

Le 17.05.2020 à 17:46, Junio C Hamano a écrit :
> "Guillaume G. via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> Subject: Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach
> 
> Are we still adding 'only-active'?
> 
> Does updating the cover letter needs a better support from our
> tools, I wonder...
> 

Hi Junio,

Probably no, it's me. I wasn't sure for traceability changing subject 
name was allowed. I assume for next time it is.

new subject would be: submodule--helper.c: add filters to foreach

Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach

[Guillaume Galeazzi]

Le 17.05.2020 à 17:46, Junio C Hamano a écrit :
> 
> Does updating the cover letter needs a better support from our
> tools, I wonder...
> 

Hello, well there was an issue that some markdown and new line of the 
cover letter was removed by GGG. How to move forward?

The original text before GGG was:

On repository with multiple submodules, one may need to run
a command based on submodule trait.

This changes add flags to `submodule--helper foreach` to fulfill this 
need by:

Adding the flag `--[no-]active` to filter submodule based on the active
state.
Adding the flag `--[no-]populated` to filter submodule based on the
fact that it is populated or not.
Adding the flag `-b|--branch <branch>` to filter submodule based on
the tracking branch.