From: Junio C Hamano <gitster@pobox•com>
To: Stefan Beller <sbeller@google•com>
Cc: "git\@vger.kernel.org" <git@vger•kernel.org>,
Jonathan Nieder <jrnieder@gmail•com>,
Jens Lehmann <Jens.Lehmann@web•de>
Subject: Re: [PATCHv8 6/9] fetching submodules: respect `submodule.fetchJobs` config option
Date: Fri, 05 Feb 2016 11:59:08 -0800 [thread overview]
Message-ID: <xmqqegcrc4j7.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAGZ79kaamt=7mLv23JyJiWL9cJ8deV4JWH_Kv0Csovvqs5SBcg@mail.gmail.com> (Stefan Beller's message of "Fri, 5 Feb 2016 10:50:36 -0800")
Stefan Beller <sbeller@google•com> writes:
> 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.
>>> @@ -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 ;-)
next prev parent reply other threads:[~2016-02-05 19:59 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-04 22:09 [PATCHv8 0/9] Expose submodule parallelism to the user Stefan Beller
2016-02-04 22:09 ` [PATCHv8 1/9] submodule-config: keep update strategy around Stefan Beller
2016-02-05 0:59 ` Jonathan Nieder
2016-02-05 20:25 ` Stefan Beller
2016-02-05 20:33 ` Jonathan Nieder
2016-02-05 20:36 ` Stefan Beller
2016-02-04 22:09 ` [PATCHv8 2/9] submodule-config: drop check against NULL Stefan Beller
2016-02-05 1:05 ` Jonathan Nieder
2016-02-04 22:09 ` [PATCHv8 3/9] submodule-config: remove name_and_item_from_var Stefan Beller
2016-02-06 0:46 ` Jonathan Nieder
2016-02-06 1:21 ` Stefan Beller
2016-02-04 22:09 ` [PATCHv8 4/9] submodule-config: slightly simplify lookup_or_create_by_name Stefan Beller
2016-02-06 0:48 ` Jonathan Nieder
2016-02-04 22:09 ` [PATCHv8 5/9] submodule-config: introduce parse_generic_submodule_config Stefan Beller
2016-02-06 1:23 ` Jonathan Nieder
2016-02-06 1:36 ` Stefan Beller
2016-02-04 22:09 ` [PATCHv8 6/9] fetching submodules: respect `submodule.fetchJobs` config option Stefan Beller
2016-02-05 3:29 ` Junio C Hamano
2016-02-05 18:50 ` Stefan Beller
2016-02-05 19:59 ` Junio C Hamano [this message]
2016-02-04 22:09 ` [PATCHv8 7/9] git submodule update: have a dedicated helper for cloning Stefan Beller
2016-02-04 22:09 ` [PATCHv8 8/9] submodule update: expose parallelism to the user Stefan Beller
2016-02-04 22:09 ` [PATCHv8 9/9] clone: allow an explicit argument for parallel submodule clones Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqegcrc4j7.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Jens.Lehmann@web$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox