From: "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: derrickstolee@github.com, chooglen@google.com, gitster@pobox.com,
Victoria Dye <vdye@github.com>
Subject: [PATCH v2 0/3] Fix behavior of worktree config in submodules
Date: Fri, 26 May 2023 01:32:57 +0000 [thread overview]
Message-ID: <pull.1536.v2.git.1685064781.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1536.git.1684883872.gitgitgadget@gmail.com>
About a year ago, discussion on the sparse index integration of 'git grep'
surfaced larger incompatibilities between sparse-checkout and submodules
[1]. This series fixes one of the underlying issues to that incompatibility,
which is that the worktree config of the submodule (where
'core.sparseCheckout', 'core.sparseCheckoutCone', and 'index.sparse' are
set) is not used when operating on the submodule from its super project
(e.g., in a command with '--recurse-submodules').
The outcome of this series is that 'extensions.worktreeConfig' and the
contents of the repository's worktree config are read and applied to (and
only to) the relevant repo when working in a super project/submodule setup.
This alone doesn't fix sparse-checkout/submodule interoperability; the
additional changes needed for that will be submitted in a later series. I'm
also hoping this will help (or at least not hurt) the work to avoid use of
global state in config parsing [2].
Changes since V1
================
* In 't3007', replaced manual 'git config'/'test_when_finished "git config
--unset"' pairs with 'test_config' helper. Updated 'test_config' to
handle the '--worktree' option.
* Updated commit messages & test comments to better explain the purpose and
more subtle functionality details to the new tests
* Added a commit to move 'struct repository' out of 'git_config_source',
rather than creating a dummy 'config_source' just to hold a repository
instance.
* Changed the config setting in the new tests from 'feature.experimental'
to 'index.sparse' to tie these changes to their intended use case.
* "super project" -> "superproject"
Thanks!
* Victoria
[1]
https://lore.kernel.org/git/093827ae-41ef-5f7c-7829-647536ce1305@github.com/
[2]
https://lore.kernel.org/git/pull.1497.git.git.1682104398.gitgitgadget@gmail.com/
Victoria Dye (3):
config: use gitdir to get worktree config
config: pass 'repo' directly to 'config_with_options()'
repository: move 'repository_format_worktree_config' to repo scope
builtin/config.c | 17 +++++----
builtin/worktree.c | 2 +-
config.c | 49 ++++++++++++++++----------
config.h | 4 +--
environment.c | 1 -
environment.h | 1 -
repository.c | 1 +
repository.h | 1 +
setup.c | 10 ++++--
submodule-config.c | 3 +-
t/t3007-ls-files-recurse-submodules.sh | 33 +++++++++++++++++
t/test-lib-functions.sh | 13 +++++--
worktree.c | 4 +--
13 files changed, 102 insertions(+), 37 deletions(-)
base-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1536%2Fvdye%2Fvdye%2Fsubmodule-worktree-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1536/vdye/vdye/submodule-worktree-config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1536
Range-diff vs v1:
1: aead2fe1ce1 ! 1: fb597cdfeb0 config: use gitdir to get worktree config
@@ Commit message
The worktree config is loaded from the path returned by
'git_pathdup("config.worktree")', the 'config.worktree' relative to the
gitdir of 'the_repository'. If loading the config for a submodule, this path
- is incorrect, since 'the_repository' is the super project. Conversely,
- 'opts->git_dir' is the gitdir of the submodule being configured, so the
- config file in that location should be read instead.
+ is incorrect, since 'the_repository' is the superproject. 'opts->git_dir' is
+ the gitdir of the submodule being configured, so the config file in that
+ location should be read instead.
To ensure the use of 'opts->git_dir' is safe, require that 'opts->git_dir'
is set if-and-only-if 'opts->commondir' is set (rather than "only-if" as it
is now). In all current usage of 'config_options', these values are set
together, so the stricter check does not change any behavior.
- Finally, add tests to 't3007-ls-files-recurse-submodules.sh' to demonstrate
- the corrected config loading behavior. Note that behavior still isn't ideal
- because 'extensions.worktreeConfig' in the super project controls whether or
- not the worktree config is used in the submodule. This will be fixed in a
- later patch.
+ Finally, add tests to 't3007-ls-files-recurse-submodules.sh' to verify the
+ corrected config is loaded. Use 'ls-files' to test this because, unlike some
+ other '--recurse-submodules' commands, 'ls-files' parses the config of the
+ submodule in the same process as the superproject (via 'show_submodule()' ->
+ 'repo_read_index()' -> 'prepare_repo_settings()'). As a result,
+ 'the_repository' points to the config of the superproject but the
+ commondir/gitdir in the config sequence will be that of the submodule,
+ providing the exact scenario needed to verify this patch.
+
+ The first test ('--recurse-submodules parses submodule repo config') checks
+ that the submodule's *repo* config is read when running 'ls-files' on the
+ superproject; this confirms already-working behavior, serving as a reference
+ for how worktree config parsing should behave. The second test
+ ('--recurse-submodules parses submodule worktree config') tests the same
+ scenario as the previous but instead using the *worktree* config,
+ demonstrating the corrected behavior. The 'test_config' helper is extended
+ for this case so that it properly applies the '--worktree' option to the
+ configure/unconfigure operations it performs.
+
+ Note that, although the submodule worktree config is now parsed instead of
+ the superproject's, 'extensions.worktreeConfig' in the superproject still
+ controls whether or not the worktree config is enabled at all in the
+ submodule. This will be fixed in a later patch.
Signed-off-by: Victoria Dye <vdye@github.com>
@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodule
'
+test_expect_success '--recurse-submodules parses submodule repo config' '
-+ test_when_finished "git -C submodule config --unset feature.experimental" &&
-+ git -C submodule config feature.experimental "invalid non-boolean value" &&
++ test_config -C submodule index.sparse "invalid non-boolean value" &&
+ test_must_fail git ls-files --recurse-submodules 2>err &&
+ grep "bad boolean config value" err
+'
+
+test_expect_success '--recurse-submodules parses submodule worktree config' '
-+ test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
-+ test_when_finished "git -C submodule config --worktree --unset feature.experimental" &&
-+ test_when_finished "git config --unset extensions.worktreeConfig" &&
++ test_config -C submodule extensions.worktreeConfig true &&
++ test_config -C submodule --worktree index.sparse "invalid non-boolean value" &&
+
-+ git -C submodule config extensions.worktreeConfig true &&
-+ git -C submodule config --worktree feature.experimental "invalid non-boolean value" &&
-+
-+ # NEEDSWORK: the extensions.worktreeConfig is set globally based on super
-+ # project, so we need to enable it in the super project.
-+ git config extensions.worktreeConfig true &&
++ # NEEDSWORK: the extensions.worktreeConfig is set globally based on
++ # superproject, so we need to enable it in the superproject.
++ test_config extensions.worktreeConfig true &&
+
+ test_must_fail git ls-files --recurse-submodules 2>err &&
+ grep "bad boolean config value" err
@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodule
test_incompatible_with_recurse_submodules () {
test_expect_success "--recurse-submodules and $1 are incompatible" "
test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+
+ ## t/test-lib-functions.sh ##
+@@ t/test-lib-functions.sh: test_config () {
+ config_dir=$1
+ shift
+ fi
+- test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
+- git ${config_dir:+-C "$config_dir"} config "$@"
++
++ # If --worktree is provided, use it to configure/unconfigure
++ is_worktree=
++ if test "$1" = --worktree
++ then
++ is_worktree=1
++ shift
++ fi
++
++ test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} ${is_worktree:+--worktree} '$1'" &&
++ git ${config_dir:+-C "$config_dir"} config ${is_worktree:+--worktree} "$@"
+ }
+
+ test_config_global () {
-: ----------- > 2: 26a36423a8a config: pass 'repo' directly to 'config_with_options()'
2: 5ed9100a770 ! 3: 506a2cf8c73 repository: move 'repository_format_worktree_config' to repo scope
@@ Commit message
Move 'repository_format_worktree_config' out of the global scope and into
the 'repository' struct. This change is similar to how
'repository_format_partial_clone' was moved in ebaf3bcf1ae (repository: move
- global r_f_p_c to repo struct, 2021-06-17), adding to the 'repository'
+ global r_f_p_c to repo struct, 2021-06-17), adding it to the 'repository'
struct and updating 'setup.c' & 'repository.c' functions to assign the value
- appropriately. In addition, update usage of the setting to reference the
- relevant context's repo or, as a fallback, 'the_repository'.
+ appropriately.
- The primary goal of this change is to be able to load worktree config for a
- submodule depending on whether that submodule - not the super project - has
+ The primary goal of this change is to be able to load the worktree config of
+ a submodule depending on whether that submodule - not its superproject - has
'extensions.worktreeConfig' enabled. To ensure 'do_git_config_sequence()'
- has access to the newly repo-scoped configuration:
-
- - update 'repo_read_config()' to create a 'config_source' to hold the
- repo instance
- - add a 'repo' argument to 'do_git_config_sequence()'
- - update 'config_with_options' to call 'do_git_config_sequence()' with
- 'config_source.repo', or 'the_repository' as a fallback
+ has access to the newly repo-scoped configuration, add a 'struct repository'
+ argument to 'do_git_config_sequence()' and pass it the 'repo' value from
+ 'config_with_options()'.
Finally, add/update tests in 't3007-ls-files-recurse-submodules.sh' to
- verify 'extensions.worktreeConfig' is read an used independently by super
- projects and submodules.
+ verify 'extensions.worktreeConfig' is read an used independently by
+ superprojects and submodules.
Signed-off-by: Victoria Dye <vdye@github.com>
@@ config.c: static int do_git_config_sequence(struct config_reader *reader,
!access_or_die(worktree_config, R_OK, 0)) {
ret += git_config_from_file(fn, worktree_config, data);
}
-@@ config.c: int config_with_options(config_fn_t fn, void *data,
- data = &inc;
- }
-
-- if (config_source)
-+ if (config_source && config_source->scope != CONFIG_SCOPE_UNKNOWN)
- config_reader_set_scope(&the_reader, config_source->scope);
-
- /*
@@ config.c: int config_with_options(config_fn_t fn, void *data,
ret = git_config_from_blob_ref(fn, repo, config_source->blob,
data);
} else {
- ret = do_git_config_sequence(&the_reader, opts, fn, data);
-+ struct repository *repo = config_source && config_source->repo ?
-+ config_source->repo : the_repository;
+ ret = do_git_config_sequence(&the_reader, opts, repo, fn, data);
}
if (inc.remote_urls) {
-@@ config.c: static void repo_read_config(struct repository *repo)
- {
- struct config_options opts = { 0 };
- struct configset_add_data data = CONFIGSET_ADD_INIT;
-+ struct git_config_source config_source = { 0 };
-
- opts.respect_includes = 1;
- opts.commondir = repo->commondir;
- opts.git_dir = repo->gitdir;
-
-+ config_source.repo = repo;
-+
- if (!repo->config)
- CALLOC_ARRAY(repo->config, 1);
- else
-@@ config.c: static void repo_read_config(struct repository *repo)
- data.config_set = repo->config;
- data.config_reader = &the_reader;
-
-- if (config_with_options(config_set_callback, &data, NULL, &opts) < 0)
-+ if (config_with_options(config_set_callback, &data, &config_source, &opts) < 0)
- /*
- * config_with_options() normally returns only
- * zero, as most errors are fatal, and
@@ config.c: int repo_config_set_worktree_gently(struct repository *r,
const char *key, const char *value)
{
@@ setup.c: void check_repository_format(struct repository_format *fmt)
clear_repository_format(&repo_fmt);
## t/t3007-ls-files-recurse-submodules.sh ##
-@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodules parses submodule repo config' '
- test_expect_success '--recurse-submodules parses submodule worktree config' '
- test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
- test_when_finished "git -C submodule config --worktree --unset feature.experimental" &&
-- test_when_finished "git config --unset extensions.worktreeConfig" &&
+@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodules parses submodule worktree config' '
+ test_config -C submodule extensions.worktreeConfig true &&
+ test_config -C submodule --worktree index.sparse "invalid non-boolean value" &&
- git -C submodule config extensions.worktreeConfig true &&
- git -C submodule config --worktree feature.experimental "invalid non-boolean value" &&
-
-- # NEEDSWORK: the extensions.worktreeConfig is set globally based on super
-- # project, so we need to enable it in the super project.
-- git config extensions.worktreeConfig true &&
+- # NEEDSWORK: the extensions.worktreeConfig is set globally based on
+- # superproject, so we need to enable it in the superproject.
+- test_config extensions.worktreeConfig true &&
-
test_must_fail git ls-files --recurse-submodules 2>err &&
grep "bad boolean config value" err
'
+test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' '
-+ test_when_finished "git config --unset extensions.worktreeConfig" &&
-+
+ # Enable worktree config in both super project & submodule, set an
-+ # invalid config in the submodule worktree config, then disable worktree
-+ # config in the submodule. The invalid worktree config should not be
-+ # picked up.
-+ git config extensions.worktreeConfig true &&
-+ git -C submodule config extensions.worktreeConfig true &&
-+ git -C submodule config --worktree feature.experimental "invalid non-boolean value" &&
-+ git -C submodule config --unset extensions.worktreeConfig &&
++ # invalid config in the submodule worktree config
++ test_config extensions.worktreeConfig true &&
++ test_config -C submodule extensions.worktreeConfig true &&
++ test_config -C submodule --worktree index.sparse "invalid non-boolean value" &&
++
++ # Now, disable the worktree config in the submodule. Note that we need
++ # to manually re-enable extensions.worktreeConfig when the test is
++ # finished, otherwise the test_unconfig of index.sparse will not work.
++ test_unconfig -C submodule extensions.worktreeConfig &&
++ test_when_finished "git -C submodule config extensions.worktreeConfig true" &&
+
++ # With extensions.worktreeConfig disabled in the submodule, the invalid
++ # worktree config is not picked up.
+ git ls-files --recurse-submodules 2>err &&
+ ! grep "bad boolean config value" err
+'
--
gitgitgadget
next prev parent reply other threads:[~2023-05-26 1:33 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-23 23:17 [PATCH 0/2] Fix behavior of worktree config in submodules Victoria Dye via GitGitGadget
2023-05-23 23:17 ` [PATCH 1/2] config: use gitdir to get worktree config Victoria Dye via GitGitGadget
2023-05-25 1:05 ` Glen Choo
2023-05-25 20:05 ` Derrick Stolee
2023-05-23 23:17 ` [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
2023-05-25 1:29 ` Glen Choo
2023-05-25 16:09 ` Glen Choo
2023-05-25 20:02 ` Victoria Dye
2023-05-25 20:13 ` Derrick Stolee
2023-05-24 10:25 ` [PATCH 0/2] Fix behavior of worktree config in submodules Junio C Hamano
2023-05-25 19:56 ` Glen Choo
2023-05-26 1:32 ` Victoria Dye via GitGitGadget [this message]
2023-05-26 1:32 ` [PATCH v2 1/3] config: use gitdir to get worktree config Victoria Dye via GitGitGadget
2023-05-26 1:32 ` [PATCH v2 2/3] config: pass 'repo' directly to 'config_with_options()' Victoria Dye via GitGitGadget
2023-05-26 1:33 ` [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
2023-05-31 22:17 ` Glen Choo
2023-06-01 4:43 ` Junio C Hamano
2023-06-12 21:37 ` Glen Choo
2023-06-07 22:29 ` Victoria Dye
2023-06-12 18:10 ` Glen Choo
2023-06-12 19:45 ` Victoria Dye
2023-06-12 20:23 ` Glen Choo
2023-06-12 23:04 ` [PATCH] setup: copy repository_format using helper Glen Choo
2023-06-13 0:03 ` Victoria Dye
2023-06-13 18:25 ` Glen Choo
2023-06-13 19:45 ` Junio C Hamano
2023-05-26 15:48 ` [PATCH v2 0/3] Fix behavior of worktree config in submodules Derrick Stolee
2023-06-13 22:09 ` Glen Choo
2023-06-13 22:17 ` Victoria Dye
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1536.v2.git.1685064781.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=chooglen@google.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).