submodule--helper.c: add filters to foreach

[gzzi]

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.

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

[gitgitgadget]

Welcome to GitGitGadget

Hi @gzzi, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget]

There is an issue in commit 46c432a0f053aa731b69e9a2fb9156b326d3e530:
Prefixed commit message must be in lower case: submodule--helper.c: Add only-active to foreach

[dscho]

/allow

[gitgitgadget]

User gzzi is now allowed to use GitGitGadget.

WARNING: gzzi has no public email address set on GitHub

[gzzi]

/submit

[gitgitgadget]

Submitted as pull.631.git.1589099162707.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Shourya Shukla wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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?

[gitgitgadget]

On the Git mailing list, Shourya Shukla wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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 "==".

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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.

[gzzi]

/submit

[gitgitgadget]

Submitted as pull.631.v2.git.1589697023.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Guillaume Galeazzi wrote (reply to this):

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.