{"thread":{"id":"53440","subject":"[PATCH] submodule--helper.c: add only-active to foreach","startedAt":"2020-05-10T08:26:07Z","lastAt":"2020-08-18T15:59:26Z","messageCount":22,"participants":["Guillaume G. via GitGitGadget","Shourya Shukla","Guillaume Galeazzi","Eric Sunshine","Junio C Hamano","Guillaume Galeazzi via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397485","messageId":"pull.631.git.1589099162707.gitgitgadget@gmail.com","threadId":"53440","inReplyTo":null,"subject":"[PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume G. via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-10T08:26:02Z","receivedAt":"2020-05-10T08:26:07Z","isPatch":true,"sender":{"key":"name:Guillaume G.","avatar":null},"body":"From: Guillaume Galeazzi \n\nOn repository with some submodule not active, it could be needed to run\na command only for active submodule. Today it can be achived with the\ncommand:\n\ngit submodule foreach 'git -C $toplevel submodule--helper is-active \\\n$sm_path && pwd || :'\n\nGoal of this change is to make it more accessible by adding the flag\n--only-active to the submodule--helper command. Previous example\nbecome:\n\ngit submodule--helper foreach --only-active pwd\n\nSigned-off-by: Guillaume Galeazzi \n---\n submodule--helper.c: add only-active to foreach\n \n On repository with some submodule not active, it could be needed to run\n a command only for active submodule. Today it can be achived with the\n command:\n \n git submodule foreach 'git -C $toplevel submodule--helper is-active \n $sm_path && pwd || :'\n \n Goal of this change is to make it more accessible by adding the flag\n --only-active to the submodule--helper command. Previous example become:\n \n git submodule--helper foreach --only-active pwd\n \n Signed-off-by: Guillaume Galeazzi guillaume.galeazzi@gmail.com\n [guillaume.galeazzi@gmail.com]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-631%2Fgzzi%2Fsubmodule-only-active-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-631/gzzi/submodule-only-active-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/631\n\n builtin/submodule--helper.c | 8 +++++++-\n t/t7407-submodule-foreach.sh | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c882..1a275403764 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -450,6 +450,7 @@ struct foreach_cb {\n \tconst char *prefix;\n \tint quiet;\n \tint recursive;\n+\tint only_active;\n };\n #define FOREACH_CB_INIT { 0 }\n \n@@ -464,6 +465,9 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tchar *displaypath;\n \n+\tif (info->only_active && !is_submodule_active(the_repository, path))\n+\t\treturn;\n+\n \tdisplaypath = get_submodule_displaypath(path, info->prefix);\n \n \tsub = submodule_from_path(the_repository, &null_oid, path);\n@@ -565,11 +569,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&info.quiet, N_(\"Suppress output of entering each submodule command\")),\n \t\tOPT_BOOL(0, \"recursive\", &info.recursive,\n \t\t\t N_(\"Recurse into nested submodules\")),\n+\t\tOPT_BOOL(0, \"only-active\", &info.only_active,\n+\t\t\t N_(\"Call command only for active submodules\")),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--] \"),\n+\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--only-active] [--] \"),\n \t\tNULL\n \t};\n \ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6b2aa917e11..f90a16e3e67 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -80,6 +80,22 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n \ttest_i18ncmp expect actual\n '\n \n+sub3sha1=$(cd super/sub3 && git rev-parse HEAD)\n+cat > expect < ../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >expect < On repository with some submodule not active, it could be needed to run\n> a command only for active submodule. Today it can be achived with the\n> command:\n\nSpelling: achive -> achieve\n\nMaybe we can keep the commit message a bit more imperative?\nSomething like:\n-------------------------\nOn a repository with some submodules not active, one may need to run a\ncommand only for an active submodule or vice-versa. To achieve this,\none may use:\ngit submodule foreach 'git -C $toplevel submodule--helper is-active \\\n$sm_path && pwd || :'\n\nSimplify this expression to make it more readable and easy-to-use by\nadding the flag `--is-active` to subcommand `foreach` of `git\nsubmodule`. Thus, simplifying the above command to:\ngit submodule--helper foreach --is-active pwd\n-------------------------\nYes, maybe renaming the flag to `--is-active` would make it a tad bit\nsimpler? This commit message may not be perfect but it seems like an\nimprovement over the previous one?\n\nTo me this option seems good. It may have some good utility in the\nfuture. Similarly, we may change the struct to:\n\tstruct foreach_cb {\n \tconst char *prefix;\n \tint quiet;\n \tint recursive;\n\tint is_active;\n\t };\n\nTherefore, the if-statement becomes:\n\tif (info->is_active && !is_submodule_active(the_repository, path))\n\t\treturn;\n\nBTW what do we return here, could you please be more specific?\nAgain, the change here as well:\n\t\tOPT_BOOL(0, \"is-active\", &info.is_active,\n\nHere, too:\n\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--is-active] [--] \"),\n\nAnd,\n\ttest_expect_success 'test \"submodule--helper foreach --is-active\" usage' '\n\nFinally,\n\tgit submodule--helper foreach --is-active \"echo \\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual\n\nWhat do you think?\n\nRegards,\nShourya Shukla\n"},{"id":"397517","messageId":"CAOEXN9yyL8T8kDmpHKTjjaG9tVS1kh34B-=PuH1hRaA7jF_K6A@mail.gmail.com","threadId":"53440","inReplyTo":"20200510164424.GA11784@konoha","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-10T21:51:35Z","receivedAt":"2020-05-10T21:51:49Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Hi Shourya\n\n>\n> > On repository with some submodule not active, it could be needed to run\n> > a command only for active submodule. Today it can be achived with the\n> > command:\n>\n> Spelling: achive -> achieve\nagree\n\n> Maybe we can keep the commit message a bit more imperative?\n> Something like:\n> -------------------------\n> On a repository with some submodules not active, one may need to run a\n> command only for an active submodule or vice-versa. To achieve this,\n> one may use:\n> git submodule foreach 'git -C $toplevel submodule--helper is-active \\\n> $sm_path && pwd || :'\n>\n> Simplify this expression to make it more readable and easy-to-use by\n> adding the flag `--is-active` to subcommand `foreach` of `git\n> submodule`. Thus, simplifying the above command to:\n> git submodule--helper foreach --is-active pwd\n> -------------------------\nAgree with the changes except vice-versa. The original patch support only\niterate the active submodule.\n\n> Yes, maybe renaming the flag to `--is-active` would make it a tad bit\n> simpler?\nis-active sound more like a question to me but I can change it.\n\n> This commit message may not be perfect but it seems like an\n> improvement over the previous one?\nyes definitely\n\n> To me this option seems good. It may have some good utility in the\n> future. Similarly, we may change the struct to:\n> struct foreach_cb {\n> const char *prefix;\n> int quiet;\n> int recursive;\n> int is_active;\n> };\n>\n> Therefore, the if-statement becomes:\n> if (info->is_active && !is_submodule_active(the_repository, path))\n> return;\n>\n> BTW what do we return here, could you please be more specific?\nThis is a void function, returning here mean we will not execute the\ncommand. Should I add a\ncomment like:\n return; // skip this submodule and go to next one\nbut maybe it would be more readable to create a intermediate function\nwhich handle only the\nfiltering part. Is it what you mean?\n\n> Again, the change here as well:\n> OPT_BOOL(0, \"is-active\", &info.is_active,\n>\n> Here, too:\n> N_(\"git submodule--helper foreach [--quiet] [--recursive] [--is-active] [--] \"),\n>\n> And,\n> test_expect_success 'test \"submodule--helper foreach --is-active\" usage' '\n>\n> Finally,\n> git submodule--helper foreach --is-active \"echo \\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual\n>\n> What do you think?\n>\n> Regards,\n> Shourya Shukla\n\nNow with the vice-versa idea in mind, I think it is maybe better to\nchange a bit the original patch\nto add the option to execute command only on inactive submodule as\nwell. Could someone need\nthat in future?\n\nBasically this would mean:\n\nOn struct foreach_cb instead of only_active adding field:\n int active;\n\nDefining some macro to hold possible value:\n #define FOREACH_ACTIVE 1\n #define FOREACH_INACTIVE 0\n #define FOREACH_ACTIVE_NOT_SET -1\n\nChanging the FOREACH_CB_INIT to\n #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }\n\nThe filter become:\nint is_active;\nif (FOREACH_ACTIVE_NOT_SET != info->active) {\n is_active = is_submodule_active(the_repository, path);\n if ((is_active && (FOREACH_ACTIVE != info->active)) ||\n (!is_active && (FOREACH_ACTIVE == info->active)))\n return;\n}\n\nIt need two additionnal function to parse the argument:\nstatic int parse_active(const char *arg)\n{\n int active = git_parse_maybe_bool(arg);\n\n if (active < 0)\n die(_(\"invalid --active option: %s\"), arg);\n\n return active;\n}\n\nstatic int parse_opt_active_cb(const struct option *opt, const char *arg,\n int unset)\n{\n if (unset)\n *(int *)opt->value = FOREACH_ACTIVE_NOT_SET;\n else if (arg)\n *(int *)opt->value = parse_active(arg);\n else\n *(int *)opt->value = FOREACH_ACTIVE;\n\n return 0;\n}\n\nAnd the option OPT_BOOL become a OPT_CALLBACK_F:\nOPT_CALLBACK_F(0, \"active\", &info.active, \"true|false\",\n N_(\"Call command depending on submodule active state\"),\n PARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n parse_opt_active_cb),\n\nThe help git_submodule_helper_usage:\nN_(\"git submodule--helper foreach [--quiet] [--recursive]\n[--active[=true|false]] [--] \"),\n\nthe added test change to:\ngit submodule--helper foreach --active \"echo\n\\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual\n\nand adding a test for the inactive submodule:\ncat > expect < ../actual\ngit submodule--helper foreach --active=false \"echo\n\\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual\n) &&\ntest_i18ncmp expect actual\n'\n\nWhat do you think?\n\nRegards,\nGuillaume\n"},{"id":"397518","messageId":"CAPig+cRX+M5qhCuKtsCpw8ySdW358utNOiMR7dwYwR4S4aMT7A@mail.gmail.com","threadId":"53440","inReplyTo":"CAOEXN9yyL8T8kDmpHKTjjaG9tVS1kh34B-=PuH1hRaA7jF_K6A@mail.gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-10T22:42:01Z","receivedAt":"2020-05-10T22:47:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 10, 2020 at 5:53 PM Guillaume Galeazzi\n wrote:\n> > Yes, maybe renaming the flag to `--is-active` would make it a tad bit\n> > simpler?\n> is-active sound more like a question to me but I can change it.\n\nI'm not a submodule user nor have I been following this discussion,\nbut perhaps the name --active-only would be better?\n"},{"id":"397600","messageId":"20200512141520.GA8133@konoha","threadId":"53440","inReplyTo":"CAOEXN9yyL8T8kDmpHKTjjaG9tVS1kh34B-=PuH1hRaA7jF_K6A@mail.gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-12T14:15:20Z","receivedAt":"2020-05-12T14:15:31Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 10/05 11:51, Guillaume Galeazzi wrote:\n\nBefore I comment on the patch, I want to apologise for the delay in the\nreply. I got caught up with some stuff.\n\n> Now with the vice-versa idea in mind, I think it is maybe better to\n> change a bit the original patch\n> to add the option to execute command only on inactive submodule as\n> well. Could someone need\n> that in future?\n> \n> Basically this would mean:\n> \n> On struct foreach_cb instead of only_active adding field:\n> int active;\n\nYeah, keeping the option name as `active` would be better if we were\nto go for the inactive submodules option as well.\n\n> Defining some macro to hold possible value:\n> #define FOREACH_ACTIVE 1\n> #define FOREACH_INACTIVE 0\n> #define FOREACH_ACTIVE_NOT_SET -1\n> \n> Changing the FOREACH_CB_INIT to\n> #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }\n\nDo we really need to include the last macro here?\n\n> The filter become:\n> int is_active;\n> if (FOREACH_ACTIVE_NOT_SET != info->active) {\n> is_active = is_submodule_active(the_repository, path);\n> if ((is_active && (FOREACH_ACTIVE != info->active)) ||\n> (!is_active && (FOREACH_ACTIVE == info->active)))\n> return;\n> }\n\nIs it okay to compare a macro directly? I have not actually seen it\nhappen so I am a bit skeptical. I am tagging along some people who\nwill be able to weigh in a solid opinion regarding this.\n\n> It need two additionnal function to parse the argument:\n> static int parse_active(const char *arg)\n> {\n> int active = git_parse_maybe_bool(arg);\n> \n> if (active < 0)\n> die(_(\"invalid --active option: %s\"), arg);\n> \n> return active;\n> }\n\nAlright, this one is used for parsing out the active submodules right?\n\n> static int parse_opt_active_cb(const struct option *opt, const char *arg,\n> int unset)\n> {\n> if (unset)\n> *(int *)opt->value = FOREACH_ACTIVE_NOT_SET;\n> else if (arg)\n> *(int *)opt->value = parse_active(arg);\n> else\n> *(int *)opt->value = FOREACH_ACTIVE;\n> \n> return 0;\n> }\n> \n> And the option OPT_BOOL become a OPT_CALLBACK_F:\n> OPT_CALLBACK_F(0, \"active\", &info.active, \"true|false\",\n> N_(\"Call command depending on submodule active state\"),\n> PARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n> parse_opt_active_cb),\n> \n> The help git_submodule_helper_usage:\n> N_(\"git submodule--helper foreach [--quiet] [--recursive]\n> [--active[=true|false]] [--] \"),\n\nWhat I have inferred right now is that we introduce the `--active`\noption which will take a T/F value depending on user input. We have 3\nmacros to check for the value of `active`, but I don't understand the\nsignificance of changing the FOREACH_CB_INIT macro to accomodate the\nthird option. And we use a function to parse out the active\nsubmodules.\n\nInstead of the return statement you wrote, won't it be better to call\nparse_active() depending on the case? Meaning that we call\nparse_active() when `active=true`.\n\nRegards,\nShourya Shukla\n"},{"id":"397616","messageId":"xmqq8shxc7ct.fsf@gitster.c.googlers.com","threadId":"53440","inReplyTo":"pull.631.git.1589099162707.gitgitgadget@gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-12T18:53:54Z","receivedAt":"2020-05-12T18:54:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Guillaume G. via GitGitGadget\" writes:\n\n> From: Guillaume Galeazzi \n>\n> On repository with some submodule not active, it could be needed to run\n> a command only for active submodule. Today it can be achived with the\n> command:\n>\n> git submodule foreach 'git -C $toplevel submodule--helper is-active \\\n> $sm_path && pwd || :'\n\n\"it could be needed\" is being too modest.\n\n\tTo iterate only on active submodules, we can do ...\n\n\t ... << the above command >> ...\n\n\tbut it is inefficient to ask about each and every submodule.\n\nmay be convincing enough. \n\nIf iterating over only active ones is useful, surely it would also\nbe useful to be able to iterate over only inactive ones, right? \n\nSo, before getting married too much to the use-case of \"only active\nones\" and getting our eyes clouded from seeing a larger picture,\nlet's see what other \"traits\" of submodules we can use to pick which\nones to act on.\n\nAre there attributes other than \"is-active\" that we may want to and\ncan check about submodules? There is is_submodule_populated() next\nto is_submodule_active(), which might be a candidate. IOW, what I am\nwondering is if it makes sense to extend this to\n\n\tgit submodule foreach --trait=is-active ...\n\tgit submodule foreach --trait=!is-active ...\n\tgit submodule foreach --trait=is-populated ...\n\nto allow iterating only on submodules with/without given trait (I am\nnot suggesting the actual option name, but merely making sure that\n'is-active' is not anything special but one of the possibilities\nthat can be used to limit the iteration using the same mechanism).\n\n> builtin/submodule--helper.c | 8 +++++++-\n> t/t7407-submodule-foreach.sh | 16 ++++++++++++++++\n> 2 files changed, 23 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c882..1a275403764 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -450,6 +450,7 @@ struct foreach_cb {\n> \tconst char *prefix;\n> \tint quiet;\n> \tint recursive;\n> +\tint only_active;\n\nAnd I tend to agree with Eric downthread that active_only would be a\nmore natural name if we want to have this field.\n\n> @@ -464,6 +465,9 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tchar *displaypath;\n> \n> +\tif (info->only_active && !is_submodule_active(the_repository, path))\n> +\t\treturn;\n> +\n> \tdisplaypath = get_submodule_displaypath(path, info->prefix);\n> \n> \tsub = submodule_from_path(the_repository, &null_oid, path);\n> @@ -565,11 +569,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n> \t\tOPT__QUIET(&info.quiet, N_(\"Suppress output of entering each submodule command\")),\n> \t\tOPT_BOOL(0, \"recursive\", &info.recursive,\n> \t\t\t N_(\"Recurse into nested submodules\")),\n> +\t\tOPT_BOOL(0, \"only-active\", &info.only_active,\n> +\t\t\t N_(\"Call command only for active submodules\")),\n> \t\tOPT_END()\n> \t};\n> \n> \tconst char *const git_submodule_helper_usage[] = {\n> -\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--] \"),\n> +\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--only-active] [--] \"),\n> \t\tNULL\n> \t};\n> \n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 6b2aa917e11..f90a16e3e67 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -80,6 +80,22 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n> \ttest_i18ncmp expect actual\n> '\n> \n> +sub3sha1=$(cd super/sub3 && git rev-parse HEAD)\n> +cat > expect < +Entering 'sub3'\n> +$pwd/clone-foo3-sub3-$sub3sha1\n> +EOF\n> +\n> +test_expect_success 'test \"submodule--helper foreach --only-active\" usage' '\n> +\ttest_when_finished \"git -C clone config --unset submodule.foo1.active\" &&\n> +\t(\n> +\t\tcd clone &&\n> +\t\tgit config --bool submodule.foo1.active \"false\" &&\n> +\t\tgit submodule--helper foreach --only-active \"echo \\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual\n> +\t) &&\n> +\ttest_i18ncmp expect actual\n> +'\n> +\n> cat >expect < Entering '../sub1'\n> $pwd/clone-foo1-sub1-../sub1-$sub1sha1\n>\n> base-commit: b994622632154fc3b17fb40a38819ad954a5fb88\n"},{"id":"397703","messageId":"cf25c55e-e0c8-6374-3adf-84f4314869e9@gmail.com","threadId":"53440","inReplyTo":"xmqq8shxc7ct.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-13T05:17:30Z","receivedAt":"2020-05-13T05:17:48Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Hi Junio\n\n> If iterating over only active ones is useful, surely it would also\n> be useful to be able to iterate over only inactive ones, right?\n> \n> So, before getting married too much to the use-case of \"only active\n> ones\" and getting our eyes clouded from seeing a larger picture,\n> let's see what other \"traits\" of submodules we can use to pick which\n> ones to act on.\n> \n> Are there attributes other than \"is-active\" that we may want to and\n> can check about submodules? There is is_submodule_populated() next\n> to is_submodule_active(), which might be a candidate. IOW, what I am\n> wondering is if it makes sense to extend this to\n> \n> \tgit submodule foreach --trait=is-active ...\n> \tgit submodule foreach --trait=!is-active ...\n> \tgit submodule foreach --trait=is-populated ...\n> \n> to allow iterating only on submodules with/without given trait (I am\n> not suggesting the actual option name, but merely making sure that\n> 'is-active' is not anything special but one of the possibilities\n> that can be used to limit the iteration using the same mechanism).\n\nThe idea that other candidate are possible seem good. But then users \nwill need combination like is-active && !is-populated. We will end up \nimplementing a complex code to filter based on expression which is IMHO \noverkill.\n\nIf is-populated is needed, that could be implemented this way:\n\n git submodule--helper --is-populated[=true|false]\n\nthis would allow combination with the is active filter and the\nprevious example would be\n\n git submodule--helper --is-active --is-populated=false \n\nRegards,\nGG\n"},{"id":"397716","messageId":"xmqqwo5fvoe1.fsf@gitster.c.googlers.com","threadId":"53440","inReplyTo":"cf25c55e-e0c8-6374-3adf-84f4314869e9@gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T15:35:34Z","receivedAt":"2020-05-13T15:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Guillaume Galeazzi writes:\n\n>> \tgit submodule foreach --trait=is-active ...\n>> \tgit submodule foreach --trait=!is-active ...\n>> \tgit submodule foreach --trait=is-populated ...\n>>\n>> to allow iterating only on submodules with/without given trait (I am\n>> not suggesting the actual option name, but merely making sure that\n>> 'is-active' is not anything special but one of the possibilities\n>> that can be used to limit the iteration using the same mechanism).\n>\n> The idea that other candidate are possible seem good. But then users\n> will need combination like is-active && !is-populated. ...\n> ... this would allow combination with the is active filter and the\n> previous example would be\n>\n> git submodule--helper --is-active --is-populated=false \n\nThere is no difference between that and\n\n\tgit submodule--helper --trait=is-active --trait=is-populated\n\nso I fail to see what new you are proposing, sorry.\n\n"},{"id":"397758","messageId":"83b37142-f6f0-2f5d-e92c-de3891e8fe18@gmail.com","threadId":"53440","inReplyTo":"xmqqwo5fvoe1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-13T20:07:54Z","receivedAt":"2020-05-13T20:08:00Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":">>> \tgit submodule foreach --trait=is-active ...\n>>> \tgit submodule foreach --trait=!is-active ...\n>>> \tgit submodule foreach --trait=is-populated ...\n>>>\n>>> to allow iterating only on submodules with/without given trait (I am\n>>> not suggesting the actual option name, but merely making sure that\n>>> 'is-active' is not anything special but one of the possibilities\n>>> that can be used to limit the iteration using the same mechanism).\n>>\n>> The idea that other candidate are possible seem good. But then users\n>> will need combination like is-active && !is-populated. ...\n>> ... this would allow combination with the is active filter and the\n>> previous example would be\n>>\n>> git submodule--helper --is-active --is-populated=false \n> \n> There is no difference between that and\n> \n> \tgit submodule--helper --trait=is-active --trait=is-populated\n> \n> so I fail to see what new you are proposing, sorry.\n> \n\nThe difference is that you repeat twice the same flag. Sometime \nrepeating a flag overwrite the previous value, but it depend how it is \nimplemented. In current case it should be possible to implement it this \nway, if this is required.\n\nRegarding previous example, it use '!' to negate the value. Not all \npeople know the meaning of it. A new proposal would be:\n\n git submodule--helper foreach [--trait=[not-](active|populated)]\n\nWhat do you think?\n"},{"id":"397766","messageId":"xmqqwo5fshe0.fsf@gitster.c.googlers.com","threadId":"53440","inReplyTo":"83b37142-f6f0-2f5d-e92c-de3891e8fe18@gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T20:35:03Z","receivedAt":"2020-05-13T20:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Guillaume Galeazzi writes:\n\n> The difference is that you repeat twice the same flag. Sometime\n> repeating a flag overwrite the previous value,...\n\nI already said I was *not* suggesting a concrete syntax. The more\nimportant point was to make us realize that we need to think outside\nof \"active-only\" and make sure we can support other kinds of selection\ncriteria for submodules.\n\nSo whatever syntax you would want to use to specify more than one,\ncombine, and negate, I do not care too deeply, as long as it is in\nline with what we use in the other parts of the system ;-).\n\n> Regarding previous example, it use '!' to negate the value. Not all\n> people know the meaning of it.\n\nWe mark the bottom commit by negating with ^ (e.g. \"git log ^maint master\"),\nwe mark \"not ignored\" entries by prefixing with '!' in .gitignore,\nwe mark \"unset\" entries by prefixing with '-' in .gitattributes (the\n'!' prefix is used to mark \"unspecified\" entries), and \"rev-list --boundary\"\noutput uses '~' prefix to mean \"this is not part of the range\".\n\nSo there are many one-letter \"not\" we already use, and there seem to\nbe no rule to pick which one in what context X-<.\n\nSo spelling out \"--no-blah\" to mean \"not with blah\" is probably a\ngood thing to do (especially if readers do not mind being English\ncentric).\n\n"},{"id":"397871","messageId":"37d9ea7b-8edc-e7e6-4f79-045d616de3e2@gmail.com","threadId":"53440","inReplyTo":"xmqqwo5fshe0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-15T11:04:56Z","receivedAt":"2020-05-15T11:05:03Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Le 13.05.2020 à 22:35, Junio C Hamano a écrit :\n > Guillaume Galeazzi writes:\n >\n > I already said I was *not* suggesting a concrete syntax. The more\n > important point was to make us realize that we need to think outside\n > of \"active-only\" and make sure we can support other kinds of selection\n > criteria for submodules.\n\nOk get it now. So after looking a bit, another trait that could be\ninteresting filtering on is remote tracking branch. A v2 with filtering \nbased on active (or not), populated (or not), and tracked remote branch \nis ready on my side. It also include the rename of the struct member \nonly_active to active_only. Let me know when I can /submit.\n\n > So spelling out \"--no-blah\" to mean \"not with blah\" is probably a\n > good thing to do (especially if readers do not mind being English\n > centric).\n >\n\nGreat, it make it a bit simpler to code, thanks for the tips.\n"},{"id":"397892","messageId":"037991cd-d6ed-e55a-72ed-f49041460ea2@gmail.com","threadId":"53440","inReplyTo":"CAPig+cRX+M5qhCuKtsCpw8ySdW358utNOiMR7dwYwR4S4aMT7A@mail.gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-15T16:29:52Z","receivedAt":"2020-05-15T16:29:59Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Le 11.05.2020 à 00:42, Eric Sunshine a écrit :\n> On Sun, May 10, 2020 at 5:53 PM Guillaume Galeazzi\n> wrote:\n>>> Yes, maybe renaming the flag to `--is-active` would make it a tad bit\n>>> simpler?\n>> is-active sound more like a question to me but I can change it.\n> \n> I'm not a submodule user nor have I been following this discussion,\n> but perhaps the name --active-only would be better?\n> \n\nTo flow up on that topic, the flag can be `--[no-]active`. It simplify \ncode and allow the optional negation. On the source, the variable will \nbe renamed active_only.\n"},{"id":"397896","messageId":"b45268b1-de5f-051c-f956-8a17c067f4c6@gmail.com","threadId":"53440","inReplyTo":"20200512141520.GA8133@konoha","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-15T16:51:57Z","receivedAt":"2020-05-15T16:52:04Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Le 12.05.2020 à 16:15, Shourya Shukla a écrit :\n> On 10/05 11:51, Guillaume Galeazzi wrote:\n> \n> \n>> Defining some macro to hold possible value:\n>> #define FOREACH_ACTIVE 1\n>> #define FOREACH_INACTIVE 0\n>> #define FOREACH_ACTIVE_NOT_SET -1\n>>\n>> Changing the FOREACH_CB_INIT to\n>> #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }\n> \n> Do we really need to include the last macro here?\n\nAfter a cross check, yes it is the correct place to initialise the new \nactive_only member of foreach_cb. But it will be changed to use \ndesignated initializers.\n\n>> The filter become:\n>> int is_active;\n>> if (FOREACH_ACTIVE_NOT_SET != info->active) {\n>> is_active = is_submodule_active(the_repository, path);\n>> if ((is_active && (FOREACH_ACTIVE != info->active)) ||\n>> (!is_active && (FOREACH_ACTIVE == info->active)))\n>> return;\n>> }\n> \n> Is it okay to compare a macro directly? I have not actually seen it\n> happen so I am a bit skeptical. I am tagging along some people who\n> will be able to weigh in a solid opinion regarding this.\n\nYes it is okay, a `#define SOMETHING WHATEVER` will just inform the c \npreprocessor to replace the `SOMETHING` by `WHATEVER`. The only thing \nthe final c compiler will see is `WHATEVER`. In our case a integer value.\n\nGoal here was to avoid magic number, but after looking to the code it \nseem accepted that true is 1 and false is 0. To comply with that, in \nnext version it will be replace it with:\n\n\tif (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {\n\t\tis_active = is_submodule_active(the_repository, path);\n\t\tif (is_active != info->active_only)\n\t\t\treturn;\n\t}\n\n> \n>> It need two additionnal function to parse the argument:\n>> static int parse_active(const char *arg)\n>> {\n>> int active = git_parse_maybe_bool(arg);\n>>\n>> if (active < 0)\n>> die(_(\"invalid --active option: %s\"), arg);\n>>\n>> return active;\n>> }\n> \n> Alright, this one is used for parsing out the active submodules right\nAs suggested on other mail of this patch, it will be removed and take \nthe shortcut `--no-active`.\n\n>> And the option OPT_BOOL become a OPT_CALLBACK_F:\n>> OPT_CALLBACK_F(0, \"active\", &info.active, \"true|false\",\n>> N_(\"Call command depending on submodule active state\"),\n>> PARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n>> parse_opt_active_cb),\n>>\n>> The help git_submodule_helper_usage:\n>> N_(\"git submodule--helper foreach [--quiet] [--recursive]\n>> [--active[=true|false]] [--] \"),\n> \n> What I have inferred right now is that we introduce the `--active`\n> option which will take a T/F value depending on user input. We have 3\n> macros to check for the value of `active`, but I don't understand the\n> significance of changing the FOREACH_CB_INIT macro to accomodate the\n> third option. And we use a function to parse out the active\n> submodules.\n\nThe change on `FOREACH_CB_INIT` are to keep original behaviour of the \ncommand if new flags are not given.\n\n> Instead of the return statement you wrote, won't it be better to call\n> parse_active() depending on the case? Meaning that we call\n> parse_active() when `active=true`.\n> \n> Regards,\n> Shourya Shukla\n> \n\nThe code to parse command T/F will be removed.\n\nRegards,\n\nGuillaume\n"},{"id":"397898","messageId":"xmqq7dxdm8qi.fsf@gitster.c.googlers.com","threadId":"53440","inReplyTo":"b45268b1-de5f-051c-f956-8a17c067f4c6@gmail.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-15T17:03:01Z","receivedAt":"2020-05-15T17:03:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Guillaume Galeazzi writes:\n\n> Goal here was to avoid magic number, but after looking to the code it\n> seem accepted that true is 1 and false is 0. To comply with that, in\n> next version it will be replace it with:\n>\n> \tif (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {\n\nIt still is unusual to have a constant on the left hand side of the\n\"!=\" or \"==\" operator, though. Having a constant on the left hand\nside of \"<\" and \"<=\" is justifiable, but not for \"!=\" and \"==\".\n"},{"id":"397926","messageId":"CAOEXN9yjd6r54K=5_Mdqr+cGJJ-ngaerfdbfD2m6RVZys-QmVQ@mail.gmail.com","threadId":"53440","inReplyTo":"xmqq7dxdm8qi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-15T18:53:51Z","receivedAt":"2020-05-15T18:54:02Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Le ven. 15 mai 2020 à 19:03, Junio C Hamano a écrit :\n>\n> Guillaume Galeazzi writes:\n>\n> > Goal here was to avoid magic number, but after looking to the code it\n> > seem accepted that true is 1 and false is 0. To comply with that, in\n> > next version it will be replace it with:\n> >\n> > if (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {\n>\n> It still is unusual to have a constant on the left hand side of the\n> \"!=\" or \"==\" operator, though. Having a constant on the left hand\n> side of \"<\" and \"<=\" is justifiable, but not for \"!=\" and \"==\".\n\nIt is call Yoda condition. As it compare with a constant, the compiler\nwill throw an error if you write only = instead of != or ==.\n\nBut after a quick check, this wasn't needed as compiler warn\n builtin/submodule--helper.c:570:2: error: suggest parentheses\naround assignment used as truth value [-Werror=parentheses]\n\nAnd I am not a Yoda condition adept at all. So it will be removed.\n"},{"id":"398000","messageId":"pull.631.v2.git.1589697023.gitgitgadget@gmail.com","threadId":"53440","inReplyTo":"pull.631.git.1589099162707.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] submodule--helper.c: add only-active to foreach","fromName":"Guillaume G. via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-17T06:30:20Z","receivedAt":"2020-05-17T06:30:29Z","isPatch":true,"sender":{"key":"name:Guillaume G.","avatar":null},"body":"On repository with multiple submodules, one may need to run a command based\non submodule trait.\n\nThis changes add flags to submodule--helper foreachto fulfill this need by:\n\nAdding the flag --[no-]active to filter submodule based on the active state.\nAdding the flag --[no-]populated to filter submodule based on the fact that\nit is populated or not. Adding the flag -b|--branch to filter\nsubmodule based on the tracking branch.\n\nSigned-off-by: Guillaume Galeazzi guillaume.galeazzi@gmail.com\n[guillaume.galeazzi@gmail.com]\n\nGuillaume Galeazzi (3):\n submodule--helper.c: add active to foreach\n submodule--helper.c: add populated to foreach\n submodule--helper.c: add branch to foreach\n\n builtin/submodule--helper.c | 60 ++++++++++++++++++++++++++++++------\n t/t7407-submodule-foreach.sh | 60 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 110 insertions(+), 10 deletions(-)\n\n\nbase-commit: b994622632154fc3b17fb40a38819ad954a5fb88\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-631%2Fgzzi%2Fsubmodule-only-active-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-631/gzzi/submodule-only-active-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/631\n\nRange-diff vs v1:\n\n 1: 77bb291af5d ! 1: 90defeb5a75 submodule--helper.c: add only-active to foreach\n @@ Metadata\n Author: Guillaume Galeazzi \n \n ## Commit message ##\n - submodule--helper.c: add only-active to foreach\n -\n - On repository with some submodule not active, it could be needed to run\n - a command only for active submodule. Today it can be achived with the\n - command:\n + submodule--helper.c: add active to foreach\n \n + On a repository with some submodules not active, one may need to run a\n + command only for an active submodule or vice-versa. To achieve this,\n + one may use:\n git submodule foreach 'git -C $toplevel submodule--helper is-active \\\n $sm_path && pwd || :'\n \n - Goal of this change is to make it more accessible by adding the flag\n - --only-active to the submodule--helper command. Previous example\n - become:\n -\n - git submodule--helper foreach --only-active pwd\n + Simplify this expression to make it more readable and easy-to-use by\n + adding the flat `--[no-]active` to subcommand `foreach` of `git\n + submodule`. Thus, simplifying the above command to:\n + git submodule--helper foreach --active pwd\n \n Signed-off-by: Guillaume Galeazzi \n \n @@ builtin/submodule--helper.c: struct foreach_cb {\n \tconst char *prefix;\n \tint quiet;\n \tint recursive;\n -+\tint only_active;\n ++\tint active_only;\n };\n - #define FOREACH_CB_INIT { 0 }\n +-#define FOREACH_CB_INIT { 0 }\n + \n +-static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n +-\t\t\t\t void *cb_data)\n ++#define FOREACH_BOOL_FILTER_NOT_SET -1\n ++\n ++#define FOREACH_CB_INIT { .active_only=FOREACH_BOOL_FILTER_NOT_SET }\n ++\n ++static void runcommand_in_submodule(const struct cache_entry *list_item,\n ++\t\t\t\t struct foreach_cb *info)\n + {\n +-\tstruct foreach_cb *info = cb_data;\n + \tconst char *path = list_item->name;\n + \tconst struct object_id *ce_oid = &list_item->oid;\n \n @@ builtin/submodule--helper.c: static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n - \tstruct child_process cp = CHILD_PROCESS_INIT;\n - \tchar *displaypath;\n + \tfree(displaypath);\n + }\n \n -+\tif (info->only_active && !is_submodule_active(the_repository, path))\n -+\t\treturn;\n ++static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_item,\n ++\t\t\t\t\t\tvoid *cb_data)\n ++{\n ++\tconst char *path = list_item->name;\n ++\tstruct foreach_cb *info = cb_data;\n ++\tint is_active;\n +\n - \tdisplaypath = get_submodule_displaypath(path, info->prefix);\n - \n - \tsub = submodule_from_path(the_repository, &null_oid, path);\n ++\tif (info->active_only != FOREACH_BOOL_FILTER_NOT_SET) {\n ++\t\tis_active = is_submodule_active(the_repository, path);\n ++\t\tif (is_active != info->active_only)\n ++\t\t\treturn;\n ++\t}\n ++\n ++\truncommand_in_submodule(list_item, info);\n ++}\n ++\n + static int module_foreach(int argc, const char **argv, const char *prefix)\n + {\n + \tstruct foreach_cb info = FOREACH_CB_INIT;\n @@ builtin/submodule--helper.c: static int module_foreach(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&info.quiet, N_(\"Suppress output of entering each submodule command\")),\n \t\tOPT_BOOL(0, \"recursive\", &info.recursive,\n \t\t\t N_(\"Recurse into nested submodules\")),\n -+\t\tOPT_BOOL(0, \"only-active\", &info.only_active,\n -+\t\t\t N_(\"Call command only for active submodules\")),\n ++\t\tOPT_BOOL(0, \"active\", &info.active_only,\n ++\t\t\t N_(\"Call command depending on submodule active state\")),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n -\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--] \"),\n -+\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--only-active] [--] \"),\n ++\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--] \"),\n \t\tNULL\n \t};\n \n +@@ builtin/submodule--helper.c: static int module_foreach(int argc, const char **argv, const char *prefix)\n + \tinfo.argv = argv;\n + \tinfo.prefix = prefix;\n + \n +-\tfor_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);\n ++\tfor_each_listed_submodule(&list, runcommand_in_submodule_filtered_cb, &info);\n + \n + \treturn 0;\n + }\n \n ## t/t7407-submodule-foreach.sh ##\n @@ t/t7407-submodule-foreach.sh: test_expect_success 'test basic \"submodule foreach\" usage' '\n \ttest_i18ncmp expect actual\n '\n \n -+sub3sha1=$(cd super/sub3 && git rev-parse HEAD)\n +cat > expect < ../actual\n ++\t) &&\n ++\ttest_i18ncmp expect actual\n ++'\n ++\n ++cat > expect < ../actual\n ++\t\tgit submodule--helper foreach --no-active \"echo \\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual\n +\t) &&\n +\ttest_i18ncmp expect actual\n +'\n -: ----------- > 2: f9cbdcdeacf submodule--helper.c: add populated to foreach\n -: ----------- > 3: 65504cb780a submodule--helper.c: add branch to foreach\n\n-- \ngitgitgadget\n"},{"id":"398001","messageId":"90defeb5a7594cce1a900fc5dd4fdc4ba9794005.1589697023.git.gitgitgadget@gmail.com","threadId":"53440","inReplyTo":"pull.631.v2.git.1589697023.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] submodule--helper.c: add active to foreach","fromName":"Guillaume Galeazzi via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-17T06:30:21Z","receivedAt":"2020-05-17T06:30:33Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"From: Guillaume Galeazzi \n\nOn a repository with some submodules not active, one may need to run a\ncommand only for an active submodule or vice-versa. To achieve this,\none may use:\ngit submodule foreach 'git -C $toplevel submodule--helper is-active \\\n$sm_path && pwd || :'\n\nSimplify this expression to make it more readable and easy-to-use by\nadding the flat `--[no-]active` to subcommand `foreach` of `git\nsubmodule`. Thus, simplifying the above command to:\ngit submodule--helper foreach --active pwd\n\nSigned-off-by: Guillaume Galeazzi \n---\n builtin/submodule--helper.c | 33 +++++++++++++++++++++++++++------\n t/t7407-submodule-foreach.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c882..4b17e3b3b11 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -450,13 +450,16 @@ struct foreach_cb {\n \tconst char *prefix;\n \tint quiet;\n \tint recursive;\n+\tint active_only;\n };\n-#define FOREACH_CB_INIT { 0 }\n \n-static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n-\t\t\t\t void *cb_data)\n+#define FOREACH_BOOL_FILTER_NOT_SET -1\n+\n+#define FOREACH_CB_INIT { .active_only=FOREACH_BOOL_FILTER_NOT_SET }\n+\n+static void runcommand_in_submodule(const struct cache_entry *list_item,\n+\t\t\t\t struct foreach_cb *info)\n {\n-\tstruct foreach_cb *info = cb_data;\n \tconst char *path = list_item->name;\n \tconst struct object_id *ce_oid = &list_item->oid;\n \n@@ -555,6 +558,22 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n \tfree(displaypath);\n }\n \n+static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_item,\n+\t\t\t\t\t\tvoid *cb_data)\n+{\n+\tconst char *path = list_item->name;\n+\tstruct foreach_cb *info = cb_data;\n+\tint is_active;\n+\n+\tif (info->active_only != FOREACH_BOOL_FILTER_NOT_SET) {\n+\t\tis_active = is_submodule_active(the_repository, path);\n+\t\tif (is_active != info->active_only)\n+\t\t\treturn;\n+\t}\n+\n+\truncommand_in_submodule(list_item, info);\n+}\n+\n static int module_foreach(int argc, const char **argv, const char *prefix)\n {\n \tstruct foreach_cb info = FOREACH_CB_INIT;\n@@ -565,11 +584,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&info.quiet, N_(\"Suppress output of entering each submodule command\")),\n \t\tOPT_BOOL(0, \"recursive\", &info.recursive,\n \t\t\t N_(\"Recurse into nested submodules\")),\n+\t\tOPT_BOOL(0, \"active\", &info.active_only,\n+\t\t\t N_(\"Call command depending on submodule active state\")),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--] \"),\n+\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--] \"),\n \t\tNULL\n \t};\n \n@@ -583,7 +604,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \tinfo.argv = argv;\n \tinfo.prefix = prefix;\n \n-\tfor_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);\n+\tfor_each_listed_submodule(&list, runcommand_in_submodule_filtered_cb, &info);\n \n \treturn 0;\n }\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6b2aa917e11..57c5ae70c8d 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -80,6 +80,36 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n \ttest_i18ncmp expect actual\n '\n \n+cat > expect < ../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+cat > expect < ../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >expect <\n\nOn a repository with some submodules tracking different branch, one\nmay need to run a command only for submodule tracking one of specified\nbranch. This change, add flags `-b ` and --branch ` to\nsubcommand `foreach` of `git submodule--helper`.\n\nSigned-off-by: Guillaume Galeazzi \n---\n builtin/submodule--helper.c | 21 +++++++++++++++++++--\n t/t7407-submodule-foreach.sh | 15 +++++++++++++++\n 2 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex eb17e8c5293..066d1974605 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -452,11 +452,13 @@ struct foreach_cb {\n \tint recursive;\n \tint active_only;\n \tint populated_only;\n+\tstruct string_list remote_branch_filter;\n };\n \n #define FOREACH_BOOL_FILTER_NOT_SET -1\n \n-#define FOREACH_CB_INIT { .active_only = FOREACH_BOOL_FILTER_NOT_SET, .populated_only = 1 }\n+#define FOREACH_CB_INIT { .active_only = FOREACH_BOOL_FILTER_NOT_SET, \\\n+\t.populated_only = 1, .remote_branch_filter = STRING_LIST_INIT_NODUP }\n \n static void runcommand_in_submodule(const struct cache_entry *list_item,\n \t\t\t\t struct foreach_cb *info)\n@@ -555,12 +557,15 @@ static void runcommand_in_submodule(const struct cache_entry *list_item,\n \tfree(displaypath);\n }\n \n+static const char *remote_submodule_branch(const char *path);\n+\n static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_item,\n \t\t\t\t\t\tvoid *cb_data)\n {\n \tconst char *path = list_item->name;\n \tstruct foreach_cb *info = cb_data;\n \tint is_active;\n+\tconst char *branch;\n \n \tif (info->active_only != FOREACH_BOOL_FILTER_NOT_SET) {\n \t\tis_active = is_submodule_active(the_repository, path);\n@@ -571,6 +576,14 @@ static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_i\n \tif (info->populated_only != is_submodule_populated_gently(path, NULL))\n \t\treturn;\n \n+\tif (info->remote_branch_filter.nr) {\n+\t\tbranch = remote_submodule_branch(path);\n+\t\tif (!branch)\n+\t\t\treturn;\n+\t\tif (!unsorted_string_list_has_string(&info->remote_branch_filter, branch))\n+\t\t\treturn;\n+\t}\n+\n \truncommand_in_submodule(list_item, info);\n }\n \n@@ -588,11 +601,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"Call command depending on submodule active state\")),\n \t\tOPT_BOOL(0, \"populated\", &info.populated_only,\n \t\t\t N_(\"Call command depending on submodule populated state\")),\n+\t\tOPT_STRING_LIST('b', \"branch\", &info.remote_branch_filter,\n+\t\t\t N_(\"branch\"), N_(\"Call command only if submodule remote branch is one of given\")),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--[no-]populated] [--] \"),\n+\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--[no-]populated] [-b|--branch ] [--] \"),\n \t\tNULL\n \t};\n \n@@ -608,6 +623,8 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \n \tfor_each_listed_submodule(&list, runcommand_in_submodule_filtered_cb, &info);\n \n+\tstring_list_clear(&info.remote_branch_filter, 0);\n+\n \treturn 0;\n }\n \ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex be6cd80d464..2da19ac54ce 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -125,6 +125,21 @@ test_expect_success 'test \"submodule--helper foreach --no-active\" usage' '\n \ttest_i18ncmp expect actual\n '\n \n+cat > expect < ../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >expect <\n\nOn a repository with some submodules not initialised, one may need to\nrun a command only for not initilaised submodule or vice-versa. This\nchange, add the flag `--[no-]populated` to subcommand `foreach` of `git\nsubmodule--helper`.\n\nSigned-off-by: Guillaume Galeazzi \n---\n builtin/submodule--helper.c | 14 ++++++++------\n t/t7407-submodule-foreach.sh | 15 +++++++++++++++\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4b17e3b3b11..eb17e8c5293 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -451,11 +451,12 @@ struct foreach_cb {\n \tint quiet;\n \tint recursive;\n \tint active_only;\n+\tint populated_only;\n };\n \n #define FOREACH_BOOL_FILTER_NOT_SET -1\n \n-#define FOREACH_CB_INIT { .active_only=FOREACH_BOOL_FILTER_NOT_SET }\n+#define FOREACH_CB_INIT { .active_only = FOREACH_BOOL_FILTER_NOT_SET, .populated_only = 1 }\n \n static void runcommand_in_submodule(const struct cache_entry *list_item,\n \t\t\t\t struct foreach_cb *info)\n@@ -475,9 +476,6 @@ static void runcommand_in_submodule(const struct cache_entry *list_item,\n \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n \t\t\tdisplaypath);\n \n-\tif (!is_submodule_populated_gently(path, NULL))\n-\t\tgoto cleanup;\n-\n \tprepare_submodule_repo_env(&cp.env_array);\n \n \t/*\n@@ -554,7 +552,6 @@ static void runcommand_in_submodule(const struct cache_entry *list_item,\n \t\t\t\tdisplaypath);\n \t}\n \n-cleanup:\n \tfree(displaypath);\n }\n \n@@ -571,6 +568,9 @@ static void runcommand_in_submodule_filtered_cb(const struct cache_entry *list_i\n \t\t\treturn;\n \t}\n \n+\tif (info->populated_only != is_submodule_populated_gently(path, NULL))\n+\t\treturn;\n+\n \truncommand_in_submodule(list_item, info);\n }\n \n@@ -586,11 +586,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"Recurse into nested submodules\")),\n \t\tOPT_BOOL(0, \"active\", &info.active_only,\n \t\t\t N_(\"Call command depending on submodule active state\")),\n+\t\tOPT_BOOL(0, \"populated\", &info.populated_only,\n+\t\t\t N_(\"Call command depending on submodule populated state\")),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--] \"),\n+\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--[no-]active] [--[no-]populated] [--] \"),\n \t\tNULL\n \t};\n \ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 57c5ae70c8d..be6cd80d464 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -80,6 +80,21 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n \ttest_i18ncmp expect actual\n '\n \n+sub2sha1=$(cd super/sub2 && git rev-parse HEAD)\n+\n+cat > expect < ../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat > expect < writes:\n\n> Subject: Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach\n\nAre we still adding 'only-active'?\n\nDoes updating the cover letter needs a better support from our\ntools, I wonder...\n\n"},{"id":"398029","messageId":"b9240d11-5ae9-301c-43b1-191532489ee4@gmail.com","threadId":"53440","inReplyTo":"xmqq8shq7edu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-05-17T19:47:17Z","receivedAt":"2020-05-17T19:47:23Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Le 17.05.2020 à 17:46, Junio C Hamano a écrit :\n> \"Guillaume G. via GitGitGadget\" writes:\n> \n>> Subject: Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach\n> \n> Are we still adding 'only-active'?\n> \n> Does updating the cover letter needs a better support from our\n> tools, I wonder...\n> \n\nHi Junio,\n\nProbably no, it's me. I wasn't sure for traceability changing subject \nname was allowed. I assume for next time it is.\n\nnew subject would be: submodule--helper.c: add filters to foreach\n"},{"id":"403956","messageId":"91f0cfdc-dfba-80c1-6581-d8a240cbcc4b@gmail.com","threadId":"53440","inReplyTo":"xmqq8shq7edu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/3] submodule--helper.c: add only-active to foreach","fromName":"Guillaume Galeazzi","fromEmail":"guillaume.galeazzi@gmail.com","sentAt":"2020-08-18T15:57:56Z","receivedAt":"2020-08-18T15:59:26Z","isPatch":true,"sender":{"key":"guillaume.galeazzi@gmail.com","avatar":null},"body":"Le 17.05.2020 à 17:46, Junio C Hamano a écrit :\n> \n> Does updating the cover letter needs a better support from our\n> tools, I wonder...\n> \n\nHello, well there was an issue that some markdown and new line of the \ncover letter was removed by GGG. How to move forward?\n\nThe original text before GGG was:\n\nOn repository with multiple submodules, one may need to run\na command based on submodule trait.\n\nThis changes add flags to `submodule--helper foreach` to fulfill this \nneed by:\n\nAdding the flag `--[no-]active` to filter submodule based on the active\nstate.\nAdding the flag `--[no-]populated` to filter submodule based on the\nfact that it is populated or not.\nAdding the flag `-b|--branch ` to filter submodule based on\nthe tracking branch.\n\n"}]}