decoupling a submodule's existence and its url

Currently the submodule.<name>.url config option is used to determine if a given submodule exists and is interesting to the user. This however doesn't work very well because the URL is a config option for the scope of a repository, whereas the existence of a submodule is an option scoped to the working tree.

In a future with worktree support for submodules, there will be multiple working trees, each of which may only need a subset of the submodules checked out. The URL (which is where the submodule repository can be obtained) should not differ between different working trees.

It may also be convenient for users to more easily specify groups of submodules they are interested in as apposed to running "git submodule init <path>" on each submodule they want checked out in their working tree.

To this end, the config option submodule.active is introduced which holds a pathspec that specifies which submodules should exist in the working tree.

Quoted 11 lines · Brandon Williams

Currently the submodule.<name>.url config option is used to determine if a given submodule exists and is interesting to the user. This however doesn't work very well because the URL is a config option for the scope of a repository, whereas the existence of a submodule is an option scoped to the working tree.

In a future with worktree support for submodules, there will be multiple working trees, each of which may only need a subset of the submodules checked out. The URL (which is where the submodule repository can be obtained) should not differ between different working trees.

Quoted 8 lines

It may also be convenient for users to more easily specify groups of submodules they are interested in as apposed to running "git submodule init <path>" on each submodule they want checked out in their working tree.

To this end, the config option submodule.active is introduced which holds a pathspec that specifies which submodules should exist in the working tree.

Hmph. submodule.active in .git/config would be shared the same way submodule.<name>.url in .git/config is shared across the worktrees that stems from the same primary repository, no?

Perhaps there are some other uses of this submodule.active idea, but I do not see how it is relevant to solving "multiple worktrees" issue. Per-worktree config would solve it with the current submodule.<name>.url without submodule.active list, I would think [*1*].

Also as a grouping measure, submodule.active that lists submodule paths feels hard to use. When switching between two branches in the superproject that have the same submodule bound at two different paths, who is responsible for updating submodule.active in superproject's config? If it were a list of submodule names, this objection does not apply, though.

[Footnote]

*1* At the conceptual level, I agree that .url that also means "we
are interested in this one" feels like somewhat an unclean design, but that is not what you are "fixing", is it?

IIRC most of the series is actually refactoring, i.e. providing a central function that answers
"is this submodule active/initialized?" and then making use of this function.

Maybe it would be easier to reroll these refactorings first without adding in the change of behavior.

Off the list we talked about different models how to indicate that a submodule is active.

Current situation:
submodule.<name>.URL is used as a boolean flag

Possible future A:
1) If submodule.active exists use that
2) otherwise go be individual .URL configs

submodule.active is a config that contains a pathspec, i.e. specifies the group of submodules instead of marking each submodule individually.

How to get to future A:
* apply this patch series

Possible future B:
1) the boolean submodule.<name>.exists is used if existent
2) If that boolean is missing we'd respect a grouping feature such as submodule.active
3) fallback to the .URL as a boolean indicator.

How to get to future B:
1) possibly take the refactoring part from this series
2) implement the .exists flag (that can solve the worktree problem, as then we don't have to abuse the .URL as a boolean indicator any more.)
3) implement the grouping feature that takes precedence over .URL, but yields precedence to the .exists flag. (This would solve the grouping problem)

By having two different series (2) and (3) we'd solve just one thing at a time with each series, such that this seems easier to understand and review.

The question is if this future B is also easier to use for users and I think it would be, despite being technically more complex.

Most users only have a few submodules. Also the assumption is to have either interest in very few specific submodules or all of them. For both of these cases the .exists is a good idea as it is very explicit, but verbose.

The grouping feature would help in the case of lots of submodules, e.g. to select all of them you would just give an easy pathspec "." and that would help going forward as by such a submodule spec all new submodules would be "auto-on".

Possible future C:
Similar to B, but with branch specific configuration. submodule.<branchname>.active would work as a grouping feature. I wonder how it would work with the .exists flag.

Stefan

Quoted 6 lines · Junio C Hamano

Also as a grouping measure, submodule.active that lists submodule paths feels hard to use. When switching between two branches in the superproject that have the same submodule bound at two different paths, who is responsible for updating submodule.active in superproject's config? If it were a list of submodule names, this objection does not apply, though.

I agree that if you are listing every submodule path by hand then this may not be the best approach and would be difficult to use. The idea is that this would allow a user to set a general pathspec to identify a group of modules they are interested in. Perhaps once attributes can be used in pathspecs a user could group submodules by setting a particular attribute and then submodule.active would have a value like ":(attr:foo)" to indicate I'm interested in all submodules with the "foo" attribute.

There are a lot of places where an explicit check for submodule."<name>".url is done to see if a submodule exists. In order to more easily facilitate the use of the submodule.active config option to indicate active submodules, add a helper which can be used to query if a submodule is active or not.

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/submodule--helper.c    | 11 ++++++++
 t/t7413-submodule-is-active.sh | 63 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100755 t/t7413-submodule-is-active.sh

builtin/submodule--helper.c +11 -0

index df0d9c166..dac02604d 100644
@@ -1128,6 +1128,16 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
        return 0;
 }
 
