[PATCH 0/4] Submodule Groups

[PATCH 0/4] Submodule Groups

[Stefan Beller]

This applies on top of the updated sb/submodule-init branch.

Why?
====
Consider having a real large software project in Git with each component
in a submodule (such as an operating system, Android, Debian, Fedora,
no toy OS such as https://github.com/gittup/gittup as that doesn't quite
demonstrate the scale of the problem).

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 meaning to a "groups" field in the .gitmodules file.

So you could have a .gitmodules file such as:

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

and by this series you can work on an arbitrary subgroup of these submodules such
using these commands:
 
    git submodule add --group default --group devel git://... ..
    # will add a submodule, adding 2 submodule
    # groups to its entry in .gitmodule (default and devel)
    
    
    git submodule update --groups
    # All submodules as selected by groups (submodule.groups in .git/config)
    # will be initialized if they are not, before updating.
    
    git clone --group default,default2 --group devel git://...
    # will clone the superproject and recursively
    # checkout any submodule being in at least one of the groups (default, default2, devel)


Changes to v1
=============

* All entries are stored as separate lines (both in .git/config as well as in
  the .gitmodules file)

* No harm is done to `init` as it is implied by `update` now. :)

* I tried to keep it as simple as possible (update and clone being the minimal set
  of supported commands required, `add --group` being syntactic sugar for editing
  the .gitmodules file.

This is also available at https://github.com/stefanbeller/git/tree/submodule-groups-v2

Thanks,
Stefan


Stefan Beller (4):
  git submodule: Teach add to accept --group
  submodule-config: keep groups around
  submodule update: Initialize all group-selected submodules by default
  builtin/clone: support submodule groups

 Documentation/git-clone.txt     |  13 +++++
 Documentation/git-submodule.txt |   8 ++-
 builtin/clone.c                 |  46 +++++++++++++++--
 builtin/submodule--helper.c     |  30 ++++++++++-
 git-submodule.sh                |  15 ++++++
 submodule-config.c              |  13 +++++
 submodule-config.h              |   1 +
 t/t7400-submodule-basic.sh      | 112 ++++++++++++++++++++++++++++++++++++++++
 8 files changed, 233 insertions(+), 5 deletions(-)

-- 
2.7.0.rc0.41.g89994f2.dirty

[PATCH 1/4] git submodule: Teach add to accept --group

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt |  8 +++++++-
 git-submodule.sh                | 15 +++++++++++++++
 t/t7400-submodule-basic.sh      | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 13adebf..135c739 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] [-g <group>][--name <name>]
 	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
@@ -59,6 +59,9 @@ instead of treating the other project as a submodule. Directories
 that come from both projects can be cloned and checked out as a whole
 if you choose to go that route.
 
+If you manage a large set of submodules, but do not require all of them
+to be checked out, you should look into the submodule groups feature.
+
 COMMANDS
 --------
 add::
@@ -101,6 +104,9 @@ is the superproject and submodule repositories will be kept
 together in the same relative location, and only the
 superproject's URL needs to be provided: git-submodule will correctly
 locate the submodule using the relative URL in .gitmodules.
++
+If at least one group argument was given, all groups are recorded in the
+.gitmodules file in the groups field.
 
 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 6fce0dc..ab0f209 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -130,6 +130,7 @@ cmd_add()
 {
 	# parse $args after "submodule ... add".
 	reference_path=
+	submodule_groups=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -165,6 +166,10 @@ cmd_add()
 		--depth=*)
 			depth=$1
 			;;
+		-g|--group)
+			submodule_groups=${submodule_groups:+${submodule_groups};}"$2"
+			shift
+			;;
 		--)
 			shift
 			break
@@ -292,6 +297,16 @@ 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" &&
+	if test -n "$submodule_groups"
+	then
+		OIFS=$IFS
+		IFS=';'
+		for group in $submodule_groups
+		do
+			git config --add -f .gitmodules submodule."$sm_name".group "${group}"
+		done
+		IFS=$OIFS
+	fi &&
 	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 5991e3c..b468278 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -986,6 +986,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) &&
 	(
@@ -999,5 +1000,36 @@ test_expect_success 'submodule add clone shallow submodule' '
 	)
 '
 
+test_expect_success 'submodule add records a group' '
+	test_when_finished "rm -rf super" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --group groupA file://"$pwd"/example2 submodule &&
+		git config -f .gitmodules submodule."submodule".group >actual &&
+		echo groupA >expected &&
+		test_cmp expected actual
+	)
+'
+
+cat >expected <<-EOF
+groupA
+groupB
+EOF
+
+test_expect_success 'submodule add records groups' '
+	test_when_finished "rm -rf super" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --group groupA -g groupB file://"$pwd"/example2 submodule &&
+		git config --get-all -f .gitmodules submodule."submodule".group >../actual
+	) &&
+	test_cmp expected actual
+'
 
 test_done
-- 
2.7.0.rc0.41.g89994f2.dirty

[PATCH 2/4] submodule-config: keep groups around

[Stefan Beller]

We need the submodule groups in a later patch.

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

diff --git a/submodule-config.c b/submodule-config.c
index a32259e..b5453d0 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -60,6 +60,8 @@ static void free_one_config(struct submodule_entry *entry)
 {
 	free((void *) entry->config->path);
 	free((void *) entry->config->name);
+	if (entry->config->groups)
+		string_list_clear(entry->config->groups, 0);
 	free(entry->config);
 }
 
@@ -184,6 +186,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	submodule->update = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
+	submodule->groups = NULL;
 
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -324,6 +327,16 @@ static int parse_specific_submodule_config(const char *subsection, int subsectio
 			free((void *) submodule->update);
 			submodule->update = xstrdup(value);
 		}
+	} else if (!strcmp(key, "group")) {
+		if (!value)
+			ret = config_error_nonbool(var);
+		else {
+			if (!submodule->groups) {
+				submodule->groups = xmalloc(sizeof(*submodule->groups));
+				string_list_init(submodule->groups, 1);
+			}
+			string_list_insert(submodule->groups, value);
+		}
 	}
 
 	return ret;
