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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2016, 19:59 UTC
Message-ID: <xmqqegcrc4j7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kaamt=7mLv23JyJiWL9cJ8deV4JWH_Kv0Csovvqs5SBcg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

> On Thu, Feb 4, 2016 at 7:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>> ...
>>> +static unsigned long parallel_jobs = -1;
>>
>> ... but I do not think this does....
>
> 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.

But that is not really -1, but -1 casted to unsigned long that is casted back to int.

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

That is not my design but yours ;-)

Expecting config_parallel_submodules() to return -1 when there is no variable defined contradicts what you wrote in the documentation, I think, where it says "this variable defaults to 1 when not set".

> Then the code in fetch_populated_submodules, would be as simple as
>
>     if (max_parallel_jobs < 0)
>         max_parallel_jobs = config_parallel_submodules();

Yup.

Show 19 quoted lines

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

I do not see it as being overly strict, though.

If I were a user of this feature, I'd rather see a problem pointed out to me (e.g "you spelled 'true' but that fetchJobs expects a positive integer") to help me fix it, rather than having to see this warning every time I try to run submodule commands.

What benefit does a user get by being loose here? Probably I am not considering some valid use cases...

Show 20 quoted lines

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

Alternatively (I haven't weighed pros and cons myself), you can make parallel_jobs an "int", call git_parse_ulong() using a temporary "unsigned long" and after doing range check assign it to parallel_jobs. That would make this function really trivial ;-)