Re: [PATCHv9 4/6] git submodule update: have a dedicated helper for cloning
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Feb 10, 2016, 00:37 UTC
- Message-ID
- <20160210003741.GJ28749@google.com>
- In-Reply-To
- <1455051274-15256-5-git-send-email-sbeller@google.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.
[...]
Show 5 quoted lines
> 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.
[...]
Show 9 quoted lines
> +++ 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.
Show 12 quoted lines
> + /* 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.
[...]
Show 8 quoted lines
> +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, ...).
[...]
Show 9 quoted lines
> + /*
> + * 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