diff --git a/submodule-config.h b/submodule-config.h
index d9bbf9a..332f5be 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -17,6 +17,7 @@ struct submodule {
 	const char *update;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
+	struct string_list *groups;
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-- 
2.7.0.rc0.41.g89994f2.dirty

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

[Stefan Beller]

All submodules which are selected via submodule groups
("submodule.groups") are initialized if they were not initialized
before updating the submodules.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 30 +++++++++++++++++++++++++++++-
 t/t7400-submodule-basic.sh  | 26 ++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4684f16..def382e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -577,6 +577,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 
 struct submodule_update_clone {
 	/* states */
+	struct string_list *submodule_groups;
 	int count;
 	int print_unmatched;
 	/* configuration */
@@ -590,7 +591,7 @@ struct submodule_update_clone {
 	struct string_list projectlines;
 	struct pathspec pathspec;
 };
-#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
+#define SUBMODULE_UPDATE_CLONE_INIT {NULL, 0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
 
 static void fill_clone_command(struct child_process *cp, int quiet,
 			       const char *prefix, const char *path,
@@ -633,6 +634,7 @@ static int update_clone_get_next_task(struct child_process *cp,
 		const char *update_module = NULL;
 		char *url = NULL;
 		int needs_cloning = 0;
+		int in_submodule_groups = 0;
 
 		if (ce_stage(ce)) {
 			if (pp->recursive_prefix)
@@ -676,6 +678,22 @@ static int update_clone_get_next_task(struct child_process *cp,
 		strbuf_reset(&sb);
 		strbuf_addf(&sb, "submodule.%s.url", sub->name);
 		git_config_get_string(sb.buf, &url);
+		if (pp->submodule_groups && sub->groups){
+			struct string_list_item *item;
+			for_each_string_list_item(item, sub->groups) {
+				if (string_list_has_string(
+				    pp->submodule_groups, item->string)) {
+					in_submodule_groups = 1;
+					break;
+				}
+			}
+			if (in_submodule_groups) {
+				if (!url) {
+					init_submodule(sub->name, pp->prefix, pp->quiet);
+					url = xstrdup(sub->url);
+				}
+			}
+		}
 		if (!url) {
 			/*
 			 * Only mention uninitialized submodules when its
@@ -687,6 +705,7 @@ static int update_clone_get_next_task(struct child_process *cp,
 			continue;
 		}
 
+
 		strbuf_reset(&sb);
 		strbuf_addf(&sb, "%s/.git", ce->name);
 		needs_cloning = !file_exists(sb.buf);
@@ -742,6 +761,7 @@ static int update_clone_task_finished(int result,
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
 	int max_jobs = -1;
+	const struct string_list *list;
 	struct string_list_item *item;
 	struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -786,6 +806,14 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	/* Overlay the parsed .gitmodules file with .git/config */
 	git_config(git_submodule_config, NULL);
 
+	list = git_config_get_value_multi("submodule.groups");
+	if (list) {
+		pp.submodule_groups = xmalloc(sizeof(*pp.submodule_groups));
+		string_list_init(pp.submodule_groups, 1);
+		for_each_string_list_item(item, list)
+			string_list_insert(pp.submodule_groups, item->string);
+	}
+
 	if (max_jobs < 0)
 		max_jobs = config_parallel_submodules();
 	if (max_jobs < 0)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b468278..e6a008c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1032,4 +1032,30 @@ test_expect_success 'submodule add records groups' '
 	test_cmp expected actual
 '
 
+cat <<EOF > expected
+submodule
+-submodule1
+EOF
+
+test_expect_success 'submodule update works with groups implied' '
+	test_when_finished "rm -rf super super_clone" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --group groupA file://"$pwd"/example2 submodule &&
+		git submodule add file://"$pwd"/example2 submodule1 &&
+		git commit -a -m "create repository with 2 submodules, one is in a group"
+	) &&
+	git clone super super_clone &&
+	(
+		cd super_clone &&
+		git config submodule.groups groupA &&
+		git submodule update &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp actual expected
+'
+
 test_done
-- 
2.7.0.rc0.41.g89994f2.dirty

[PATCH 4/4] builtin/clone: support submodule groups

[Stefan Beller]

When invocing clone with the groups option,
the repository will be configured to the specified groups.
This has implications for the submodule commands, such as
update.

Having groups configured will imply init for all uninitialized
submodules belonging to at least one of the configured groups,
such that new submodules in the group are initialized
automatically.

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

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6db7b6d..685fb7c 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -214,6 +214,19 @@ objects from the source repository into a pack in the cloned repository.
 	repository does not have a worktree/checkout (i.e. if any of
 	`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
 
+--group::
+	After the clone is created, all submodules which are part of the
+	given groups are cloned. To specify multiple groups, you can either
+	give the group argument multiple times or comma separate the groups.
+	This option will be recorded in the `submodule.groups` config,
+	which will affect the behavior of other submodule related commands,
+	such as `git submodule update`.
+	This option implies recursive submodule checkout. If you don't
+	want to recurse into nested submodules, you need to specify
+	`--no-recursive`. The group selection will be passed on recursively,
+	i.e. if a submodule is cloned because of group membership, its
+	submodules will be cloned according to group membership, too.
+
 --separate-git-dir=<git dir>::
 	Instead of placing the cloned repository where it is supposed
 	to be, place the cloned repository at the specified directory,
diff --git a/builtin/clone.c b/builtin/clone.c
index b004fb4..72aea3a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,6 +51,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 submodule_groups;
+
+static int groups_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),
@@ -95,6 +111,8 @@ static struct option builtin_clone_options[] = {
 		   N_("separate git dir from working tree")),
 	OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
 			N_("set config inside the new repository")),
+	OPT_CALLBACK('g', "group", &submodule_groups, N_("string"),
+			N_("clone specific submodule groups"), groups_cb),
 	OPT_END()
 };
 
@@ -723,9 +741,17 @@ static int checkout(void)
 	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
 			   sha1_to_hex(sha1), "1", NULL);
 
-	if (!err && option_recursive) {
+	if (err)
+		goto out;
+
+	if (option_recursive || submodule_groups.nr > 0) {
 		struct argv_array args = ARGV_ARRAY_INIT;
-		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
+		argv_array_pushl(&args, "submodule", "update", NULL);
+
+		if (option_recursive) {
+			argv_array_pushf(&args, "--init");
+			argv_array_pushf(&args, "--recursive");
+		}
 
 		if (max_jobs != -1)
 			argv_array_pushf(&args, "--jobs=%d", max_jobs);
@@ -733,7 +759,7 @@ static int checkout(void)
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
-
+out:
 	return err;
 }
 
@@ -867,6 +893,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			die(_("--bare and --separate-git-dir are incompatible."));
 		option_no_checkout = 1;
 	}
+	if (option_recursive == -1) {
+		if (submodule_groups.nr > 0)
+			option_recursive = 1; /* submodule groups implies recursive */
+		else
+			option_recursive = 0; /* preserve historical default */
+	}
+	if (submodule_groups.nr > 0) {
+		struct string_list_item *item;
+		struct strbuf sb = STRBUF_INIT;
+		for_each_string_list_item(item, &submodule_groups) {
+			strbuf_addf(&sb, "submodule.groups=%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 e6a008c..5ed5250 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1058,4 +1058,58 @@ test_expect_success 'submodule update works with groups implied' '
 	test_cmp actual expected
 '
 
+cat <<EOF > expected
+submodule
+-submodule1
+EOF
+
+test_expect_success 'clone --group works' '
+	test_when_finished "rm -rf super super_clone" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --group groupA file://"$pwd"/example2 submodule &&
+		git submodule add file://"$pwd"/example2 submodule1 &&
+		git commit -a -m "create repository with 2 submodules, one is in a group"
+	) &&
+	git clone --group groupA super super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp actual expected
+'
+
+cat <<EOF > expected
+-submodule1
+submoduleA
+submoduleB
+submoduleC
+-submoduleD
+EOF
+
+test_expect_success 'clone with multiple groups works' '
+	test_when_finished "rm -rf super super_clone" &&
+	mkdir super &&
+	pwd=$(pwd) &&
+	(
+		cd super &&
+		git init &&
+		git submodule add --group groupA file://"$pwd"/example2 submoduleA &&
+		git submodule add --group groupB file://"$pwd"/example2 submoduleB &&
+		git submodule add --group groupC file://"$pwd"/example2 submoduleC &&
+		git submodule add --group groupD file://"$pwd"/example2 submoduleD &&
+		git submodule add file://"$pwd"/example2 submodule1 &&
+		git commit -a -m "create repository with submodules groups"
+	) &&
+	git clone --group groupA,groupB --group groupC super super_clone &&
+	(
+		cd super_clone &&
+		git submodule status |cut -c1,42-52 | tr -d " " >../actual
+	) &&
+	test_cmp actual expected
+'
+
 test_done
-- 
2.7.0.rc0.41.g89994f2.dirty

Re: [PATCH 1/4] git submodule: Teach add to accept --group

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 6fce0dc..ab0f209 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -130,6 +130,7 @@ cmd_add()
>  {
>  	# parse $args after "submodule ... add".
>  	reference_path=
> +	submodule_groups=

This can just be called $groups in the context of this script.  I do
not foresee we would be planning to deal with other kinds of groups
here.

>  	while test $# -ne 0
>  	do
>  		case "$1" in
> @@ -165,6 +166,10 @@ cmd_add()
>  		--depth=*)
>  			depth=$1
>  			;;
> +		-g|--group)
> +			submodule_groups=${submodule_groups:+${submodule_groups};}"$2"
> +			shift
> +			;;

You would want to accept "--group=<name>" as well, just like
existing --reference and --depth do.  It won't be much more code,
and when you move to C (hence parse_options) you'd get it for free
anyway.

> @@ -292,6 +297,16 @@ 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" &&
> +	if test -n "$submodule_groups"
> +	then
> +		OIFS=$IFS
> +		IFS=';'

I do not quite understand the choice of ';' here.  If and only if
you _must_ accept multi-word name that has spaces in between as a
group name, the above construct may make sense, but I do not think
we have such requirement.  Why not separate with $IFS letters just
like any other normal list managed in shell scripts do?  Is there
anything special about names of submodule groups?

Re: [PATCH 2/4] submodule-config: keep groups around

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> We need the submodule groups in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule-config.c | 13 +++++++++++++
>  submodule-config.h |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index a32259e..b5453d0 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -60,6 +60,8 @@ static void free_one_config(struct submodule_entry *entry)
>  {
>  	free((void *) entry->config->path);
>  	free((void *) entry->config->name);
> +	if (entry->config->groups)
> +		string_list_clear(entry->config->groups, 0);

You are allocating entry->config->groups itself in the in the hunk
around ll.327, so that also need to be freed, I would think.

>  	free(entry->config);
>  }
>  
> @@ -184,6 +186,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
>  	submodule->update = NULL;
>  	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
>  	submodule->ignore = NULL;
> +	submodule->groups = NULL;
>  
>  	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
>  
> @@ -324,6 +327,16 @@ static int parse_specific_submodule_config(const char *subsection, int subsectio
>  			free((void *) submodule->update);
>  			submodule->update = xstrdup(value);
>  		}
> +	} else if (!strcmp(key, "group")) {
> +		if (!value)
> +			ret = config_error_nonbool(var);
> +		else {
> +			if (!submodule->groups) {
> +				submodule->groups = xmalloc(sizeof(*submodule->groups));
> +				string_list_init(submodule->groups, 1);
> +			}
> +			string_list_insert(submodule->groups, value);
> +		}
>  	}
>  
>  	return ret;
> diff --git a/submodule-config.h b/submodule-config.h
> index d9bbf9a..332f5be 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -17,6 +17,7 @@ struct submodule {
>  	const char *update;
>  	/* the sha1 blob id of the responsible .gitmodules file */
>  	unsigned char gitmodules_sha1[20];
> +	struct string_list *groups;
>  };

Is there a case where you need to enumerate and show the groups a
submodule belongs to to the end users?  Using string_list_insert()
to manage this list would mean you will lose the original ordering
you saw the list of groups in their .gitmodules files, which may or
may not matter.  I'd assume that the ordering should not matter, but
that is something the user may want to see documented.

>  
>  int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

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

More importantly.

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

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

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

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

Re: [PATCH 4/4] builtin/clone: support submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When invocing clone with the groups option,
> the repository will be configured to the specified groups.
> This has implications for the submodule commands, such as
> update.
>
> Having groups configured will imply init for all uninitialized
> submodules belonging to at least one of the configured groups,
> such that new submodules in the group are initialized
> automatically.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git-clone.txt | 13 +++++++++++
>  builtin/clone.c             | 46 +++++++++++++++++++++++++++++++++++---
>  t/t7400-submodule-basic.sh  | 54 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 6db7b6d..685fb7c 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -214,6 +214,19 @@ objects from the source repository into a pack in the cloned repository.
>  	repository does not have a worktree/checkout (i.e. if any of
>  	`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
>  
> +--group::

This is misnamed but in an opposite way as the variables and fields
in previous patches.  "Clone" is not primarily about submodules, so
a plain "group" in the context of "clone" does not hint that this
option is about submodules.  It also does not tell the user anything
about what the parameter is for (i.e. there are submodules that are
and are not in the group that is specified with this option.  What
special things happens to the ones in the group?)

I am guessing that it is about the auto-vivifying behaviour like 3/4
did, and if that is the case, perhaps "--init-submodule=<group>" or
something like that?

Oh by the way, I think you meant this option to take an argument by
reading the description (you say "part of the given groups").  Follow
the example of existing option you see below in the context and make
sure that the reader knows they have to give an argument to it.

    A tangent. I think it would be handy to have a way to name a
    single submodule and have the code treat as if there were a
    group that contains that single submodule already defined and
    the user specified that group.  Then this option (and any other
    option that takes a submodule group name) can instead take a
    submodule name and the user can expect a natural thing to happen.

> +	After the clone is created, all submodules which are part of the
> +	given groups are cloned. To specify multiple groups, you can either
> +	give the group argument multiple times or comma separate the groups.
> +	This option will be recorded in the `submodule.groups` config,
> +	which will affect the behavior of other submodule related commands,
> +	such as `git submodule update`.
> +	This option implies recursive submodule checkout. If you don't
> +	want to recurse into nested submodules, you need to specify
> +	`--no-recursive`. The group selection will be passed on recursively,
> +	i.e. if a submodule is cloned because of group membership, its
> +	submodules will be cloned according to group membership, too.
> +

>  --separate-git-dir=<git dir>::
>  	Instead of placing the cloned repository where it is supposed
>  	to be, place the cloned repository at the specified directory,
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b004fb4..72aea3a 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -51,6 +51,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 submodule_groups;
> +
> +static int groups_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);

Why not just do "--group A --group B" without "--group A,B"?  I do
not think you accept "git submodule add --group A,B" anyway [*1*],
so doing "nice" things like this is just being inconsistent and does
not help the users.  And I do personally do not think we should make
things consistent by accepting "--group A,B" everywhere.

[Footnote]

*1* Technically, your 'add --group' accepts A;B, I think, but then
this ',' is still inconsistent with it ;-)

Re: [PATCH 1/4] git submodule: Teach add to accept --group

[Stefan Beller]

On Wed, Jan 20, 2016 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 6fce0dc..ab0f209 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -130,6 +130,7 @@ cmd_add()
>>  {
>>       # parse $args after "submodule ... add".
>>       reference_path=
>> +     submodule_groups=
>
> This can just be called $groups in the context of this script.  I do
> not foresee we would be planning to deal with other kinds of groups
> here.

ok, done.

>
>>       while test $# -ne 0
>>       do
>>               case "$1" in
>> @@ -165,6 +166,10 @@ cmd_add()
>>               --depth=*)
>>                       depth=$1
>>                       ;;
>> +             -g|--group)
>> +                     submodule_groups=${submodule_groups:+${submodule_groups};}"$2"
>> +                     shift
>> +                     ;;
>
> You would want to accept "--group=<name>" as well, just like
> existing --reference and --depth do.  It won't be much more code,
> and when you move to C (hence parse_options) you'd get it for free
> anyway.

I am not sure, if I will to move `add` to C any time soon. Sure I desire
less shell and more C[1], but I'd think my time could be spent better than
just converting scripts to C. Sometimes I have to though, such as in the
case of `init` as the the call out from C to shell is too ugly and the effort to
do that is not that much less.

>
>> @@ -292,6 +297,16 @@ 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" &&
>> +     if test -n "$submodule_groups"
>> +     then
>> +             OIFS=$IFS
>> +             IFS=';'
>
> I do not quite understand the choice of ';' here.  If and only if
> you _must_ accept multi-word name that has spaces in between as a
> group name, the above construct may make sense, but I do not think
> we have such requirement.  Why not separate with $IFS letters just
> like any other normal list managed in shell scripts do?  Is there
> anything special about names of submodule groups?

Just prior to writing this patch, I spent a good amount of time understanding
the error message handling in the parts of the shell code, where the $IFS is
used, so my mind was deceived to imitate that.

I agree, we can just use a space as separator here.

Thanks,
Stefan

[1] Reason why I have such a disdain to shell is that I do not
understand nearly as
much as C. I am self-taught in both C and shell, so I do not claim to
be perfect.
However C is a small but powerful language. After reading "The C Programming
Language" by Kernighan and Ritchie I do not think I found a thing that
was really
unknown to me. However even plain posix-shell is complex enough, that there
is no such thing as a compact book to read to understand all its concepts IMHO.

Re: [PATCH 1/4] git submodule: Teach add to accept --group

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>> @@ -165,6 +166,10 @@ cmd_add()
>>>               --depth=*)
>>>                       depth=$1
>>>                       ;;
>>> +             -g|--group)
>>> +                     submodule_groups=${submodule_groups:+${submodule_groups};}"$2"
>>> +                     shift
>>> +                     ;;
>>
>> You would want to accept "--group=<name>" as well, just like
>> existing --reference and --depth do.  It won't be much more code,
>> and when you move to C (hence parse_options) you'd get it for free
>> anyway.
>
> I am not sure, if I will to move `add` to C any time soon. Sure I desire
> less shell and more C[1], but I'd think my time could be spent better than
> just converting scripts to C. Sometimes I have to though, such as in the
> case of `init` as the the call out from C to shell is too ugly and the effort to
> do that is not that much less.

You can do so in less time than you spent making the above 5-line
excuse.  It won't be much more code, and it is not ugly at all.

	--group=*)
        	group=$group ${1#--group=} ;;

or something, right?

Re: [PATCH 1/4] git submodule: Teach add to accept --group

[Stefan Beller]

On Wed, Jan 20, 2016 at 4:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>>> @@ -165,6 +166,10 @@ cmd_add()
>>>>               --depth=*)
>>>>                       depth=$1
>>>>                       ;;
>>>> +             -g|--group)
>>>> +                     submodule_groups=${submodule_groups:+${submodule_groups};}"$2"
>>>> +                     shift
>>>> +                     ;;
>>>
>>> You would want to accept "--group=<name>" as well, just like
>>> existing --reference and --depth do.  It won't be much more code,
>>> and when you move to C (hence parse_options) you'd get it for free
>>> anyway.
>>
>> I am not sure, if I will to move `add` to C any time soon. Sure I desire
>> less shell and more C[1], but I'd think my time could be spent better than
>> just converting scripts to C. Sometimes I have to though, such as in the
>> case of `init` as the the call out from C to shell is too ugly and the effort to
>> do that is not that much less.
>
> You can do so in less time than you spent making the above 5-line
> excuse.  It won't be much more code, and it is not ugly at all.
>
>         --group=*)
>                 group=$group ${1#--group=} ;;
>
> or something, right?

Right, that's what I have here now. I should have said that.

I was not trying to excuse anything, but more explaining the situation
when reading the second sentence. (You seem to assume I'd want to rewrite
all the shell scripts. Which I am not)

Re: [PATCH 2/4] submodule-config: keep groups around

[Stefan Beller]

On Wed, Jan 20, 2016 at 1:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>       free((void *) entry->config->name);
>> +     if (entry->config->groups)
>> +             string_list_clear(entry->config->groups, 0);
>
> You are allocating entry->config->groups itself in the in the hunk
> around ll.327, so that also need to be freed, I would think.

done


>>       unsigned char gitmodules_sha1[20];
>> +     struct string_list *groups;
>>  };
>
> Is there a case where you need to enumerate and show the groups a
> submodule belongs to to the end users?  Using string_list_insert()
> to manage this list would mean you will lose the original ordering
> you saw the list of groups in their .gitmodules files, which may or
> may not matter.  I'd assume that the ordering should not matter, but
> that is something the user may want to see documented.

