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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2017, 00:55 UTC
Message-ID: <xmqq8tgus6zw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170929094453.4499-3-pc44800@gmail.com>

Prathamesh Chavan <pc44800@gmail.com> writes:

Show 22 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.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@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.

Show 12 quoted lines

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