On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> Currently the submodule.<name>.url config option is used to determine if
> a given submodule is of interest to the user. This ends up being
> cumbersome in a world where we want to have different submodules checked
> out in different worktrees or a more generalized mechanism to select
> which submodules are of interest.
>
> 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
Did you mean opposed?
> 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.
>
> Its important to note that submodule.active functions differently than
> the other configuration options since it takes a pathspec. This allows
> users to adopt at least two new workflows:
>
> 1. Submodules can be grouped with a leading directory, such that a
> pathspec e.g. 'lib/' would cover all library-ish modules to allow
> those who are interested in library-ish modules to set
> "submodule.active = lib/" just once to say any and all modules in
> 'lib/' are interesting.
>
> 2. Once the pathspec-attribute feature is invented, users can label
> submodules with attributes to group them, so that a broad pathspec
> with attribute requirements, e.g. ':(attr:lib)', can be used to say
> any and all modules with the 'lib' attribute are interesting.
> Since the .gitattributes file, just like the .gitmodules file, is
> tracked by the superproject, when a submodule moves in the
> superproject tree, the project can adjust which path gets the
> attribute in .gitattributes, just like it can adjust which path has
> the submodule in .gitmodules.
>
> Neither of these two additional configuration options solve the problem
> of wanting different submodules checked out in different worktrees
> because multiple worktrees share .git/config. Only once per-worktree
> configurations become a reality can this be solved, but this is a
> necessary preparatory step for that future.
>
> Given these multiple ways to check if a submodule is of interest, 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
> if a submodule is interesting.
>
> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> ---
> Documentation/config.txt | 15 ++++++++++--
> submodule.c | 50 ++++++++++++++++++++++++++++++++------
> t/t7413-submodule-is-active.sh | 55 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 110 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5e5c2ae5f..d2d79b9d4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2920,8 +2920,9 @@ submodule.<name>.url::
> The URL for a submodule. This variable is copied from the .gitmodules
> file to the git config via 'git submodule init'. The user can change
> the configured URL before obtaining the submodule via 'git submodule
> - update'. After obtaining the submodule, the presence of this variable
> - is used as a sign whether the submodule is of interest to git commands.
> + update'. If neither submodule.<name>.active or submodule.active are
> + set, the presence of this variable is used as a fallback to indicate
> + whether the submodule is of interest to git commands.
> See linkgit:git-submodule[1] and linkgit:gitmodules[5] for details.
>
> submodule.<name>.update::
> @@ -2959,6 +2960,16 @@ submodule.<name>.ignore::
> "--ignore-submodules" option. The 'git submodule' commands are not
> affected by this setting.
>
> +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.
> +
> +submodule.active::
> + A repeated field which contains a pathspec used to match against a
> + submodule's path to determine if the submodule is of interest to git
> + commands.
> +
> submodule.fetchJobs::
> Specifies how many submodules are fetched/cloned at the same time.
> A positive integer allows up to that number of submodules fetched
> diff --git a/submodule.c b/submodule.c
> index 0a2831d84..ad2779ee7 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -212,25 +212,59 @@ void gitmodules_config_sha1(const unsigned char *commit_sha1)
> }
>
> /*
> + * NEEDSWORK: With the addition of different configuration options to determine
> + * if a submodule is of interests, the validity of this function's name comes
> + * into question. Once the dust has settled and more concrete terminology is
> + * decided upon, come up with a more proper name for this function. One
> + * potential candidate could be 'is_submodule_active()'.
> + *
> * Determine if a submodule has been initialized at a given 'path'
> */
> int is_submodule_initialized(const char *path)
> {
> int ret = 0;
> - const struct submodule *module = NULL;
> + char *key = NULL;
> + char *value = NULL;
> + 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;
>
> - if (module) {
> - char *key = xstrfmt("submodule.%s.url", module->name);
> - char *value = NULL;
> + /* 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);
>
> - ret = !git_config_get_string(key, &value);
> + /* submodule.active is set */
> + sl = git_config_get_value_multi("submodule.active");
> + if (sl) {
> + struct pathspec ps;
> + struct argv_array args = ARGV_ARRAY_INIT;
> + const struct string_list_item *item;
>
> - free(value);
> - free(key);
> + for_each_string_list_item(item, sl) {
> + argv_array_push(&args, item->string);
> + }
> +
> + parse_pathspec(&ps, 0, 0, NULL, args.argv);
> + ret = match_pathspec(&ps, path, strlen(path), 0, NULL, 1);
> +
> + argv_array_clear(&args);
> + clear_pathspec(&ps);
> + return ret;
> }
>
> + /* fallback to checking if the URL is set */
> + key = xstrfmt("submodule.%s.url", module->name);
> + ret = !git_config_get_string(key, &value);
> +
> + free(value);
> + free(key);
> return ret;
> }
>
> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
> index f18e0c925..ea1644b58 100755
> --- a/t/t7413-submodule-is-active.sh
> +++ b/t/t7413-submodule-is-active.sh
> @@ -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' '
> + test_when_finished "git -C super config --unset submodule.sub1.active" &&
Sorry for late notice: This can be solved more elegantly with
test_config[_global].
If this is the only nit, we can fix it later, though.
> + test_when_finished "git -C super config submodule.sub1.URL ../sub" &&
All tests look like they are structured very similarly; we also want
to test all combinations,
so maybe it is easier to make a function
test_active_case()
{
on_submodule=$1
expectation=$2
with_URL=$3
with_active_flag=$4
with_active_pathspec=$5
test_expect_success 'is_active for $1 $2 $3 $4 $5' '
# do the actual test
'
}
# here put all combinations for a submodule + the non-submodule case
I am not sure though, it would be a more disciplined approach, but not
necessarily
be covering the corner cases just as nice, i.e. having multiple
pathspecs, including 'exclude's.
Just food for thought.
Thanks,
Stefan