| Message ID | pull.1536.v2.git.1685064781.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix behavior of worktree config in submodules | expand |
On 5/25/2023 9:32 PM, Victoria Dye via GitGitGadget wrote: > 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 for these updates. I'm happy with this version. Thanks, -Stolee
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > * 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! Discounting the discussions on the side thread (which we've decided are mostly out of scope) I think this version is good enough to merge as-is. In https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com I said that this series is better if we squash in a patch to drop the setup code from discover_git_directory(), but on hindsight, I think it also makes perfect sense for me to send that as a standalone patch. Let me know if you plan to squash it in or not so I'll know whether to send it :)
Glen Choo wrote: > "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> * 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! Discounting the discussions on the side thread (which we've > decided are mostly out of scope) I think this version is good enough to > merge as-is. > > In > > https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com > > I said that this series is better if we squash in a patch to drop the > setup code from discover_git_directory(), but on hindsight, I think it > also makes perfect sense for me to send that as a standalone patch. Let > me know if you plan to squash it in or not so I'll know whether to send > it :) Thanks for the re-review! This series was just merged to 'next', so I think sending the new patch separately would be the least disruptive option.
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 +'