Re: [PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 6, 2017, 21:56 UTC
Message-ID: <CAPig+cT31XM9nW7sytukbQQ_O_15np6oepazKJaoNuHey+kiBA@mail.gmail.com>
In-Reply-To: <20171006132415.2876-3-pc44800@gmail.com>

Same disclaimer as in my review of patch 1/3: I didn't see a URL in the cover letter pointing at discussions of earlier iterations, so below comments may be at odds with what went on previously...

On Fri, Oct 6, 2017 at 9:24 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:

Show 17 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.
>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -14,6 +14,11 @@
>  #include "refs.h"
>  #include "connect.h"
>
> +#define OPT_QUIET (1 << 0)
> +
> +typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
> +                                 void *cb_data);

What is the reason for having the definition of 'each_submodule_fn' so far removed textually from its first reference by for_each_listed_submodule() below?

Show 15 quoted lines

>  static char *get_default_remote(void)
>  {
>         char *dest = NULL, *ret;
> @@ -348,7 +353,23 @@ 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);
> +}

I'm very curious about the justification for introducing a for-each function for what amounts to the simplest sort of loop possible: a canonical for-loop with a one-line body. I could easily understand the desire for such a function if either the loop conditions or the body of the loop, or both, were complex, but this does not seem to be the case. Even the callers of this new function, in this patch and in 3/3, are as simple as possible: one-liners (simple function calls).

Although this sort of for-each function can, at times, be helpful, there are costs: extra boilerplate and increased complexity for clients since it requires callback functions and (optionally) callback data. The separation of logic into a callback function can make code more difficult to reason about than when it is simply the body of a for-loop.

So, unless the plan for the future is that this for-each function will have considerable additional functionality baked into it, I'm having a difficult time understanding why this change is desirable.

Show 6 quoted lines

> +struct init_cb {
> +       const char *prefix;
> +       unsigned int flags;
> +};
> +
> +#define INIT_CB_INIT { NULL, 0 }

Why are these definitions so far removed from init_submodule_cb() below?

Show 11 quoted lines

> +static void init_submodule(const char *path, const char *prefix,
> +                          unsigned int flags)
>  {
>         const struct submodule *sub;
>         struct strbuf sb = STRBUF_INIT;
> @@ -410,7 +431,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 (!(flags & OPT_QUIET))

This change of having init_submodule() accept a 'flags' argument, rather than a single boolean, increases reviewer burden, since the reviewer is forced to puzzle out how this change relates to the stated intention of the patch since it is not mentioned at all by the commit message.

It's also conceptually unrelated to the introduction of a for-each function, thus should be instead be done by a separate preparatory patch.

Show 39 quoted lines

>                         fprintf(stderr,
>                                 _("Submodule '%s' (%s) registered for path '%s'\n"),
>                                 sub->name, url, displaypath);
> @@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>         free(upd);
>  }
>
> +static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
> +{
> +       struct init_cb *info = cb_data;
> +       init_submodule(list_item->name, info->prefix, info->flags);
> +}
> +
>  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")),
> @@ -467,8 +494,11 @@ 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;
> +       if (quiet)
> +               info.flags |= OPT_QUIET;
> +
> +       for_each_listed_submodule(&list, init_submodule_cb, &info);
>
>         return 0;
>  }
> --
> 2.14.2