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

Stefan Beller [off-list ref] writes:

On Thu, Feb 4, 2016 at 7:29 PM, Junio C Hamano [off-list ref] wrote:

quoted

Stefan Beller [off-list ref] writes:
...

quoted

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

quoted

quoted

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

quoted

quoted

+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

quoted

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