[PATCH 00/15] submodule groups (once again)

[PATCH 00/15] submodule groups (once again)

[Stefan Beller]

New in this series: git status, git diff and all remaining git submodule subcommands.

One pain point I am still aware of:
`git diff` and `git status` completely ignore submodules which are not in the
default group. I am not sure if that is a reasonable default.

A poor analogy could be the .gitignore file configuration:
If an entry exists in .gitignore, the corresponding file is ignored if
(and only if) the file is not part of the repository, i.e changes to 
a tracked (but ignored) file are still shown. Another way of saying it:
The ignore mechanism doesn't influence the diff machinery.

git diff is supposed to view the differences between "what would I
get after checkout" (i.e. what is in the index run through smudge filters)
compared to the actual worktree.
With the submodule default group set, we would expect to not see some
submodules checked out. But if such a submodule is in the worktree,
we may want to show a message instead:

    $ git status
    ... # normal git status stuff
        More than 2 submodules (123 actually) including 
            'path/foo'
            'lib/baz'
            # have a reasonable maximum for the number of submodules shown
        are checked out, but not part of the default group.
        You can add these submodules via
            git config --add submodule.defaultGroup ./path/foo
            git config --add submodule.defaultGroup ./lib/baz

Once we have such a message, we would need to train `git checkout` to checkout
and drop the right submodules for switching branches.

It has been a while since last posting this series and it is substantially
different in scope (and I have rewritten most of the patches), so I'll not
provide an intra-diff or a version number for this series.

What is this series about?
==========================

If you have lots of submodules, you probably don't need all of them at once, 
but you have functional units. Some submodules are absolutely required, 
some are optional and only for very specific purposes. 

This patch series adds labels to submodules in the .gitmodules file. 

So you could have a .gitmodules file such as: 

[submodule "gcc"] 
        path = gcc 
        url = git://... 
        label = default
        label = devel 
[submodule "linux"] 
        path = linux 
        url = git://... 
        label = default 
[submodule "nethack"] 
        path = nethack 
        url = git://... 
        label = optional
        label = games 

and by this series you can work on an arbitrary group of these submodules
composed by the labels, names or paths of the submodules.

    git clone --recurse-submodules --init-submodule=label --init-submodule=label2   git://... 
    # will clone the superproject and recursively 
    # checkout any submodule being labeled label or label2
    
    git submodule add --label <name> git://... ..
    # record a label while adding a submodule
    
    git config submodule.defaultGroups default
    git config --add submodule.defaultGroups devel
    # configure which submodules you are interested in.
    
    git submodule update
    # update only the submodules in the default group if that is configured.
    
    git status
    git diff
    git submodule summary
    # show only changes to submodules which are in the default group.

Any feedback welcome, specially on the design level! 
(Do we want to have it stored in the .gitmodules file? Do we want to have 
the groups configured in .git/config as "submodule.groups", any other way 
to make it future proof and extend the groups syntax?) 

Thanks, 
Stefan 

Stefan Beller (15):
  string_list: add string_list_duplicate
  submodule doc: write down what we want to achieve in this series
  submodule add: label submodules if asked to
  submodule-config: keep labels around
  submodule-config: check if submodule a submodule is in a group
  submodule init: redirect stdout to stderr
  submodule deinit: loose requirement for giving '.'
  submodule--helper list: respect submodule groups
  submodule--helper init: respect submodule groups
  submodule--helper update_clone: respect submodule groups
  diff: ignore submodules excluded by groups
  git submodule summary respects groups
  cmd_status: respect submodule groups
  cmd_diff: respect submodule groups
  clone: allow specification of submodules to be cloned

 Documentation/config.txt        |   4 +
 Documentation/git-clone.txt     |   6 +
 Documentation/git-submodule.txt |  13 +-
 builtin/clone.c                 |  40 +++++-
 builtin/commit.c                |   3 +
 builtin/diff.c                  |   2 +
 builtin/submodule--helper.c     |  94 ++++++++++++-
 diff.c                          |   3 +
 diff.h                          |   1 +
 git-submodule.sh                |  24 +++-
 string-list.c                   |  18 +++
 string-list.h                   |   2 +
 submodule-config.c              |  66 ++++++++++
 submodule-config.h              |   5 +
 t/t7400-submodule-basic.sh      | 129 +++++++++++++++++-
 t/t7406-submodule-update.sh     |  24 +++-
 t/t7413-submodule--helper.sh    | 285 ++++++++++++++++++++++++++++++++++++++++
 wt-status.c                     |   2 +
 wt-status.h                     |   1 +
 19 files changed, 701 insertions(+), 21 deletions(-)
 create mode 100755 t/t7413-submodule--helper.sh

-- 
2.8.0.41.g8d9aeb3

[PATCH 01/15] string_list: add string_list_duplicate

[Stefan Beller]

The result of git_config_get_value_multi do not seem to be stable and
may get overwritten. Have an easy way to preserve the results of that
query.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 string-list.c | 18 ++++++++++++++++++
 string-list.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/string-list.c b/string-list.c
index 2a32a3f..f981c8a 100644
--- a/string-list.c
+++ b/string-list.c
@@ -7,6 +7,24 @@ void string_list_init(struct string_list *list, int strdup_strings)
 	list->strdup_strings = strdup_strings;
 }
 
+struct string_list *string_list_duplicate(const struct string_list *src,
+					  int strdup_strings)
+{
+	struct string_list *list;
+	struct string_list_item *item;
+
+	if (!src)
+		return NULL;
+
+	list = xmalloc(sizeof(*list));
+	string_list_init(list, strdup_strings);
+	for_each_string_list_item(item, src)
+		string_list_append(list, item->string);
+
+	return list;
+}
+
+
 /* if there is no exact match, point to the index where the entry could be
  * inserted */
 static int get_entry_index(const struct string_list *list, const char *string,
diff --git a/string-list.h b/string-list.h
index d3809a1..1a5612f 100644
--- a/string-list.h
+++ b/string-list.h
@@ -19,6 +19,8 @@ struct string_list {
 #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
 
 void string_list_init(struct string_list *list, int strdup_strings);
+struct string_list *string_list_duplicate(const struct string_list *src,
+					  int strdup_strings);
 
 void print_string_list(const struct string_list *p, const char *text);
 void string_list_clear(struct string_list *list, int free_util);
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 01/15] string_list: add string_list_duplicate

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The result of git_config_get_value_multi do not seem to be stable and
> may get overwritten. Have an easy way to preserve the results of that
> query.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

This morning I reviewed a patch that adds something whose name ends
with _copy(), and this may want to follow suit.

Should strdup_strings() be a separate parameter, or should it follow
what src->strdup_strings has?

"Do not seem to be stable" does not build confidence in the code,
making it sound as if the developer is basing his work on guess not
analysis, and makes it hard to tell if this is a wrong workaround to
a valid issue (e.g. it could be "making the result stable" is what
we want in the longer term) or a valid solution to a problem that
would be common across callers of that API.

>  string-list.c | 18 ++++++++++++++++++
>  string-list.h |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/string-list.c b/string-list.c
> index 2a32a3f..f981c8a 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -7,6 +7,24 @@ void string_list_init(struct string_list *list, int strdup_strings)
>  	list->strdup_strings = strdup_strings;
>  }
>  
> +struct string_list *string_list_duplicate(const struct string_list *src,
> +					  int strdup_strings)
> +{
> +	struct string_list *list;
> +	struct string_list_item *item;
> +
> +	if (!src)
> +		return NULL;
> +
> +	list = xmalloc(sizeof(*list));
> +	string_list_init(list, strdup_strings);
> +	for_each_string_list_item(item, src)
> +		string_list_append(list, item->string);
> +
> +	return list;
> +}
> +
> +
>  /* if there is no exact match, point to the index where the entry could be
>   * inserted */
>  static int get_entry_index(const struct string_list *list, const char *string,
> diff --git a/string-list.h b/string-list.h
> index d3809a1..1a5612f 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -19,6 +19,8 @@ struct string_list {
>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>  
>  void string_list_init(struct string_list *list, int strdup_strings);
> +struct string_list *string_list_duplicate(const struct string_list *src,
> +					  int strdup_strings);
>  
>  void print_string_list(const struct string_list *p, const char *text);
>  void string_list_clear(struct string_list *list, int free_util);

Re: [PATCH 01/15] string_list: add string_list_duplicate

[Stefan Beller]

On Tue, Apr 26, 2016 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The result of git_config_get_value_multi do not seem to be stable and
>> may get overwritten. Have an easy way to preserve the results of that
>> query.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> This morning I reviewed a patch that adds something whose name ends
> with _copy(), and this may want to follow suit.

ok, in case we need this patch, I'll rename.

>
> Should strdup_strings() be a separate parameter, or should it follow
> what src->strdup_strings has?
>
> "Do not seem to be stable" does not build confidence in the code,
> making it sound as if the developer is basing his work on guess not
> analysis, and makes it hard to tell if this is a wrong workaround to
> a valid issue (e.g. it could be "making the result stable" is what
> we want in the longer term) or a valid solution to a problem that
> would be common across callers of that API.

When not duplicating git_config_get_value_multi("submodule.defaultGroup");
I run into

Program received signal SIGSEGV, Segmentation fault.
0x0000000000579097 in get_entry_index (list=0x876848, string=0x8721e0
"./submodule1", exact_match=0x7fffffffd6bc) at string-list.c:38
38 int compare = cmp(string, list->items[middle].string);
(gdb) bt
#0  0x0000000000579097 in get_entry_index (list=0x876848,
string=0x8721e0 "./submodule1", exact_match=0x7fffffffd6bc) at
string-list.c:38
#1  0x00000000005792d5 in string_list_has_string (list=0x876848,
string=0x8721e0 "./submodule1") at string-list.c:91
#2  0x000000000057e78c in submodule_in_group (group=0x876848,
sub=0x878510) at submodule-config.c:558
#3  0x0000000000497cf9 in module_init (argc=0, argv=0x7fffffffdb28,
prefix=0x0) at builtin/submodule--helper.c:441
#4  0x00000000004993f6 in cmd_submodule__helper (argc=2,
argv=0x7fffffffdb20, prefix=0x0) at builtin/submodule--helper.c:927
#5  0x000000000040582e in run_builtin (p=0x83c0c0 <commands+2400>,
argc=2, argv=0x7fffffffdb20) at git.c:353
#6  0x0000000000405a1d in handle_builtin (argc=2, argv=0x7fffffffdb20)
at git.c:540
#7  0x0000000000405b3f in run_argv (argcp=0x7fffffffd9fc,
argv=0x7fffffffda10) at git.c:594
#8  0x0000000000405d32 in main (argc=2, av=0x7fffffffdb18) at git.c:701
(gdb) print list->items[middle].string
Cannot access memory at address 0x746c006fd40bd151

So the string list seems to be corrupted here.
Someone stomping over our memory? How long is the result
of git_config_get_value_multi deemed to be stable and usable?

I did not find out myself how to properly answer these.
So it was symptom based programming (copying that stuff makes the
error go away).

This only happens in one code path, which is
        group = NULL;
        if (!pathspec.nr)
                group = //string_list_duplicate(
                        (struct string_list*)

git_config_get_value_multi("submodule.defaultGroup");//, 1);
        if (group) {
                gitmodules_config();
                for (i = 0; i < list.nr; i++) {
                        const struct submodule *sub =
                                submodule_from_path(null_sha1,
                                                    list.entries[i]->name);
                        if (submodule_in_group(group, sub))
                                init_submodule(list.entries[i]->name,
prefix, quiet);
                }
        }
        ... // group is not further used

Maybe gitmodules_config needs to be called before git_config_get_value_multi,
as it changes that?

I dunno, will debug more later.

Thanks,
Stefan




