[PATCH 0/7] submodule groups

[PATCH 0/7] submodule groups

[Stefan Beller]

I started from scratch as I think there were some sharp edges in the design.
My thinking shifted from "submodule groups" towards "actually it's just an
enhanced pathspec, called submodulespec".

The meat is found in the last 3 patches.

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 --init-submodule=\*<label> --init-submodule=:<name> git://...
    # will clone the superproject checkout
    # any submodule being labeled <label> or named <name>

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

    git submodule update [--init] \*label2
    # update only the submodules labeled "label2"
    
    git config submodule.updateGroups default
    git config --add submodule.updateGroups devel
    # configure which submodules you are interested in....
    git submodule update [--init-default-group] 
    # ... and update them    

    git status 
    git diff
    git submodule summary
    # unlike the last series, these are not touched
    
    git submodule status \*label2
    # only show information about "label2" submodules.
    

Any feedback welcome, specially on the design level!

Thanks,
Stefan

Prior series found here: http://thread.gmane.org/gmane.comp.version-control.git/292666

Stefan Beller (7):
  submodule--helper: add valid-label-name
  submodule add: label submodules if asked to
  submodule-config: keep labels around
  submodule-config: check if a submodule is in a group
  submodule--helper module_list_compute: allow label or name arguments
  submodule update: learn partial initialization
  clone: allow specification of submodules to be cloned

 Documentation/config.txt        |   5 ++
 Documentation/git-clone.txt     |  26 ++++--
 Documentation/git-submodule.txt |  30 +++++--
 builtin/clone.c                 |  40 ++++++++-
 builtin/submodule--helper.c     | 146 +++++++++++++++++++++++++++---
 git-submodule.sh                |  38 ++++++--
 submodule-config.c              |  66 ++++++++++++++
 submodule-config.h              |   5 ++
 t/t7400-submodule-basic.sh      | 176 ++++++++++++++++++++++++++++++++++++
 t/t7412-submodule--helper.sh    | 193 ++++++++++++++++++++++++++++++++++++++++
 10 files changed, 692 insertions(+), 33 deletions(-)
 create mode 100755 t/t7412-submodule--helper.sh

-- 
2.8.0.35.g58985d9.dirty

[PATCH 1/7] submodule--helper: add valid-label-name

[Stefan Beller]

We could allow more than just alphanumeric and dash characters
for submodule labels. As a precaution we'll first allow only this
subset and later on we can extend it once we have more experience
with them.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 30 ++++++++++++++++++++++++++-
 t/t7412-submodule--helper.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7f0941d..d3f4684 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -831,6 +831,33 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int submodule_valid_label_name(const char *label)
+{
+	if (!label || !strlen(label))
+		return 0;
+
+	if (!isalnum(*label))
+		return 0;
+
+	while (*label) {
+		if (!(isalnum(*label) ||
+			*label == '-'))
+			return 0;
+		label++;
+	}
+
+	return 1;
+}
+
+static int valid_label_name(int argc, const char **argv, const char *prefix)
+{
+	if (argc == 2 && submodule_valid_label_name(argv[1]))
+		return 0;
+
+	die(_("submodule label must start with an alphanumeric character"
+	      "and must contain alphanumeric characters or dashes only."));
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -843,7 +870,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},
+	{"valid-label-name", valid_label_name}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
new file mode 100755
index 0000000..3af315c
--- /dev/null
+++ b/t/t7412-submodule--helper.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+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 'valid-label-name tests empty label' '
+	test_must_fail git submodule--helper valid-label-name 2>actual &&
+	test_i18ngrep alphanumeric actual &&
+	test_must_fail git submodule--helper valid-label-name "" 2>actual &&
+	test_i18ngrep alphanumeric actual
+'
+
+test_expect_success 'valid-label-name tests correct label asdf' '
+	git submodule--helper valid-label-name asdf 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'valid-label-name tests correct label a' '
+	git submodule--helper valid-label-name a 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'valid-label-name tests correct label a-b' '
+	git submodule--helper valid-label-name a-b 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'valid-label-name fails with multiple arguments' '
+	test_must_fail git submodule--helper valid-label-name a b 2>actual &&
+	test_i18ngrep alphanumeric actual
+'
+
+test_expect_success 'valid-label-name fails with white spaced arguments' '
+	test_must_fail git submodule--helper valid-label-name "a b" 2>actual &&
+	test_i18ngrep alphanumeric actual
+'
+
+test_expect_success 'valid-label-name fails with utf8 characters' '
+	test_must_fail git submodule--helper valid-label-name ☺ 2>actual &&
+	test_i18ngrep alphanumeric actual
+'
+
+test_done
-- 
2.8.0.35.g58985d9.dirty

Re: [PATCH 1/7] submodule--helper: add valid-label-name

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +static int submodule_valid_label_name(const char *label)
> +{
> +	if (!label || !strlen(label))
> +		return 0;
> +
> +	if (!isalnum(*label))
> +		return 0;

I'd limit this one to isalpha() if I were doing this to make the
restriction similar to identifiers in traditional programming
language.

> +	while (*label) {
> +		if (!(isalnum(*label) ||
> +			*label == '-'))

And throw in '_' to the mix while at it.

> +			return 0;
> +		label++;
> +	}
> +
> +	return 1;
> +}

If the convention is "0 is good", then please signal "bad" with a
negative value, not just "non-zero".

[PATCH 2/7] submodule add: label submodules if asked to

[Stefan Beller]

When adding new submodules, you can specify the labels 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 |  4 +++-
 git-submodule.sh                | 16 ++++++++++++++-
 t/t7400-submodule-basic.sh      | 44 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 13adebf..9ba8895 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>...]
@@ -101,6 +101,8 @@ 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.
++
+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..c1213d8 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,15 @@ cmd_add()
 		--depth=*)
 			depth=$1
 			;;
+		-l|--label)
+			git submodule--helper valid-label-name "$2" || exit
+			labels="${labels} $2"
+			shift
+			;;
+		--label=*)
+			git submodule--helper valid-label-name "${1#--label=}" || exit
+			labels="${labels} ${1#--label=}"
+			;;
 		--)
 			shift
 			break
