Re: [PATCH v5 2/4] 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 | 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.

> 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(_oid, path);
> + sub = submodule_from_path(_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(, "submodule.%s.active", sub->name);
>   git_config_set_gently(sb.buf, "true");
>   strbuf_reset();
> @@ -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(, 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();
>  
> - 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(, init_submodule, );
>  
>   return 0;
>  }

[PATCH v5 2/4] 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 | 36 
 1 file changed, 28 insertions(+), 8 deletions(-)

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(_oid, path);
+   sub = submodule_from_path(_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(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
strbuf_reset();
@@ -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(, 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();
 
-   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(, init_submodule, );
 
return 0;
 }
-- 
2.13.0