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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html