+static int is_active(int argc, const char **argv, const char *prefix)
+{
+       if (argc != 2)
+               die("submodule--helper is-active takes exactly 1 arguments");
+
+       gitmodules_config();
+
+       return !is_submodule_initialized(argv[1]);
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {

@@ -1147,6 +1157,7 @@ static struct cmd_struct commands[] = {
        {"init", module_init, 0},
        {"remote-branch", resolve_remote_submodule_branch, 0},
        {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
+       {"is-active", is_active, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)

t/t7413-submodule-is-active.sh new file +24 -0

new file mode 100755
index 000000000..683487020
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='Test submodule--helper is-active
+
+This test verifies that `git submodue--helper is-active` correclty identifies
+submodules which are "active" and interesting to the user.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+       git init sub &&
+       test_commit -C sub initial &&
+       git init super &&
+       test_commit -C super initial &&
+       git -C super submodule add ../sub sub1 &&
+       git -C super submodule add ../sub sub2 &&
+       git -C super commit -a -m "add 2 submodules at sub{1,2}"
+'
+
+test_expect_success 'is-active works with urls' '
+       git -C super submodule--helper is-active sub1 &&
+       git -C super submodule--helper is-active sub2 &&
+

Quoted 72 lines · Brandon Williams

There are a lot of places where an explicit check for submodule."<name>".url is done to see if a submodule exists. In order to more easily facilitate the use of the submodule.active config option to indicate active submodules, add a helper which can be used to query if a submodule is active or not.

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/submodule--helper.c    | 11 ++++++++
 t/t7413-submodule-is-active.sh | 63 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100755 t/t7413-submodule-is-active.sh

builtin/submodule--helper.c +11 -0

index df0d9c166..dac02604d 100644
@@ -1128,6 +1128,16 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
        return 0;
 }
 
+static int is_active(int argc, const char **argv, const char *prefix)
+{
+       if (argc != 2)
+               die("submodule--helper is-active takes exactly 1 arguments");
+
+       gitmodules_config();
+
+       return !is_submodule_initialized(argv[1]);
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {

@@ -1147,6 +1157,7 @@ static struct cmd_struct commands[] = {
        {"init", module_init, 0},
        {"remote-branch", resolve_remote_submodule_branch, 0},
        {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
+       {"is-active", is_active, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)

t/t7413-submodule-is-active.sh new file +24 -0

new file mode 100755
index 000000000..683487020
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='Test submodule--helper is-active
+
+This test verifies that `git submodue--helper is-active` correclty identifies
+submodules which are "active" and interesting to the user.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+       git init sub &&
+       test_commit -C sub initial &&
+       git init super &&
+       test_commit -C super initial &&
+       git -C super submodule add ../sub sub1 &&
+       git -C super submodule add ../sub sub2 &&
+       git -C super commit -a -m "add 2 submodules at sub{1,2}"
+'
+
+test_expect_success 'is-active works with urls' '
+       git -C super submodule--helper is-active sub1 &&
+       git -C super submodule--helper is-active sub2 &&
+

t/t7413-submodule-is-active.sh line 34

Quoted 10 lines

t/t7413-submodule-is-active.sh new file +10 -0

+       git -C super config --unset submodule.sub1.URL &&
+       test_must_fail git -C super submodule--helper is-active sub1 &&
+       git -C super config submodule.sub1.URL ../sub &&
+       git -C super submodule--helper is-active sub1
+'
+
+test_expect_success 'is-active works with basic submodule.active config' '
+       git -C super config --add submodule.active "." &&
+       git -C super config --unset submodule.sub1.URL &&
+       git -C super config --unset submodule.sub2.URL &&

I think we'd want to unset only one of them here

t/t7413-submodule-is-active.sh line 23

Quoted 3 lines

t/t7413-submodule-is-active.sh new file +3 -0

+
+       git -C super submodule--helper is-active sub1 &&
+       git -C super submodule--helper is-active sub2 &&

to test 2 different cases of one being active by config setting only and the other having both.

I could not spot test for having the URL set but the config setting set, not including the submodule, e.g.

    git -C super config  submodule.sub1.URL ../sub &&
    git -C super submodule.active  ":(exclude)sub1" &&

which would be expected to not be active, as once the configuration is there it takes precedence over any (no)URL setting?

Quoted 3 lines · Brandon Williams

(quoted diff) +0 -0

@@ -795,14 +794,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
        }
 

builtin/submodule--helper.c static int prepare_to_clone_next_submodule(const struct cache_entry *ce, · line 799

Quoted 5 lines

builtin/submodule--helper.c +1 -1

        /*
-        * Looking up the url in .git/config.
+        * Check if the submodule has been initialized.
         * We must not fall back to .gitmodules as we only want
         * to process configured submodules.

This sentence "we must not ..." is also no longer accurate, as we do exactly that when using sub->url instead of just url below.

There are two motivations for decoupling a submodule's existence from the url that is stored in .git/config.

1. Worktrees can't really be used with submodules since the existence is checked based on the shared .git/config. This means that two different worktress have to have the same initialized submodules. By decoupling a submodule's existence from the url, two different work trees can be configured to have different submodules checked out.

2. Easily configure gorups of submodules that a user is interested in. In a repository with hundreds of submodules it would be difficult to easily tell git which modules to worry about without having to individually init all of them. Instead, a pathspec can be used to more easily select or deselect groups of submodules.

This patch series works to do this decoupling and instead allows a user to configure submodule.active with a pathspec to use to check if a submodule is initialized.

The user could have configured the submodule to have a different URL from the one in the superproject's config. To account for this read what the submodule has configured for remote.origin.url and use that instead.

# Remove the .git/config entries (unless the user already did it) if test -n "$(git config --get-regexp submodule."$name\.")"

                        git config --remove-section submodule."$name" 2>/dev/null &&
                        say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
                fi

--
2.12.0.246.ga2ecc84866-goog

Quoted 4 lines · Brandon Williams

The user could have configured the submodule to have a different URL from the one in the superproject's config. To account for this read what the submodule has configured for remote.origin.url and use that instead.

When reading this commit message, I first thought this is an unrelated bug fix. However it is not unrelated. In a later patch you introduce a mode where submodule.<name>.url may not exist in .git/config any more, (there may be a .existence flag instead?) such that url="", which is obviously a bad UI:

Submodule '$name' (<empty string>) unregistered for path '$displaypath'"

Like the first patch of this series, we could have a helper function "git submodule--helper url <name>" that figures out how to get the URL:
1) Look into that GIT_DIR
2) if non-existent check .git/config for the URL or
3) fall back to .gitmodules?

Instead of having such a helper, we directly look into that first option hoping it exists, however it doesn't have to:

git submodule init <ps>

  # no command in between
  git submodule deinit <ps>
  # submodule was not cloned yet, but we still test positive for

Quoted 2 lines

# Remove the .git/config entries (unless the user already did it) if test -n "$(git config --get-regexp submodule."$name\.")"

I am not sure if there is an easy way out here.

Thinking about the name for such a url helper lookup, I wonder if we want to have

git submodule--helper show-url <name>

as potentially we end up having this issue for a lot
of different things (e.g. submodule.<name>.shallow = (true,false), or in case the submodule is cloned you can give the actual depth as an integral number), so maybe we'd want to introduce one layer of indirection

    git submodule--helper ask-property \
       (is-active/URL/depth/size/..) <name>

So with that said, I wonder if we also want to ease up:

    GIT_DIR="$(git rev-parse --git-path modules/$name

to be

    GIT_DIR=$(git submodule--helper show-git-dir $name)

git-submodule.sh Submodule work tree '\$displaypath' contains a .git directory · line 458

Quoted 5 lines

then
# Remove the whole section so we have a clean state when # the user later decides to init this submodule again

git-submodule.sh +1 -1

-                       url=$(git config submodule."$name".url)
+                       url=$(GIT_DIR="$(git rev-parse --git-path modules/$name)" git config remote.origin.url)

In the submodule helper we have get_default_remote(), which we do not have in shell
(which we seemed to have in shell?), so maybe it's worth not using origin here, although I guess it is rare that the original remote is named other than origin.

Quoted 6 lines

                        git config --remove-section submodule."$name" 2>/dev/null &&
                        say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
                fi

--
2.12.0.246.ga2ecc84866-goog

Thanks,
Stefan

The new switch --init-active initializes the submodules which are configured in submodule.active instead of those given as command line arguments before updating. In the first implementation this is made incompatible with further command line arguments as it is unclear what the user means by

    git submodule update --init --init-active <paths>

This new switch allows users to record more complex patterns as it saves retyping them whenever you invoke update.

Based on a patch by Stefan Beller <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

git-submodule.sh +0 -0

@@ -568,7 +573,17 @@ cmd_update()
 

Quoted 19 lines · Brandon Williams

The new switch --init-active initializes the submodules which are configured in submodule.active instead of those given as command line arguments before updating. In the first implementation this is made incompatible with further command line arguments as it is unclear what the user means by

    git submodule update --init --init-active <paths>

This new switch allows users to record more complex patterns as it saves retyping them whenever you invoke update.

Based on a patch by Stefan Beller <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

(quoted diff) +0 -0

@@ -568,7 +573,17 @@ cmd_update()
 

git-submodule.sh cmd_update() · line 582

Quoted 10 lines

git-submodule.sh +7 -1

        if test -n "$init"
        then
-               cmd_init "--" "$@" || return
+               if test "$init" = "by_config"
+               then
+                       if test $# -gt 0
+                       then
+                               die "$(gettext "path arguments are incompatible with --init-active")"
+                       fi
+                       cmd_init "--" $(git config --get-all submodule.active) || return

What happens with submodule.<name>.actives here?

When initializing a submodule set the submodule.<name>.active config to true to indicate that the submodule is active.

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/submodule--helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

builtin/submodule--helper.c +7 -1

index bceb62a20..44f2c02ba 100644
@@ -329,6 +329,13 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
                die(_("No url found for submodule path '%s' in .gitmodules"),
                        displaypath);
 
+       /* Set active flag for the submodule being initialized */
+       if (!is_submodule_initialized(path)) {
+               strbuf_reset(&sb);
+               strbuf_addf(&sb, "submodule.%s.active", sub->name);
+               git_config_set_gently(sb.buf, "true");
+       }
+
        /*
         * Copy url setting when it is not set yet.
         * To look up the url in .git/config, we must not fall back to
--

2.12.0.246.ga2ecc84866-goog

Quoted 2 lines · Brandon Williams

When initializing a submodule set the submodule.<name>.active config to true to indicate that the submodule is active.

So by this patch an init of a submodule performs both a "regular init" (copy URL and the update setting) as well as setting the new submodule.<name>.active.

And both happens in the same config file (i.e. no worktree support here)

Later on we *could* remove the URL as the .active is the new flag of existence.

But enough of my speculation, I am left wondering what this patch accomplishes, as this states no agenda as why it is useful on its own.

Quoted 27 lines

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/submodule--helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

builtin/submodule--helper.c +7 -1

index bceb62a20..44f2c02ba 100644
@@ -329,6 +329,13 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
                die(_("No url found for submodule path '%s' in .gitmodules"),
                        displaypath);
 
+       /* Set active flag for the submodule being initialized */
+       if (!is_submodule_initialized(path)) {
+               strbuf_reset(&sb);
+               strbuf_addf(&sb, "submodule.%s.active", sub->name);
+               git_config_set_gently(sb.buf, "true");
+       }
+
        /*
         * Copy url setting when it is not set yet.
         * To look up the url in .git/config, we must not fall back to
--

2.12.0.246.ga2ecc84866-goog

There are a lot of places where an explicit check for submodule."<name>".url is done to see if a submodule exists. In order to centralize this check introduce a helper which can be used to query if a submodule is active or not.

#define SUPPORT_SUPER_PREFIX (1<<0)

+ {"is-active", is_active, 0},

There are a lot of places where an explicit check for submodule."<name>".url is done to see if a submodule exists. In order to centralize this check introduce a helper which can be used to query if a submodule is active or not.

t/t7413-submodule-is-active.sh new file +19 -0

new file mode 100755
index 000000000..f18e0c925
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='Test submodule--helper is-active
+
+This test verifies that `git submodue--helper is-active` correclty identifies
+submodules which are "active" and interesting to the user.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	git init sub &&
+	test_commit -C sub initial &&
+	git init super &&
+	test_commit -C super initial &&
+	git -C super submodule add ../sub sub1 &&
+	git -C super submodule add ../sub sub2 &&
+	git -C super commit -a -m "add 2 submodules at sub{1,2}"
+'

Here is my attempt to rewrite the whole thing.

The definition of which submodules are of interest by the user is tied to the configuration submodule.<name>.url; when it is set to a non-empty string, it is of interest. We'd want to be able to later change this definition, but there are many places that explicitly check this condition in the scripted Porcelain.

Introduce the "is-active" subcommand to "submodule--helper", so that the exact definition of what submodule is of interest can be centrally defined (and changed in later steps). In a few patches that follow, this helper is used to replace the explicit checks of the configuration variable in scripts.

You are adding the "is-active" subcommand, and the implementation detail to do so is by using a new is_active() function, so the patch title needs a bit of tweaking, too.

Subject: submodule--helper: add is-active subcommand

Signed-off-by Brandon Williams <[email protected]>

---
 git-submodule.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

git-submodule.sh +0 -1

index 136e26a2c..ab233712d 100755
@@ -1010,14 +1010,13 @@ cmd_status()
 	do
 		die_if_unmatched "$mode" "$sha1"
 		name=$(git submodule--helper name "$sm_path") || exit
-		url=$(git config submodule."$name".url)
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")

Quoted 15 lines · Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 git-submodule.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

git-submodule.sh +0 -1

index 136e26a2c..ab233712d 100755
@@ -1010,14 +1010,13 @@ cmd_status()
      do
              die_if_unmatched "$mode" "$sha1"
              name=$(git submodule--helper name "$sm_path") || exit
-             url=$(git config submodule."$name".url)
              displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")

git-submodule.sh cmd_status() · line 1022

Quoted 10 lines

git-submodule.sh +1 -1

              if test "$stage" = U
              then
                      say "U$sha1 $displaypath"
                      continue
              fi
-             if test -z "$url" ||
+             if ! git submodule--helper is-active "$sm_path" ||
              {
                      ! test -d "$sm_path"/.git &&
                      ! test -f "$sm_path"/.git

The $name is no longer used after this step in cmd_status function, as the sole purpose of learning the name from the path was so that we can ask if the submodule has .URL defined and the query is done by name, not path.

This actually raises a question.

Shouldn't "submodule--helper is-active" ask about submodule while identifying the submodule in question by name? Or do all (or most of) the callers start from path and ask is-active on them so that it is handier to let them ask by path?

There we take the path as name and if a real name exists, the name is overwritten with the real name, i.e.

    name = name_for_path(path) ? name_for_path(path) : path;

which IMHO is overly accepting and we should just die in case of !name_for_path.

So to answer your original question, I think the codebase currently thinks by_path is handier, the name is a mere internal field in "struct submodule", useful for looking up its git dir.

Quoted 22 lines · Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 git-submodule.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

git-submodule.sh +1 -2

index 136e26a2c..ab233712d 100755
@@ -1010,14 +1010,13 @@ cmd_status()
 	do
 		die_if_unmatched "$mode" "$sha1"
 		name=$(git submodule--helper name "$sm_path") || exit
-		url=$(git config submodule."$name".url)
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 		if test "$stage" = U
 		then
 			say "U$sha1 $displaypath"
 			continue
 		fi
-		if test -z "$url" ||
+		if ! git submodule--helper is-active "$sm_path" ||

git-submodule.sh cmd_status() · line 1022

Quoted 3 lines

git-submodule.sh +0 -0

 		{
 			! test -d "$sm_path"/.git &&
 			! test -f "$sm_path"/.git

The $name is no longer used after this step in cmd_status function, as the sole purpose of learning the name from the path was so that we can ask if the submodule has .URL defined and the query is done by name, not path.

This actually raises a question.

Shouldn't "submodule--helper is-active" ask about submodule while identifying the submodule in question by name? Or do all (or most of) the callers start from path and ask is-active on them so that it is handier to let them ask by path?

Quoted 24 lines · Junio C Hamano

Quoted 22 lines · Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 git-submodule.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

git-submodule.sh +1 -2

index 136e26a2c..ab233712d 100755
@@ -1010,14 +1010,13 @@ cmd_status()
 	do
 		die_if_unmatched "$mode" "$sha1"
 		name=$(git submodule--helper name "$sm_path") || exit
-		url=$(git config submodule."$name".url)
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 		if test "$stage" = U
 		then
 			say "U$sha1 $displaypath"
 			continue
 		fi
-		if test -z "$url" ||
+		if ! git submodule--helper is-active "$sm_path" ||

Quoted 8 lines

git-submodule.sh cmd_status() · line 1022

Quoted 3 lines

git-submodule.sh +0 -0

 		{
 			! test -d "$sm_path"/.git &&
 			! test -f "$sm_path"/.git

The $name is no longer used after this step in cmd_status function, as the sole purpose of learning the name from the path was so that we can ask if the submodule has .URL defined and the query is done by name, not path.

Thanks, I'll remove computing the name.

Quoted 8 lines

This actually raises a question.

Shouldn't "submodule--helper is-active" ask about submodule while identifying the submodule in question by name? Or do all (or most of) the callers start from path and ask is-active on them so that it is handier to let them ask by path?

As you can Stefan already discussed the path is a much easier handle to work with simply because that is what is more readily available as the name is an internal detail.

Signature · 1 line

Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

git-submodule.sh +0 -0

index ab233712d..e2d08595f 100755

Quoted 10 lines · Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

git-submodule.sh +0 -0

index ab233712d..e2d08595f 100755

git-submodule.sh cmd_sync() · line 1117

Quoted 8 lines

git-submodule.sh +1 -1

@@ -1111,7 +1111,7 @@ cmd_sync()
 			;;
 		esac
 
-		if git config "submodule.$name.url" >/dev/null 2>/dev/null
+		if git submodule--helper is-active "$sm_path"
 		then
 			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 			say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"

This is not a problem this patch introduces, but the loop this hunk is in seems a bit inefficient. It maps the sm_path to its name and then asks .gitmodules the URL the upstream suggests to clone it from, munges it with a large case statement and discards all of that if the module is not active.

Adding this patch on top would be a way to remove the inefficiency and one level of nesting while at it, I think, but I may have missed something, so please double check, and if you agree that this is a good way to go, please do so as a preparatory clean-up.

Thanks.

 git-submodule.sh | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

git-submodule.sh +11 -8

index e2d08595f0..dcdd36fa64 100755
@@ -1089,6 +1089,12 @@ cmd_sync()
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode" "$sha1"
+
+		if ! git submodule--helper is-active "$sm_path"
+		then
+			continue
+		fi
+
 		name=$(git submodule--helper name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
 

@@ -1111,14 +1117,12 @@ cmd_sync()
 			;;
 		esac
 
-		if git submodule--helper is-active "$sm_path"
-		then
-			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-			say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"
-			git config submodule."$name".url "$super_config_url"
+		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
+		say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"
+		git config submodule."$name".url "$super_config_url"
 
-			if test -e "$sm_path"/.git
-			then
+		if test -e "$sm_path"/.git
+		then
 			(
 				sanitize_submodule_env
 				cd "$sm_path"

@@ -1131,7 +1135,6 @@ cmd_sync()
 					eval cmd_sync
 				fi
 			)
-			fi
 		fi
 	done
 }

builtin/submodule--helper.c +2 -9

-	/*
-	 * Looking up the url in .git/config.
-	 * We must not fall back to .gitmodules as we only want
-	 * to process configured submodules.
-	 */
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "submodule.%s.url", sub->name);
-	git_config_get_string(sb.buf, &url);
-	if (!url) {
+	/* Check if the submodule has been initialized. */
+	if (!is_submodule_initialized(ce->name)) {
 		next_submodule_warn_missing(suc, out, displaypath);
 		goto cleanup;
 	}

builtin/submodule--helper.c +0 -1

@@ -845,7 +837,6 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		argv_array_push(&child->args, suc->depth);
 
 cleanup:
-	free(url);
 	strbuf_reset(&displaypath_sb);
 	strbuf_reset(&sb);

Quoted 15 lines · Brandon Williams

builtin/submodule--helper.c +2 -9

-     /*
-      * Looking up the url in .git/config.
-      * We must not fall back to .gitmodules as we only want
-      * to process configured submodules.
-      */
-     strbuf_reset(&sb);
-     strbuf_addf(&sb, "submodule.%s.url", sub->name);
-     git_config_get_string(sb.buf, &url);
-     if (!url) {
+     /* Check if the submodule has been initialized. */
+     if (!is_submodule_initialized(ce->name)) {
              next_submodule_warn_missing(suc, out, displaypath);
              goto cleanup;
      }

builtin/submodule--helper.c static int prepare_to_clone_next_submodule(const struct cache_entry *ce, · line 830

Quoted 5 lines

builtin/submodule--helper.c +1 -1

@@ -835,7 +827,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
              argv_array_push(&child->args, "--depth=1");
      argv_array_pushl(&child->args, "--path", sub->path, NULL);
      argv_array_pushl(&child->args, "--name", sub->name, NULL);
-     argv_array_pushl(&child->args, "--url", url, NULL);
+     argv_array_pushl(&child->args, "--url", sub->url, NULL);

Even without this patch, we already had an instance of struct submodule available in this function, so the query to .git/config this patch removed was unnecessary?

I am wondering what was meant by the comment "We must not fall back to..." that is being removed---is that because sub->url can come from .gitmodules that is in-tree, not from .git/config?

If that is the case, doesn't the
change in this hunk change behaviour from using the URL the user prefers to using the URL the upstream suggests, overriding user's configuration?

Quoted 3 lines

I am wondering what was meant by the comment "We must not fall back to..." that is being removed---is that because sub->url can come from .gitmodules that is in-tree, not from .git/config?

Yes. We want to check for the submodule being "initialized", i.e. having a url in .git/config. (and the struct submodule reads in both .git/config and .gitmodules and overlays them with a given precedence order)

Quoted 27 lines · Junio C Hamano

Quoted 15 lines · Brandon Williams

builtin/submodule--helper.c +2 -9

-     /*
-      * Looking up the url in .git/config.
-      * We must not fall back to .gitmodules as we only want
-      * to process configured submodules.
-      */
-     strbuf_reset(&sb);
-     strbuf_addf(&sb, "submodule.%s.url", sub->name);
-     git_config_get_string(sb.buf, &url);
-     if (!url) {
+     /* Check if the submodule has been initialized. */
+     if (!is_submodule_initialized(ce->name)) {
              next_submodule_warn_missing(suc, out, displaypath);
              goto cleanup;
      }

builtin/submodule--helper.c static int prepare_to_clone_next_submodule(const struct cache_entry *ce, · line 830

Quoted 5 lines

builtin/submodule--helper.c +1 -1

@@ -835,7 +827,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
              argv_array_push(&child->args, "--depth=1");
      argv_array_pushl(&child->args, "--path", sub->path, NULL);
      argv_array_pushl(&child->args, "--name", sub->name, NULL);
-     argv_array_pushl(&child->args, "--url", url, NULL);
+     argv_array_pushl(&child->args, "--url", sub->url, NULL);

Even without this patch, we already had an instance of struct submodule available in this function, so the query to .git/config this patch removed was unnecessary?

Quoted 3 lines

I am wondering what was meant by the comment "We must not fall back to..." that is being removed---is that because sub->url can come from .gitmodules that is in-tree, not from .git/config?

Yes. We want to check for the submodule being "initialized", i.e. having a url in .git/config. (and the struct submodule reads in both .git/config and .gitmodules and overlays them with a given precedence order)

Quoted 3 lines

If that is the case, doesn't the
change in this hunk change behaviour from using the URL the user prefers to using the URL the upstream suggests, overriding user's configuration?

The mentioned precedence makes sure to have the right order:

    /* Overlay the parsed .gitmodules file with .git/config */
    gitmodules_config();
    git_config(submodule_config, NULL);

such that the sub->url is correct as a URL, but not correct as a boolean indicator if the submodule is "initialized".

Currently the submodule.<name>.url config option is used to determine if a given submodule exists and is interesting to the user. This however doesn't work very well because the URL is a config option for the scope of a repository, whereas the existence of a submodule is an option scoped to the working tree.

In a future with worktree support for submodules, there will be multiple working trees, each of which may only need a subset of the submodules checked out. The URL (which is where the submodule repository can be obtained) should not differ between different working trees.

It may also be convenient for users to more easily specify groups of submodules they are interested in as apposed to running "git submodule init <path>" on each submodule they want checked out in their working tree.

To this end two config options are introduced, submodule.active and submodule.<name>.active. The submodule.active config holds a pathspec that specifies which submodules should exist in the working tree. The submodule.<name>.active config is a boolean flag used to indicate if that particular submodule should exist in the working tree.

Given these multiple ways to check for a submodule's existence the more fine-grained submodule.<name>.active option has the highest order of precedence followed by the pathspec check against submodule.active. To ensure backwards compatibility, if neither of these options are set git falls back to checking the submodule.<name>.url option to determine a submodule's existence.

+ cc Jens, FYI.

Once upon a time I brought up different addressing/activating mechanism for submodules and I remember Jens having some uneasy thoughts about that back in the day. This series addresses the user confusion and documentation better than what I had back then.

Quoted 34 lines · Brandon Williams

Currently the submodule.<name>.url config option is used to determine if a given submodule exists and is interesting to the user. This however doesn't work very well because the URL is a config option for the scope of a repository, whereas the existence of a submodule is an option scoped to the working tree.

In a future with worktree support for submodules, there will be multiple working trees, each of which may only need a subset of the submodules checked out. The URL (which is where the submodule repository can be obtained) should not differ between different working trees.

It may also be convenient for users to more easily specify groups of submodules they are interested in as apposed to running "git submodule init <path>" on each submodule they want checked out in their working tree.

To this end two config options are introduced, submodule.active and submodule.<name>.active. The submodule.active config holds a pathspec that specifies which submodules should exist in the working tree. The submodule.<name>.active config is a boolean flag used to indicate if that particular submodule should exist in the working tree.

Given these multiple ways to check for a submodule's existence the more fine-grained submodule.<name>.active option has the highest order of precedence followed by the pathspec check against submodule.active. To ensure backwards compatibility, if neither of these options are set git falls back to checking the submodule.<name>.url option to determine a submodule's existence.

Documentation/config.txt submodule.<name>.ignore:: · line 2966

Quoted 4 lines

Documentation/config.txt +4 -0

+submodule.<name>.active::
+       Boolean value indicating if the submodule is of interest to git
+       commands.  This config option takes precedence over the
+       submodule.active config option.

... which itself takes precedence over the (deprecated) .URL We conveniently do not talk about the URL here anymore. But! We need to change submodule.<name>.URL now?

Currently the submodule.<name>.url config option is used to determine if a given submodule exists and is interesting to the user. This however doesn't work very well because the URL is a config option for the scope of a repository, whereas the existence of a submodule is an option scoped to the working tree.

In a future with worktree support for submodules, there will be multiple working trees, each of which may only need a subset of the submodules checked out. The URL (which is where the submodule repository can be obtained) should not differ between different working trees.

It may also be convenient for users to more easily specify groups of submodules they are interested in as apposed to running "git submodule init <path>" on each submodule they want checked out in their working tree.

To this end two config options are introduced, submodule.active and submodule.<name>.active. The submodule.active config holds a pathspec that specifies which submodules should exist in the working tree. The submodule.<name>.active config is a boolean flag used to indicate if that particular submodule should exist in the working tree.

submodule.c +25 -4

index 0a2831d84..2b33bd70f 100644
@@ -217,13 +217,41 @@ void gitmodules_config_sha1(const unsigned char *commit_sha1)
 int is_submodule_initialized(const char *path)
 {
 	int ret = 0;
-	const struct submodule *module = NULL;
+	char *key;
+	const struct string_list *sl;
+	const struct submodule *module = submodule_from_path(null_sha1, path);
 
-	module = submodule_from_path(null_sha1, path);
+	/* early return if there isn't a path->module mapping */
+	if (!module)
+		return 0;
+
+	/* submodule.<name>.active is set */
+	key = xstrfmt("submodule.%s.active", module->name);
+	if (!git_config_get_bool(key, &ret)) {
+		free(key);
+		return ret;
+	}
+	free(key);
+
+	sl = git_config_get_value_multi("submodule.active");
 
-	if (module) {
-		char *key = xstrfmt("submodule.%s.url", module->name);
+	if (sl) {
+		struct pathspec ps;
+		struct argv_array args = ARGV_ARRAY_INIT;
+		const struct string_list_item *item;
+
+		for_each_string_list_item(item, sl) {
+			argv_array_push(&args, item->string);
+		}
+

t/t7413-submodule-is-active.sh +1 -0

index f18e0c925..c41b899ab 100755
@@ -28,4 +28,59 @@ test_expect_success 'is-active works with urls' '
 	git -C super submodule--helper is-active sub1
 '
 
+test_expect_success 'is-active works with submodule.<name>.active config' '

Also submodule.active that takes pathspec and not name is an oddball (use of "name" not "path" is to prepare for a submodule whose location in the superproject changes depending on the commit in the superproject), and we need to justify with an explanation. I think you envision two cases. 1. we encourage projects to adopt a convention that submodules are grouped with leading directory, so

that pathspec e.g. lib/, would cover _all_ library-ish modules to allow those who are interested in library-ish modules to set ".active = lib/" just once to say any and all modules in lib/ are interesting. 2. another convention the projects can adopt, when pathspec-attribute feature is invented, is to label submodules with attribute to group them, so that a broad pathspec with attribute requirement, e.g. .:(attr:lib), can be used to say any and all modules with 'lib' attribute are interesting.

The above two points (justifications, intended uses and future plans) need to be clarified around here (and possibly in the documentation), I would think.

Quoted 5 lines · Brandon Williams

Currently the submodule.<name>.url config option is used to determine if a given submodule exists and is interesting to the user. This however doesn't work very well because the URL is a config option for the scope of a repository, whereas the existence of a submodule is an option scoped to the working tree.

A submodule exists if it is exists, whether the user is interested in it or not. If it should be checked out in the working tree is a different matter, but that should be a logical AND between "is it of interest?" and "is the superproject tree has a gitlink for it in its working tree?".

So I do not agree with "This however doesn't work" at all. I'd understand it if you said "This is cumbersome if we want to do this and that, which are different from what we have done traditionally" and explain what this and that are and how they are different.

Quoted 4 lines

In a future with worktree support for submodules, there will be multiple working trees, each of which may only need a subset of the submodules checked out. The URL (which is where the submodule repository can be obtained) should not differ between different working trees.

And this makes the motivation a bit clearer. When the user wants to have multiple worktrees for the same superproject. In such a setting, the same submodule in two worktrees typically want to have the same URL. It may be different from what the upstream suggests in the .gitmodules file, but the difference, i.e. the site specific customization of the URL, should be the same between the two worktrees. But one worktree may be and the other worktree may not be interested in that submodule, and with shared .git/config file, you cannot have submodule.<name>.url set to one value and unset at the same time.

This series does not solve the "two worktrees cannot have private parts in the configuration namespace" issue, but assuming it will be solved by some other series, it anticipates that submodule.<name>.URL would want to be shared between two worktrees most of time (note that there will be users who want two separate .URL for the same submodule while sharing the object database via worktrees mechanism, and you'll need to prepare for them, too), and another "bit" that tells if the submodule is of interest would want to be private to each worktree.

That is the motivation, the reason why you want .URL to stop serving the dual purpose of overriding upstream-suggested URL and indicating the submodule is interesting to the user.

Quoted 10 lines

It may also be convenient for users to more easily specify groups of submodules they are interested in as apposed to running "git submodule init <path>" on each submodule they want checked out in their working tree.

To this end two config options are introduced, submodule.active and submodule.<name>.active. The submodule.active config holds a pathspec that specifies which submodules should exist in the working tree. The submodule.<name>.active config is a boolean flag used to indicate if that particular submodule should exist in the working tree.

And because two worktrees always share their .git/config, these new configuration variables are useless to help workflow with multiple worktrees with the current system, until "per-worktree configuration" is invented. But we prepare for that future in this step.

Also submodule.active that takes pathspec and not name is an oddball (use of "name" not "path" is to prepare for a submodule whose location in the superproject changes depending on the commit in the superproject), and we need to justify with an explanation. I think you envision two cases. 1. we encourage projects to adopt a convention that submodules are grouped with leading directory, so that pathspec e.g. lib/, would cover _all_ library-ish modules to allow those who are interested in library-ish modules to set ".active = lib/" just once to say any and all modules in lib/ are interesting. 2. another convention the projects can adopt, when pathspec-attribute feature is invented, is to label submodules with attribute to group them, so that a broad pathspec with attribute requirement, e.g. .:(attr:lib), can be used to say any and all modules with 'lib' attribute are interesting.

The above two points (justifications, intended uses and future plans) need to be clarified around here (and possibly in the documentation), I would think.

Quoted 49 lines

submodule.c +32 -4

index 0a2831d84..2b33bd70f 100644
@@ -217,13 +217,41 @@ void gitmodules_config_sha1(const unsigned char *commit_sha1)
 int is_submodule_initialized(const char *path)
 {
 	int ret = 0;
-	const struct submodule *module = NULL;
+	char *key;
+	const struct string_list *sl;
+	const struct submodule *module = submodule_from_path(null_sha1, path);
 
-	module = submodule_from_path(null_sha1, path);
+	/* early return if there isn't a path->module mapping */
+	if (!module)
+		return 0;
+
+	/* submodule.<name>.active is set */
+	key = xstrfmt("submodule.%s.active", module->name);
+	if (!git_config_get_bool(key, &ret)) {
+		free(key);
+		return ret;
+	}
+	free(key);
+
+	sl = git_config_get_value_multi("submodule.active");
 
-	if (module) {
-		char *key = xstrfmt("submodule.%s.url", module->name);
+	if (sl) {
+		struct pathspec ps;
+		struct argv_array args = ARGV_ARRAY_INIT;
+		const struct string_list_item *item;
+
+		for_each_string_list_item(item, sl) {
+			argv_array_push(&args, item->string);
+		}
+
+		parse_pathspec(&ps, 0, 0, 0, args.argv);
+		ret = match_pathspec(&ps, path, strlen(path), 0, NULL, 1);
+
+		argv_array_clear(&args);
+		clear_pathspec(&ps);
+	} else {
 		char *value = NULL;
+		key = xstrfmt("submodule.%s.url", module->name);
 

submodule.c void gitmodules_config_sha1(const unsigned char *commit_sha1) · line 256

Quoted 1 line

submodule.c +0 -0

 		ret = !git_config_get_string(key, &value);

It probably is easier to read if you had a final "return ret" in the "if (sl) {...}" part, just like you have one for the codepath that deals with "submodule.<name>.active", and flatten the else clause. That would make it clear that we have three ways with decreasing precedence.

At this point, the answer from function is even less about "is it initialized?" but about "is it of interest?" (or "is it to be initialized?"). We'd probably want a /* NEEDSWORK */ comment before the function to remind us to come up with a better name after the dust settles.

Quoted 9 lines

t/t7413-submodule-is-active.sh +0 -0

index f18e0c925..c41b899ab 100755
@@ -28,4 +28,59 @@ test_expect_success 'is-active works with urls' '
 	git -C super submodule--helper is-active sub1
 '
 

t/t7413-submodule-is-active.sh test_expect_success 'is-active works with urls' ' · line 41

Quoted 11 lines

t/t7413-submodule-is-active.sh +11 -0

+test_expect_success 'is-active works with submodule.<name>.active config' '
+	git -C super config --bool submodule.sub1.active "false" &&
+	test_must_fail git -C super submodule--helper is-active sub1 &&
+
+	git -C super config --bool submodule.sub1.active "true" &&
+	git -C super config --unset submodule.sub1.URL &&
+	git -C super submodule--helper is-active sub1 &&
+
+	git -C super config submodule.sub1.URL ../sub &&
+	git -C super config --unset submodule.sub1.active
+'

The last "unset" is done to clean the customization this test did, in order to give a predictable beginning state to the next test? If so, use test_when_finished instead of &&-cascading it at the end.

Quoted 4 lines

t/t7413-submodule-is-active.sh +3 -0

+ ...
+test_expect_success 'is-active with submodule.active and submodule.<name>.active' '
+	git -C super config --add submodule.active "sub1" &&

t/t7413-submodule-is-active.sh test_expect_success 'is-active works with urls' ' · line 84

Quoted 10 lines

t/t7413-submodule-is-active.sh +10 -0

+	git -C super config --bool submodule.sub1.active "false" &&
+	git -C super config --bool submodule.sub2.active "true" &&
+
+	test_must_fail git -C super submodule--helper is-active sub1 &&
+	git -C super submodule--helper is-active sub2 &&
+
+	git -C super config --unset-all submodule.active &&
+	git -C super config --unset submodule.sub1.active &&
+	git -C super config --unset submodule.sub2.active
+'

Likewise for all the new tests in this patch.

Thanks.

builtin/submodule--helper.c +0 -0

index f38e332c5..a3acc9e4c 100644
@@ -270,6 +270,34 @@ static int module_list_compute(int argc, const char **argv,
 	return result;

Documentation/git-submodule.txt the submodule itself. · line 394

Quoted 4 lines · Brandon Williams

+--init-active::

Documentation/git-submodule.txt +3 -0

+	This option is only valid for the update command.
+	Initialize all submodules configured in "`submodule.active`"
+	that have not been updated before.

You mean s/not been updated/not been initialized/ here?

Quoted 9 lines

builtin/submodule--helper.c +0 -0

index f38e332c5..a3acc9e4c 100644
@@ -270,6 +270,34 @@ static int module_list_compute(int argc, const char **argv,
 	return result;
 }
 

builtin/submodule--helper.c static int module_list_compute(int argc, const char **argv, · line 280

Quoted 8 lines

builtin/submodule--helper.c +8 -0

+static void module_list_active(struct module_list *list)
+{
+	int i;
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
+	gitmodules_config();

To this codepath, it probably would not make any difference, but in general, calling gitmodules_config() after you did git_config() would be a bug, right? Otherwise, gitmodules_config() would overwrite submodule.* configuration you read with git_config().

I have a feeling that we need to make this function hard to misuse; it is not a fault of _this_ patch, but still any and all additions to any of the submodule*.c files needs careful vetting.

builtin/submodule--helper.c +5 -0

+static void module_list_active(struct module_list *list)
+{
+	int i;
+
+	if (read_cache() < 0)

builtin/submodule--helper.c +11 -0

+		ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
+		list->entries[list->nr++] = ce;
+		while (i + 1 < active_nr &&
+		       !strcmp(ce->name, active_cache[i + 1]->name))
+			/*
+			 * Skip entries with the same name in different stages
+			 * to make sure an entry is returned only once.
+			 */
+			i++;
+	}
+}

Quoted 14 lines · Brandon Williams

builtin/submodule--helper.c +14 -0

+static void module_list_active(struct module_list *list)
+{
+	int i;
+
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
+
+	gitmodules_config();
+
+	for (i = 0; i < active_nr; i++) {
+		const struct cache_entry *ce = active_cache[i];
+
+		if (!S_ISGITLINK(ce->ce_mode) ||
+		    !is_submodule_initialized(ce->name))

builtin/submodule--helper.c static int module_list_compute(int argc, const char **argv, · line 287

Quoted 1 line

builtin/submodule--helper.c +1 -0

+			continue;

This, because "is_submodule_initialized()" is not "is it initialized?" but "is it interesting?", will catch a submodule that used to be uninteresting but has become interesting (e.g. a new submodule added to somewhere in "lib/" when submodule.active lists "lib/" as one of the pathspecs to match interesting submodules) and automatically updates it.

The new switch passes the pathspec to git submodule update --init-active which is called after the actual clone is done.

Additionally this configures the submodule.active option to be the given pathspec, such that any future invocation of git submodule update --init-active will keep up with the pathspec.

Based on a patch by Stefan Beller <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---
 Documentation/git-clone.txt | 23 ++++++++++-----
 builtin/clone.c             | 36 +++++++++++++++++++++--
 t/t7400-submodule-basic.sh  | 70 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 9 deletions(-)

Documentation/git-clone.txt +0 -0

index 35cc34b2f..9692eab30 100644

Documentation/git-clone.txt +0 -2

@@ -217,12 +218,20 @@ 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

builtin/clone.c +0 -0

index 3f63edbbf..c6731379b 100644
@@ -56,6 +56,16 @@ static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
 static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;

Quoted 17 lines · Brandon Williams

The new switch passes the pathspec to git submodule update --init-active which is called after the actual clone is done.

Additionally this configures the submodule.active option to be the given pathspec, such that any future invocation of git submodule update --init-active will keep up with the pathspec.

Based on a patch by Stefan Beller <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---
 Documentation/git-clone.txt | 23 ++++++++++-----
 builtin/clone.c             | 36 +++++++++++++++++++++--
 t/t7400-submodule-basic.sh  | 70 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 9 deletions(-)

Documentation/git-clone.txt SYNOPSIS · line 19

Quoted 11 lines

Documentation/git-clone.txt +2 -1

index 35cc34b2f..9692eab30 100644
@@ -15,7 +15,8 @@ SYNOPSIS
 	  [--dissociate] [--separate-git-dir <git dir>]
 	  [--depth <depth>] [--[no-]single-branch]
 	  [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
-	  [--jobs <n>] [--] <repository> [<directory>]
+	  [--submodule-spec <pathspec>] [--jobs <n>] [--]
+	  <repository> [<directory>]

Hmph. Can we then make "--recurse-submodules" an obsolete way to spell "--submodule-spec ."? I am not actively suggesting to deprecate it; I am trying to see if there are semantic differences between the two.

I am also wondering "--recurse-submodules=<pathspec>" would be a better UI, instead of introducing yet another option.

Quoted 13 lines

Documentation/git-clone.txt +2 -6

@@ -217,12 +218,20 @@ 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

Documentation/git-clone.txt objects from the source repository into a pack in the cloned repository. · line 226

Quoted 4 lines

Documentation/git-clone.txt +4 -0

+	running `git submodule update --recursive --init` 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)

With reflowing it is unnecessarily harder to spot what got changed. "and clone" is inserted, "--init" and "--recursive" were swapped. Any other changes?

Quoted 13 lines

builtin/clone.c +4 -0

index 3f63edbbf..c6731379b 100644
@@ -56,6 +56,16 @@ static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
 static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list submodule_spec;
+
+static int submodule_spec_cb(const struct option *opt, const char *arg, int unset)
+{

builtin/clone.c static struct string_list option_required_reference = STRING_LIST_INIT_NODUP; · line 68

Quoted 6 lines

builtin/clone.c +6 -0

+	if (unset)
+		return -1;
+
+	string_list_append((struct string_list *)opt->value, arg);
+	return 0;
+}

Hmph, doesn't OPT_STRING_LIST work for this thing?

Quoted 19 lines · Junio C Hamano

Quoted 17 lines · Brandon Williams

The new switch passes the pathspec to git submodule update --init-active which is called after the actual clone is done.

Additionally this configures the submodule.active option to be the given pathspec, such that any future invocation of git submodule update --init-active will keep up with the pathspec.

Based on a patch by Stefan Beller <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---
 Documentation/git-clone.txt | 23 ++++++++++-----
 builtin/clone.c             | 36 +++++++++++++++++++++--
 t/t7400-submodule-basic.sh  | 70 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 9 deletions(-)

Quoted 16 lines

Documentation/git-clone.txt SYNOPSIS · line 19

Quoted 11 lines

Documentation/git-clone.txt +2 -1

index 35cc34b2f..9692eab30 100644
@@ -15,7 +15,8 @@ SYNOPSIS
      [--dissociate] [--separate-git-dir <git dir>]
      [--depth <depth>] [--[no-]single-branch]
      [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
-     [--jobs <n>] [--] <repository> [<directory>]
+     [--submodule-spec <pathspec>] [--jobs <n>] [--]
+     <repository> [<directory>]

Hmph. Can we then make "--recurse-submodules" an obsolete way to spell "--submodule-spec ."? I am not actively suggesting to deprecate it; I am trying to see if there are semantic differences between the two.

We can if you think that would be better. That way if at clone time you say "I want the submodules too" that your default config tracks all submodules even new ones yet to be added.

Quoted 1 line

Quoted 2 lines

I am also wondering "--recurse-submodules=<pathspec>" would be a better UI, instead of introducing yet another option.

Yeah we could do that, have --recurse-submodules be a repeated option and if you don't specify a value it defaults to "."

Any thoughts on this Stefan?

When initializing a submodule set the submodule.<name>.active config to true to indicate that the submodule is active.

Signed-off-by Brandon Williams <[email protected]>

---

 builtin/submodule--helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

builtin/submodule--helper.c +7 -0

index a3acc9e4c..b669ed031 100644
@@ -361,6 +361,13 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 		die(_("No url found for submodule path '%s' in .gitmodules"),
 			displaypath);
 
+	/* Set active flag for the submodule being initialized */
+	if (!is_submodule_initialized(path)) {
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "submodule.%s.active", sub->name);
+		git_config_set_gently(sb.buf, "true");
+	}
+
 	/*
 	 * Copy url setting when it is not set yet.
 	 * To look up the url in .git/config, we must not fall back to

Quoted 5 lines · Brandon Williams

When initializing a submodule set the submodule.<name>.active config to true to indicate that the submodule is active.

Signed-off-by Brandon Williams <[email protected]>

---

Hmph. When you do

	git clone --submodule-spec=lib/

and resulting repository says "submodule.active = lib/", you are saying that you are interested in anything in "lib/", now or in the future--that is the point of 08/10.

It is unclear what your desire would be for a submodule that was bound to somewhere in "lib/" in the superproject when you cloned when it later gets moved outside "lib/". With changes up to 08/10, that submodule will no longer be interesting, but with this 09/10, when recursing clone will activate it upon the initial clone, the submodule is explicitly marked as active by its name, so no matter where it gets moved later, it will remain to be interesting.

I am not sure if that is a desired behaviour.

Quoted 22 lines

 builtin/submodule--helper.c | 7 +++++++
 1 file changed, 7 insertions(+)

builtin/submodule--helper.c +7 -0

index a3acc9e4c..b669ed031 100644
@@ -361,6 +361,13 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 		die(_("No url found for submodule path '%s' in .gitmodules"),
 			displaypath);
 
+	/* Set active flag for the submodule being initialized */
+	if (!is_submodule_initialized(path)) {
+		strbuf_reset(&sb);
+		strbuf_addf(&sb, "submodule.%s.active", sub->name);
+		git_config_set_gently(sb.buf, "true");
+	}
+
 	/*
 	 * Copy url setting when it is not set yet.
 	 * To look up the url in .git/config, we must not fall back to

So if we did

  git clone --submodule-spec=lib/

then each module under 'lib/' won't have their submodule.<name>.active config set since submodule.active=lib/ already encompasses them.