Re: [PATCHv8 6/9] fetching submodules: respect `submodule.fetchJobs` config option

From

Stefan Beller <sbeller@google.com>

Date

Feb 5, 2016, 18:50 UTC

Message-ID

<CAGZ79kaamt=7mLv23JyJiWL9cJ8deV4JWH_Kv0Csovvqs5SBcg@mail.gmail.com>

In-Reply-To

<xmqqa8nfeswj.fsf@gitster.mtv.corp.google.com>

On Thu, Feb 4, 2016 at 7:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 586840d..5aa1c2d 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
>>  static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
>>  static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>>  static int tags = TAGS_DEFAULT, unshallow, update_shallow;
>> -static int max_children = 1;
>> +static int max_children = -1;
>
> This makes sense as you would later be doing "If left unspecified,
> i.e. -1, then fall back to blah" ...

>
> g> diff --git a/submodule-config.c b/submodule-config.c
>> index 2841c5e..339b59d 100644
>> --- a/submodule-config.c
>> +++ b/submodule-config.c
>> @@ -32,6 +32,7 @@ enum lookup_type {
>>
>>  static struct submodule_cache cache;
>>  static int is_cache_init;
>> +static unsigned long parallel_jobs = -1;
>
> ... but I do not think this does.  For one thing, you would not be
> doing "If parallel_jobs < -1, then that is unspecified yet" for the
> unsigned long variable, and for another, I do not think you would be
> behaving differently for the first time (i.e. "unspecified yet" case).
>
> I think you want to initialize this to 1, if your "not configured at
> all" default is supposed to be 1.

Please read on in the patch, such that you find

@@ -751,6 +751,11 @@ int fetch_populated_submodules(
(This is where the max_children from builtin/fetch is passed into,
named max_parallel_jobs now)

+       if (max_parallel_jobs < 0)
+               max_parallel_jobs = config_parallel_submodules();
+       if (max_parallel_jobs < 0)
+               max_parallel_jobs = 1;
+

So if we don't get the config option from builtin/fetch, we ask for
config_parallel_submodules(), which is what you were seeing above
initialized as -1, too.  And if that is still -1, we default to 1 there.

So from a design perspective, you're rather saying we want to move part of
this logic into submodule-config.c, such that
config_parallel_submodules never returns -1,
but either 1 as the default or the actual configuration?
Then the code in fetch_populated_submodules, would be as simple as

    if (max_parallel_jobs < 0)
        max_parallel_jobs = config_parallel_submodules();
    // done , as config_parallel_submodules(); gives the last
authoritive answer.

Well looking at submodule-config.c this might be easier, too.

Ok.

>
>> @@ -233,6 +234,13 @@ static int parse_generic_submodule_config(const char *key,
>>                                         const char *value,
>>                                         struct parse_config_parameter *me)
>>  {
>> +     if (!strcmp(key, "fetchjobs")) {
>> +             if (!git_parse_ulong(value, &parallel_jobs)) {
>> +                     warning(_("Error parsing submodule.fetchJobs; Defaulting to 1."));
>> +                     parallel_jobs = 1;
>
> Hmph, this is not a die() because...?  Not a rhetorical question.

Because this config option doesn't alter the state of the repository.
After the fact you should not be able to tell how much parallel operations
were going on.

(As opposed to other options which alter the state of the repository)

I'll change it to die(...), though it sounds overly strict to me in this case.
But I guess consistency beats overstrictness here.

>
>> +unsigned long config_parallel_submodules(void)
>> +{
>> +     return parallel_jobs;
>> +}
>
> It is not a crime to make this return "int", as the code that
> eventually uses it will assign it to a variable that will be
> passed to run_processes_parallel() that takes an "int".

ok

>
> In fact, returning "int" would be preferrable.  You are causing
> truncation somewhere in the callchain anyway.  If the truncation
> bothers you, this function or immediately after calling
> git_parse_ulong() would be a good place to do a range check.  The
> variable parallel_jobs has to stay "unsigned long" in any case as
> you are calling git_parse_ulong().

ok, that should be the best place.

>
>> diff --git a/submodule.c b/submodule.c
>> index b83939c..eb7d54b 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -751,6 +751,11 @@ int fetch_populated_submodules(const struct argv_array *options,
>>       argv_array_push(&spf.args, "--recurse-submodules-default");
>>       /* default value, "--submodule-prefix" and its value are added later */
>>
>> +     if (max_parallel_jobs < 0)
>> +             max_parallel_jobs = config_parallel_submodules();
>> +     if (max_parallel_jobs < 0)
>> +             max_parallel_jobs = 1;
>> +
>>       calculate_changed_submodule_paths();
>>       run_processes_parallel(max_parallel_jobs,
>>                              get_next_submodule,