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