Stefan Beller <sbeller@xxxxxxxxxx> writes:
> On Thu, Feb 4, 2016 at 7:29 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Stefan Beller <sbeller@xxxxxxxxxx> 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.
>>> @@ -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, ¶llel_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...
>>> +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 ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html