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

[Junio C Hamano]

Prathamesh Chavan  writes:

> 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 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 39 ++-
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cdae54426..20a1ef868 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -14,6 +14,11 @@
>  #include "refs.h"
>  #include "connect.h"
>  
> +#define CB_OPT_QUIET (1<<0)

Is the purpose of this bit to make the callback quiet?  I do not
think so.  Is there a reason why we cannot call it just OPT_QUIET or
something instead?

When the set of functions that pay attention to these flags include
both ones that are callable for a single submodule and ones meant as
callbacks for for-each interface, having to flip bit whose name
screams "CallBack!" in a caller of a single-short version feels very
wrong.

"make style" tells me to format the above like so:

#define OPT_QUIET (1 << 0)

and I think I agree.

> @@ -349,7 +354,22 @@ 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);
> +}

Good.

> +struct init_cb {

I take it is a short-hand for "submodule init callback"?  As long as
the name stays inside this file, I think we are OK.

> + const char *prefix;
> + unsigned int cb_flags;

Call this just "flags"; call-back ness is plenty clear from the fact
that it lives in a structure meant as a callback interface already.

> +};

Blank line here?

> +#define INIT_CB_INIT { NULL, 0 }
> +
> +static void init_submodule(const char *path, const char *prefix,
> +unsigned int cb_flags)

Call this also "flags"; a direct caller of this function that wants
to initialize a single submodule without going thru the for-each
callback interface would not be passing "callback flags"--they are
just passing a set of flags.

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

[Prathamesh Chavan]

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 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 39 ++-
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index cdae54426..20a1ef868 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -14,6 +14,11 @@
 #include "refs.h"
 #include "connect.h"
 
+#define CB_OPT_QUIET   (1<<0)
+
+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
+ void *cb_data);
+
 static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
@@ -349,7 +354,22 @@ 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 cb_flags;
+};
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const char *path, const char *prefix,
+  unsigned int cb_flags)
 {
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
@@ -411,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 (!(cb_flags & CB_OPT_QUIET))
fprintf(stderr,
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
@@ -438,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->cb_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(, N_("Suppress output for initializing a 
submodule")),
@@ -468,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();
 
-   for (i = 0; i < list.nr; i++)
-   init_submodule(list.entries[i]->name, prefix, quiet);
+   info.prefix = prefix;
+   if (quiet)
+   info.cb_flags |= CB_OPT_QUIET;
+
+   for_each_listed_submodule(, init_submodule_cb, );
 
return 0;
 }
-- 
2.13.0