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

From: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
Date: May 10, 2020, 21:51 UTC
Message-ID: <CAOEXN9yyL8T8kDmpHKTjjaG9tVS1kh34B-=PuH1hRaA7jF_K6A@mail.gmail.com>
In-Reply-To: <20200510164424.GA11784@konoha>

Hi Shourya

Show 6 quoted lines

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

Show 14 quoted lines

> 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

Show 14 quoted lines

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

Show 16 quoted lines

> 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