Re: [PATCHv9 3/6] fetching submodules: respect `submodule.fetchJobs` config option

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:

> +++ b/submodule.c
[...]
> @@ -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;

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

[...]
> @@ -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?

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

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