In this patch series we only need to check the items of that list one by one,
no need for a sorted version, so I'll change it to be unsorted and
document that.

>
>>
>>  int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);

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

[Stefan Beller]

On Wed, Jan 20, 2016 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> All submodules which are selected via submodule groups
>> ("submodule.groups") are initialized if they were not initialized
>> before updating the submodules.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/submodule--helper.c | 30 +++++++++++++++++++++++++++++-
>>  t/t7400-submodule-basic.sh  | 26 ++++++++++++++++++++++++++
>>  2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 4684f16..def382e 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -577,6 +577,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>
>>  struct submodule_update_clone {
>>       /* states */
>> +     struct string_list *submodule_groups;
>
> Again, do we need a field named submodule_groups in a struct whose
> name already makes it clear this is about submodule?
>
> More importantly.
>
> If this were used to implement "there are various groups defined,
> and the user tells us that submodules in this and that groups are to
> be automatically initialized", then naming the field with a name
> that is more specific than just "groups" makes tons of sense, but
> even in such a case, the best adjective to clarify what kind of
> "group" this field is about is not "this is a submodule group".
>
> The answer I would give to a question "what kind of group this field
> is about?" would be "this is an auto-init group", so I'd have that
> 'auto-init' ness somewhere in its name.

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

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

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

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

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

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

>
>> @@ -633,6 +634,7 @@ static int update_clone_get_next_task(struct child_process *cp,
>>               const char *update_module = NULL;
>>               char *url = NULL;
>>               int needs_cloning = 0;
>> +             int in_submodule_groups = 0;
>
> Likewise.  The significance of the membership in this group is
> totally unclear from this variable name.  I read this boolean to be
> keeping track of "is this submodule to be auto initialized"?

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

Re: [PATCH 2/4] submodule-config: keep groups around

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In this patch series we only need to check the items of that list one by one,
> no need for a sorted version, so I'll change it to be unsorted and
> document that.

I actually was going to suggest keeping the sorted one.  The order
may be changed from the order specified in the file, but the sorted
variant has one very desirable property, namely de-duping, that the
code that uses this data structure may find useful.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, Jan 20, 2016 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> If this were used to implement "there are various groups defined,
>> and the user tells us that submodules in this and that groups are to
>> be automatically initialized", then naming the field with a name
>> that is more specific than just "groups" makes tons of sense, but
>> even in such a case, the best adjective to clarify what kind of
>> "group" this field is about is not "this is a submodule group".
>>
>> The answer I would give to a question "what kind of group this field
>> is about?" would be "this is an auto-init group", so I'd have that
>> 'auto-init' ness somewhere in its name.
>
> So you mean something like `autoinit` would maybe already suffice.

If you have (and I wish we have, but I do not seem to be able to
come up with myself offhand) a shorter and more intuitive word than
auto-init, that may be preferred, but basically yes.  I'd call that
autoinit_group or something, as you might want to come up with ways
other than using groups to specify which ones are to be automatically
initialized.

Come to think of it, I think we should drop "auto" from the name.

The only reason we think of this as "auto" is because we know that
historically "clone" was purely about cloning and anything submodule
related had to be done as a separate step, and this series makes the
second step a part of the "clone" proper.  But from the point of the
end user, who specifies that "init" to be done for the specified set
of submodules, there is no longer anything "auto" about it.  We are
doing what the user asked us to do; "auto" is what we do without
explicitly being told, but the user is telling as explicitly which
modules to work on with the option.

> Which leads to another question if we want to extend the concept of
> these submodule groups a little bit, such that we also allow
> direct names in there, in .git/config we may have the configuration
>
>     [submodule]
>         groups = <groupname-as-annotated-in-.gitmodules>
>         groups = <explicit-submodule>
>
> such that `git clone --group=default --group=mySingleSubmodule ....` would work.

I am not sure what you mean by the above snippet.  With

	[submodule "foo"]
        	group = default
	[submodule "bar"]
        	group = optional
	[submodule "baz"]
        	group = optional
                path = sub/baz

it would be nice if you can say "modules in the default group",
"modules in the default group and the module 'bar'", "modules at
path sub/baz", etc.

Am I repeating more or less the same thing as you wanted to say with
the above?  If so, yes, I do think there should be a uniform syntax
that lets users specify set of modules via different mechanisms.

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

I do think --group option is a mistake, as you are only saying
"please give a name of a group to this option" without hinting how
the modules in the specified group are to be treated differently, or
more importantly, the option is about submodules.

    Side note: this is a common delusion developers fall into
    thinking that the feature they currently working on is the most
    important thing in the world.  In the context of "clone", there
    is no reason to expect that "groups of submodules" is any
    special than groups of other things.  For the same reason, I
    think --init=<group> is a mistake, as it is not clear from the
    option name that we are initializing submodules in the context
    of "clone".

So perhaps --init-module.

Once we establish a uniform convention for specifying a group of
submodules is by giving the names of groups, the "group" ness of the
option argument becomes less and less important, as that would be
implicitly known by users.  "clone --init=A" would be more
understandable than "clone --group=A", as everybody would know A is
naming a set of submodules either way, but the latter does not say
what will happen to the chosen modules.

There could be other ways to specify the modules, and as long as we
can come up with the "uniform convention for specifying a group of
submodules", "clone --init-module=$X --init-module=$Y", would be
understandable by the users when $X specifies the modules by their
group name and $Y specifies another set of modules by something else,
e.g. their names or paths to them.

    A strawman.  You can pass (1) the path to a submodule, (2) you
    can pass a colon followed by the name of a submodule, or (3) you
    can pass an asterisk followed by the name of a group.  (1) and
    (2) specifies a single submodule, (3) specifies the submodules
    that belong to the group.  I.e.

    $ git clone --init-module='*default' --init-module=sub/module

    would be a way to say "clone and then initialize the submodule
    at path sub/module and also those in the default group.

    This strawman makes "path" the default way, merely because many
    subcommands of "git submodule" already specify which submodule
    to operate on by taking paths arguments, and '*' prefix as the
    sign to specify by a group, as an asterisk looks like specifying
    multiple things.  ':' is just another prefix that is unlikely to
    be in a pathname.

    This is merely an illustration of the kind of syntax that can be
    used to name a set of modules using different ways.  I am sure
    people can come up with a different and better syntax, but the
    point of this illustration is not the exact syntax but showing
    that a way to uniformly specify a set of modules to operate on
    would allow us not having to worry about making 'groups' any
    special.

I however do not see why you think "submodule.group" needs to be
spelled differently (it should be "submodule.$name.group", though).
The 'group' is merely a convenient way to name and choose a set of
modules.  The name of the operation, i.e. what is to be done to the
chosen modules, should be orthogonal, so I do not think you should
have "submodule.autoinitgroup" or somesuch.

> However:
> At this point in time we only care about auto-initing submodules
> to get submodules somewhat easier to handle when having lots of them.
> Maybe we also want to add other features to these "groups" concept, e.g.
> all submodules of one groups should have the "(force-)checkout" update strategy.
> If the submodule consists of binaries only, this would make lots of sense to me.
>
> So it is not yet clear to me if we want to extend the grouping feature
> later on for
> other things, which is why I named it by its concept. A group can be used for
> different purposes, where as "all submodules having the same auto-init-tag can
> be treated the same using one update strategy" just adds to user confusion,
> hence I'd think telling the user about groups is the right thing to do?

"clone" and other commands may want to gain use of some mechanism to
specify a set of modules, and 'group' is one of the mechanisms to do
so.  Also "clone" and other commands may want to gain features to do
different things to such sets of modules.  So there are two orthogonal
axes.

Which one is more pleasant to use from the end-user's point of view?

 (1) options are named after _how_ you specify the submodules:

     $ clone --init-group=A --init-name=B --init-path=C

     and what is done to them is implicit.

 (2) options are named after _what_ is done to them:

     $ clone --init-module=A --init-module=\*B --init-module=/C

     and how the set is specified is implicit to the syntax.

And more importantly, which one is more extensible in the future
when you want to add more features that work on multiple but not all
submodules?

	clone --init-module='*A' --distim-module=':B'

would be one clear way to say that modules in group A would be
inited, while the submodule B would be distimmed.  What would an
equivalent of it look like in your "clone --group=A --name=B" world?
Wouldn't the design along the lines of (2) above be much better?

Re: [PATCH 1/4] git submodule: Teach add to accept --group

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Right, that's what I have here now. I should have said that.
>
> I was not trying to excuse anything, but more explaining the situation
> when reading the second sentence. (You seem to assume I'd want to rewrite
> all the shell scripts. Which I am not)

I think it is a reasonable way to spend your time by not rewriting
everything in C but the parts the matter the most.  But it makes it
even more important to do the part that will be left in shell for a
longer time in such a way that will be helpful to the end users (by
accepting both "--group name" and "--group=name", in the codepath
under discussion).

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

[Stefan Beller]

On Wed, Jan 20, 2016 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I am not sure what you mean by the above snippet.  With
>
>         [submodule "foo"]
>                 group = default
>         [submodule "bar"]
>                 group = optional
>         [submodule "baz"]
>                 group = optional
>                 path = sub/baz
>
> it would be nice if you can say "modules in the default group",
> "modules in the default group and the module 'bar'", "modules at
> path sub/baz", etc.
>
> Am I repeating more or less the same thing as you wanted to say with
> the above?  If so, yes, I do think there should be a uniform syntax
> that lets users specify set of modules via different mechanisms.

Yes, we have the same vision here. However I wonder if we really need
syntax to differentiate between groups, names and paths. At least
for groups we can make it a requirement to have the same identifier as
another name or path.

Technically it is possible for names and paths to mix up such as
         [submodule "bar"]
                 path = baz
         [submodule "baz"]
                 path = bar

such that we would need to have a way to tell apart if the user means
the name or path. This could be solved implicit by some sort of precedence
(If there is a name then take it by name, otherwise take the path.)


>> Of course then the --group option would need to be named
>> differently in git clone and probably the submodule.groups should
>> also be named differently.
>
> I do think --group option is a mistake, as you are only saying
> "please give a name of a group to this option" without hinting how
> the modules in the specified group are to be treated differently, or
> more importantly, the option is about submodules.
>
>     Side note: this is a common delusion developers fall into
>     thinking that the feature they currently working on is the most
>     important thing in the world.

I fell into that trap, sure.

> In the context of "clone", there
>     is no reason to expect that "groups of submodules" is any
>     special than groups of other things.  For the same reason, I
>     think --init=<group> is a mistake, as it is not clear from the
>     option name that we are initializing submodules in the context
>     of "clone".
>
> So perhaps --init-module.

Why not spell it out completely (--init-submodule)?
Then we don't have to add to the glossary that a module is the same as
a submodule. (Could there be other modules such as co-modules,
which happen to be not in the tree, but in "../sibling" relative to the
root of the repository?)



>
> Once we establish a uniform convention for specifying a group of
> submodules is by giving the names of groups, the "group" ness of the
> option argument becomes less and less important, as that would be
> implicitly known by users.  "clone --init=A" would be more
> understandable than "clone --group=A", as everybody would know A is
> naming a set of submodules either way, but the latter does not say
> what will happen to the chosen modules.

So keep the "groups" in .gitmodules, but ban the name from clone options.
In the .gitmodules file we could also name it "set" just as you used it in that
paragraph.

>
> There could be other ways to specify the modules, and as long as we
> can come up with the "uniform convention for specifying a group of
> submodules", "clone --init-module=$X --init-module=$Y", would be
> understandable by the users when $X specifies the modules by their
> group name and $Y specifies another set of modules by something else,
> e.g. their names or paths to them.

ok.

>
>     A strawman.  You can pass (1) the path to a submodule, (2) you
>     can pass a colon followed by the name of a submodule, or (3) you
>     can pass an asterisk followed by the name of a group.  (1) and
>     (2) specifies a single submodule, (3) specifies the submodules
>     that belong to the group.  I.e.
>
>     $ git clone --init-module='*default' --init-module=sub/module
>
>     would be a way to say "clone and then initialize the submodule
>     at path sub/module and also those in the default group.
>
>     This strawman makes "path" the default way, merely because many
>     subcommands of "git submodule" already specify which submodule
>     to operate on by taking paths arguments, and '*' prefix as the
>     sign to specify by a group, as an asterisk looks like specifying
>     multiple things.  ':' is just another prefix that is unlikely to
>     be in a pathname.

But it could be in a pathname. Another strawman:

    You can pass either (1) the path (2) or the name (3) or the group
    of submodules to --init-submodule if there are no collisions in the
    three name spaces. When there are namespaces, you need to be
    explicit, such as --init-submodule=path:foo:baz.
    Note that "path:" is the selector which of the three namespaces
    we mean and "foo:baz" is a path asking for pain.



>
>     This is merely an illustration of the kind of syntax that can be
>     used to name a set of modules using different ways.  I am sure
>     people can come up with a different and better syntax, but the
>     point of this illustration is not the exact syntax but showing
>     that a way to uniformly specify a set of modules to operate on
>     would allow us not having to worry about making 'groups' any
>     special.

ok

> The 'group' is merely a convenient way to name and choose a set of
> modules.

But not just at the time of cloning. If later upstream decides that
group=default includes another submodule, this submodule should
be initialized and fetched in the next call to "submodule update",
such that downstream has a working repository.

This idea makes groups more than just a temporary collection for
init-purposes, but we need to store the selection.

>
> I however do not see why you think "submodule.group" needs to be
> spelled differently (it should be "submodule.$name.group", though).

"submodule.$name.group" to be found in .gitmodules, maybe overwritten in
.git/config tells for each submodule its memberships of groups.

"submodule.group" should be found in .git/config only, to tell some time
after cloning which group selection was made, such that we can check
if new submodules need to be initialized (or even automatically removed).

    "git submodule update" may initialze and fetch new modules if the
    .gitmodules file changed their view of what "default" is.

>  The name of the operation, i.e. what is to be done to the
> chosen modules, should be orthogonal, so I do not think you should
> have "submodule.autoinitgroup" or somesuch.

I agree there.

>
>> However:
>> At this point in time we only care about auto-initing submodules
>> to get submodules somewhat easier to handle when having lots of them.
>> Maybe we also want to add other features to these "groups" concept, e.g.
>> all submodules of one groups should have the "(force-)checkout" update strategy.
>> If the submodule consists of binaries only, this would make lots of sense to me.
>>
>> So it is not yet clear to me if we want to extend the grouping feature
>> later on for
>> other things, which is why I named it by its concept. A group can be used for
>> different purposes, where as "all submodules having the same auto-init-tag can
>> be treated the same using one update strategy" just adds to user confusion,
>> hence I'd think telling the user about groups is the right thing to do?
>
> "clone" and other commands may want to gain use of some mechanism to
> specify a set of modules, and 'group' is one of the mechanisms to do
> so.  Also "clone" and other commands may want to gain features to do
> different things to such sets of modules.  So there are two orthogonal
> axes.
>
> Which one is more pleasant to use from the end-user's point of view?
>
>  (1) options are named after _how_ you specify the submodules:
>
>      $ clone --init-group=A --init-name=B --init-path=C
>
>      and what is done to them is implicit.
>
>  (2) options are named after _what_ is done to them:
>
>      $ clone --init-module=A --init-module=\*B --init-module=/C
>
>      and how the set is specified is implicit to the syntax.

I do think (2) is easier, unless the syntax is crazy or counterintuitive.

>
> And more importantly, which one is more extensible in the future
> when you want to add more features that work on multiple but not all
> submodules?
>
>         clone --init-module='*A' --distim-module=':B'
>
> would be one clear way to say that modules in group A would be
> inited, while the submodule B would be distimmed.  What would an
> equivalent of it look like in your "clone --group=A --name=B" world?
> Wouldn't the design along the lines of (2) above be much better?

I agree on (2) being better.

However as pointed out, we want more than to just init submodules at time
of clone, so it would be a

    clone --init-module=A --init-module=\*B --init-module=/C \
        --and-remember-to-keep-track-of=A \
        --and-remember-to-keep-track-of=\*B \
        --and-remember-to-keep-track-of=/C

I am hoping we can put that in shorter options, such as

    clone --init-module=A --init-module=\*B --init-module=/C \
        --remember-init-for-tracking

whereas:

    --remember-init-for-tracking: Submodule groups which are given to clone
        will be remembered, such that each invocation of "update" will make sure
        that group is fully there, i.e. new submodules in the group
will be initialized
        before updating.

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> "submodule.$name.group" to be found in .gitmodules, maybe overwritten in
> .git/config tells for each submodule its memberships of groups.
>
> "submodule.group" should be found in .git/config only, to tell some time
> after cloning which group selection was made, such that we can check
> if new submodules need to be initialized (or even automatically removed).

Ahh, it wasn't clear that was what you were trying to do.  If that
is the case, then ...

>     "git submodule update" may initialze and fetch new modules if the
>     .gitmodules file changed their view of what "default" is.
>
>>  The name of the operation, i.e. what is to be done to the
>> chosen modules, should be orthogonal, so I do not think you should
>> have "submodule.autoinitgroup" or somesuch.
>
> I agree there.

... I do not agree that the name of "submodule.group" variable
should be neutral to the operation that is done to the groups of
submodules named by the variable at all.

You are recording the criteria to choose a set of submodules there
in the configuration, with a plan to keep doing something to them
(e.g. "they will never be left uninitialized, i.e. initialied if a
new submodule is added to the set").  The name of the configuration
must have something that tells what that "something" is, in order to
(1) hint the purpose of specifying the group selection criteria
there, and (2) allow different selection criteria to keep doning
another kind of something to them and distinguish these two group
selection criteria.

E.g. "submodule.autoInitialize = *default !:foo" may mean all the
modules in the default group (now or added in the future) except the
module with name 'foo' will be initialized as needed, while
"submodule.autoDistim = *optional" may want to be defined to allow
the system to automatically distim (whatever that operation is that
will be added to Git in later versions) the modules in the optional
group.

> I am hoping we can put that in shorter options, such as
>
>     clone --init-module=A --init-module=\*B --init-module=/C \
>         --remember-init-for-tracking
>
> whereas:
>
>     --remember-init-for-tracking: Submodule groups which are given
> to clone will be remembered, such that each invocation of "update"
> will make sure that group is fully there, i.e. new submodules in
> the group will be initialized before updating.

Is there a need for --no-remember-init-for-tracking?  I do not think
it would be useful.

When the upstream adds a new module and defines it to be part of the
default group _after_ you cloned with --init set to 'default', and
you do not need that new module, at that point you can tweak your
submodule.autoInitialize definition to exclude that new module.

Tweaking submodule.autoInitialize definition to contain nothing
after cloning with --init because you do not want the autoinit
criteria kept in the resulting repository is merely a special case
of that.

So I do not think --remember-init-for-tracking is necessary.  Just
make it _always_ on and be done with it, I would say.

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

[Junio C Hamano]

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

> Ahh, it wasn't clear that was what you were trying to do.  If that
> is the case, then ...
> ...
> So I do not think --remember-init-for-tracking is necessary.  Just
> make it _always_ on and be done with it, I would say.

By the way, I forgot to say that we're basically in agreement on
everything in your message that I didn't touch in my response.  Also
I forgot to say that I am happy to see that we are designing a new
mechanism to name a set of modules in a convenient way so that the
users can do things to them without hassle.

;-)

Re: [PATCH 0/4] Submodule Groups

[Sebastian Schuberth]

On 20.01.2016 04:34, Stefan Beller wrote:

> So you could have a .gitmodules file such as:
> 
> [submodule "gcc"]
>          path = gcc
>          url = git://...
>          groups = default
>          groups = devel

On the quick I was unable to find the rationale why entries are now stored as separated lines compared to v1. I liked the comma-separated approach better as it's more compact.

Anyway, if it's only one group per line, I'd find it more fitting to call the entry "group" instead of "groups" as it will always refer to a single group only. Also that would better match the "--group" command line option naming for "submodule add".

However, if I'd read the single line "group = default" in a .gitmodules file, it wouldn't be immediately clear to me that "group" can appear multiple times per submodule. "groups = default" would me more hinting is this regard because the plural is used, but without reading the docs I'd assume multiple groups would be specified like "groups = default,devel".

Long story short, my personal favorite still would be 

[submodule "gcc"]
         groups = default,devel

followed by

[submodule "gcc"]
         group = default
         group = devel

-- 
Sebastian Schuberth

Re: [PATCH 0/4] Submodule Groups

[Stefan Beller]

On Thu, Jan 21, 2016 at 1:17 PM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
> On 20.01.2016 04:34, Stefan Beller wrote:
>
>> So you could have a .gitmodules file such as:
>>
>> [submodule "gcc"]
>>          path = gcc
>>          url = git://...
>>          groups = default
>>          groups = devel
>
> On the quick I was unable to find the rationale why entries are now stored as separated lines compared to v1. I liked the comma-separated approach better as it's more compact.

IIUC the line oriented way is preferred as it is our standard. Do we
have any other options stored as a comma separated list?

>
> Anyway, if it's only one group per line, I'd find it more fitting to call the entry "group" instead of "groups" as it will always refer to a single group only. Also that would better match the "--group" command line option naming for "submodule add".

Makes sense to use singular then. However per discussion with Junio in
[PATCH 3/4] submodule update: Initialize all group-selected submodules
by default, we want to not name it "group", as it's unclear what a group is
supposed to mean. What does a group do? which operations are supported?

So for git clone, we'd rather use "--init-submodule" which can be passed a
name, path or group.

For storing that selection we'd go with "submodule.autoInitialize" in
.git /config.

The third user visible place submodule.$NAME.group however can stay in that
variable name as to point out the the concept of a submodule set/collection.
The groups concept may be used in the future for more than initialzing
submodules.

Instead of having a submodule -> set assignment, we could do it the
other way round:

     [submodule "gcc"]
         ...

     [submodule-set "default"]
        submodule = gcc
        submodule = foo
        submodule = by/path/*

This may be more handy from our perspective (while designing it and
writing the code),
but I'd assume this is less useful for the user. How often does a user ask:
"How many/Which submodules are in $GROUP" as opposed to "What about
submodule foo,
is that part of group $GROUP?"

>
> However, if I'd read the single line "group = default" in a .gitmodules file, it wouldn't be immediately clear to me that "group" can appear multiple times per submodule. "groups = default" would me more hinting is this regard because the plural is used, but without reading the docs I'd assume multiple groups would be specified like "groups = default,devel".
>
> Long story short, my personal favorite still would be
>
> [submodule "gcc"]
>          groups = default,devel
>
> followed by
>
> [submodule "gcc"]
>          group = default
>          group = devel

As asked above, how many comma separated things do we have in git configs?
I'd really not want to add more mental complexity to Git. As far as I
remember we have
rather double configs than one long line separated somehow.
(The only thing that comes to mind is multiple remote urls for pushing)

Thanks,
Stefan

>
> --
> Sebastian Schuberth

Re: [PATCH 0/4] Submodule Groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Instead of having a submodule -> set assignment, we could do it the
> other way round:
>
>      [submodule "gcc"]
>          ...
>
>      [submodule-set "default"]
>         submodule = gcc
>         submodule = foo
>         submodule = by/path/*
>
> This may be more handy from our perspective (while designing it and
> writing the code),
> but I'd assume this is less useful for the user. How often does a user ask:
> "How many/Which submodules are in $GROUP" as opposed to "What about
> submodule foo,
> is that part of group $GROUP?"

I suspect that we will end up needing to support both styles.  The
latter style is easier when you want to express a larger set as a
collection of groups, e.g.

    [submodule "gcc"]
    	group = development-tools

    [submodule "emacs"]
    	group = editors

    [submodule "frotz"]
        group = games

    [submoduleGroup "default"]
        member = *development-tools
        member = *editors
        member = :frotz
	member = !*xyzzy

might be a way to say "the default group includes everything in the
'development-tools' or 'editors' group, plus 'frotz' module, but do
not include modules in the xyzzy group" or something like that.

Re: [PATCH 0/4] Submodule Groups

[Junio C Hamano]

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

> I suspect that we will end up needing to support both styles.  The
> latter style is easier when you want to express a larger set as a
> collection of groups, e.g.
> ...
> might be a way to say "the default group includes everything in the
> 'development-tools' or 'editors' group, plus 'frotz' module, but do
> not include modules in the xyzzy group" or something like that.

I forgot to say that I personally do not think we need to support
such synthetic groups from day one.

Re: [PATCH 0/4] Submodule Groups

[Stefan Beller]

On Thu, Jan 21, 2016 at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Instead of having a submodule -> set assignment, we could do it the
>> other way round:
>>
>>      [submodule "gcc"]
>>          ...
>>
>>      [submodule-set "default"]
>>         submodule = gcc
>>         submodule = foo
>>         submodule = by/path/*
>>
>> This may be more handy from our perspective (while designing it and
>> writing the code),
>> but I'd assume this is less useful for the user. How often does a user ask:
>> "How many/Which submodules are in $GROUP" as opposed to "What about
>> submodule foo,
>> is that part of group $GROUP?"
>
> I suspect that we will end up needing to support both styles.

I think having both is bad as it may contradict each other?
What is supposed to happen here:

     [submodule "frotz"]
         group = default

     [submoduleGroup "default"]
         member = !:frotz

Initially I disliked your proposal of : and * to indicate name and groups,
but the example you gave is very clear and understandable on the syntax level.

>  The
> latter style is easier when you want to express a larger set as a
> collection of groups, e.g.
>
>     [submodule "gcc"]
>         group = development-tools
>
>     [submodule "emacs"]
>         group = editors
>
>     [submodule "frotz"]
>         group = games
>
>     [submoduleGroup "default"]
>         member = *development-tools
>         member = *editors
>         member = :frotz
>         member = !*xyzzy
>
> might be a way to say "the default group includes everything in the
> 'development-tools' or 'editors' group, plus 'frotz' module, but do
> not include modules in the xyzzy group" or something like that.

> I forgot to say that I personally do not think we need to support
> such synthetic groups from day one.

So groups of groups, we discovered recursion today. :)
Having this discussion makes me realize, the groups handling logic
will be more complex than I anticipated for the first RFC patch series.

The two basic questions the logic has to answer is:
 * Give me all the submodules that fit these specifiers (i.e. the
    --init-submodules collection from clone)
 * Given submodule X, does it fit the specifier ( a new uninitialized
    submodule during a later update)

The second question can be answered by answering the first question
and then checking if X is in the set. However that may be not the most
efficient solution.

Re: [PATCH 0/4] Submodule Groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I think having both is bad as it may contradict each other?
> What is supposed to happen here:
>
>      [submodule "frotz"]
>          group = default
>
>      [submoduleGroup "default"]
>          member = !:frotz

What is supposed to happen is that you will write code to diagnose
this as an error, and give helpful error message to the user.

> So groups of groups, we discovered recursion today. :)
> Having this discussion makes me realize, the groups handling logic
> will be more complex than I anticipated for the first RFC patch series.

That is why I think we do not have to have a very complex group
logic from day one.

> The two basic questions the logic has to answer is:
>  * Give me all the submodules that fit these specifiers (i.e. the
>     --init-submodules collection from clone)
>  * Given submodule X, does it fit the specifier ( a new uninitialized
>     submodule during a later update)

Yes.

Re: [PATCH 0/4] Submodule Groups

[Sebastian Schuberth]

On Thu, Jan 21, 2016 at 10:56 PM, Stefan Beller <sbeller@google.com> wrote:

>>> [submodule "gcc"]
>>>          path = gcc
>>>          url = git://...
>>>          groups = default
>>>          groups = devel
>>
>> On the quick I was unable to find the rationale why entries are now stored as separated lines compared to v1. I liked the comma-separated approach better as it's more compact.
>
> IIUC the line oriented way is preferred as it is our standard. Do we
> have any other options stored as a comma separated list?

Out of my head I cannot think of any. But that shouldn't mean we
cannot introduce such comma separated list if it makes sense.

> Makes sense to use singular then. However per discussion with Junio in
> [PATCH 3/4] submodule update: Initialize all group-selected submodules
> by default, we want to not name it "group", as it's unclear what a group is
> supposed to mean. What does a group do? which operations are supported?

How about calling it "label" instead of "group"? IMO with the word
"label" it's more clean that a single submodule can have multiple
labels, as the concept of labels is familiar to the user already from
applications like Firefox (bookmarks), Google Mail, Mac OS X Finder
(files) etc.

> Instead of having a submodule -> set assignment, we could do it the
> other way round:
>
>      [submodule "gcc"]
>          ...
>
>      [submodule-set "default"]
>         submodule = gcc
>         submodule = foo
>         submodule = by/path/*

In your example you're now introducing "set" as a new term. Shouldn't
this better be "submodule-group" then? I actually like this idea quite
a bit as it completely solves the problem about being clear that a
submodule can belong to mutiple groups.

> but I'd assume this is less useful for the user. How often does a user ask:
> "How many/Which submodules are in $GROUP" as opposed to "What about
> submodule foo,
> is that part of group $GROUP?"

True, but for answering that question a user would not look at
.gitmodules, but run a command, and the implementation of that command
would completely hide that complexity from the user.

> As asked above, how many comma separated things do we have in git configs?
> I'd really not want to add more mental complexity to Git. As far as I

I don't think it can get much worse anyway ;-)

> remember we have
> rather double configs than one long line separated somehow.
> (The only thing that comes to mind is multiple remote urls for pushing)

I believe so, too. But I'd see the introduction of comma-separated
values as an exit-strategy to that. More settings could make use of
that in the future, then.

-- 
Sebastian Schuberth