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