@@ -292,6 +302,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 814ee63..0adc4e4 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1056,6 +1056,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) &&
 	(
@@ -1094,5 +1095,48 @@ 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 >../expect
+	) &&
+	test_cmp expect actual
+'
+
+cat >expect <<-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 expect actual
+'
+
+test_expect_success 'submodule add recording wrong labels reports an error' '
+	test_when_finished "rm -rf super" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		test_must_fail git submodule add --label="labelA labelB" file://"$pwd"/example2 submodule 2>../actual
+	) &&
+	test_i18ngrep alphanumeric actual
+'
 
 test_done
-- 
2.8.0.35.g58985d9.dirty

Re: [PATCH 2/7] submodule add: label submodules if asked to

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 814ee63..0adc4e4 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1056,6 +1056,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) &&
>  	(
> @@ -1094,5 +1095,48 @@ 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 >../expect
> +	) &&
> +	test_cmp expect actual
> +'
> +
> +cat >expect <<-EOF
> +labelA
> +labelB
> +EOF
> +
> +test_expect_success 'submodule add records multiple labels' '

The existing tests in this file may be littered with this bad
construct, but please do not add more example of running things
outside of test_expect_{success,failure} block unless there is a
good reason to do so.

Re: [PATCH 2/7] submodule add: label submodules if asked to

[Stefan Beller]

>> +cat >expect <<-EOF
>> +labelA
>> +labelB
>> +EOF
>> +
>> +test_expect_success 'submodule add records multiple labels' '
>
> The existing tests in this file may be littered with this bad
> construct, but please do not add more example of running things
> outside of test_expect_{success,failure} block unless there is a
> good reason to do so.

I thought that was the standard for tests as I have seen
them quite a few times. Looking for those "cat >expect ..." constructs
more closely they are often found inside tests as well. Makes sense
if the expectation is used for only one test.

[PATCH 3/7] 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.35.g58985d9.dirty

Re: [PATCH 3/7] submodule-config: keep labels around

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Hmph, is there a reason to do this, instead of embedding an instance
of "struct string_list" inside submodule structure?

I am not yet claiming that embedding is better.  Just wondering if
it makes it easier to handle initialization as seen in the hunk
below, and also _clear() procedure.

> @@ -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);
> +		}
>  	}

Re: [PATCH 3/7] submodule-config: keep labels around

[Stefan Beller]

On Tue, May 10, 2016 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -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;
>
> Hmph, is there a reason to do this, instead of embedding an instance
> of "struct string_list" inside submodule structure?
>
> I am not yet claiming that embedding is better.  Just wondering if
> it makes it easier to handle initialization as seen in the hunk
> below, and also _clear() procedure.

Thanks for pointing out that alternative.  That looks so much
better in this patch. Let's see how the follow up patches develop.
As we'd not check != NULL first, but check against the count of the
string list. (I expect no problems down that road though).

Re: [PATCH 3/7] submodule-config: keep labels around

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, May 10, 2016 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> @@ -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;
>>
>> Hmph, is there a reason to do this, instead of embedding an instance
>> of "struct string_list" inside submodule structure?
>>
>> I am not yet claiming that embedding is better.  Just wondering if
>> it makes it easier to handle initialization as seen in the hunk
>> below, and also _clear() procedure.
>
> Thanks for pointing out that alternative.  That looks so much
> better in this patch. Let's see how the follow up patches develop.
> As we'd not check != NULL first, but check against the count of the
> string list. (I expect no problems down that road though).

I also wonder if we can say the same for the .ignore field, by the
way, if we agree that it is a better direction to go.

Re: [PATCH 3/7] submodule-config: keep labels around

[Stefan Beller]

+Heiko

On Wed, May 11, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, May 10, 2016 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> @@ -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;
>>>
>>> Hmph, is there a reason to do this, instead of embedding an instance
>>> of "struct string_list" inside submodule structure?
>>>
>>> I am not yet claiming that embedding is better.  Just wondering if
>>> it makes it easier to handle initialization as seen in the hunk
>>> below, and also _clear() procedure.
>>
>> Thanks for pointing out that alternative.  That looks so much
>> better in this patch. Let's see how the follow up patches develop.
>> As we'd not check != NULL first, but check against the count of the
>> string list. (I expect no problems down that road though).
>
> I also wonder if we can say the same for the .ignore field, by the
> way, if we agree that it is a better direction to go.

I don't think this is a good idea, as it's just a string, like .{url,
name, path}

Instead of storing the string we could store an enum {UNTRACKED,
DIRTY, ALL, NONE, NOT_INIT} though. That would be better I guess.

[PATCH 4/7] submodule-config: check if a submodule is in a group

[Stefan Beller]

In later patches we need to tell if a submodule is in a group,
so expose a handy test function in both C and shell.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c  | 42 +++++++++++++++++++++++++++++++-
 submodule-config.c           | 50 ++++++++++++++++++++++++++++++++++++++
 submodule-config.h           |  3 +++
 t/t7412-submodule--helper.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 152 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d3f4684..6ffd1c1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -858,6 +858,45 @@ static int valid_label_name(int argc, const char **argv, const char *prefix)
 	      "and must contain alphanumeric characters or dashes only."));
 }
 
+static 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_("comma separated 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);
+
+	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.updateGroup");
+	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 *);
@@ -871,7 +910,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
 	{"init", module_init},
