submodule--helper.c: add only-active to foreach

From: Guillaume Galeazzi <[email protected]>

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

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

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

index 1a4b391c882..1a275403764 100644
@@ -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
 	};
 

index 6b2aa917e11..f90a16e3e67 100755
@@ -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

Hey Guillaume,

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

Hi Shourya

agree

Agree with the changes except vice-versa. The original patch support only iterate the active submodule.

is-active sound more like a question to me but I can change it.

yes definitely

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?

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

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

Le 11.05.2020 à 00:42, Eric Sunshine a écrit :

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.

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.

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

Do we really need to include the last macro here?

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.

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

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

Le 12.05.2020 à 16:15, Shourya Shukla a écrit :

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.

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;
	}

As suggested on other mail of this patch, it will be removed and take the shortcut --no-active.

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

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

Regards,

Guillaume

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.

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

Hi Junio

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

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?

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

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

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.

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

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.

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 <[email protected]>
      
       ## 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 <[email protected]>
      
     @@ 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

From: Guillaume Galeazzi <[email protected]>

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

---
 builtin/submodule--helper.c  | 33 +++++++++++++++++++++++++++------
 t/t7407-submodule-foreach.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

index 1a4b391c882..4b17e3b3b11 100644
@@ -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;
 }

index 6b2aa917e11..57c5ae70c8d 100755
@@ -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

From: Guillaume Galeazzi <[email protected]>

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.

---
 builtin/submodule--helper.c  | 14 ++++++++------
 t/t7407-submodule-foreach.sh | 15 +++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

index 4b17e3b3b11..eb17e8c5293 100644
@@ -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
 	};
 

index 57c5ae70c8d..be6cd80d464 100755
@@ -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

From: Guillaume Galeazzi <[email protected]>

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

---
 builtin/submodule--helper.c  | 21 +++++++++++++++++++--
 t/t7407-submodule-foreach.sh | 15 +++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

index eb17e8c5293..066d1974605 100644
@@ -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;
 }
 

index be6cd80d464..2da19ac54ce 100755
@@ -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

Are we still adding 'only-active'?

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

Le 17.05.2020 à 17:46, Junio C Hamano a écrit :

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

Le 17.05.2020 à 17:46, Junio C Hamano a écrit :

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.