Re: [PATCH v5 2/4] submodule--helper: introduce for_each_listed_submodule()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 25, 2017, 03:43 UTC
- Message-ID
- <xmqqmv5jfnod.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170924120858.26813-3-pc44800@gmail.com>
Prathamesh Chavan <pc44800@gmail.com> writes:
Show 12 quoted lines
> Introduce function for_each_listed_submodule() and replace a loop > in module_init() with a call to it. > > The new function will also be used in other parts of the > system in later patches. > > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: Stefan Beller <sbeller@google.com> > Signed-off-by: Prathamesh Chavan <pc44800@gmail.com> > --- > builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-)
This looks more or less perfectly done, but I say this basing it on an assumption that nobody ever would want to initialize a single submodule without going through module_init() interface.
If there will be a caller that would have made a direct call to init_submodule() if this patch did not exist, then I think this patch is better done by keeping the external interface to the init_submodule() function intact, and introducing an intermediate helper init_submodule_cb() function that is to be used as the callback used in for_each_listed_submodule() API. In general, we should make sure that these callback functions that take "void *cb_data" parameters are called _only_ by these iterators that defined the callback (in this case, for_each_listed_submodule()) and not other functions.
Show 91 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d24ac9028..d12790b5c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -14,6 +14,9 @@
> #include "refs.h"
> #include "connect.h"
>
> +typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
> + void *cb_data);
> +
> static char *get_default_remote(void)
> {
> char *dest = NULL, *ret;
> @@ -352,15 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> -static void init_submodule(const char *path, const char *prefix, int quiet)
> +static void for_each_listed_submodule(const struct module_list *list,
> + each_submodule_fn fn, void *cb_data)
> +{
> + int i;
> + for (i = 0; i < list->nr; i++)
> + fn(list->entries[i], cb_data);
> +}
> +
> +struct init_cb {
> + const char *prefix;
> + unsigned int quiet: 1;
> +};
> +#define INIT_CB_INIT { NULL, 0 }
> +
> +static void init_submodule(const struct cache_entry *list_item, void *cb_data)
> {
> + struct init_cb *info = cb_data;
> const struct submodule *sub;
> struct strbuf sb = STRBUF_INIT;
> char *upd = NULL, *url = NULL, *displaypath;
>
> - displaypath = get_submodule_displaypath(path, prefix);
> + displaypath = get_submodule_displaypath(list_item->name, info->prefix);
>
> - sub = submodule_from_path(&null_oid, path);
> + sub = submodule_from_path(&null_oid, list_item->name);
>
> if (!sub)
> die(_("No url found for submodule path '%s' in .gitmodules"),
> @@ -372,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
> *
> * Set active flag for the submodule being initialized
> */
> - if (!is_submodule_active(the_repository, path)) {
> + if (!is_submodule_active(the_repository, list_item->name)) {
> strbuf_addf(&sb, "submodule.%s.active", sub->name);
> git_config_set_gently(sb.buf, "true");
> strbuf_reset(&sb);
> @@ -414,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
> if (git_config_set_gently(sb.buf, url))
> die(_("Failed to register url for submodule path '%s'"),
> displaypath);
> - if (!quiet)
> + if (!info->quiet)
> fprintf(stderr,
> _("Submodule '%s' (%s) registered for path '%s'\n"),
> sub->name, url, displaypath);
> @@ -443,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>
> static int module_init(int argc, const char **argv, const char *prefix)
> {
> + struct init_cb info = INIT_CB_INIT;
> struct pathspec pathspec;
> struct module_list list = MODULE_LIST_INIT;
> int quiet = 0;
> - int i;
>
> struct option module_init_options[] = {
> OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
> @@ -471,8 +489,10 @@ static int module_init(int argc, const char **argv, const char *prefix)
> if (!argc && git_config_get_value_multi("submodule.active"))
> module_list_active(&list);
>
> - for (i = 0; i < list.nr; i++)
> - init_submodule(list.entries[i]->name, prefix, quiet);
> + info.prefix = prefix;
> + info.quiet = !!quiet;
> +
> + for_each_listed_submodule(&list, init_submodule, &info);
>
> return 0;
> }