Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning

[Jonathan Nieder <jrnieder@gmail.com>]

Stefan Beller wrote:

> This introduces a new helper function in git submodule--helper
> which takes care of cloning all submodules, which we want to
> parallelize eventually.

Neat.

[...]
> As we can only access the stderr channel from within the parallel
> processing engine, we need to reroute the error message for
> specified but initialized submodules to stderr. As it is an error
> message, this should have gone to stderr in the first place, so it
> is a bug fix along the way.

In principle this could have happened in a separate preparatory patch
(so that the move to C would have no functional effect) but I don't
think that benefit alone is worth the churn of going back and doing
that.

[...]
> +++ b/builtin/submodule--helper.c
> @@ -255,6 +255,235 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +static int git_submodule_config(const char *var, const char *value, void *cb)
> +{
> +	return submodule_config(var, value, cb);
> +}

Can callers use submodule_config directly?


> +struct submodule_update_clone {
> +	/* states */
> +	int count;
> +	int print_unmatched;

I'd add a blank line here.

> +	/* configuration */
> +	int quiet;
> +	const char *reference;
> +	const char *depth;
> +	const char *update;
> +	const char *recursive_prefix;
> +	const char *prefix;
> +	struct module_list list;
> +	struct string_list projectlines;
> +	struct pathspec pathspec;
> +};
> +#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}

What is this struct used for?  Maybe this would be clearer if it
appeared immediately before the function that used it.

This is the shared cb object passed to run_processes_parallel, right?
Some name like parallel_clone_opts or parallel_clone_cb might work.

What do the fields represent?  E.g., what is count a count of, what
does it mean when print_unmatched is true, etc?

Would it make sense to put the options in a separate struct from the
state fields (so e.g. it could be const)?  The options are easier to
understand because they correspond to command-line options, while the
state fields are something different and internal.

[...]
> +static int update_clone_get_next_task(struct child_process *cp,
> +				      struct strbuf *err,
> +				      void *pp_cb,
> +				      void **pp_task_cb)
> +{
> +	struct submodule_update_clone *pp = pp_cb;
> +
> +	for (; pp->count < pp->list.nr; pp->count++) {

Could some of this loop body be factored out into separate functions?
(e.g. whether to skip a submodule, getting the display path, ...).

[...]
> +		/*
> +		 * Looking up the url in .git/config.
> +		 * We must not fall back to .gitmodules as we only want
> +		 * to process configured submodules.
> +		 */
> +		strbuf_reset(&sb);
> +		strbuf_addf(&sb, "submodule.%s.url", sub->name);
> +		git_config_get_string(sb.buf, &url);
> +		if (!url) {

I can see how the submodule API would be overkill for this.  But it's
still tempting to use it.  'struct submodule' could gain a field
representing whether the submodule is initialized (i.e., whether it
appears in .git/config).

I wonder whether the strbuf_reset + addf idiom would be a good thing
to factor out into a mkpath()-like function --- i.e., something like

		git_config_get_string(fmt(&sb, "submodule.%s.url", sub->name), &url);

That's a little less risky than mkpath was because it explicitly
mentions &sb but maybe it's too magical.

[...]
> +static int update_clone_start_failure(struct child_process *cp,

Will review the rest when I get home.

Thanks,
Jonathan