Re: [PATCH 3/4] submodule update: Initialize all group-selected submodules by default

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2016, 21:30 UTC
Message-ID: <xmqqr3hc3pmz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1453260880-628-4-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 19 quoted lines

> All submodules which are selected via submodule groups
> ("submodule.groups") are initialized if they were not initialized
> before updating the submodules.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 30 +++++++++++++++++++++++++++++-
>  t/t7400-submodule-basic.sh  | 26 ++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4684f16..def382e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -577,6 +577,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  
>  struct submodule_update_clone {
>  	/* states */
> +	struct string_list *submodule_groups;

Again, do we need a field named submodule_groups in a struct whose name already makes it clear this is about submodule?

More importantly.

If this were used to implement "there are various groups defined, and the user tells us that submodules in this and that groups are to be automatically initialized", then naming the field with a name that is more specific than just "groups" makes tons of sense, but even in such a case, the best adjective to clarify what kind of "group" this field is about is not "this is a submodule group".

The answer I would give to a question "what kind of group this field is about?" would be "this is an auto-init group", so I'd have that 'auto-init' ness somewhere in its name.

Show 5 quoted lines

> @@ -633,6 +634,7 @@ static int update_clone_get_next_task(struct child_process *cp,
>  		const char *update_module = NULL;
>  		char *url = NULL;
>  		int needs_cloning = 0;
> +		int in_submodule_groups = 0;

Likewise. The significance of the membership in this group is totally unclear from this variable name. I read this boolean to be keeping track of "is this submodule to be auto initialized"?