Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Feb 9, 2016, 22:34 UTC
- Message-ID
- <20160209223400.GI28749@google.com>
- In-Reply-To
- <1455051274-15256-4-git-send-email-sbeller@google.com>
Hi,
Stefan Beller wrote:
> +++ b/submodule.c
[...]
Show 9 quoted lines
> @@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
>
> int submodule_config(const char *var, const char *value, void *cb)
> {
> - if (starts_with(var, "submodule."))
> + if (!strcmp(var, "submodule.fetchjobs")) {
> + unsigned long val;
> + if (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)
> + die(_("Error parsing submodule.fetchJobs %s"), value);'val < 0' would be more idiomatic than '0 > val'. More importantly, val is an unsigned long. How could it be negative?
Is it intended that val == INT_MAX is not permitted? I would have expected something like the following to work:
unsigned long val = git_config_ulong(var, value);
if (val > INT_MAX) {
errno = ERANGE;
die_bad_number(var, value);
}(using die_bad_number from config.c). Or config.c could gain a git_config_nonnegative_int helper:
static int git_parse_nonnegative_int(const char *value, int *ret)
{
uintmax_t tmp;
if (!git_parse_unsigned(value, &tmp, maximum_signed_value_of_type(int)))
return 0;
*ret = tmp;
return 1;
} int git_config_nonnegative_int(const char *name, const char *value)
{
int ret;
if (!git_parse_nonnegative_int(value, &ret))
die_bad_number(name, value);
return ret;
}allowing
parallel_jobs = git_config_nonnegative_int(var, val); return 0;
[...]
Show 6 quoted lines
> @@ -751,6 +758,9 @@ 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 = parallel_jobs;
Makes sense.
[...]
Show 9 quoted lines
> @@ -1097,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
> strbuf_release(&rel_path);
> free((void *)real_work_tree);
> }
> +
> +int parallel_submodules(void)
> +{
> + return parallel_jobs;
> +}Is this helper used?
[...]
Show 6 quoted lines
> +++ b/submodule.h > @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam > struct string_list *needs_pushing); > int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name); > void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); > +int parallel_submodules(void);
optional trick: one way to avoid merge conflicts is to add each function at some logical place in the file instead of at the end. Another nice side-effect is that it makes it easier to read through the header since functions appear in some appropriate order.
E.g. this method could go near the config functions, I suppose.
Show 7 quoted lines
> --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -471,4 +471,18 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea > test_i18ncmp expect.err actual.err > ' > > +test_expect_success 'fetching submodules respects parallel settings' '
Makes sense. Same trick about inserting in some appropriate place in the middle of the file applies here, too. In tests it also ends up being useful for finding when tests overlap or when there's a gap in coverage.
The documentation says that '0' does something appropriate (DWIM or something? I didn't understand it). Perhaps a test for that behavior would be useful, too.
Thanks, Jonathan