-	{"valid-label-name", valid_label_name}
+	{"valid-label-name", valid_label_name},
+	{"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..7f38ebd 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 at 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/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
index 3af315c..042f3f5 100755
--- a/t/t7412-submodule--helper.sh
+++ b/t/t7412-submodule--helper.sh
@@ -46,4 +46,62 @@ test_expect_success 'valid-label-name fails with utf8 characters' '
 	test_i18ngrep alphanumeric actual
 '
 
+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 submodule add ../sub sub_name &&
+		git mv sub_name sub_path &&
+		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_must_fail git submodule--helper in-group --group=\*bit1 sub_path &&
+
+		# 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 sub_path &&
+
+		# tests by name:
+			       git submodule--helper in-group --group=:sub0 sub0 &&
+		test_must_fail git submodule--helper in-group --group=:sub0 sub1 &&
+			       git submodule--helper in-group --group=:sub_name sub_path &&
+
+		# 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_done
-- 
2.8.0.35.g58985d9.dirty

Re: [PATCH 4/7] submodule-config: check if a submodule is in a group

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +static int in_group(int argc, const char **argv, const char *prefix)
> + ...
> +	if (!group)
> +		list = git_config_get_value_multi("submodule.updateGroup");
> +	else {
> +		string_list_split(&actual_list, group, ',', -1);

Is this expected to be used only by test scripts, or will it have
real scripted Porcelain as its users?  I am wondering if
concatenation with ' ' would be more natural if it is the latter (if
only used for testing, I don't care that much).

[PATCH 5/7] submodule--helper module_list_compute: allow label or name arguments

[Stefan Beller]

Additionally to taking a pathspec, `module_list_compute` will also take
labels and submodule names, when these are prefixed by '*' and ':'
respectively.

`module_list_compute` is used by other functions in the submodule helper:
* module_list, used by `submodule {deinit, status, sync, foreach}`
* module_init, used by `submodule init`
* update_clone, used by `submodule update`

{foreach, update} do not pass on command line arguments to the listing
function such that these are not affected. The rest of them {deinit,
status, sync, init} will be exercised in additional tests.

As the labeling requires lookup of .gitmodules files, we need to make sure
the submodule config cache is initialized in all the functions, that's why
there are additional calls to gitmodules_config() and	git_config(...);

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt | 22 +++++++++--
 builtin/submodule--helper.c     | 76 +++++++++++++++++++++++++++++++-----
 git-submodule.sh                |  8 ++--
 t/t7412-submodule--helper.sh    | 86 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9ba8895..35ca355 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -11,16 +11,16 @@ SYNOPSIS
 [verse]
 '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>...]
-'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
+'git submodule' [--quiet] status [--cached] [--recursive] [--] [<submodulespec>...]
+'git submodule' [--quiet] init [--] [<submodulespec>...]
+'git submodule' [--quiet] deinit [-f|--force] [--] <submodulespec>...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 	      [-f|--force] [--rebase|--merge] [--reference <repository>]
 	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
-'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
+'git submodule' [--quiet] sync [--recursive] [--] [<submodulespec>...]
 
 
 DESCRIPTION
@@ -37,6 +37,20 @@ 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 working on submodules you can specify the desired submodule
+group or give no specification at all to apply the command to the
+default group of submodules, which includes all submodules.
+To group submodules you can make use of
+ . a pathspec,
+ . their name,
+ . labels.
+It is undefined behavior to give a spec that is ambigious for a name,
+pathspec or label. To make the specification unambigous, you can prefix
+ . pathspecs with './',
+ . names with ':',
+ . labels with '*'.
+Labels must consist of alphanumeric characters or the dash character.
+
 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.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6ffd1c1..ba43c80 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -12,6 +12,7 @@
 #include "remote.h"
 #include "refs.h"
 #include "connect.h"
+#include "argv-array.h"
 
 static char *get_default_remote(void)
 {
@@ -215,6 +216,36 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+static void split_argv_pathspec_groups(int argc, const char **argv,
+				       const char ***pathspec_argv,
+				       struct string_list *group)
+{
+	int i;
+	struct argv_array ps = ARGV_ARRAY_INIT;
+	for (i = 0; i < argc; i++) {
+		if (starts_with(argv[i], "*")
+		    || starts_with(argv[i], ":")) {
+			string_list_insert(group, argv[i]);
+		} else if (starts_with(argv[i], "./")) {
+			argv_array_push(&ps, argv[i]);
+		} else {
+			/*
+			* NEEDSWORK:
+			* Improve autodetection of items with no prefixes,
+			* roughly like
+			* if (label_or_name_exists(argv[i]))
+			*	string_list_insert(group, argv[i]);
+			* else
+			*	argv_array_push(&ps, argv[i]);
+			*/
+			argv_array_push(&ps, argv[i]);
+		}
+	}
+
+	*pathspec_argv = argv_array_detach(&ps);
+	argv_array_clear(&ps);
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -228,10 +259,15 @@ static int module_list_compute(int argc, const char **argv,
 {
 	int i, result = 0;
 	char *ps_matched = NULL;
+	const char **pathspec_argv;
+	struct string_list group = STRING_LIST_INIT_DUP;
+
+	split_argv_pathspec_groups(argc, argv, &pathspec_argv, &group);
+
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
-		       prefix, argv);
+		       prefix, pathspec_argv);
 
 	if (pathspec->nr)
 		ps_matched = xcalloc(pathspec->nr, 1);
@@ -242,10 +278,27 @@ static int module_list_compute(int argc, const char **argv,
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
 
-		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
-				    0, ps_matched, 1) ||
-		    !S_ISGITLINK(ce->ce_mode))
-			continue;
+		if (!group.nr) {
+			if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
+					    0, ps_matched, 1) ||
+			    !S_ISGITLINK(ce->ce_mode))
+				continue;
+		} else {
+			const struct submodule *sub;
+			int ps = 0, gr = 0;
+			if (!S_ISGITLINK(ce->ce_mode))
+				continue;
+			sub = submodule_from_path(null_sha1, ce->name);
+
+			ps = match_pathspec(pathspec, ce->name, ce_namelen(ce),
+					    0, ps_matched, 1);
+			gr = submodule_in_group(&group, sub);
+
+			if (!pathspec->nr && !gr)
+				continue;
+			if (!ps && !gr)
+				continue;
+		}
 
 		ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
 		list->entries[list->nr++] = ce;
@@ -287,6 +340,9 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_list_options,
 			     git_submodule_helper_usage, 0);
 
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
 		printf("#unmatched\n");
 		return 1;
@@ -415,6 +471,9 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_init_options,
 			     git_submodule_helper_usage, 0);
 
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
 
@@ -795,16 +854,15 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		if (parse_submodule_update_strategy(update, &suc.update) < 0)
 			die(_("bad value for update parameter"));
 
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
 	if (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)
 		return 1;
 
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
-	/* Overlay the parsed .gitmodules file with .git/config */
-	gitmodules_config();
-	git_config(submodule_config, NULL);
-
 	if (max_jobs < 0)
 		max_jobs = parallel_submodules();
 
diff --git a/git-submodule.sh b/git-submodule.sh
index c1213d8..c8e36c5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -6,13 +6,13 @@
 
 dashless=$(basename "$0" | sed -e 's/-/ /')
 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>...
+   or: $dashless [--quiet] status [--cached] [--recursive] [--] [<submodulespec>...]
+   or: $dashless [--quiet] init [--] [<submodulespec>...]
+   or: $dashless [--quiet] deinit [-f|--force] [--] <submodulespec>...
    or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
-   or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
+   or: $dashless [--quiet] sync [--recursive] [--] [<submodulespec>...]"
 OPTIONS_SPEC=
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
index 042f3f5..0f2e5c6 100755
--- a/t/t7412-submodule--helper.sh
+++ b/t/t7412-submodule--helper.sh
@@ -104,4 +104,90 @@ test_expect_success 'in-group' '
 	)
 '
 
