threads / patch / 42279

patch, 7 partssubmodule add: label submodules if asked to

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

## tl;dr

30 messages between May 11, 2016 and May 12, 2016. Diffs are folded; open one to read it.

replies: 29people: 2as markdown or json

Stefan Beller· May 11, 2016, 00:59 UTC · lore

[PATCH 0/7] submodule groups

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
Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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

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
Show changes to 2 files +78 −1

builtin/submodule--helper.c, 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
Junio C Hamano· May 11, 2016, 01:11 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> +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.
Show 6 quoted lines
> +			return 0;
> +		label++;
> +	}
> +
> +	return 1;
> +}

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

Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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(-)
Show changes to 3 files +62 −2

Documentation/git-submodule.txt, git-submodule.sh, t/t7400-submodule-basic.sh

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
Junio C Hamano· May 11, 2016, 01:13 UTC · re: Stefan Beller · lore

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

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

Stefan Beller· May 11, 2016, 17:26 UTC · re: Junio C Hamano · lore

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

Show 11 quoted lines
>> +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.

Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to 2 files +18 −0

submodule-config.c, submodule-config.h

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
Junio C Hamano· May 11, 2016, 01:15 UTC · re: Stefan Beller · lore

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

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

Show 16 quoted lines
> @@ -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);
> +		}
>  	}
Stefan Beller· May 11, 2016, 17:41 UTC · re: Junio C Hamano · lore

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

On Tue, May 10, 2016 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> 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).

Junio C Hamano· May 11, 2016, 21:28 UTC · re: Stefan Beller · lore

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

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

Stefan Beller· May 11, 2016, 21:39 UTC · re: Junio C Hamano · lore

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

+Heiko
On Wed, May 11, 2016 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> 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.

Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 4 files +152 −1

builtin/submodule--helper.c, submodule-config.c, submodule-config.h, t/t7412-submodule--helper.sh

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
Junio C Hamano· May 11, 2016, 01:19 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 6 quoted lines
> +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).

Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 4 files +175 −17

Documentation/git-submodule.txt, builtin/submodule--helper.c, git-submodule.sh, t/t7412-submodule--helper.sh

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
Junio C Hamano· May 11, 2016, 01:29 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 12 quoted lines
> +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".

Show 10 quoted lines
> +		} 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]);
> +		}
> +	}
Show 20 quoted lines
> +		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?

Junio C Hamano· May 11, 2016, 02:24 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> 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*".

Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 4 files +72 −4

Documentation/config.txt, Documentation/git-submodule.txt, git-submodule.sh, t/t7400-submodule-basic.sh

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
Stefan Beller· May 11, 2016, 00:59 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 3 files +136 −9

Documentation/git-clone.txt, builtin/clone.c, t/t7400-submodule-basic.sh

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
Junio C Hamano· May 11, 2016, 02:08 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] submodule groups

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.
Stefan Beller· May 11, 2016, 23:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

On Tue, May 10, 2016 at 7:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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

Junio C Hamano· May 11, 2016, 23:39 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] submodule groups

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

Junio C Hamano· May 11, 2016, 23:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
>>   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.

Stefan Beller· May 11, 2016, 23:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

On Wed, May 11, 2016 at 4:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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

Junio C Hamano· May 12, 2016, 00:00 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] submodule groups

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
>     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'.

Junio C Hamano· May 12, 2016, 04:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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)".

Junio C Hamano· May 12, 2016, 05:50 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] submodule groups

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

Stefan Beller· May 12, 2016, 15:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

On Wed, May 11, 2016 at 10:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
>
>     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.

Junio C Hamano· May 12, 2016, 15:58 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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.

Stefan Beller· May 12, 2016, 16:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] submodule groups

On Thu, May 12, 2016 at 8:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 43 quoted lines
> 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

Junio C Hamano· May 12, 2016, 16:53 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] submodule groups

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

Show 11 quoted lines
> 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.

← back to recent threads