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

From: Stefan Beller <sbeller@google.com>
Date: Jan 21, 2016, 01:44 UTC
Message-ID: <CAGZ79kYJJ+JxgLBqD1Y_UWACUd7yJGnU+rnwEnU6X041deftjQ@mail.gmail.com>
In-Reply-To: <xmqqr3hc3pmz.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 20, 2016 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 37 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> 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.

So you mean something like `autoinit` would maybe already suffice. Which leads to another question if we want to extend the concept of these submodule groups a little bit, such that we also allow direct names in there, in .git/config we may have the configuration

    [submodule]
        groups = <groupname-as-annotated-in-.gitmodules>
        groups = <explicit-submodule>

such that `git clone --group=default --group=mySingleSubmodule ....` would work.

Of course then the --group option would need to be named differently in git clone and probably the submodule.groups should also be named differently.

However: At this point in time we only care about auto-initing submodules to get submodules somewhat easier to handle when having lots of them. Maybe we also want to add other features to these "groups" concept, e.g. all submodules of one groups should have the "(force-)checkout" update strategy. If the submodule consists of binaries only, this would make lots of sense to me.

So it is not yet clear to me if we want to extend the grouping feature later on for other things, which is why I named it by its concept. A group can be used for different purposes, where as "all submodules having the same auto-init-tag can be treated the same using one update strategy" just adds to user confusion, hence I'd think telling the user about groups is the right thing to do?

Show 10 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"?

In that part of the code it makes sense to rename it to auto_init, I'd guess.