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

From: Guillaume Galeazzi <guillaume.galeazzi@gmail.com>
Date: May 15, 2020, 16:51 UTC
Message-ID: <b45268b1-de5f-051c-f956-8a17c067f4c6@gmail.com>
In-Reply-To: <20200512141520.GA8133@konoha>

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

Show 12 quoted lines

> On 10/05 11:51, Guillaume Galeazzi wrote:
> 
> 
>> Defining some macro to hold possible value:
>>          #define FOREACH_ACTIVE 1
>>          #define FOREACH_INACTIVE 0
>>          #define FOREACH_ACTIVE_NOT_SET -1
>>
>> Changing the FOREACH_CB_INIT to
>>          #define FOREACH_CB_INIT { 0, NULL, NULL, 0, 0, FOREACH_ACTIVE_NOT_SET }
> 
> Do we really need to include the last macro here?

After a cross check, yes it is the correct place to initialise the new active_only member of foreach_cb. But it will be changed to use designated initializers.

Show 12 quoted lines

>> The filter become:
>> int is_active;
>> if (FOREACH_ACTIVE_NOT_SET != info->active) {
>>          is_active = is_submodule_active(the_repository, path);
>>          if ((is_active && (FOREACH_ACTIVE != info->active)) ||
>>              (!is_active && (FOREACH_ACTIVE == info->active)))
>>                  return;
>> }
> 
> Is it okay to compare a macro directly? I have not actually seen it
> happen so I am a bit skeptical. I am tagging along some people who
> will be able to weigh in a solid opinion regarding this.

Yes it is okay, a `#define SOMETHING WHATEVER` will just inform the c preprocessor to replace the `SOMETHING` by `WHATEVER`. The only thing the final c compiler will see is `WHATEVER`. In our case a integer value.

Goal here was to avoid magic number, but after looking to the code it seem accepted that true is 1 and false is 0. To comply with that, in next version it will be replace it with:

	if (FOREACH_BOOL_FILTER_NOT_SET != info->active_only) {
		is_active = is_submodule_active(the_repository, path);
		if (is_active != info->active_only)
			return;
	}

Show 13 quoted lines

> 
>> It need two additionnal function to parse the argument:
>> static int parse_active(const char *arg)
>> {
>>          int active = git_parse_maybe_bool(arg);
>>
>>          if (active < 0)
>>                  die(_("invalid --active option: %s"), arg);
>>
>>          return active;
>> }
> 
> Alright, this one is used for parsing out the active submodules right

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

Show 16 quoted lines

>> And the option OPT_BOOL become a OPT_CALLBACK_F:
>> OPT_CALLBACK_F(0, "active", &info.active, "true|false",
>>          N_("Call command depending on submodule active state"),
>>          PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>>          parse_opt_active_cb),
>>
>> The help git_submodule_helper_usage:
>> N_("git submodule--helper foreach [--quiet] [--recursive]
>> [--active[=true|false]] [--] <command>"),
> 
> What I have inferred right now is that we introduce the `--active`
> option which will take a T/F value depending on user input. We have 3
> macros to check for the value of `active`, but I don't understand the
> significance of changing the FOREACH_CB_INIT macro to accomodate the
> third option. And we use a function to parse out the active
> submodules.

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

Show 7 quoted lines

> Instead of the return statement you wrote, won't it be better to call
> parse_active() depending on the case? Meaning that we call
> parse_active() when `active=true`.
> 
> Regards,
> Shourya Shukla
>

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

Regards,

Guillaume