>
>>  string-list.c | 18 ++++++++++++++++++
>>  string-list.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/string-list.c b/string-list.c
>> index 2a32a3f..f981c8a 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -7,6 +7,24 @@ void string_list_init(struct string_list *list, int strdup_strings)
>>       list->strdup_strings = strdup_strings;
>>  }
>>
>> +struct string_list *string_list_duplicate(const struct string_list *src,
>> +                                       int strdup_strings)
>> +{
>> +     struct string_list *list;
>> +     struct string_list_item *item;
>> +
>> +     if (!src)
>> +             return NULL;
>> +
>> +     list = xmalloc(sizeof(*list));
>> +     string_list_init(list, strdup_strings);
>> +     for_each_string_list_item(item, src)
>> +             string_list_append(list, item->string);
>> +
>> +     return list;
>> +}
>> +
>> +
>>  /* if there is no exact match, point to the index where the entry could be
>>   * inserted */
>>  static int get_entry_index(const struct string_list *list, const char *string,
>> diff --git a/string-list.h b/string-list.h
>> index d3809a1..1a5612f 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -19,6 +19,8 @@ struct string_list {
>>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>>
>>  void string_list_init(struct string_list *list, int strdup_strings);
>> +struct string_list *string_list_duplicate(const struct string_list *src,
>> +                                       int strdup_strings);
>>
>>  void print_string_list(const struct string_list *p, const char *text);
>>  void string_list_clear(struct string_list *list, int free_util);

Re: [PATCH 01/15] string_list: add string_list_duplicate

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When not duplicating git_config_get_value_multi("submodule.defaultGroup");
> I run into
>
> Program received signal SIGSEGV, Segmentation fault.
> ...
> So the string list seems to be corrupted here.
> Someone stomping over our memory? How long is the result
> of git_config_get_value_multi deemed to be stable and usable?

That call goes to

    git_config_get_value_multi()
    -> git_configset_get_value_multi()
       -> configset_find_element()

the returned value from there would be either NULL or the list of
values that belong to the config cache layer, i.e. a caller of the
API can peek but is not allowed to modify it.

So if you are modifying the value you obtain from the API, you must
make a copy; otherwise you will "stomp over" memory that belongs to
the config cache layer, not to you.

Re: [PATCH 01/15] string_list: add string_list_duplicate

[Stefan Beller]

On Wed, Apr 27, 2016 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> When not duplicating git_config_get_value_multi("submodule.defaultGroup");
>> I run into
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> ...
>> So the string list seems to be corrupted here.
>> Someone stomping over our memory? How long is the result
>> of git_config_get_value_multi deemed to be stable and usable?
>
> That call goes to
>
>     git_config_get_value_multi()
>     -> git_configset_get_value_multi()
>        -> configset_find_element()
>
> the returned value from there would be either NULL or the list of
> values that belong to the config cache layer, i.e. a caller of the
> API can peek but is not allowed to modify it.
>
> So if you are modifying the value you obtain from the API, you must
> make a copy; otherwise you will "stomp over" memory that belongs to
> the config cache layer, not to you.

Yes, we do not modify the string_list (the return of git_config_get_value_multi)

Another way to corrupt it is to change the configuration (e.g. add
things to the config hashmap such that it reallocates and grows).

The problem arises after a call to submodule_from_path(...);
which may change the cache for submodules. I assumed that was
completely different from the regular config cache, but somehow there is
a relation, which I have not tracked down yet.

Thanks,
Stefan

Re: [PATCH 01/15] string_list: add string_list_duplicate

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Another way to corrupt it is to change the configuration (e.g. add
> things to the config hashmap such that it reallocates and grows).

You're right.  But doesn't it hint that there is a deeper problem?

By making a copy and keeping it, you would hold onto a stale value
and would not see the result of updates you yourself make to the
system.

Re: [PATCH 01/15] string_list: add string_list_duplicate

[Stefan Beller]

On Wed, Apr 27, 2016 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Another way to corrupt it is to change the configuration (e.g. add
>> things to the config hashmap such that it reallocates and grows).
>
> You're right.  But doesn't it hint that there is a deeper problem?
>
> By making a copy and keeping it, you would hold onto a stale value
> and would not see the result of updates you yourself make to the
> system.
>

In this case the value doesn't go stale. We do not change
"submodule.defaultGroup", but only new submodule.$NAME.url
and such. The memory for accessing it goes stale, so in this case
it is okay. I don't think we want to see repeated calls to
git_config_get_value_multi
like :

        if (!pathspec.nr && git_config_get_value_multi(
                        "submodule.defaultGroup")) {
                gitmodules_config();
                for (i = 0; i < list.nr; i++) {
                        const struct submodule *sub =
                                submodule_from_path(null_sha1,
                                                    list.entries[i]->name);
                        group =
git_config_get_value_multi("submodule.defaultGroup")
                        if (submodule_in_group(group, sub))
                                init_submodule(list.entries[i]->name,
prefix, quiet);
                }
        }

Maybe I am overestimating the cost of git_config_get_value_multi, so it is no
problem?

Thanks,
Stefan

[PATCH 02/15] submodule doc: write down what we want to achieve in this series

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt        | 4 ++++
 Documentation/git-submodule.txt | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 59d7046..c5b6a4e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2735,6 +2735,10 @@ submodule.fetchJobs::
 	in parallel. A value of 0 will give some reasonable default.
 	If unset, it defaults to 1.
 
+submodule.defaultGroup::
+	Specifies the group of submodules to be operated on
+	in a command if no submodules were specified in the command.
+
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
 	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 13adebf..8c4bbe2 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -37,6 +37,14 @@ these will not be checked out by default; the 'init' and 'update'
 subcommands will maintain submodules checked out and at
 appropriate revision in your working tree.
 
+When operating on submodules you can either give paths to specify the
+desired submodules or give no paths at all to apply the command to the
+default group of submodules. By default all submodules are included in
+the default group. You can change the default group by configuring
+submodule.defaultGroup. Once the default group is configured any
+submodule operation without a specified set of submodules will use
+the default group as the set to operate on.
+
 Submodules are composed from a so-called `gitlink` tree entry
 in the main repository that refers to a particular commit object
 within the inner repository that is completely separate.
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 02/15] submodule doc: write down what we want to achieve in this series

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +When operating on submodules you can either give paths to specify the
> +desired submodules or give no paths at all to apply the command to the
> +default group of submodules.

So, "git submodule foo path1 path2" would act on path1 and path2 but
would omit path3.  If you have path1 and path3 but not path2 in the
default group, and do not give any path, i.e. "git submodule foo",
it would act on path1 and path3 but would omit path2.

OK so far.

> +By default all submodules are included in
> +the default group.

So if you do not do anything special, i.e. do not define any group,
"git submodule foo" would act on path1, path2 and path3.

I think that is in line with the way how module_list aka
"submodule--helper list" works.

> +You can change the default group by configuring
> +submodule.defaultGroup. Once the default group is configured any
> +submodule operation without a specified set of submodules will use
> +the default group as the set to operate on.
> +
>  Submodules are composed from a so-called `gitlink` tree entry
>  in the main repository that refers to a particular commit object
>  within the inner repository that is completely separate.

[PATCH 03/15] submodule add: label submodules if asked to

[Stefan Beller]

When adding new submodules, you can specify the
label(s) the submodule belongs to by giving one or more
--label arguments. This will record each label in the
.gitmodules file as a value of the key
"submodule.$NAME.label".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt |  5 ++++-
 git-submodule.sh                | 14 +++++++++++++-
 t/t7400-submodule-basic.sh      | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 8c4bbe2..349ead8 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 --------
 [verse]
-'git submodule' [--quiet] add [-b <branch>] [-f|--force] [--name <name>]
+'git submodule' [--quiet] add [-b <branch>] [-f|--force] [-l|--label <label>]
 	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
@@ -109,6 +109,9 @@ is the superproject and submodule repositories will be kept
 together in the same relative location, and only the
 superproject's URL needs to be provided: git-submodule will correctly
 locate the submodule using the relative URL in .gitmodules.
++
+If at least one label argument was given, all labels are recorded in the
+.gitmodules file in the label fields.
 
 status::
 	Show the status of the submodules. This will print the SHA-1 of the
diff --git a/git-submodule.sh b/git-submodule.sh
index 82e95a9..d7a5e1a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,7 +5,7 @@
 # Copyright (c) 2007 Lars Hjemli
 
 dashless=$(basename "$0" | sed -e 's/-/ /')
-USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
+USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [-l|--label <label>][--] <repository> [<path>]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
    or: $dashless [--quiet] deinit [-f|--force] [--] <path>...
@@ -130,6 +130,7 @@ cmd_add()
 {
 	# parse $args after "submodule ... add".
 	reference_path=
+	labels=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -165,6 +166,13 @@ cmd_add()
 		--depth=*)
 			depth=$1
 			;;
+		-l|--label)
+			labels="${labels} $2"
+			shift
+			;;
+		--label=*)
+			labels="${labels} ${1#--label=}"
+			;;
 		--)
 			shift
 			break
@@ -292,6 +300,10 @@ Use -f if you really want to add it." >&2
 
 	git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
 	git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+	for label in $labels
+	do
+		git config --add -f .gitmodules submodule."$sm_name".label "${label}"
+	done &&
 	if test -n "$branch"
 	then
 		git config -f .gitmodules submodule."$sm_name".branch "$branch"
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f99f674..e9d1d58 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1040,6 +1040,7 @@ test_expect_success 'submodule with UTF-8 name' '
 '
 
 test_expect_success 'submodule add clone shallow submodule' '
+	test_when_finished "rm -rf super" &&
 	mkdir super &&
 	pwd=$(pwd) &&
 	(
@@ -1078,5 +1079,36 @@ test_expect_success 'submodule helper list is not confused by common prefixes' '
 	test_cmp expect actual
 '
 
+test_expect_success 'submodule add records a label' '
+	test_when_finished "rm -rf super" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label labelA file://"$pwd"/example2 submodule &&
+		git config -f .gitmodules submodule."submodule".label >actual &&
+		echo labelA >expected &&
+		test_cmp expected actual
+	)
+'
+
+cat >expected <<-EOF
+labelA
+labelB
+EOF
+
+test_expect_success 'submodule add records multiple labels' '
+	test_when_finished "rm -rf super" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label=labelA -l labelB file://"$pwd"/example2 submodule &&
+		git config --get-all -f .gitmodules submodule."submodule".label >../actual
+	) &&
+	test_cmp expected actual
+'
 
 test_done
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 03/15] submodule add: label submodules if asked to

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When adding new submodules, you can specify the
> label(s) the submodule belongs to by giving one or more
> --label arguments. This will record each label in the
> .gitmodules file as a value of the key
> "submodule.$NAME.label".
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git-submodule.txt |  5 ++++-
>  git-submodule.sh                | 14 +++++++++++++-
>  t/t7400-submodule-basic.sh      | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 8c4bbe2..349ead8 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
>  SYNOPSIS
>  --------
>  [verse]
> -'git submodule' [--quiet] add [-b <branch>] [-f|--force] [--name <name>]
> +'git submodule' [--quiet] add [-b <branch>] [-f|--force] [-l|--label <label>]
>  	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
>  'git submodule' [--quiet] init [--] [<path>...]
> @@ -109,6 +109,9 @@ is the superproject and submodule repositories will be kept
>  together in the same relative location, and only the
>  superproject's URL needs to be provided: git-submodule will correctly
>  locate the submodule using the relative URL in .gitmodules.
> ++
> +If at least one label argument was given, all labels are recorded in the
> +.gitmodules file in the label fields.

I think this is better without "If ... given,".

I am not sure if it is sensible to make "label" namespace always
global to be shared with the project by updating .gitmodules,
though (it can cut both ways, so this is not an objection).

> @@ -165,6 +166,13 @@ cmd_add()
>  		--depth=*)
>  			depth=$1
>  			;;
> +		-l|--label)
> +			labels="${labels} $2"
> +			shift
> +			;;
> +		--label=*)
> +			labels="${labels} ${1#--label=}"
> +			;;
>  		--)
>  			shift
>  			break
> @@ -292,6 +300,10 @@ Use -f if you really want to add it." >&2
>  
>  	git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
>  	git config -f .gitmodules submodule."$sm_name".url "$repo" &&
> +	for label in $labels
> +	do
> +		git config --add -f .gitmodules submodule."$sm_name".label "${label}"
> +	done &&

Is the acceptable syntax for "label" defined and documented
somewhere?

I didn't see it in the documentation patch.  I am seeing that we do
not allow $IFS whitespaces in them, but are there other restrictions
we want to enforce?  Remember, starting with narrow and widening as
we discover the need is the right way to design things.  Once we
start allowing random strings, it is very hard to later reject some
to carve out namespace for ourselves.

The above implementation happens to allow users to say

	git submodule add -l "labelA labelB" -- $there $path

and give two labels to the module, and that will be something you
end up needing to support forever, unless you restrict early.

Re: [PATCH 03/15] submodule add: label submodules if asked to

[Jacob Keller]

On Tue, Apr 26, 2016 at 1:50 PM, Stefan Beller <sbeller@google.com> wrote:
> When adding new submodules, you can specify the
> label(s) the submodule belongs to by giving one or more
> --label arguments. This will record each label in the
> .gitmodules file as a value of the key
> "submodule.$NAME.label".
>

Ok so labels will be in the .gitmodules file. This means that if we go
back in history using git bisect or something similar, we'll
potentially change what the default submodules are for example?

This is sort of why having some submodule data appear in the
.git/config file is useful since it helps keep things like the remote
url safe from being updated when doing this sort of thing.

I am not sure if labels will be that important in this case?

Thanks,
Jake

Re: [PATCH 03/15] submodule add: label submodules if asked to

[Stefan Beller]

On Tue, Apr 26, 2016 at 3:50 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Apr 26, 2016 at 1:50 PM, Stefan Beller <sbeller@google.com> wrote:
>> When adding new submodules, you can specify the
>> label(s) the submodule belongs to by giving one or more
>> --label arguments. This will record each label in the
>> .gitmodules file as a value of the key
>> "submodule.$NAME.label".
>>
>
> Ok so labels will be in the .gitmodules file. This means that if we go
> back in history using git bisect or something similar, we'll
> potentially change what the default submodules are for example?

Good point! Yes that's exactly what we want.

Consider this:
We have:
    libA
    libB

Now libB grows a lot and someone decides to split it up into
2 libraries, so we'll have:
    libA
    libB-small
    libC-was-part-of-B

As libB (the big one repo containing all the code) was no open source,
we can deprecate it fast and just keep developing in libB-small and
libC-was-part-of-B. As we do only small changes at a time, we'll keep
those 2 in the same label set, so the users can pick up the changes via

    git submodule update
    #  libB-small and  libC-was-part-of-B are in both the same group,
e.g. default
    # so they will be checked out.

When a bug is found in the future, you'd use `git bisect` to find it.
In a repository with no
submodules (analogy: think of splitting a file in two. Checking out
older revisions
will give the one old file, checking out newer revisions will give 2 new files.)
that works great. With submodules we want to have the same properties.

    git checkout tag-before-libB-split
    git submodule update
    # get libB checked out

    git checkout tag-after-libB-split
    git submodule update
    # get libB-small and libC-was-part-of-B checked out

Ideally (read: in a later patch series), checkout will automatically
checkout submodules for you if you have configured `submodule.updateOnCheckout`,
such that switching old and new revisions will add or delete
submodules automatically.

>
> This is sort of why having some submodule data appear in the
> .git/config file is useful since it helps keep things like the remote
> url safe from being updated when doing this sort of thing.