+test_expect_success 'submodule init respects label' '
+	test_when_finished "rm -rf super_clone" &&
+	suburl=$(pwd)/sub &&
+	git clone super super_clone &&
+	(
+		cd super_clone &&
+		git submodule init \*bit1 &&
+		test_must_fail git config submodule.sub0.url &&
+		test        "$(git config submodule.sub1.url)" = "$suburl" &&
+		test_must_fail git config submodule.sub2.url &&
+		test        "$(git config submodule.sub3.url)" = "$suburl"
+	)
+'
+
+test_expect_success 'submodule init respects name' '
+	test_when_finished "rm -rf super_clone" &&
+	suburl=$(pwd)/sub &&
+	git clone super super_clone &&
+	(
+		cd super_clone &&
+		git submodule init :sub_name &&
+		test_must_fail git config submodule.sub0.url &&
+		test "$(git config submodule.sub_name.url)" = "$suburl"
+	)
+'
+
+test_expect_success 'submodule deinit respects label' '
+	suburl=$(pwd)/sub &&
+	(
+		cd super &&
+		git submodule init &&
+		git submodule deinit \*bit2 &&
+		test        "$(git config submodule.sub0.url)" = "$suburl" &&
+		test        "$(git config submodule.sub1.url)" = "$suburl" &&
+		test_must_fail git config submodule.sub2.url &&
+		test_must_fail git config submodule.sub3.url
+	)
+'
+
+test_expect_success 'submodule deinit respects label and path' '
+	suburl=$(pwd)/sub &&
+	(
+		cd super &&
+		git submodule init &&
+		git submodule deinit \*bit2 ./sub1 &&
+		test        "$(git config submodule.sub0.url)" = "$suburl" &&
+		test_must_fail git config submodule.sub1.url &&
+		test_must_fail git config submodule.sub2.url &&
+		test_must_fail git config submodule.sub3.url
+	)
+'
+
+cat >expect <<-EOF
+-sub1
+-sub3
+EOF
+
+test_expect_success 'submodule status respects label and path' '
+	suburl=$(pwd)/sub &&
+	(
+		cd super &&
+		git submodule deinit .
+		git submodule init &&
+		git submodule status \*bit1 ./sub1 | cut -c 1,43- >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'submodule sync respects name and path' '
+	suburl=$(pwd)/sub &&
+	(
+		cd super &&
+		git submodule init &&
+		git config submodule."sub0".url "wrong" &&
+		git config submodule."sub1".url "wrong" &&
+		git config submodule."sub2".url "wrong" &&
+		git config submodule."sub3".url "wrong" &&
+		git submodule sync :sub_name ./sub1 | cut -c 1,43- >../actual &&
+		test "$(git config submodule.sub0.url)" = "wrong" &&
+		test "$(git config submodule.sub1.url)" = "$suburl" &&
+		test "$(git config submodule.sub2.url)" = "wrong" &&
+		test "$(git config submodule.sub3.url)" = "wrong" &&
+		test "$(git config submodule.sub_name.url)" = "$suburl"
+	)
+'
+
 test_done
-- 
2.8.0.35.g58985d9.dirty

Re: [PATCH 5/7] submodule--helper module_list_compute: allow label or name arguments

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +static void split_argv_pathspec_groups(int argc, const char **argv,
> +				       const char ***pathspec_argv,
> +				       struct string_list *group)
> +{
> +	int i;
> +	struct argv_array ps = ARGV_ARRAY_INIT;
> +	for (i = 0; i < argc; i++) {
> +		if (starts_with(argv[i], "*")
> +		    || starts_with(argv[i], ":")) {
> +			string_list_insert(group, argv[i]);
> +		} else if (starts_with(argv[i], "./")) {
> +			argv_array_push(&ps, argv[i]);

I'd suggest stripping "./" when you do this.  That is, make the rule
to be "*label is a label, :name is a name, and everything else is a
path.  You can prefix ./ to an unfortunate path that begins with an
asterisk or a colon and we will strip ./ disambiguator".

> +		} else {
> +			/*
> +			* NEEDSWORK:
> +			* Improve autodetection of items with no prefixes,
> +			* roughly like
> +			* if (label_or_name_exists(argv[i]))
> +			*	string_list_insert(group, argv[i]);
> +			* else
> +			*	argv_array_push(&ps, argv[i]);
> +			*/

I do not think this is desirable.  "label" that changes its meaning
between "a path ./label" to "a label *label" would force people to
give unnecessary prefix "./" when naming their path, from fear of
colliding with a label that may or may not exist.

> +			argv_array_push(&ps, argv[i]);
> +		}
> +	}



> +		if (!group.nr) {
> +			if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +					    0, ps_matched, 1) ||
> +			    !S_ISGITLINK(ce->ce_mode))
> +				continue;
> +		} else {
> +			const struct submodule *sub;
> +			int ps = 0, gr = 0;
> +			if (!S_ISGITLINK(ce->ce_mode))
> +				continue;
> +			sub = submodule_from_path(null_sha1, ce->name);
> +
> +			ps = match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +					    0, ps_matched, 1);
> +			gr = submodule_in_group(&group, sub);
> +
> +			if (!pathspec->nr && !gr)
> +				continue;
> +			if (!ps && !gr)
> +				continue;

Hmph, so the rule is "a submodule that is in the specified group is
chosen, even if it is outside the subset of paths narrowed by the
given pathspec."  I think that is consistent with the way how the
list of arguments given to module_list (i.e. a pathspec element plus
a group specifier OR together just like two pathspec elements do not
force a path to match both, and instead they OR together).

Is that rule (i.e. specifiers are ORed together) written down
somewhere for users?

Re: [PATCH 5/7] submodule--helper module_list_compute: allow label or name arguments

[Junio C Hamano]

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

