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

[Eric Sunshine <sunshine@sunshineco.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:
> 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?

>  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.

> +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?

> +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.

>                         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