(Unrelated, but my thoughts on this)
The remote url mechanism is broken with the .gitmodules file in some use cases:
Consider there is an upstream "kernel.org" which hosts a repository with
submodules. Now you want to mirror that superproject to "kernel.mymirror.org"
and you start with

    git clone --mirror git://kernel.org/superproject

When the superproject uses relative URLs for the submodules, this will
break your mirror,
if you did not mirror them exactly with the same relative path. Then
cloning from
your mirror will result in broken submodule URLs.

So you would want an additional mechanism for URLs. Jonathan came up
with the idea of having another configuration file in a refs/submodules/config
branch which essentially allows to annotate/enhance the .gitmodules file.

So in such a configuration you could say:

    defaultRelativeURLBase = kernel.org
    [submodule."foo"]
        relativeURLBase = kernel.mymirror.org
        protocol = ssh
        # ssh as opposed to git:// which was inherited from the superproject

which allows finer controls of submodule/repository mirroring.
As the refs/submodule/configuration is not part of the history of the
superproject,
mirroring can be done without changing history, but still having URLs changed to
the internal mirror.

>
> I am not sure if labels will be that important in this case?

I am not sure. If it turns out to be a problem, maybe we can
introduce a hook, that will be called on adding new submodules via lables?

Thanks,
Stefan

>
> Thanks,
> Jake

Re: [PATCH 03/15] submodule add: label submodules if asked to

[Jacob Keller]

On Tue, Apr 26, 2016 at 4:19 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Apr 26, 2016 at 3:50 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Apr 26, 2016 at 1:50 PM, Stefan Beller <sbeller@google.com> wrote:
>>> When adding new submodules, you can specify the
>>> label(s) the submodule belongs to by giving one or more
>>> --label arguments. This will record each label in the
>>> .gitmodules file as a value of the key
>>> "submodule.$NAME.label".
>>>
>>
>> Ok so labels will be in the .gitmodules file. This means that if we go
>> back in history using git bisect or something similar, we'll
>> potentially change what the default submodules are for example?
>
> Good point! Yes that's exactly what we want.
>

Ok. That makes sense.

> Ideally (read: in a later patch series), checkout will automatically
> checkout submodules for you if you have configured `submodule.updateOnCheckout`,
> such that switching old and new revisions will add or delete
> submodules automatically.
>

This is something I definitely would like to be able to tell people.

>>
>> This is sort of why having some submodule data appear in the
>> .git/config file is useful since it helps keep things like the remote
>> url safe from being updated when doing this sort of thing.
>
> (Unrelated, but my thoughts on this)
> The remote url mechanism is broken with the .gitmodules file in some use cases:
> Consider there is an upstream "kernel.org" which hosts a repository with
> submodules. Now you want to mirror that superproject to "kernel.mymirror.org"
> and you start with
>
>     git clone --mirror git://kernel.org/superproject
>
> When the superproject uses relative URLs for the submodules, this will
> break your mirror,
> if you did not mirror them exactly with the same relative path. Then
> cloning from
> your mirror will result in broken submodule URLs.
>
> So you would want an additional mechanism for URLs. Jonathan came up
> with the idea of having another configuration file in a refs/submodules/config
> branch which essentially allows to annotate/enhance the .gitmodules file.
>

I like the idea of refs/submodules/config.

> So in such a configuration you could say:
>
>     defaultRelativeURLBase = kernel.org
>     [submodule."foo"]
>         relativeURLBase = kernel.mymirror.org
>         protocol = ssh
>         # ssh as opposed to git:// which was inherited from the superproject
>
> which allows finer controls of submodule/repository mirroring.
> As the refs/submodule/configuration is not part of the history of the
> superproject,
> mirroring can be done without changing history, but still having URLs changed to
> the internal mirror.
>
>>
>> I am not sure if labels will be that important in this case?
>
> I am not sure. If it turns out to be a problem, maybe we can
> introduce a hook, that will be called on adding new submodules via lables?
>
> Thanks,
> Stefan
>
>>
>> Thanks,
>> Jake

[PATCH 04/15] submodule-config: keep labels around

[Stefan Beller]

We need the submodule labels in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 16 ++++++++++++++++
 submodule-config.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index b82d1fb..0cdb47e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -60,6 +60,10 @@ static void free_one_config(struct submodule_entry *entry)
 	free((void *) entry->config->path);
 	free((void *) entry->config->name);
 	free((void *) entry->config->update_strategy.command);
+	if (entry->config->labels) {
+		string_list_clear(entry->config->labels, 0);
+		free(entry->config->labels);
+	}
 	free(entry->config);
 }
 
@@ -199,6 +203,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	submodule->update_strategy.command = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
+	submodule->labels = NULL;
 
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -353,6 +358,17 @@ static int parse_config(const char *var, const char *value, void *data)
 		else if (parse_submodule_update_strategy(value,
 			 &submodule->update_strategy) < 0)
 				die(_("invalid value for %s"), var);
+	} else if (!strcmp(item.buf, "label")) {
+		if (!value)
+			ret = config_error_nonbool(var);
+		else {
+			if (!submodule->labels) {
+				submodule->labels =
+					xmalloc(sizeof(*submodule->labels));
+				string_list_init(submodule->labels, 1);
+			}
+			string_list_insert(submodule->labels, value);
+		}
 	}
 
 	strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index e4857f5..d57da59 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -18,6 +18,8 @@ struct submodule {
 	struct submodule_update_strategy update_strategy;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
+	/* sorted, not as on disk */
+	struct string_list *labels;
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-- 
2.8.0.41.g8d9aeb3

[PATCH 05/15] submodule-config: check if submodule a submodule is in a group

[Stefan Beller]

In later patches we need to tell if a submodule is in a group,
which is defined by name, path or labels.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 43 ++++++++++++++++++++++-
 submodule-config.c           | 50 +++++++++++++++++++++++++++
 submodule-config.h           |  3 ++
 t/t7413-submodule--helper.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100755 t/t7413-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b6d4f27..23d7224 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -814,6 +814,46 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+int in_group(int argc, const char **argv, const char *prefix)
+{
+	const struct string_list *list;
+	struct string_list actual_list = STRING_LIST_INIT_DUP;
+	const struct submodule *sub;
+	const char *group = NULL;
+
+	struct option default_group_options[] = {
+		OPT_STRING('g', "group", &group, N_("group"),
+				N_("group specifier for submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper in-group <path>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, default_group_options,
+			     git_submodule_helper_usage, 0);
+
+	/* Overlay the parsed .gitmodules file with .git/config */
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	if (argc != 1)
+		usage(git_submodule_helper_usage[0]);
+
+	sub = submodule_from_path(null_sha1, argv[0]);
+
+	if (!group)
+		list = git_config_get_value_multi("submodule.defaultGroup");
+	else {
+		string_list_split(&actual_list, group, ',', -1);
+		list = &actual_list;
+	}
+
+	return !submodule_in_group(list, sub);
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -826,7 +866,8 @@ static struct cmd_struct commands[] = {
 	{"update-clone", update_clone},
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
-	{"init", module_init}
+	{"init", module_init},
+	{"in-group", in_group}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/submodule-config.c b/submodule-config.c
index 0cdb47e..ebed0f2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -522,3 +522,53 @@ void submodule_free(void)
 	cache_free(&cache);
 	is_cache_init = 0;
 }
+
+int submodule_in_group(const struct string_list *group,
+		       const struct submodule *sub)
+{
+	int matched = 0;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!group)
+		/*
+		 * If no group is specified all, all submodules match to
+		 * keep traditional behavior
+		 */
+		return 1;
+
+	if (sub->labels) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, sub->labels) {
+			strbuf_reset(&sb);
+			strbuf_addf(&sb, "*%s", item->string);
+			if (string_list_has_string(group, sb.buf)) {
+				matched = 1;
+				break;
+			}
+		}
+	}
+	if (sub->path) {
+		/*
+		 * NEEDSWORK: This currently works only for
+		 * exact paths, but we want to enable
+		 * inexact matches such wildcards.
+		 */
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "./%s", sub->path);
+		if (string_list_has_string(group, sb.buf))
+			matched = 1;
+	}
+	if (sub->name) {
+		/*
+		 * NEEDSWORK: Same as with path. Do we want to
+		 * support wildcards or such?
+		 */
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, ":%s", sub->name);
+		if (string_list_has_string(group, sb.buf))
+			matched = 1;
+	}
+	strbuf_release(&sb);
+
+	return matched;
+}
diff --git a/submodule-config.h b/submodule-config.h
index d57da59..4c696cc 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -31,4 +31,7 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
 		const char *path);
 void submodule_free(void);
 
+int submodule_in_group(const struct string_list *group,
+		       const struct submodule *sub);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
new file mode 100755
index 0000000..c6939ab
--- /dev/null
+++ b/t/t7413-submodule--helper.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+# This should be merged with t7412 eventually.
+# (currently in flight as jk/submodule-c-credential)
+
+
+test_description='Basic plumbing support of submodule--helper
+
+This test verifies the submodule--helper plumbing command used to implement
+git-submodule.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup superproject with submodules' '
+
+	mkdir sub &&
+	(
+		cd sub &&
+		git init &&
+		test_commit test
+		test_commit test2
+	) &&
+	mkdir super &&
+	(
+		cd super &&
+		git init &&
+		git submodule add ../sub sub0 &&
+		git submodule add -l bit1 ../sub sub1 &&
+		git submodule add -l bit2 ../sub sub2 &&
+		git submodule add -l bit2 -l bit1 ../sub sub3 &&
+		git commit -m "add labeled submodules"
+	)
+'
+
+test_expect_success 'in-group' '
+	(
+		cd super &&
+		# we do not specify a group nor have set a default group,
+		# any submodule should be in the default group:
+		git submodule--helper in-group sub0 &&
+		git submodule--helper in-group sub1 &&
+		git submodule--helper in-group sub2 &&
+		git submodule--helper in-group sub3 &&
+
+		# test bit1:
+		test_must_fail git submodule--helper in-group --group=*bit1 sub0 &&
+			       git submodule--helper in-group --group=*bit1 sub1 &&
+		test_must_fail git submodule--helper in-group --group=*bit1 sub2 &&
+			       git submodule--helper in-group --group=*bit1 sub3 &&
+
+		# test by path:
+			       git submodule--helper in-group --group=./sub0 sub0 &&
+		test_must_fail git submodule--helper in-group --group=./sub0 sub1 &&
+		test_must_fail git submodule--helper in-group --group=./sub0 sub2 &&
+		test_must_fail git submodule--helper in-group --group=./sub0 sub3 &&
+
+		# test by name:
+			       git submodule--helper in-group --group=:sub0 sub0 &&
+		test_must_fail git submodule--helper in-group --group=:sub0 sub1 &&
+		test_must_fail git submodule--helper in-group --group=:sub0 sub2 &&
+		test_must_fail git submodule--helper in-group --group=:sub0 sub3 &&
+
+		# logical OR of path and labels
+			       git submodule--helper in-group --group=*bit1,./sub0 sub0 &&
+			       git submodule--helper in-group --group=*bit1,./sub0 sub1 &&
+		test_must_fail git submodule--helper in-group --group=*bit1,./sub0 sub2 &&
+			       git submodule--helper in-group --group=*bit1,./sub0 sub3 &&
+
+		# test if the config option is picked up
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+
+			       git submodule--helper in-group sub0 &&
+			       git submodule--helper in-group sub1 &&
+		test_must_fail git submodule--helper in-group sub2 &&
+			       git submodule--helper in-group sub3
+	)
+'
+
+test_done
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 05/15] submodule-config: check if submodule a submodule is in a group

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b6d4f27..23d7224 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -814,6 +814,46 @@ static int update_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +int in_group(int argc, const char **argv, const char *prefix)

It is inconceivable that "submodule group" will be the only user of
the concept whose name is "group".  Please do not give such a
generic name to a helper function that is specific to "submodule
group" and make it global.  Naming a file-scope static helper
function as in_group() is perfectly fine; it is clear that such a
function in submodule--helper.c is about submodule group.

> +	if (!group)
> +		list = git_config_get_value_multi("submodule.defaultGroup");
> +	else {
> +		string_list_split(&actual_list, group, ',', -1);
> +		list = &actual_list;

Hmm, where did this syntax to use comma-separated things come from?
Did I miss it in 02/15?

> +	if (sub->labels) {
> +		struct string_list_item *item;
> +		for_each_string_list_item(item, sub->labels) {
> +			strbuf_reset(&sb);
> +			strbuf_addf(&sb, "*%s", item->string);
> +			if (string_list_has_string(group, sb.buf)) {
> +				matched = 1;
> +				break;
> +			}
> +		}
> +	}
> +	if (sub->path) {
> +		/*
> +		 * NEEDSWORK: This currently works only for
> +		 * exact paths, but we want to enable
> +		 * inexact matches such wildcards.
> +		 */
> +		strbuf_reset(&sb);
> +		strbuf_addf(&sb, "./%s", sub->path);
> +		if (string_list_has_string(group, sb.buf))
> +			matched = 1;
> +	}
> +	if (sub->name) {
> +		/*
> +		 * NEEDSWORK: Same as with path. Do we want to
> +		 * support wildcards or such?
> +		 */
> +		strbuf_reset(&sb);
> +		strbuf_addf(&sb, ":%s", sub->name);
> +		if (string_list_has_string(group, sb.buf))
> +			matched = 1;
> +	}
> +	strbuf_release(&sb);

I see room for bikeshedding here, but the material to bikeshed
around is not even documented yet ;-)

 * a token prefixed with '*' is a label.
 * a token prefixed with './' is a path.
 * a token prefixed with ':' is a name.