> Stefan Beller <sbeller@google.com> writes:
>
>> +static void split_argv_pathspec_groups(int argc, const char **argv,
>> +				       const char ***pathspec_argv,
>> +				       struct string_list *group)
>> +{
>> +	int i;
>> +	struct argv_array ps = ARGV_ARRAY_INIT;
>> +	for (i = 0; i < argc; i++) {
>> +		if (starts_with(argv[i], "*")
>> +		    || starts_with(argv[i], ":")) {
>> +			string_list_insert(group, argv[i]);
>> +		} else if (starts_with(argv[i], "./")) {
>> +			argv_array_push(&ps, argv[i]);
>
> I'd suggest stripping "./" when you do this.  That is, make the rule
> to be "*label is a label, :name is a name, and everything else is a
> path.  You can prefix ./ to an unfortunate path that begins with an
> asterisk or a colon and we will strip ./ disambiguator".

To clarify, "./$path and $path are the same so why strip it?" is an
understandable, even though naive, question. The reason is because
you do not want to contaminate the code that parses pathspecs with
the knowledge of this submodule-group specific prefix.  "./$path"
and "$path" may be equivalent for a literal pathspec, but I'd want
to see user be able to say

   git submodule foreach './:(icase)foo'

and find Foo, foo, FOO, etc.

I would also recommend to strip '*' and ':' from group names and
module names, and maintain two separate lists.  You would eventually
want to do "*default*" to name all groups whose name matches with
the pattern "default*", as if it were a pathspec matched against the
available group names, and that will keep the door open for future
extension like  "*:(icase)default*".

[PATCH 6/7] submodule update: learn partial initialization

[Stefan Beller]

The new switch `--init-default-group` updates the submodules which are
configured in `submodule.updateGroup`

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 59d7046..0f20019 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2735,6 +2735,11 @@ submodule.fetchJobs::
 	in parallel. A value of 0 will give some reasonable default.
 	If unset, it defaults to 1.
 
+submodule.updateGroup::
+	Specifies the group of submodules when `git submodule --init-group`
+	is called with no arguments. This setting is recorded in the initial
+	clone when `--init-submodule` was given.
+
 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 35ca355..e658d15 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -14,9 +14,9 @@ SYNOPSIS
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<submodulespec>...]
 'git submodule' [--quiet] init [--] [<submodulespec>...]
 'git submodule' [--quiet] deinit [-f|--force] [--] <submodulespec>...
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
+'git submodule' [--quiet] update [--init[-default-group]] [--remote] [-N|--no-fetch]
 	      [-f|--force] [--rebase|--merge] [--reference <repository>]
-	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
+	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<submodulespec>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
diff --git a/git-submodule.sh b/git-submodule.sh
index c8e36c5..2b0b0cb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -522,7 +522,12 @@ cmd_update()
 			GIT_QUIET=1
 			;;
 		-i|--init)
-			init=1
+			test -z $init || test $init = by_args || die "$(gettext "Only one of --init or --init-default-group may be used.")"
+			init=by_args
+			;;
+		--init-default-group)
+			test -z $init || test $init = by_config || die "$(gettext "Only one of --init or --init-default-group may be used.")"
+			init=by_config
 			;;
 		--remote)
 			remote=1
@@ -585,7 +590,12 @@ cmd_update()
 
 	if test -n "$init"
 	then
