Prathamesh Chavan <pc44...@gmail.com> 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 <christian.cou...@gmail.com>
> Mentored-by: Stefan Beller <sbel...@google.com>
> Signed-off-by: Prathamesh Chavan <pc44...@gmail.com>
> ---
> 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.