Hopefully I will see some description like that in later patches.
I'll read on.

Re: [PATCH 05/15] submodule-config: check if submodule a submodule is in a group

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I see room for bikeshedding here, but the material to bikeshed
> around is not even documented yet ;-)
>
>  * a token prefixed with '*' is a label.
>  * a token prefixed with './' is a path.
>  * a token prefixed with ':' is a name.
>
> Hopefully I will see some description like that in later patches.
> I'll read on.

Extending this on a bit, I would suggest tweaking the above slightly
and make the rule more like this:

  * a token prefixed with '*' is a label.

  * a token prefixed with ':' is a name.

  * everything else is a path, but "./" at the front is skipped,
    which can be used to disambiguate an unfortunate path that
    begins with ':' or '*'.

A bigger thing I am wondering is if it is bettter to do _without_
adding a new --group=X option everywhere.  I am assuming that most
if not all submodule subcommands already use "module_list" aka
"submodule--helper list" that takes paths, and to them, extending
that interface to also understand the groups and names would be a
more natural way to extend the UI, no?  e.g.

	$ git submodule update -- 'path1' 'path2'
        $ git submodule update -- '*default'
        $ git submodule update -- ':named'

instead of

	$ git submodule update -- 'path1 'path2'
        $ git submodule update --group='*default' --
        $ git submodule update --group=':named' --

which special-cases the way to specify a set of submodules by
listing their paths.

Re: [PATCH 05/15] submodule-config: check if submodule a submodule is in a group

[Stefan Beller]

On Tue, Apr 26, 2016 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I see room for bikeshedding here, but the material to bikeshed
>> around is not even documented yet ;-)
>>
>>  * a token prefixed with '*' is a label.
>>  * a token prefixed with './' is a path.
>>  * a token prefixed with ':' is a name.
>>
>> Hopefully I will see some description like that in later patches.
>> I'll read on.
>
> Extending this on a bit, I would suggest tweaking the above slightly
> and make the rule more like this:
>
>   * a token prefixed with '*' is a label.
>
>   * a token prefixed with ':' is a name.
>
>   * everything else is a path, but "./" at the front is skipped,
>     which can be used to disambiguate an unfortunate path that
>     begins with ':' or '*'.
>
> A bigger thing I am wondering is if it is bettter to do _without_
> adding a new --group=X option everywhere.  I am assuming that most
> if not all submodule subcommands already use "module_list" aka
> "submodule--helper list" that takes paths, and to them, extending
> that interface to also understand the groups and names would be a
> more natural way to extend the UI, no?  e.g.
>
>         $ git submodule update -- 'path1' 'path2'
>         $ git submodule update -- '*default'
>         $ git submodule update -- ':named'
>
> instead of
>
>         $ git submodule update -- 'path1 'path2'
>         $ git submodule update --group='*default' --
>         $ git submodule update --group=':named' --
>
> which special-cases the way to specify a set of submodules by
> listing their paths.

This is indeed a better way.

Currently there is no way to initialize another group as that group
specified by submodule.defaultGroup. But having the possibility
to use the grouping in such a way is more flexible.

Thanks,
Stefan

[PATCH 06/15] submodule init: redirect stdout to stderr

[Stefan Beller]

Reroute the output of stdout to stderr as it is just informative
messages, not to be consumed by machines.

We want to init submodules from the helper for `submodule update`
in a later patch and the stdout output of said helper is consumed
by the parts of `submodule update` which are still written in shell.
So we have to be careful which messages are on stdout.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c |  3 ++-
 t/t7406-submodule-update.sh | 24 ++++++++++++++++++------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 23d7224..7b9a4d7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -362,7 +362,8 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 			die(_("Failed to register url for submodule path '%s'"),
 			    displaypath);
 		if (!quiet)
-			printf(_("Submodule '%s' (%s) registered for path '%s'\n"),
+			fprintf(stderr,
+				_("Submodule '%s' (%s) registered for path '%s'\n"),
 				sub->name, url, displaypath);
 	}
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index fd741f5..5f27879 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -108,24 +108,36 @@ pwd=$(pwd)
 
 cat <<EOF >expect
 Submodule path '../super': checked out '$supersha1'
-Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
-Submodule 'none' ($pwd/none) registered for path '../super/none'
-Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
-Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
 Submodule path '../super/merging': checked out '$mergingsha1'
 Submodule path '../super/none': checked out '$nonesha1'
 Submodule path '../super/rebasing': checked out '$rebasingsha1'
 Submodule path '../super/submodule': checked out '$submodulesha1'
 EOF
 
+cat <<EOF >expect2
+Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
+Submodule 'none' ($pwd/none) registered for path '../super/none'
+Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
+Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
+Cloning into '$pwd/recursivesuper/super/merging'...
+done.
+Cloning into '$pwd/recursivesuper/super/none'...
+done.
+Cloning into '$pwd/recursivesuper/super/rebasing'...
+done.
+Cloning into '$pwd/recursivesuper/super/submodule'...
+done.
+EOF
+
 test_expect_success 'submodule update --init --recursive from subdirectory' '
 	git -C recursivesuper/super reset --hard HEAD^ &&
 	(cd recursivesuper &&
 	 mkdir tmp &&
 	 cd tmp &&
-	 git submodule update --init --recursive ../super >../../actual
+	 git submodule update --init --recursive ../super >../../actual 2>../../actual2
 	) &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_cmp expect2 actual2
 '
 
 apos="'";
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 06/15] submodule init: redirect stdout to stderr

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Reroute the output of stdout to stderr as it is just informative
> messages, not to be consumed by machines.
>
> We want to init submodules from the helper for `submodule update`
> in a later patch and the stdout output of said helper is consumed
> by the parts of `submodule update` which are still written in shell.
> So we have to be careful which messages are on stdout.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

I do not mind if this step is split out and queued as a separate
follow-up to sb/submodule-init.  The grouping and labelling is a
bigger and more important change that deserves attention without
distraction than this single step, and making as many such things
graduate and allowing us to forget about them is better ;-)

>  builtin/submodule--helper.c |  3 ++-
>  t/t7406-submodule-update.sh | 24 ++++++++++++++++++------
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 23d7224..7b9a4d7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -362,7 +362,8 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  			die(_("Failed to register url for submodule path '%s'"),
>  			    displaypath);
>  		if (!quiet)
> -			printf(_("Submodule '%s' (%s) registered for path '%s'\n"),
> +			fprintf(stderr,
> +				_("Submodule '%s' (%s) registered for path '%s'\n"),
>  				sub->name, url, displaypath);
>  	}
>  
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index fd741f5..5f27879 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -108,24 +108,36 @@ pwd=$(pwd)
>  
>  cat <<EOF >expect
>  Submodule path '../super': checked out '$supersha1'
> -Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
> -Submodule 'none' ($pwd/none) registered for path '../super/none'
> -Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
> -Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
>  Submodule path '../super/merging': checked out '$mergingsha1'
>  Submodule path '../super/none': checked out '$nonesha1'
>  Submodule path '../super/rebasing': checked out '$rebasingsha1'
>  Submodule path '../super/submodule': checked out '$submodulesha1'
>  EOF
>  
> +cat <<EOF >expect2
> +Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
> +Submodule 'none' ($pwd/none) registered for path '../super/none'
> +Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
> +Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
> +Cloning into '$pwd/recursivesuper/super/merging'...
> +done.
> +Cloning into '$pwd/recursivesuper/super/none'...
> +done.
> +Cloning into '$pwd/recursivesuper/super/rebasing'...
> +done.
> +Cloning into '$pwd/recursivesuper/super/submodule'...
> +done.
> +EOF
> +
>  test_expect_success 'submodule update --init --recursive from subdirectory' '
>  	git -C recursivesuper/super reset --hard HEAD^ &&
>  	(cd recursivesuper &&
>  	 mkdir tmp &&
>  	 cd tmp &&
> -	 git submodule update --init --recursive ../super >../../actual
> +	 git submodule update --init --recursive ../super >../../actual 2>../../actual2
>  	) &&
> -	test_cmp expect actual
> +	test_cmp expect actual &&
> +	test_cmp expect2 actual2
>  '
>  
>  apos="'";

Re: [PATCH 06/15] submodule init: redirect stdout to stderr

[Stefan Beller]

On Fri, Apr 29, 2016 at 11:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Reroute the output of stdout to stderr as it is just informative
>> messages, not to be consumed by machines.
>>
>> We want to init submodules from the helper for `submodule update`
>> in a later patch and the stdout output of said helper is consumed
>> by the parts of `submodule update` which are still written in shell.
>> So we have to be careful which messages are on stdout.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I do not mind if this step is split out and queued as a separate
> follow-up to sb/submodule-init.  The grouping and labelling is a
> bigger and more important change that deserves attention without
> distraction than this single step, and making as many such things
> graduate and allowing us to forget about them is better ;-)

Care to apply this onto the sb/submodule-init then?

(It applies cleanly for me on top of "submodule--helper update-clone:
abort gracefully on missing .gitmodules")

I'll drop this patch in the groups series.

Thanks,
Stefan

>
>>  builtin/submodule--helper.c |  3 ++-
>>  t/t7406-submodule-update.sh | 24 ++++++++++++++++++------
>>  2 files changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 23d7224..7b9a4d7 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -362,7 +362,8 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>>                       die(_("Failed to register url for submodule path '%s'"),
>>                           displaypath);
>>               if (!quiet)
>> -                     printf(_("Submodule '%s' (%s) registered for path '%s'\n"),
>> +                     fprintf(stderr,
>> +                             _("Submodule '%s' (%s) registered for path '%s'\n"),
>>                               sub->name, url, displaypath);
>>       }
>>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index fd741f5..5f27879 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -108,24 +108,36 @@ pwd=$(pwd)
>>
>>  cat <<EOF >expect
>>  Submodule path '../super': checked out '$supersha1'
>> -Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
>> -Submodule 'none' ($pwd/none) registered for path '../super/none'
>> -Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
>> -Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
>>  Submodule path '../super/merging': checked out '$mergingsha1'
>>  Submodule path '../super/none': checked out '$nonesha1'
>>  Submodule path '../super/rebasing': checked out '$rebasingsha1'
>>  Submodule path '../super/submodule': checked out '$submodulesha1'
>>  EOF
>>
>> +cat <<EOF >expect2
>> +Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
>> +Submodule 'none' ($pwd/none) registered for path '../super/none'
>> +Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
>> +Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
>> +Cloning into '$pwd/recursivesuper/super/merging'...
>> +done.
>> +Cloning into '$pwd/recursivesuper/super/none'...
>> +done.
>> +Cloning into '$pwd/recursivesuper/super/rebasing'...
>> +done.
>> +Cloning into '$pwd/recursivesuper/super/submodule'...
>> +done.
>> +EOF
>> +
>>  test_expect_success 'submodule update --init --recursive from subdirectory' '
>>       git -C recursivesuper/super reset --hard HEAD^ &&
>>       (cd recursivesuper &&
>>        mkdir tmp &&
>>        cd tmp &&
>> -      git submodule update --init --recursive ../super >../../actual
>> +      git submodule update --init --recursive ../super >../../actual 2>../../actual2
>>       ) &&
>> -     test_cmp expect actual
>> +     test_cmp expect actual &&
>> +     test_cmp expect2 actual2
>>  '
>>
>>  apos="'";

[PATCH 07/15] submodule deinit: loose requirement for giving '.'

[Stefan Beller]

This is needed later to make a distinction between 'all specified'
and the default group of submodules.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh           | 5 -----
 t/t7400-submodule-basic.sh | 1 -
 2 files changed, 6 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d7a5e1a..253ad07 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -440,11 +440,6 @@ cmd_deinit()
 		shift
 	done
 
-	if test $# = 0
-	then
-		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
-	fi
-
 	git submodule--helper list --prefix "$wt_prefix" "$@" |
 	while read mode sha1 stage sm_path
 	do
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e9d1d58..ac477b2 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -932,7 +932,6 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '
 	git submodule update --init &&
 	git config submodule.example.foo bar &&
 	git config submodule.example2.frotz nitfol &&
-	test_must_fail git submodule deinit &&
 	git submodule deinit . >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 07/15] submodule deinit: loose requirement for giving '.'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This is needed later to make a distinction between 'all specified'
> and the default group of submodules.

s/loose/lose/;

Again, this can be separated as an independent preliminary clean-up.


>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh           | 5 -----
>  t/t7400-submodule-basic.sh | 1 -
>  2 files changed, 6 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index d7a5e1a..253ad07 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -440,11 +440,6 @@ cmd_deinit()
>  		shift
>  	done
>  
> -	if test $# = 0
> -	then
> -		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> -	fi
> -
>  	git submodule--helper list --prefix "$wt_prefix" "$@" |
>  	while read mode sha1 stage sm_path
>  	do
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index e9d1d58..ac477b2 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -932,7 +932,6 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '
>  	git submodule update --init &&
>  	git config submodule.example.foo bar &&
>  	git config submodule.example2.frotz nitfol &&
> -	test_must_fail git submodule deinit &&
>  	git submodule deinit . >actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -z "$(git config --get-regexp "submodule\.example2\.")" &&

[PATCH 08/15] submodule--helper list: respect submodule groups

[Stefan Beller]

As submodule--helper list is the building block for some submodule
commands (foreach, deinit, status, sync), also add tests for those.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 13 ++++++
 t/t7413-submodule--helper.sh | 97 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7b9a4d7..adb6188 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -271,6 +271,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	int i;
 	struct pathspec pathspec;
 	struct module_list list = MODULE_LIST_INIT;