-		cmd_init "--" "$@" || return
+		additional_init=
+		if test "$init" = "by_config"
+		then
+			additional_init=$(git config --get-all submodule.updateGroup)
+		fi
+		cmd_init "--" "$@" ${additional_init:+$additional_init} || return
 	fi
 
 	{
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 0adc4e4..41e65c2 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1139,4 +1139,57 @@ test_expect_success 'submodule add recording wrong labels reports an error' '
 	test_i18ngrep alphanumeric actual
 '
 
+test_expect_success 'setup superproject with labeled submodules' '
+	mkdir sub1 &&
+	(
+		cd sub1 &&
+		git init &&
+		test_commit test
+		test_commit test2
+	) &&
+	mkdir labeledsuper &&
+	(
+		cd labeledsuper &&
+		git init &&
+		git submodule add ../sub1 sub0 &&
+		git submodule add -l bit1 ../sub1 sub1 &&
+		git submodule add -l bit2 ../sub1 sub2 &&
+		git submodule add -l bit2 -l bit1 ../sub1 sub3 &&
+		git commit -m "add labeled submodules"
+	)
+'
+
+cat >expect <<-EOF
+-sub0
+ sub1 (test2)
+ sub2 (test2)
+ sub3 (test2)
+EOF
+
+test_expect_success 'submodule update --init with a group' '
+	test_when_finished "rm -rf labeledsuper_clone" &&
+	pwd=$(pwd) &&
+	git clone file://"$pwd"/labeledsuper labeledsuper_clone &&
+	(
+		cd labeledsuper_clone &&
+		git submodule update --init \*bit1 ./sub2 &&
+		git submodule status |cut -c 1,43- >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'submodule update --init-default-group' '
+	test_when_finished "rm -rf labeledsuper_clone" &&
+	pwd=$(pwd) &&
+	git clone file://"$pwd"/labeledsuper labeledsuper_clone &&
+	(
+		cd labeledsuper_clone &&
+		git config submodule.updateGroup \*bit1 &&
+		git config --add submodule.updateGroup ./sub2 &&
+		git submodule update --init-default-group &&
+		git submodule status |cut -c 1,43- >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.8.0.35.g58985d9.dirty

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

[Stefan Beller]

This allows to specify a subset of all available submodules to be
initialized and cloned. It is unrelated to the `--recursive` option,
i.e. the user may still want to give `--recursive` as an option.

Originally `--recursive` implied to initialize all submodules, this
changes as well with the new option, such that only the specified
submodules are cloned, and their submodules (i.e. subsubmodules)
are cloned in full as the submodule specification is not passed on.

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

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 45d74be..4a9e8bb 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,8 @@ SYNOPSIS
 	  [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch]
-	  [--recursive | --recurse-submodules] [--jobs <n>] [--] <repository>
+	  [--recursive | --recurse-submodules] [--jobs <n>]
+	  [--init-submodule <submodulespec>] [--] <repository>
 	  [<directory>]
 
 DESCRIPTION
@@ -205,12 +206,23 @@ objects from the source repository into a pack in the cloned repository.
 
 --recursive::
 --recurse-submodules::
-	After the clone is created, initialize all submodules within,
-	using their default settings. This is equivalent to running
-	`git submodule update --init --recursive` immediately after
-	the clone is finished. This option is ignored if the cloned
-	repository does not have a worktree/checkout (i.e. if any of
-	`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+	After the clone is created, initialize and clone all submodules
+	within, using their default settings. This is equivalent to
+	running `git submodule update --recursive --init <submodulespec>`
+	immediately after the clone is finished. This option is ignored
+	if the cloned repository does not have a worktree/checkout (i.e.
+	if any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+
+--init-submodule::
+	After the clone is cloned, initialize and clone specified
+	submodules within, using their default settings. It is possible
+	to give multiple specifications by giving this argument multiple
+	times or by giving a comma separated list. This is equivalent to
+	running `git submodule update <submodulespec>` immediately
+	after the clone is finished. To specify submodules you can use
+	their path, name or labels, see linkgit:git-submodules[1]. This
+	option will be recorded in the repository config as
+	`submodule.updateGroup`.
 
 --separate-git-dir=<git dir>::
 	Instead of placing the cloned repository where it is supposed
diff --git a/builtin/clone.c b/builtin/clone.c
index 6576ecf..fa2f989 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,13 +749,22 @@ 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 && (option_recursive || init_submodules.nr > 0)) {
 		struct argv_array args = ARGV_ARRAY_INIT;
-		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
+		struct string_list_item *item;
+		argv_array_pushl(&args, "submodule", "update", NULL);
+
+		argv_array_pushf(&args, "--init");
+
+		if (option_recursive)
+			argv_array_pushf(&args, "--recursive");
 
 		if (max_jobs != -1)
 			argv_array_pushf(&args, "--jobs=%d", max_jobs);
 
+		for_each_string_list_item(item, &init_submodules)
+			argv_array_push(&args, item->string);
+
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
@@ -876,6 +903,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.updateGroup=%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 41e65c2..e7b6c1f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1192,4 +1192,83 @@ test_expect_success 'submodule update --init-default-group' '
 	test_cmp expect actual
 '
 
+cat <<EOF > expected
+-sub0
+ sub1 (test2)
+-sub2
+ sub3 (test2)
+EOF
+
+test_expect_success 'clone --init-submodule works' '
+	test_when_finished "rm -rf super super_clone" &&
+	git clone --recurse-submodules --init-submodule \*bit1 labeledsuper super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c 1,43- >../actual
+	) &&
+	test_cmp actual expected
+'
+
+cat <<EOF > expect
+ sub0 (test2)
+ sub1 (test2)
+-sub2
+ sub3 (test2)
+EOF
+test_expect_success 'clone with multiple --init-submodule options' '
+	test_when_finished "rm -rf super super_clone" &&
+	git clone --recurse-submodules --init-submodule=\*bit1 --init-submodule ./sub0 labeledsuper super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c1,43- >../actual
+	) &&
+	test_cmp expect actual
+'
+
+cat <<EOF > expect
+ submoduleA (test2)
+-submoduleB
+EOF
+
+cat <<EOF > expect2
+ submoduleA (test2)
+-submoduleB
+ submoduleC (test2)
+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"/sub1 submoduleA &&
+		git submodule add file://"$pwd"/sub1 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,43- >../actual
+	) &&
+	test_cmp expect actual &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --label LA file://"$pwd"/sub1 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-default-group &&
+		git submodule status |cut -c1,43- >../actual
+	) &&
+	test_cmp expect2 actual
+'
+
 test_done
-- 
2.8.0.35.g58985d9.dirty

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I started from scratch as I think there were some sharp edges in the design.
> My thinking shifted from "submodule groups" towards "actually it's just an
> enhanced pathspec, called submodulespec".

Except for minor things I mentioned separately, overall, this seems
quite cleanly done.

Looks very promising.

Thanks.

Re: [PATCH 0/7] submodule groups

[Stefan Beller]

On Tue, May 10, 2016 at 7:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I started from scratch as I think there were some sharp edges in the design.
>> My thinking shifted from "submodule groups" towards "actually it's just an
>> enhanced pathspec, called submodulespec".
>
> Except for minor things I mentioned separately, overall, this seems
> quite cleanly done.

I disagree (now).

I started documenting the <submodulespec> as an extension of
the pathspecs. While I thought the logical OR was the right way to go,
I think it is wrong now. So there is stuff in tests like:

    git submodule--helper matches-submodulespec sub0 ./.
./:(exclude)*0 *label-sub0

which should test if the first argument (sub0) matches the submodulespec
which follows. And it matches sub0 by matching the label, although
we told it to ignore anything ending in 0

So I wonder if we rather want to extend the pathspec magic to
include properties of blobs (i.e. submodules):

    git <command> . :(sub-label:label-sub0) :(exclude)*0

would look much more powerful too me. Properties of blobs
may also be interesting for otherwise. Imagine looking for huge files
(in a bare repo, so you have to use Git and not your shell tools):

  git ls-files . :(file-size:>1024k)

>
> Looks very promising.
>

Thanks for the encouragement!

Thanks,
Stefan

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So I wonder if we rather want to extend the pathspec magic to
> include properties of blobs (i.e. submodules):
>
>     git <command> . :(sub-label:label-sub0) :(exclude)*0
>
> would look much more powerful too me. Properties of blobs
> may also be interesting for otherwise. Imagine looking for huge files
> (in a bare repo, so you have to use Git and not your shell tools):
>
>   git ls-files . :(file-size:>1024k)

I somehow do not think this is a way normal people (read: end users)
would want to interact with Git.  Pathspec is about "paths" and
various ways to match them.  It is not about contents that happens
to be currently named by that path.  Don't tie types or sizes to it.

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

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

>>   git ls-files . :(file-size:>1024k)
>
> I somehow do not think this is a way normal people (read: end users)
> would want to interact with Git.  Pathspec is about "paths" and
> various ways to match them.  It is not about contents that happens
> to be currently named by that path.  Don't tie types or sizes to it.

To clarify, think what that non-pathspec means when used like this:

    $ git diff :(size>1M)
    $ git log --follow :(size>1M)

Which side of comparison does the "size" thing apply?  Either, both,
randomly?  More importantly, what use case of users do these
commands serve?

That is why I said that pathspec should never consider anything but
the pathname string you see.

Re: [PATCH 0/7] submodule groups

[Stefan Beller]

On Wed, May 11, 2016 at 4:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>>   git ls-files . :(file-size:>1024k)
>>
>> I somehow do not think this is a way normal people (read: end users)
>> would want to interact with Git.  Pathspec is about "paths" and
>> various ways to match them.  It is not about contents that happens
>> to be currently named by that path.  Don't tie types or sizes to it.
>
> To clarify, think what that non-pathspec means when used like this:
>
>     $ git diff :(size>1M)
>     $ git log --follow :(size>1M)
>
> Which side of comparison does the "size" thing apply?  Either, both,
> randomly?  More importantly, what use case of users do these
> commands serve?
>
> That is why I said that pathspec should never consider anything but
> the pathname string you see.

I see. That is bad indeed. So I'll go back and finish the submodulespec
to present.

Thanks,
Stefan

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>     git submodule--helper matches-submodulespec sub0 ./.
> ./:(exclude)*0 *label-sub0
>
> which should test if the first argument (sub0) matches the submodulespec
> which follows.

This, according to that "OR'ed together" definition, asks to find a
submodule

    - whose path matches pathspec ./. ./:(exclude)*0; or
    - is labeled with label-sub0.

So I'd say it is natural sub0 matches if its path is at sub0 and has
a label label-sub0.

You could instead choose to use "AND'ed together" semantics, but
that would break expectation by those who expect "This OR that"
behaviour.  Unless you are willing to support --and/--or/(/) like
"git grep" does to express a way to combine hits from individual
terms, that is an inherent limitation.

I'd suggest not to over-engineer this.  Go back and imagine how
"/bin/ls" would work is a good sanity check to gauge what complexity
levels ordinary users would feel comfortable to handle.

"ls a b" would give union of what "ls a" and "ls b" would output,
there is no negation, and the users won't die from having to say "ls
[A-Za-qs-z]*" to exclude files whose names begin with 'r'.

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

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

> I'd suggest not to over-engineer this.  Go back and imagine how
> "/bin/ls" would work is a good sanity check to gauge what complexity
> levels ordinary users would feel comfortable to handle.
>
> "ls a b" would give union of what "ls a" and "ls b" would output,
> there is no negation, and the users won't die from having to say "ls
> [A-Za-qs-z]*" to exclude files whose names begin with 'r'.

Having said all that, there is one thing we may want to consider.

For negative pathspecs, we deliberately defined its semantics to
"see if the path matches with any positive pathspec element (and it
is a non-match if no positive one matches), and then exclude if it
matches with any negative pathspec element".

That is, when you are matching 'x.o' against three-element pathspec 

	'*.c' ':(exclude)*.o' 'x.?'

you do not say "x.o does not match *.c, but it matches *.o so it is
to be excluded, ah, wait, x.? matches to revive it so it is a
match".  Instead "*.c does not and x.? does match, but *.o matches
so it is excluded".  IOW, the order of the pathspec elements given
on the command line does not matter.

Now we are adding two different criteria to specify inclusion based
on labels and names.

As implemented in the patch series under discussion, we are saying
that a submodule is included if

    - its path matches the pathspec (using the "matches one or more
      positive pathspec elements, and does not match any negaative
      pathspec element" as the definition of "matches"), OR

    - it is included in the specified set of *labels, OR

    - its name is specified by :name

There is no reason not to consider future enhancements to specify
exclusion base on these two new criretia.  A naive and easier to
implement enhancement would be to rewrite the above to (the latter
two item changes):


    - its path matches the pathspec (using the "matches one or more
      positive pathspec elements, and does not match any negaative
      pathspec element" as the definition of "matches"), OR

    - it is included in the specified set of *labels, and does not
      have any excluded *!labels, OR

    - its name is specified by :name, and is not among any excluded
      :!name.

but it does not have to be that way.  I suspect that it may make it
easier to understand and use if we OR'ed together only the positive
ones from three classes of criteria together and require at least
one of them to match, and then requiring none of the negative ones
to match.  That is, a module-spec with three elements:

     'sub*' '*label0' './:(exclude)*0'

with the implemented algorithm would choose submodules whose name
begins with 'sub' except the ones whose name ends with '0', OR those
with label0, but if we redefine the behaviour to "positive ones
together, and then exclude negative ones", then we would choose ones
whose name begins with 'sub' or ones with label0, and among these,
exclude those whose name ends with '0' (which is what your "test"
wanted to express).

The implementation of match_pathspec() does the "first positive ones
only and then negative ones" two-step process already, so I think
you could update its "int is_dir" argument to "unsigned flags",
introduce a "negative only" flag, and then do something like:

        for each path
                if (!(match_pathspec(ps, name, ..., 0) ||
                      has a "positive" label specified ||
                      has its name specified as a "postiive")
                        /* does not match any positive criteria */
                        continue;
                if (match_pathspec(ps, name, ..., NEGATIVE_ONLY) ||
                    has a "negative" label specified ||
                    has its name specified as a "negative")
                        /* explicitly excluded */
                        continue;
                /* included! */

That would open door to something like

     'sub*' '*label0' './:(exclude)*0' '*!label1'

to allow you to say "(those with label0 or whose path begins with
sub) minus (those with label1 or whose path ends with 0)".

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

I hate to bring this up in this thread, primarily because I cannot
see how to make it mesh well with the "submodule spec lets you
specify a group of submodule with labels", but for completeness of
discussion, I'll mention it anyway.

Instead of specifying "all files written in Perl" to work on by
giving a pathspec with three elements, e.g.

    git cmd -- \*.perl \*.pl \*.pm

I've often wondered if it would be a good idea to let attributes
file to specify "these paths form the group Perl" with something
like:

    *.pm        group=perl
    *.pl        group=perl
    *.perl      group=perl
    *.h         group=c
    *.c         group=c

and say

    git cmd -- ':(group=perl)'

instead.

The reason why I suspect that this may not work well with submodule
labels is because submodule labels (or any attribute we give via
.gitmodules to a submodule) are keyed by a submodule name, which is
the primary unchanging key (so that people can "mv" a submodule in
the context of the toplevel superproject without losing track of
submodule identity), not by paths to submodules, while the "group"
thing I want is merely a short-hand for pathspec elements and wants
to be keyed by paths.

But there may be somebody more clever than I who can come up with a
way to unify these two similar concepts without confusing end users.

Re: [PATCH 0/7] submodule groups

[Stefan Beller]

On Wed, May 11, 2016 at 10:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>     git cmd -- \*.perl \*.pl \*.pm
>
> I've often wondered if it would be a good idea to let attributes
> file to specify "these paths form the group Perl" with something
> like:
>
>     *.pm        group=perl
>     *.pl        group=perl
>     *.perl      group=perl
>     *.h         group=c
>     *.c         group=c
>
> and say
>
>     git cmd -- ':(group=perl)'
>
> instead.

How is that different to the file size example I gave earlier?
You could have a change which includes:

    -    *.pl    group=prolog
    +    *.pl   group=perl

What happens in the diff/log case then (just as a file can pass an
arbitrary file size within that change) ? These file attributes are as
arbitrary as sizes as well as submodule labels or names.

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

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

> The reason why I suspect that this may not work well with submodule
> labels is because submodule labels (or any attribute we give via
> .gitmodules to a submodule) are keyed by a submodule name, which is
> the primary unchanging key (so that people can "mv" a submodule in
> the context of the toplevel superproject without losing track of
> submodule identity), not by paths to submodules, while the "group"
> thing I want is merely a short-hand for pathspec elements and wants
> to be keyed by paths.
>
> But there may be somebody more clever than I who can come up with a
> way to unify these two similar concepts without confusing end users.

Thinking about this even more, if there is no requirement that
labels must be tied to submodule names, we just can scrap the idea
of "submodule labels" to group things and instead just use "path
labels", i.e. write the full path to the submodule and assign it a
label in .gitattributes and use it in place of where we used *label
in the patch series.  After all, an easy way to choose some among
many submodules logically is a subset of an easy way to choose some
among many paths.

The only reason why we added the submodule label to .gitmodules is
because we viewed it as submodule-specific thing and the "keyed by
name, not path" came as a consequence, not because any real "we must
key it by name because..." reason, I would think.

I know this is a huge departure from the design presented both at
the conceptual level and also at the implementation level, and that
is one of the reasons why I do not particularly want to like it, but
on the other hand, I am not bold enough to say that I will have a
good answer when later somebody asks "Why can we group only
submodules with labels, but not random group of paths (e.g. 'these
directories are about documentation')?"  And then, if we add path
labels to allow expressing groups of paths later, the follow-up
question would be "When should I use submodule labels and when
should I use path labels?  I can use path labels to group submodules
and say 'git submodule update -- :(group=docs)' can't I?".

And that makes me pause and step back from the "submodule labels"
idea.

Re: [PATCH 0/7] submodule groups

[Stefan Beller]

On Thu, May 12, 2016 at 8:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The reason why I suspect that this may not work well with submodule
>> labels is because submodule labels (or any attribute we give via
>> .gitmodules to a submodule) are keyed by a submodule name, which is
>> the primary unchanging key (so that people can "mv" a submodule in
>> the context of the toplevel superproject without losing track of
>> submodule identity), not by paths to submodules, while the "group"
>> thing I want is merely a short-hand for pathspec elements and wants
>> to be keyed by paths.
>>
>> But there may be somebody more clever than I who can come up with a
>> way to unify these two similar concepts without confusing end users.
>
> Thinking about this even more, if there is no requirement that
> labels must be tied to submodule names, we just can scrap the idea
> of "submodule labels" to group things and instead just use "path
> labels", i.e. write the full path to the submodule and assign it a
> label in .gitattributes and use it in place of where we used *label
> in the patch series.  After all, an easy way to choose some among
> many submodules logically is a subset of an easy way to choose some
> among many paths.
>
> The only reason why we added the submodule label to .gitmodules is
> because we viewed it as submodule-specific thing and the "keyed by
> name, not path" came as a consequence, not because any real "we must
> key it by name because..." reason, I would think.
>
> I know this is a huge departure from the design presented both at
> the conceptual level and also at the implementation level, and that
> is one of the reasons why I do not particularly want to like it, but
> on the other hand, I am not bold enough to say that I will have a
> good answer when later somebody asks "Why can we group only
> submodules with labels, but not random group of paths (e.g. 'these
> directories are about documentation')?"  And then, if we add path
> labels to allow expressing groups of paths later, the follow-up
> question would be "When should I use submodule labels and when
> should I use path labels?  I can use path labels to group submodules
> and say 'git submodule update -- :(group=docs)' can't I?".
>
> And that makes me pause and step back from the "submodule labels"
> idea.
>

It sounds better at first (and I haven't thought further).
So if we were to go with this idea:

Label paths (or even pathspecs?) in the .gitattributes file.

I think it is important to keep the property of defining the labeling
in the tree, so you can follow upstream easier.

I tried coming up with an example for labels for generic paths,
but it looks like most of the time it is a substitution for a pathspec,
I did not find a convincing example which makes it easier to use.

`:(group=docs)` in the non submodule case could be expressed
as `Documentation/**`. Well maybe we also want to include README
and some other files which need to stay outside the Documentation
directory, so I can see how it may be useful.

We do not need a special labeling command. We do not
ship with a command which writes the .gitattributes or .gitignore
for you, and labels don't require this. So I could drop the patch
for "submodule add --label".

We can still keep the submodule.defaultGroup. (In the WIP I renamed
it to updateGroup as its only feature is to have it set during clone
and remebered for `git submodule update`)

When we allow labels to be generic path labels instead of submodule
labels, the user might be tempted to ask, why the submodules can
be specified but not the individual paths, i.e.

    git clone --init-submodule="(:group=docs)" ...

may strongly hint at:

    git clone --narrow="(:group=docs)" ...

would only get parts of the repository.

For the submodule case, this may add confusion as the user would
need to configure some properties in the .gitmodules file and some
in the .gitattributes file.

I think I'll try implementing a mock and see how much code it is for
a more fundamental pathspec extension.

Thanks,
Stefan

Re: [PATCH 0/7] submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> We can still keep the submodule.defaultGroup. (In the WIP I renamed
> it to updateGroup as its only feature is to have it set during clone
> and remebered for `git submodule update`)

Yes, my idle speculation between "[submodule"x"] label=A" stored
in .gitmodules and "path/to/submodule  group=A" in .gitattributes
is completely orthogonal to the need for submodule.defaultGroup.

The only difference it brings in is how we evaluate what
submodule.defaultGroup names (i.e. via "submodule label" vs
"path label").

> When we allow labels to be generic path labels instead of submodule
> labels, the user might be tempted to ask, why the submodules can
> be specified but not the individual paths, i.e.
>
>     git clone --init-submodule="(:group=docs)" ...
>
> may strongly hint at:
>
>     git clone --narrow="(:group=docs)" ...
>
> would only get parts of the repository.

If we are to have a (proper) narrow clone, I do not think there is
any reason why the latter should not work as expected.

> For the submodule case, this may add confusion as the user would
> need to configure some properties in the .gitmodules file and some
> in the .gitattributes file.

I do not quite understand this comment.

A new reader who knows nothing about "grouping submodules" would not
have any trouble, I suspect, if all and the only grouping she learns
is "grouping paths" from the beginning.

Thanks.