Re: [PATCH v3 05/10] submodule: decouple url and submodule existence
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 14, 2017, 18:42 UTC
- Message-ID
- <xmqqzignittf.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170313214341.172676-6-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
Show 5 quoted lines
> 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.
> 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.
Show 10 quoted 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.
Show 49 quoted lines
> diff --git a/submodule.c b/submodule.c
> index 0a2831d84..2b33bd70f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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);
>
> 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.
Show 19 quoted lines
> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh > index f18e0c925..c41b899ab 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' ' > + 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.
Show 13 quoted lines
> + ... > +test_expect_success 'is-active with submodule.active and submodule.<name>.active' ' > + git -C super config --add submodule.active "sub1" && > + 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.