+	const struct string_list *group = NULL;
 
 	struct option module_list_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -292,9 +293,21 @@ static int module_list(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
+	if (!pathspec.nr)
+		group = git_config_get_value_multi("submodule.defaultGroup");
+	if (group)
+		gitmodules_config();
+
 	for (i = 0; i < list.nr; i++) {
 		const struct cache_entry *ce = list.entries[i];
 
+		if (group) {
+			const struct submodule *sub =
+				submodule_from_path(null_sha1, ce->name);
+			if (!submodule_in_group(group, sub))
+				continue;
+		}
+
 		if (ce_stage(ce))
 			printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1));
 		else
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index c6939ab..1b5d135 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -78,4 +78,101 @@ test_expect_success 'in-group' '
 	)
 '
 
+submodule_sha1=$(git -C sub rev-parse HEAD)
+
+cat >expect <<-EOF
+160000 $submodule_sha1 0	sub0
+160000 $submodule_sha1 0	sub1
+160000 $submodule_sha1 0	sub3
+EOF
+
+test_expect_success 'submodule--helper list respects groups' '
+	(
+		cd super &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule--helper list >../actual
+	) &&
+	test_cmp expect actual
+'
+
+cat >expect <<-EOF
+Entering 'sub0'
+$submodule_sha1 sub0
+Entering 'sub1'
+$submodule_sha1 sub1
+Entering 'sub3'
+$submodule_sha1 sub3
+EOF
+
+test_expect_success 'submodule foreach respects groups' '
+	(
+		cd super &&
+		git submodule foreach "echo \$sha1 \$name" >../actual
+	) &&
+	test_cmp expect actual
+'
+
+sub_priorsha1=$(git -C sub rev-parse HEAD^)
+
+cat >expect <<-EOF
++$sub_priorsha1 sub0 (test)
++$sub_priorsha1 sub1 (test)
++$sub_priorsha1 sub3 (test)
+EOF
+
+test_expect_success 'submodule status respects groups' '
+	git clone --recurse-submodules super super_clone &&
+	(
+		cd super_clone &&
+		git -C sub0 checkout HEAD^ &&
+		git -C sub1 checkout HEAD^ &&
+		git -C sub2 checkout HEAD^ &&
+		git -C sub3 checkout HEAD^ &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule status >../actual &&
+		git config --unset-all submodule.defaultGroup &&
+		git submodule update
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'submodule deinit respects groups' '
+	suburl=$(pwd)/sub &&
+	(
+		cd super_clone &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule deinit &&
+		test_must_fail git config submodule.sub0.url &&
+		test_must_fail git config submodule.sub1.url &&
+		test "$(git config submodule.sub2.url)" = "$suburl" &&
+		test_must_fail git config submodule.sub3.url &&
+		git config --unset-all submodule.defaultGroup &&
+		git submodule init
+	)
+'
+
+test_expect_success 'submodule sync respects groups' '
+	suburl=$(pwd)/sub &&
+	(
+		cd super_clone &&
+		git config submodule.sub0.url nonsense &&
+		git config submodule.sub1.url nonsense &&
+		git config submodule.sub2.url nonsense &&
+		git config submodule.sub3.url nonsense &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule sync &&
+		git config --unset-all submodule.defaultGroup &&
+		test "$(git config submodule.sub0.url)" = "$suburl" &&
+		test "$(git config submodule.sub1.url)" = "$suburl" &&
+		test "$(git config submodule.sub2.url)" = "nonsense" &&
+		test "$(git config submodule.sub3.url)" = "$suburl" &&
+		git submodule sync sub2 &&
+		test "$(git config submodule.sub2.url)" = "$suburl"
+	)
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 08/15] submodule--helper list: respect submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> As submodule--helper list is the building block for some submodule
> commands (foreach, deinit, status, sync), also add tests for those.

The title is slightly misleading, isn't it?

This step only teaches the commands to limit the operation to the
defaultGroup instead of operating on everybody.  There is no
mechanism (yet) to ask for arbitrary group(s) from the command line,
which was my expectation of "respecting submodule groups", i.e. "the
only way to limit the operation to subset of submodules used to be
by giving pathspecs to match, but now a more general submodule group
mechanism can be used" is yet to come at this point in the series.

[PATCH 09/15] submodule--helper init: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 19 +++++++++++++++++--
 t/t7413-submodule--helper.sh | 15 +++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index adb6188..29a345e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -405,6 +405,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 {
 	struct pathspec pathspec;
 	struct module_list list = MODULE_LIST_INIT;
+	struct string_list *group = NULL;
 	int quiet = 0;
 	int i;
 
@@ -427,8 +428,22 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
 
-	for (i = 0; i < list.nr; i++)
-		init_submodule(list.entries[i]->name, prefix, quiet);
+	if (!pathspec.nr)
+		group = string_list_duplicate(
+			git_config_get_value_multi("submodule.defaultGroup"), 1);
+	if (group) {
+		gitmodules_config();
+		for (i = 0; i < list.nr; i++) {
+			const struct submodule *sub =
+				submodule_from_path(null_sha1,
+						    list.entries[i]->name);
+			if (submodule_in_group(group, sub))
+				init_submodule(list.entries[i]->name, prefix, quiet);
+		}
+		string_list_clear(group, 1);
+	} else
+		for (i = 0; i < list.nr; i++)
+			init_submodule(list.entries[i]->name, prefix, quiet);
 
 	return 0;
 }
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index 1b5d135..ef12c63 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -175,4 +175,19 @@ test_expect_success 'submodule sync respects groups' '
 	)
 '
 
+test_expect_success 'submodule--helper init respects groups' '
+	(
+		cd super_clone &&
+		git submodule deinit . &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule init &&
+		git config --unset-all submodule.defaultGroup &&
+		test "$(git config submodule.sub0.url)" = "$suburl" &&
+		test "$(git config submodule.sub1.url)" = "$suburl" &&
+		test_must_fail git config submodule.sub2.url &&
+		test "$(git config submodule.sub3.url)" = "$suburl"
+	)
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

[PATCH 10/15] submodule--helper update_clone: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 16 ++++++++++++++++
 t/t7413-submodule--helper.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 29a345e..aa838c5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -610,6 +610,8 @@ struct submodule_update_clone {
 
 	/* Machine-readable status lines to be consumed by git-submodule.sh */
 	struct string_list projectlines;
+	/* The group specification we'll be processing. */
+	struct string_list *group;
 
 	/* If we want to stop as fast as possible and return an error */
 	unsigned quickstop : 1;
@@ -646,6 +648,9 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 
 	sub = submodule_from_path(null_sha1, ce->name);
 
+	if (!submodule_in_group(suc->group, sub))
+		goto cleanup;
+
 	if (suc->recursive_prefix)
 		displaypath = relative_path(suc->recursive_prefix,
 					    ce->name, &displaypath_sb);
@@ -771,6 +776,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	struct string_list_item *item;
 	struct pathspec pathspec;
 	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
+	struct string_list actual_group = STRING_LIST_INIT_DUP;
 
 	struct option module_update_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -810,6 +816,16 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)
 		return 1;
 
+	if (!pathspec.nr) {
+		const struct string_list *group =
+			group = git_config_get_value_multi("submodule.defaultGroup");
+		if (group) {
+			for_each_string_list_item(item, group)
+				string_list_append(&actual_group, item->string);
+			suc.group = &actual_group;
+		}
+	}
+
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index ef12c63..39e469f 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -190,4 +190,40 @@ test_expect_success 'submodule--helper init respects groups' '
 	)
 '
 
+cat >expect <<-EOF
+160000 $submodule_sha1 0 1	sub0
+160000 $submodule_sha1 0 1	sub1
+160000 $submodule_sha1 0 1	sub3
+EOF
+
+test_expect_success 'submodule--helper update-clone respects groups' '
+	(
+		cd super_clone &&
+		git submodule init &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule--helper update-clone >../actual &&
+		git config --unset-all submodule.defaultGroup
+	) &&
+	test_cmp expect actual
+'
+
+cat >expect <<-EOF
+Submodule path 'sub0': checked out '$submodule_sha1'
+Submodule path 'sub1': checked out '$submodule_sha1'
+Submodule path 'sub3': checked out '$submodule_sha1'
+EOF
+
+test_expect_success 'git submodule update respects groups' '
+	(
+		cd super_clone &&
+		git submodule deinit -f . &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule update --init >../actual &&
+		git config --unset-all submodule.defaultGroup
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

[PATCH 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

We do not need to do anything special to initialize the `submodule_groups`
pointer as the diff options setup will fill in 0 by default.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 3 +++
 diff.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/diff.c b/diff.c
index 059123c..5808d8a 100644
--- a/diff.c
+++ b/diff.c
@@ -4921,10 +4921,13 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
 	int ignored = 0;
 	unsigned orig_flags = options->flags;
+	const struct submodule *sub = submodule_from_path(null_sha1, path);
 	if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
 		set_diffopt_flags_from_submodule_config(options, path);
 	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
 		ignored = 1;
+	if (!submodule_in_group(options->submodule_groups, sub))
+		ignored = 1;
 	options->flags = orig_flags;
 	return ignored;
 }
diff --git a/diff.h b/diff.h
index e7d68ed..7d499fb 100644
--- a/diff.h
+++ b/diff.h
@@ -178,6 +178,7 @@ struct diff_options {
 	void *output_prefix_data;
 
 	int diff_path_counter;
+	struct string_list *submodule_groups;
 };
 
 enum color_diff {
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> We do not need to do anything special to initialize the `submodule_groups`
> pointer as the diff options setup will fill in 0 by default.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  diff.c | 3 +++
>  diff.h | 1 +
>  2 files changed, 4 insertions(+)

Isn't this going in the opposite way from what you described in 0/15
with analogy to how "ignore" mechanism works?  Just like a path is
tracked once it is tracked, whether it matches an ignore pattern,
shouldn't we be getting a summary for a submodule for any submodule
once submodule/.git/HEAD is there (i.e. we can give a comparison),
whether it is specified by a separate mechanism that acts from
sideways (e.g. the "default group").

> diff --git a/diff.c b/diff.c
> index 059123c..5808d8a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4921,10 +4921,13 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)
>  {
>  	int ignored = 0;
>  	unsigned orig_flags = options->flags;
> +	const struct submodule *sub = submodule_from_path(null_sha1, path);
>  	if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
>  		set_diffopt_flags_from_submodule_config(options, path);
>  	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
>  		ignored = 1;
> +	if (!submodule_in_group(options->submodule_groups, sub))
> +		ignored = 1;
>  	options->flags = orig_flags;
>  	return ignored;
>  }
> diff --git a/diff.h b/diff.h
> index e7d68ed..7d499fb 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -178,6 +178,7 @@ struct diff_options {
>  	void *output_prefix_data;
>  
>  	int diff_path_counter;
> +	struct string_list *submodule_groups;
>  };
>  
>  enum color_diff {

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

On Fri, Apr 29, 2016 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> We do not need to do anything special to initialize the `submodule_groups`
>> pointer as the diff options setup will fill in 0 by default.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  diff.c | 3 +++
>>  diff.h | 1 +
>>  2 files changed, 4 insertions(+)
>
> Isn't this going in the opposite way from what you described in 0/15
> with analogy to how "ignore" mechanism works?  Just like a path is
> tracked once it is tracked, whether it matches an ignore pattern,
> shouldn't we be getting a summary for a submodule for any submodule
> once submodule/.git/HEAD is there (i.e. we can give a comparison),
> whether it is specified by a separate mechanism that acts from
> sideways (e.g. the "default group").

That is why I started in 0/15 with
> One pain point I am still aware of:
as I did not do the right thing in these patches?
These patches do however:

>    git status
>    git diff
>    git submodule summary
>    # show only changes to submodules which are in the default group.

which seems to be the wrong thing then.

So here is a thought experiment:

    # get all submodules into the work tree
    git submodule update --recursive --init

    # The selected default group will not include all submodules
    git config submodule.defaultGroup "*SomeLabel"

    git status
    # What do we expect now?
    # either a "nothing to commit, working directory clean"
    # or rather what was described in 0/15:

        More than 2 submodules (123 actually) including
            'path/foo'
            'lib/baz'
        are checked out, but not part of the default group.
        You can add these submodules via
            git config --add submodule.defaultGroup ./path/foo
            git config --add submodule.defaultGroup ./lib/baz

If we want to go with the second option, the design described in 0/15
is broken. Going one step further:

    # in all submodules including the excluded via groups,
    # by resetting the groups for this command
    git -c submodule.defaultGroup= submodule foreach git reset --hard HEAD^

    git status
    # Reporting the changes from submodules in the default Group is
uncontroversial:
    On branch master
    Your branch is up-to-date with 'origin/master'.
    Changes not staged for commit:
      (use "git add <file>..." to update what will be committed)
      (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   subA (new commits)
        modified:   subB (new commits)

    no changes added to commit (use "git add" and/or "git commit -a")

    # But what about subC which is not in the default group? It was
changed as well.
    # one thing I could imagine, is similar to above:

        More than 2 submodules (123 actually) including
            modified:    'subC'  (new commits)
            modified:   'lib/baz' (new commits)
        are checked out, but not part of the default group.
        You can add these submodules via
            git config --add submodule.defaultGroup ./path/foo
            git config --add submodule.defaultGroup ./lib/baz

    # and the remaining 121 submodules are not reported in git status

    git diff
    # report them all below:
    diff --git a/subA b/subA
    index e4e79a2..6689f08 160000
    --- a/subA
    +++ b/subA
    @@ -1 +1 @@
    -Subproject commit e4e79a217576d24ef4d73b620766f62b155bcd98
    +Subproject commit 6689f08735d08a057f8d6f91af98b04960afa517
    ...
     # goes on including 123 submodule not in the default group.


In case we report these submodules which are checked out but not in
the default group, we probably want to adapt "git submodule update" to
un-checkout the work tree of the submodules in case they are clean.

Thanks,
Stefan

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

Any thoughts on my thoughts below?

On Fri, Apr 29, 2016 at 12:17 PM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Apr 29, 2016 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> We do not need to do anything special to initialize the `submodule_groups`
>>> pointer as the diff options setup will fill in 0 by default.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>  diff.c | 3 +++
>>>  diff.h | 1 +
>>>  2 files changed, 4 insertions(+)
>>
>> Isn't this going in the opposite way from what you described in 0/15
>> with analogy to how "ignore" mechanism works?  Just like a path is
>> tracked once it is tracked, whether it matches an ignore pattern,
>> shouldn't we be getting a summary for a submodule for any submodule
>> once submodule/.git/HEAD is there (i.e. we can give a comparison),
>> whether it is specified by a separate mechanism that acts from
>> sideways (e.g. the "default group").
>
> That is why I started in 0/15 with
>> One pain point I am still aware of:
> as I did not do the right thing in these patches?
> These patches do however:
>
>>    git status
>>    git diff
>>    git submodule summary
>>    # show only changes to submodules which are in the default group.
>
> which seems to be the wrong thing then.
>
> So here is a thought experiment:
>
>     # get all submodules into the work tree
>     git submodule update --recursive --init
>
>     # The selected default group will not include all submodules
>     git config submodule.defaultGroup "*SomeLabel"
>
>     git status
>     # What do we expect now?
>     # either a "nothing to commit, working directory clean"
>     # or rather what was described in 0/15:
>
>         More than 2 submodules (123 actually) including
>             'path/foo'
>             'lib/baz'
>         are checked out, but not part of the default group.
>         You can add these submodules via
>             git config --add submodule.defaultGroup ./path/foo
>             git config --add submodule.defaultGroup ./lib/baz
>
> If we want to go with the second option, the design described in 0/15
> is broken. Going one step further:
>
>     # in all submodules including the excluded via groups,
>     # by resetting the groups for this command
>     git -c submodule.defaultGroup= submodule foreach git reset --hard HEAD^
>
>     git status
>     # Reporting the changes from submodules in the default Group is
> uncontroversial:
>     On branch master
>     Your branch is up-to-date with 'origin/master'.
>     Changes not staged for commit:
>       (use "git add <file>..." to update what will be committed)
>       (use "git checkout -- <file>..." to discard changes in working directory)
>
>         modified:   subA (new commits)
>         modified:   subB (new commits)
>
>     no changes added to commit (use "git add" and/or "git commit -a")
>
>     # But what about subC which is not in the default group? It was
> changed as well.
>     # one thing I could imagine, is similar to above:
>
>         More than 2 submodules (123 actually) including
>             modified:    'subC'  (new commits)
>             modified:   'lib/baz' (new commits)
>         are checked out, but not part of the default group.
>         You can add these submodules via
>             git config --add submodule.defaultGroup ./path/foo
>             git config --add submodule.defaultGroup ./lib/baz
>
>     # and the remaining 121 submodules are not reported in git status
>
>     git diff
>     # report them all below:
>     diff --git a/subA b/subA
>     index e4e79a2..6689f08 160000
>     --- a/subA
>     +++ b/subA
>     @@ -1 +1 @@
>     -Subproject commit e4e79a217576d24ef4d73b620766f62b155bcd98
>     +Subproject commit 6689f08735d08a057f8d6f91af98b04960afa517
>     ...
>      # goes on including 123 submodule not in the default group.
>
>
> In case we report these submodules which are checked out but not in
> the default group, we probably want to adapt "git submodule update" to
> un-checkout the work tree of the submodules in case they are clean.
>
> Thanks,
> Stefan

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Any thoughts on my thoughts below?
>
>> So here is a thought experiment:
>>
>>     # get all submodules into the work tree
>>     git submodule update --recursive --init
>>
>>     # The selected default group will not include all submodules
>>     git config submodule.defaultGroup "*SomeLabel"
>>
>>     git status
>>     # What do we expect now?
>>     # either a "nothing to commit, working directory clean"
>>     # or rather what was described in 0/15:
>>
>>         More than 2 submodules (123 actually) including
>>             'path/foo'
>>             'lib/baz'
>>         are checked out, but not part of the default group.
>>         You can add these submodules via
>>             git config --add submodule.defaultGroup ./path/foo
>>             git config --add submodule.defaultGroup ./lib/baz

That may be an interesting thing to know, but I am not sure if it
adds value to the user.  The user wanted the defaultGroup to be
the set of submodules labeled with SomeLabel, and an alternative
valid suggestion could be

	'path/foo' and other submodules are not part of what you are
	interested in; if you want to deinitialize them, do

            git submodule deinit !defaultGroup

Both look equally valid to me, but offering both would be way too
much.  I'd say you should give that only with "status -v" or
something, perhaps?

>> If we want to go with the second option,

You already lost me here, as it is not clear what two "options" you
are comparing.

>> If we want to go with the second option, the design described in 0/15
>> is broken. Going one step further:
>>
>> ...
>>     # But what about subC which is not in the default group? It was
>> changed as well.

So why not show it?  Is there anything controversial?

If you are truly not interested in it by excluding it from the
default group, you wouldn't have touched it in the first place.  If
you did touch it, then you are showing some special interest that
overrides what you said in the default mechanism.

In short, I think I understood what you wanted with your analogy to
the ignore/exclude mechanism in 0/15.  Default is a handy way to say
"I do not want to bother specifying everything from the command line
every time" but we can say that it is nothing more than that.  That
is exactly how the ignore/exclude mechanism is used--"git add" by
default will not add those that are ignored when discovering paths
by recursively descending, but once added, that is part of what the
user told Git that she cares about.

>> In case we report these submodules which are checked out but not in
>> the default group, we probably want to adapt "git submodule update" to
>> un-checkout the work tree of the submodules in case they are clean.

Why?

Letting them know that they have such an option, and giving them a
way to tell which submodules fall into that category, are both good
things.  But why is it a good thing to automatically clean what the
user has checked out (which I expect that she expects to remain
until she explicitly tells us otherwise)?

We do not automatically "git rm" a clean tracked path that happens
to match .gitignore pattern and I do not think it is a good thing to
do so.

Puzzled.

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

On Thu, May 5, 2016 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Any thoughts on my thoughts below?
>>
>>> So here is a thought experiment:
>>>
>>>     # get all submodules into the work tree
>>>     git submodule update --recursive --init
>>>
>>>     # The selected default group will not include all submodules
>>>     git config submodule.defaultGroup "*SomeLabel"
>>>
>>>     git status
>>>     # What do we expect now?
>>>     # either a "nothing to commit, working directory clean"
>>>     # or rather what was described in 0/15:
>>>
>>>         More than 2 submodules (123 actually) including
>>>             'path/foo'
>>>             'lib/baz'
>>>         are checked out, but not part of the default group.
>>>         You can add these submodules via
>>>             git config --add submodule.defaultGroup ./path/foo
>>>             git config --add submodule.defaultGroup ./lib/baz
>
> That may be an interesting thing to know, but I am not sure if it
> adds value to the user.  The user wanted the defaultGroup to be
> the set of submodules labeled with SomeLabel, and an alternative
> valid suggestion could be
>
>         'path/foo' and other submodules are not part of what you are
>         interested in; if you want to deinitialize them, do
>
>             git submodule deinit !defaultGroup
>
> Both look equally valid to me, but offering both would be way too
> much.  I'd say you should give that only with "status -v" or
> something, perhaps?
>
>>> If we want to go with the second option,
>
> You already lost me here, as it is not clear what two "options" you
> are comparing.

The first option is giving nothing:

     git config submodule.defaultGroup "*SomeLabel"
     git -C submodule-not-labeled reset --hard HEAD^
     git status
     # all good, no report about  submodule-not-labeled
     # because it is not in the default group.
     # (This is implemented in the series)

The "second option" is some sort of reporting. Either what I or you proposed.

>
>>> If we want to go with the second option, the design described in 0/15
>>> is broken. Going one step further:
>>>
>>> ...
>>>     # But what about subC which is not in the default group? It was
>>> changed as well.
>
> So why not show it?  Is there anything controversial?

The user made clear to not be interested in subC by setting
up the default group.

>
> If you are truly not interested in it by excluding it from the
> default group, you wouldn't have touched it in the first place.

Well it can get out of sync by not touching it as well, because others
changed the submodule pointer who are interested in that though.

    # in the superproject
    git checkout new-version
    git submodule update
    # checks out submodules to their version

    git checkout some-ancient-version
    git submodule update
    # this would only update the submodules in the defaultGroup,
    # not those which are initialized but "uninteresting"
    # the labeling may have changed between the different versions
    git status
    # I don't want to see any submodule changes here.
    # but there may be a submodule which is not at the right version
    # as `git submodule update` only paid attention to the default group.

> If
> you did touch it, then you are showing some special interest that
> overrides what you said in the default mechanism.
>
> In short, I think I understood what you wanted with your analogy to
> the ignore/exclude mechanism in 0/15.  Default is a handy way to say
> "I do not want to bother specifying everything from the command line
> every time" but we can say that it is nothing more than that.  That
> is exactly how the ignore/exclude mechanism is used--"git add" by
> default will not add those that are ignored when discovering paths
> by recursively descending, but once added, that is part of what the
> user told Git that she cares about.
>
>>> In case we report these submodules which are checked out but not in
>>> the default group, we probably want to adapt "git submodule update" to
>>> un-checkout the work tree of the submodules in case they are clean.
>
> Why?
>
> Letting them know that they have such an option, and giving them a
> way to tell which submodules fall into that category, are both good
> things.  But why is it a good thing to automatically clean what the
> user has checked out (which I expect that she expects to remain
> until she explicitly tells us otherwise)?
>
> We do not automatically "git rm" a clean tracked path that happens
> to match .gitignore pattern and I do not think it is a good thing to
> do so.

git rm changes the index (which may show up in the next commit)

The state of a submodule (un-initialized, initialized, checked out)
doesn't change the index or anything. Only the working tree is affected.

And by flipping between the initialized and checked-out state we do not
lose any information (such as user configured remote urls) nor does it
affect the "state" (index, recorded tree, history) of the repository.

So I just wonder if

    git init <submodule group spec>
    git deinit ! <submodule group spec>

should be done in `git submodule update <submodule group spec>`
(or by having the default group configured and running `git submodule update`
with no arguments.)



>
> Puzzled.
>

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The first option is giving nothing:
>
>      git config submodule.defaultGroup "*SomeLabel"
>      git -C submodule-not-labeled reset --hard HEAD^
>      git status
>      # all good, no report about  submodule-not-labeled
>      # because it is not in the default group.
>      # (This is implemented in the series)
>
> The "second option" is some sort of reporting. Either what I or you proposed.

OK, although I didn't propose anything ;-).

>
>>
>>>> If we want to go with the second option, the design described in 0/15
>>>> is broken. Going one step further:
>>>>
>>>> ...
>>>>     # But what about subC which is not in the default group? It was
>>>> changed as well.
>>
>> So why not show it?  Is there anything controversial?
>
> The user made clear to not be interested in subC by setting
> up the default group.

I wonder if that is a valid argument.  Git's position has always
been very clear after doing this:

    git add -f foo.bin && echo \*.bin >>.gitignore

What the user might have said in the "configuration" is the default
suggestion, that is much weaker than what has been done to the
repository data.  I think "the path is recorded in the index" in the
ignore/exclude situation is similar to "the submodule is initialized
in the working tree" in this context.

> Well it can get out of sync by not touching it as well, because others
> changed the submodule pointer who are interested in that though.

Which "others" are mucking with your working tree?  (don't respond:
I'll come to the answer a few lines below).

>
>     # in the superproject
>     git checkout new-version
>     git submodule update
>     # checks out submodules to their version
>
>     git checkout some-ancient-version
>     git submodule update
>     # this would only update the submodules in the defaultGroup,
>     # not those which are initialized but "uninteresting"
>     # the labeling may have changed between the different versions

I see.  I think that is where the conceptual bug lies.  Thanks for
an illustration.

If we take an approach similar to ignore/exclude, then yes the
"default" action should be done to "defaultGroup" specified plus
what the user instantiated in the working tree already.  And that
is not limited to "update" operation.

Just like "git diff" is not the only thing that would show
difference in foo.bin in the working tree even when *.bin is
ignored, but we consistently treat foo.bin as tracked.

> The state of a submodule (un-initialized, initialized, checked out)
> doesn't change the index or anything. Only the working tree is affected.
>
> And by flipping between the initialized and checked-out state we do not
> lose any information (such as user configured remote urls) nor does it
> affect the "state" (index, recorded tree, history) of the repository.

Users want to initialize a module and keep it checked out even if
they do intend to keep it pristine and not making any changes
themselves, only so that other parts of the tree that depends on the
module can be built.  So "removing a tracked and unmodified thing
from the working tree does not hurt users" is a nonsense argument,
isn't it?  I would be very unhappy if "git status" removed pristine
paths from the working tree and toggle assume-unchanged bit in my
index automatically.

I am afraid you are focused too much on "version control" and losing
sight of those who use the data stored in "version control", perhaps
because you worked in submodule area too long and too hard?  

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

On Thu, May 5, 2016 at 3:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The first option is giving nothing:
>>
>>      git config submodule.defaultGroup "*SomeLabel"
>>      git -C submodule-not-labeled reset --hard HEAD^
>>      git status
>>      # all good, no report about  submodule-not-labeled
>>      # because it is not in the default group.
>>      # (This is implemented in the series)
>>
>> The "second option" is some sort of reporting. Either what I or you proposed.
>
> OK, although I didn't propose anything ;-).
>
>>
>>>
>>>>> If we want to go with the second option, the design described in 0/15
>>>>> is broken. Going one step further:
>>>>>
>>>>> ...
>>>>>     # But what about subC which is not in the default group? It was
>>>>> changed as well.
>>>
>>> So why not show it?  Is there anything controversial?
>>
>> The user made clear to not be interested in subC by setting
>> up the default group.
>
> I wonder if that is a valid argument.  Git's position has always
> been very clear after doing this:
>
>     git add -f foo.bin && echo \*.bin >>.gitignore
>
> What the user might have said in the "configuration" is the default
> suggestion, that is much weaker than what has been done to the
> repository data.  I think "the path is recorded in the index" in the
> ignore/exclude situation is similar to "the submodule is initialized
> in the working tree" in this context.
>
>> Well it can get out of sync by not touching it as well, because others
>> changed the submodule pointer who are interested in that though.
>
> Which "others" are mucking with your working tree?  (don't respond:
> I'll come to the answer a few lines below).
>
>>
>>     # in the superproject
>>     git checkout new-version
>>     git submodule update
>>     # checks out submodules to their version
>>
>>     git checkout some-ancient-version

I think here is when the conceptual bug happens.
We would want a

    git checkout --keep-submodules-to-pristine-default-group

(intentionally long bad name)
that option would init new submodules and deinit old submodules
which were clean before. Here we can compare it to a file,
i.e. after checkout some files are newly there, some are deleted.
(And that's totally expected)

If we had this `checkout --treat-submodules-as-files-for-defaultGroup`,
then the following command `git submodule update` would not be
required.

I think long term this is a far better approach.

I just wonder what the `checkout --recurse-submodules` should do when
there is no defaultGroup configured. (i.e. should that delete submodules
which were there before and delete them if they were clean? Just like files.)


>>     git submodule update
>>     # this would only update the submodules in the defaultGroup,
>>     # not those which are initialized but "uninteresting"
>>     # the labeling may have changed between the different versions
>
> I see.  I think that is where the conceptual bug lies.  Thanks for
> an illustration.

Yes.

>
> If we take an approach similar to ignore/exclude, then yes the
> "default" action should be done to "defaultGroup" specified plus
> what the user instantiated in the working tree already.  And that
> is not limited to "update" operation.
>
> Just like "git diff" is not the only thing that would show
> difference in foo.bin in the working tree even when *.bin is
> ignored, but we consistently treat foo.bin as tracked.
>
>> The state of a submodule (un-initialized, initialized, checked out)
>> doesn't change the index or anything. Only the working tree is affected.
>>
>> And by flipping between the initialized and checked-out state we do not
>> lose any information (such as user configured remote urls) nor does it
>> affect the "state" (index, recorded tree, history) of the repository.
>
> Users want to initialize a module and keep it checked out even if
> they do intend to keep it pristine and not making any changes
> themselves, only so that other parts of the tree that depends on the
> module can be built.

The `submodule init` command could learn to just add that path of the
extra submodule to the defaultGroup, such that it persists between
different checkouts.

> So "removing a tracked and unmodified thing
> from the working tree does not hurt users" is a nonsense argument,
> isn't it?  I would be very unhappy if "git status" removed pristine
> paths from the working tree and toggle assume-unchanged bit in my
> index automatically.

No, `checkout` did it for you. Assume we had a "defaultGroup for files",
(others call it "narrow clones")

    # in git.git
    git set-file-default-group Documentation/RelNotes
    git checkout v2.6.0

    ls Documentation/RelNotes |grep v2.7
    # no files! But this is uncontroversial as it's current behavior
    # even with fictional "files default group" set to "all files ever"

    ls builtin/
    # does it exist? No because set-file-default-group
    # did restrict out interest here.

>
> I am afraid you are focused too much on "version control" and losing
> sight of those who use the data stored in "version control", perhaps
> because you worked in submodule area too long and too hard?

Not sure what you mean here. (Neither did I work long nor hard in that area.)

I am aware that other operations such as a build system would be glad to
have the contents of the submodules there. But those would not use a
restrictive default group?

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I am aware that other operations such as a build system would be glad to
> have the contents of the submodules there. But those would not use a
> restrictive default group?

The set of submodules you "init" to the working tree are the ones
that are interesting to you.  So once the tree is populated, you do
not ever have to look at the "defaultGroup" configuration.  You just
need to look at the working tree.

But there is a chicken-and-egg problem.  What should happen after
the initial clone, or you switched to a different branch in the
superproject.

The concept of "default" would help by limiting these "checkout to
reflect my interest to my working tree" step.

So "if the user inititializes a submodule and we detect that it is
not in the default, add it to the default configuration" pretty much
feels like a tail wagging a dog arrangement to me.

Then there is another interesting issue: what should happen when the
project added a submodule when you decided what your "default" set
should be and wrote it in your configuration already.  I suspect
that an idea similar to "the elaborate thing proposed (by whom I no
longer rememvber) in the ancient days" I mentioned earlier might
leads us to a nice solution.  When you define a default group, you
remember what the set of available submodules were, and tie your
choice to that "available set".

E.g. there may be submodules A, B and C in .gitmodules, and you
chose to record a "default" that contains only A and B.  The exact
way you chose does not matter; it could have been using labels, or
you may have explicitly named it.  When you record, you remember
that the decision of using A and B was made when A, B and C were the
available submodules.  Next time when you see .gitmodules talks
about A, B and D but no longer C, then instead of using the previous
"default" choice blindly, you ask.  If the user says it still is the
right "default" to use A and B, then you _also_ remember the set.
So that when the user switches between the state of the project with
submodules A, B and C (original) and A, B and D (updated), the user
does not have to answer the same question twice.

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The set of submodules you "init" to the working tree are the ones
> that are interesting to you.  So once the tree is populated, you do
> not ever have to look at the "defaultGroup" configuration.  You just
> need to look at the working tree.
> ...

I forgot to prefix the first few paragraphs of that message with
"Here is how my version of the world should work."  I did not mean
to say "Here is how you must make your work work, or I won't talk to
you."  I was just hurried as I had to tend to other topics.

I actually do not care too deeply (except for the "automatically
remove" part, which I do not think we should do), as I do not think
there is a big fundamental difference between the two views.  To
make sure we are on the same page, let me rephrase the two views I
have in mind.

The difference is what should happen when the user does not give any
pathspec, *label, or :name to limit the set of submodules to act on,
which, traditionally meant to work on everything, and we are trying
to change that to some "default".

 (1) The default set is what the configuration file says is the
     default group.  The working tree state is ignored.

 (2) The default set is what the configuration file says is the
     default group, plus those the user showed interest by doing
     "submodule init".

Suppose that the user has a mostly satisfactory default configured,
i.e. the set of submodules the configuration file says is the default
is both necessary and sufficient to carry out her daily task.  Then
there is no difference between the two.

Further suppose that the user needs to view a submodule outside the
default group temporarily (imagine: for a day or two), while
carrying out some specific task.  Perhaps she is working on the
documentation submodule, which is her primary task hence her
configuration file specifies it as the default, but needs to see the
submodule that houses the implementation to describe the behaviour.

So she does "init code-module/"; this has explicit pathspec, so
there is no difference between the two.  Now, while reading the code
module, she finds a typo in a comment, and wants to fix it.  We will
start to see differences.

 * When she wants to get a bird's eye view of everything she cares
   about at the moment, i.e. wants to view the state of her usual
   modules plus the code-module she is visiting, (1) is more
   cumbersome.

   With (1), "diff --recursive" will not step outside of her
   configured default, so she says "diff --recursive \*default
   code-module/" to say "I want to see both my default submodule(s)
   and the one I checked out by hand".

   With (2), she does not have to do anything special, as manually
   checked out code-module/ will be acted upon, in addition to the
   configured default.


 * When she wants to see her usual modules ignoring the one-off
   checkout, (1) is easier.

   With (1), she can say "diff --recursive" and done. 

   With (2), she needs to say "diff --recursive \*default" to
   explicitly state "I may have checkouts of other submodules, but
   this time I want to view only the usual default of mine".

The difference is not that big either case.

Whichever way we choose to make the default behaviour, the user
needs to type a bit extra when asking a behaviour that is different
from the default behaviour.

The amount of "extra" in the first use case necessary for (1) is
greater than the amount of "extra" in the second use case necessary
for (2), though.  In addition, in the second use case, (1) makes it
easier for the user to miss important changes she made outside the
area of her usual attention, while (2) forces her to make a
conscious effort to exclude them.  These are the reasons why I have
a slight preference for (2) over (1).

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

On Thu, May 5, 2016 at 11:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The set of submodules you "init" to the working tree are the ones
>> that are interesting to you.  So once the tree is populated, you do
>> not ever have to look at the "defaultGroup" configuration.  You just
>> need to look at the working tree.
>> ...
>
> I forgot to prefix the first few paragraphs of that message with
> "Here is how my version of the world should work."  I did not mean
> to say "Here is how you must make your work work, or I won't talk to
> you."  I was just hurried as I had to tend to other topics.
>
> I actually do not care too deeply (except for the "automatically
> remove" part, which I do not think we should do), as I do not think
> there is a big fundamental difference between the two views.  To
> make sure we are on the same page, let me rephrase the two views I
> have in mind.

Ok, maybe we can leave that automatically remove part out for the
first series. (Eventually submodules should behave like files and we
delete files on checkout all the time, and it's a reasonable default)

So I think to reduce scope to only cover the clone/update first, all
other operations behave like today, i.e.

    git clone --recurse-submodules --init-submodule=label
--init-submodule=label2   git://...
    # will clone the superproject and recursively
    # checkout any submodule being labeled label or label2

    git config submodule.defaultGroups default
    git config --add submodule.defaultGroups devel
    # configure which submodules you are interested in.

    git submodule add --label <name> git://... ..
    # record a label while adding a submodule

    git submodule update --init-labeled(=*label)
    # will update all initialized submodules
    # and learn a new switch to also initialize the grouped
    # submodules (either the specified group or if none given
    # the default group as configured before)




    git status
    git diff
    git submodule summary
    # care about all initialized submodules, i.e. (2) below
    # a switch for recurse=label will be in a later series.



>
> The difference is what should happen when the user does not give any
> pathspec, *label, or :name to limit the set of submodules to act on,
> which, traditionally meant to work on everything, and we are trying
> to change that to some "default".
>
>  (1) The default set is what the configuration file says is the
>      default group.  The working tree state is ignored.
>
>  (2) The default set is what the configuration file says is the
>      default group, plus those the user showed interest by doing
>      "submodule init".
>
> Suppose that the user has a mostly satisfactory default configured,
> i.e. the set of submodules the configuration file says is the default
> is both necessary and sufficient to carry out her daily task.  Then
> there is no difference between the two.
>
> Further suppose that the user needs to view a submodule outside the
> default group temporarily (imagine: for a day or two), while
> carrying out some specific task.  Perhaps she is working on the
> documentation submodule, which is her primary task hence her
> configuration file specifies it as the default, but needs to see the
> submodule that houses the implementation to describe the behaviour.
>
> So she does "init code-module/"; this has explicit pathspec, so
> there is no difference between the two.  Now, while reading the code
> module, she finds a typo in a comment, and wants to fix it.  We will
> start to see differences.

Another way (3) is to add code-module/ to the "set of interesting
submodules, i.e. to the default group"

    git config --all submodule.defaultGroup ./code-module
    git submodule update

>
>  * When she wants to get a bird's eye view of everything she cares
>    about at the moment, i.e. wants to view the state of her usual
>    modules plus the code-module she is visiting, (1) is more
>    cumbersome.
>
>    With (1), "diff --recursive" will not step outside of her
>    configured default, so she says "diff --recursive \*default
>    code-module/" to say "I want to see both my default submodule(s)
>    and the one I checked out by hand".
>
>    With (2), she does not have to do anything special, as manually
>    checked out code-module/ will be acted upon, in addition to the
>    configured default.

In (3), diff --recursive will also just work fine.

>
>
>  * When she wants to see her usual modules ignoring the one-off
>    checkout, (1) is easier.

In (3) you'd do

    git config --unset submodule.defaultGroup ./code-module
    # optionally: git submodule update

>
>    With (1), she can say "diff --recursive" and done.
>
>    With (2), she needs to say "diff --recursive \*default" to
>    explicitly state "I may have checkouts of other submodules, but
>    this time I want to view only the usual default of mine".

and diff --recursive works fine again as well.

>
> The difference is not that big either case.
>
> Whichever way we choose to make the default behaviour, the user
> needs to type a bit extra when asking a behaviour that is different
> from the default behaviour.
>
> The amount of "extra" in the first use case necessary for (1) is
> greater than the amount of "extra" in the second use case necessary
> for (2), though.  In addition, in the second use case, (1) makes it
> easier for the user to miss important changes she made outside the
> area of her usual attention, while (2) forces her to make a
> conscious effort to exclude them.  These are the reasons why I have
> a slight preference for (2) over (1).
>

That makes sense.

So with (2)
 * there is no need to modify status, diff, log for the default case and the
    --recursive \*default" may come later, so the initial series can be smaller.
 *

Re: [PATCH 11/15] diff: ignore submodules excluded by groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Further suppose that the user needs to view a submodule outside the
>> default group temporarily (imagine: for a day or two), while
>> carrying out some specific task.  Perhaps she is working on the
>> documentation submodule, which is her primary task hence her
>> configuration file specifies it as the default, but needs to see the
>> submodule that houses the implementation to describe the behaviour.
>>
>> So she does "init code-module/"; this has explicit pathspec, so
>> there is no difference between the two.  Now, while reading the code
>> module, she finds a typo in a comment, and wants to fix it.  We will
>> start to see differences.
>
> Another way (3) is to add code-module/ to the "set of interesting
> submodules, i.e. to the default group"

I do not want to force her to do more than "submodule deinit" when
she is done with that temporary task that required her to have
code-module/ checked out, which is what you are doing with such a
change.  So that is a non-starter to me.

>> The amount of "extra" in the first use case necessary for (1) is
>> greater than the amount of "extra" in the second use case necessary
>> for (2), though.  In addition, in the second use case, (1) makes it
>> easier for the user to miss important changes she made outside the
>> area of her usual attention, while (2) forces her to make a
>> conscious effort to exclude them.  These are the reasons why I have
>> a slight preference for (2) over (1).
>>
>
> That makes sense.
>
> So with (2)
>  * there is no need to modify status, diff, log for the default case and the
>     --recursive \*default" may come later, so the initial series can be smaller.
>  *

I sense a premature "Send" button here ;-)

In any case, I care much more about the "making it harder to miss
things outside the usual area of attention" benefit than "the user
may have to type less more often" benefit, even though both are
slightly in favor between (1) and (2).

[PATCH 12/15] git submodule summary respects groups

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh             |  5 +++++
 t/t7413-submodule--helper.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 253ad07..f065b1f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -833,6 +833,11 @@ cmd_summary() {
 		sane_egrep '^:([0-7]* )?160000' |
 		while read mod_src mod_dst sha1_src sha1_dst status sm_path
 		do
+			# ignore modules not in group
+			if ! git submodule--helper in-group $sm_path
+			then
+				continue
+			fi
 			# Always show modules deleted or type-changed (blob<->module)
 			if test "$status" = D || test "$status" = T
 			then
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index 39e469f..d01cdc6 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -226,4 +226,30 @@ test_expect_success 'git submodule update respects groups' '
 	test_cmp expect actual
 '
 
+range_back="$(echo $submodule_sha1|cut -c1-7)...$(echo $sub_priorsha1|cut -c1-7)"
+cat >expect <<-EOF
+* sub0 $range_back (1):
+  < test2
+
+* sub1 $range_back (1):
+  < test2
+
+* sub3 $range_back (1):
+  < test2
+
+EOF
+
+test_expect_success 'git submodule summary respects groups' '
+	(
+		cd super_clone &&
+		git submodule update --init &&
+		git submodule foreach "git checkout HEAD^" &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git submodule summary >../actual &&
+		git config --unset-all submodule.defaultGroup
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 12/15] git submodule summary respects groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Same comment as 11/15 applies here.

>  git-submodule.sh             |  5 +++++
>  t/t7413-submodule--helper.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 253ad07..f065b1f 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -833,6 +833,11 @@ cmd_summary() {
>  		sane_egrep '^:([0-7]* )?160000' |
>  		while read mod_src mod_dst sha1_src sha1_dst status sm_path
>  		do
> +			# ignore modules not in group
> +			if ! git submodule--helper in-group $sm_path
> +			then
> +				continue
> +			fi
>  			# Always show modules deleted or type-changed (blob<->module)

[PATCH 13/15] cmd_status: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/commit.c             |  3 +++
 t/t7413-submodule--helper.sh | 15 +++++++++++++++
 wt-status.c                  |  2 ++
 wt-status.h                  |  1 +
 4 files changed, 21 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..d29134d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1369,6 +1369,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
 	s.ignore_submodule_arg = ignore_submodule_arg;
+	s.submodule_groups = string_list_duplicate(
+		git_config_get_value_multi("submodule.defaultGroup"), 1);
+
 	wt_status_collect(&s);
 
 	if (0 <= fd)
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index d01cdc6..a3dbfea 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -252,4 +252,19 @@ test_expect_success 'git submodule summary respects groups' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git status respects groups' '
+	# use setup from previous test
+	(
+		cd super_clone &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git status >../actual
+		git config --unset-all submodule.defaultGroup
+	) &&
+	test_i18ngrep "modified:   sub0" actual &&
+	test_i18ngrep "modified:   sub1" actual &&
+	test_i18ngrep ! "modified:   sub2" actual &&
+	test_i18ngrep "modified:   sub3" actual
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index ef74864..0d494ac 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -502,6 +502,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	}
+	rev.diffopt.submodule_groups = s->submodule_groups;
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
 	rev.diffopt.format_callback_data = s;
 	copy_pathspec(&rev.prune_data, &s->pathspec);
@@ -532,6 +533,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 		 */
 		handle_ignore_submodules_arg(&rev.diffopt, "dirty");
 	}
+	rev.diffopt.submodule_groups = s->submodule_groups;
 
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_collect_updated_cb;
diff --git a/wt-status.h b/wt-status.h
index c9b3b74..d66a2b5 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -73,6 +73,7 @@ struct wt_status {
 	struct string_list change;
 	struct string_list untracked;
 	struct string_list ignored;
+	struct string_list *submodule_groups;
 	uint32_t untracked_in_ms;
 };
 
-- 
2.8.0.41.g8d9aeb3

[PATCH 14/15] cmd_diff: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/diff.c               |  2 ++
 t/t7413-submodule--helper.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/builtin/diff.c b/builtin/diff.c
index 52c98a9..1c6abd5 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -366,6 +366,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	setup_diff_pager(&rev.diffopt);
 
+	rev.diffopt.submodule_groups = string_list_duplicate(
+		git_config_get_value_multi("submodule.defaultGroup"), 1);
 	/*
 	 * Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index a3dbfea..1ca7878 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -267,4 +267,19 @@ test_expect_success 'git status respects groups' '
 	test_i18ngrep "modified:   sub3" actual
 '
 
+test_expect_success 'git diff respects groups' '
+	# use setup from previous test
+	(
+		cd super_clone &&
+		git config --add submodule.defaultGroup *bit1 &&
+		git config --add submodule.defaultGroup ./sub0 &&
+		git diff >../actual
+		git config --unset-all submodule.defaultGroup
+	) &&
+	test_i18ngrep "diff --git a/sub0 b/sub0" actual &&
+	test_i18ngrep "diff --git a/sub1 b/sub1" actual &&
+	test_i18ngrep ! "diff --git a/sub2 b/sub2" actual &&
+	test_i18ngrep "diff --git a/sub3 b/sub3" actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

[PATCH 15/15] clone: allow specification of submodules to be cloned

[Stefan Beller]

This is in line with clone being the contraction of
    mkdir <path> && cd <path>
    git init
    git config
    git fetch
    git submodule update

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-clone.txt |  6 +++
 builtin/clone.c             | 40 +++++++++++++++++--
 t/t7400-submodule-basic.sh  | 96 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 45d74be..38b1948 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -212,6 +212,12 @@ objects from the source repository into a pack in the cloned repository.
 	repository does not have a worktree/checkout (i.e. if any of
 	`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
 
+--init-submodule::
+	After the repository is cloned, specified submodules are cloned.
+	It is possible to give multiple specifications by repeating the
+	argument. This option will be recorded in the repository config
+	as `submodule.defaultGroup`.
+
 --separate-git-dir=<git dir>::
 	Instead of placing the cloned repository where it is supposed
 	to be, place the cloned repository at the specified directory,
diff --git a/builtin/clone.c b/builtin/clone.c
index 6576ecf..8371bc2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -52,6 +52,22 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list init_submodules;
+
+static int init_submodules_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct string_list_item *item;
+	struct string_list sl = STRING_LIST_INIT_DUP;
+
+	if (unset)
+		return -1;
+
+	string_list_split(&sl, arg, ',', -1);
+	for_each_string_list_item(item, &sl)
+		string_list_append((struct string_list *)opt->value, item->string);
+
+	return 0;
+}
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
@@ -100,6 +116,8 @@ static struct option builtin_clone_options[] = {
 			TRANSPORT_FAMILY_IPV4),
 	OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
 			TRANSPORT_FAMILY_IPV6),
+	OPT_CALLBACK(0, "init-submodule", &init_submodules, N_("string"),
+			N_("clone specific submodules"), init_submodules_cb),
 	OPT_END()
 };
 
@@ -731,17 +749,24 @@ static int checkout(void)
 	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
 			   sha1_to_hex(sha1), "1", NULL);
 
-	if (!err && option_recursive) {
+	if (err)
+		goto out;
+
+	if (option_recursive || init_submodules.nr > 0) {
 		struct argv_array args = ARGV_ARRAY_INIT;
-		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
+		argv_array_pushl(&args, "submodule", "update", NULL);
 
+		if (option_recursive) {
+			argv_array_pushf(&args, "--init");
+			argv_array_pushf(&args, "--recursive");
+		}
 		if (max_jobs != -1)
 			argv_array_pushf(&args, "--jobs=%d", max_jobs);
 
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
-
+out:
 	return err;
 }
 
@@ -876,6 +901,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
+	if (init_submodules.nr > 0) {
+		struct string_list_item *item;
+		struct strbuf sb = STRBUF_INIT;
+		for_each_string_list_item(item, &init_submodules) {
+			strbuf_addf(&sb, "submodule.defaultGroup=%s", item->string);
+			string_list_append(&option_config, strbuf_detach(&sb, 0));
+		}
+	}
+
 	if (!option_origin)
 		option_origin = "origin";
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ac477b2..1fd313b 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1110,4 +1110,100 @@ test_expect_success 'submodule add records multiple labels' '
 	test_cmp expected actual
 '
 
+cat <<EOF > expected
+submodule
+EOF
+
+test_expect_success 'clone --init-submodule works' '
+	test_when_finished "rm -rf super super_clone" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label labelA file://"$pwd"/example2 submodule &&
+		git submodule add file://"$pwd"/example2 submodule1 &&
+		git commit -a -m "create repository with 2 submodules, one is in a group"
+	) &&
+	git clone --recurse-submodules --init-submodule \*labelA super super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp actual expected
+'
+
+cat <<EOF > expect
+submoduleA
+submoduleC
+submoduleE
+EOF
+
+test_expect_success 'clone with multiple --init-submodule options' '
+	test_when_finished "rm -rf super super_clone" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label groupA file://"$pwd"/example2 submoduleA &&
+		git submodule add --label groupB file://"$pwd"/example2 submoduleB &&
+		git submodule add --label groupC file://"$pwd"/example2 submoduleC &&
+		git submodule add --label groupD --name submoduleE file://"$pwd"/example2 submoduleD &&
+		git submodule add --label groupE --name submoduleD file://"$pwd"/example2 submoduleE &&
+		git submodule add file://"$pwd"/example2 submodule1 &&
+		git commit -a -m "create repository with submodules groups"
+	) &&
+	git clone --recurse-submodules --init-submodule=\*groupA --init-submodule ./submoduleC --init-submodule :submoduleD super super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp expect actual
+'
+
+cat <<EOF > expect
+submoduleA
+EOF
+
+cat <<EOF > expect2
+submoduleA
+submoduleC
+EOF
+
+test_expect_success 'clone and subsequent updates correctly auto-initialize submodules' '
+	test_when_finished "rm -rf super super_clone" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label LA file://"$pwd"/example2 submoduleA &&
+		git submodule add file://"$pwd"/example2 submoduleB &&
+		git commit -a -m "create repository with submodules groups"
+	) &&
+	git clone --recurse-submodules --init-submodule=\*LA super super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp expect actual &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label LA file://"$pwd"/example2 submoduleC &&
+		git commit -a -m "add another labled submodule"
+	) &&
+	(
+		cd super_clone &&
+		# obtain the new superproject
+		git pull &&
+		# submoduleC should just appear as it has the label LA
+		# which was configured in git clone
+		git submodule update --init &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp expect2 actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

Re: [PATCH 00/15] submodule groups (once again)

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> git diff is supposed to view the differences between "what would I
> get after checkout" (i.e. what is in the index run through smudge filters)
> compared to the actual worktree.

I do not think it affects your conclusion, but the above is wrong.
"git diff" is a preview of what you would add (i.e. what will be in
the index after passing working tree contents via the clean filter)
relative to what is actually in the index.

Re: [PATCH 00/15] submodule groups (once again)

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> What is this series about?
> ==========================
>
> If you have lots of submodules, you probably don't need all of them at once, 
> but you have functional units. Some submodules are absolutely required, 
> some are optional and only for very specific purposes. 
>
> This patch series adds labels to submodules in the .gitmodules file. 
>
> So you could have a .gitmodules file such as: 
>
> [submodule "gcc"] 
>         path = gcc 
>         url = git://... 
>         label = default
>         label = devel 
> [submodule "linux"] 
>         path = linux 
>         url = git://... 
>         label = default 
> [submodule "nethack"] 
>         path = nethack 
>         url = git://... 
>         label = optional
>         label = games 
>
> and by this series you can work on an arbitrary group of these submodules
> composed by the labels, names or paths of the submodules.
>
>     git clone --recurse-submodules --init-submodule=label --init-submodule=label2   git://... 
>     # will clone the superproject and recursively 
>     # checkout any submodule being labeled label or label2
>     
>     git submodule add --label <name> git://... ..
>     # record a label while adding a submodule
>     
>     git config submodule.defaultGroups default
>     git config --add submodule.defaultGroups devel
>     # configure which submodules you are interested in.
>     
>     git submodule update
>     # update only the submodules in the default group if that is configured.
>     
>     git status
>     git diff
>     git submodule summary
>     # show only changes to submodules which are in the default group.

Nicely designed.