* [PATCH 0/2] Fix behavior of worktree config in submodules
@ 2023-05-23 23:17 Victoria Dye via GitGitGadget
2023-05-23 23:17 ` [PATCH 1/2] config: use gitdir to get worktree config Victoria Dye via GitGitGadget
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-23 23:17 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, Victoria Dye
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].
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 (2):
config: use gitdir to get worktree config
repository: move 'repository_format_worktree_config' to repo scope
builtin/config.c | 3 +-
builtin/worktree.c | 2 +-
config.c | 42 ++++++++++++++++++--------
environment.c | 1 -
environment.h | 1 -
repository.c | 1 +
repository.h | 1 +
setup.c | 10 ++++--
t/t3007-ls-files-recurse-submodules.sh | 34 +++++++++++++++++++++
worktree.c | 4 +--
10 files changed, 78 insertions(+), 21 deletions(-)
base-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1536%2Fvdye%2Fvdye%2Fsubmodule-worktree-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1536/vdye/vdye/submodule-worktree-config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1536
--
gitgitgadget
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] config: use gitdir to get worktree config
2023-05-23 23:17 [PATCH 0/2] Fix behavior of worktree config in submodules Victoria Dye via GitGitGadget
@ 2023-05-23 23:17 ` Victoria Dye via GitGitGadget
2023-05-25 1:05 ` Glen Choo
2023-05-23 23:17 ` [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-23 23:17 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
Update 'do_git_config_sequence()' to read the worktree config from
'config.worktree' in 'opts->git_dir' rather than the gitdir of
'the_repository'.
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.
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.
Signed-off-by: Victoria Dye <vdye@github.com>
---
config.c | 28 +++++++++++++++++---------
t/t3007-ls-files-recurse-submodules.sh | 23 +++++++++++++++++++++
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/config.c b/config.c
index b79baf83e35..a93f7bfa3aa 100644
--- a/config.c
+++ b/config.c
@@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,
char *xdg_config = NULL;
char *user_config = NULL;
char *repo_config;
+ char *worktree_config;
enum config_scope prev_parsing_scope = reader->parsing_scope;
- if (opts->commondir)
+ /*
+ * Ensure that either:
+ * - the git_dir and commondir are both set, or
+ * - the git_dir and commondir are both NULL
+ */
+ if (!opts->git_dir != !opts->commondir)
+ BUG("only one of commondir and git_dir is non-NULL");
+
+ if (opts->commondir) {
repo_config = mkpathdup("%s/config", opts->commondir);
- else if (opts->git_dir)
- BUG("git_dir without commondir");
- else
+ worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
+ } else {
repo_config = NULL;
+ worktree_config = NULL;
+ }
config_reader_set_scope(reader, CONFIG_SCOPE_SYSTEM);
if (git_config_system() && system_config &&
@@ -2230,11 +2240,10 @@ static int do_git_config_sequence(struct config_reader *reader,
ret += git_config_from_file(fn, repo_config, data);
config_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);
- if (!opts->ignore_worktree && repository_format_worktree_config) {
- char *path = git_pathdup("config.worktree");
- if (!access_or_die(path, R_OK, 0))
- ret += git_config_from_file(fn, path, data);
- free(path);
+ if (!opts->ignore_worktree && worktree_config &&
+ repository_format_worktree_config &&
+ !access_or_die(worktree_config, R_OK, 0)) {
+ ret += git_config_from_file(fn, worktree_config, data);
}
config_reader_set_scope(reader, CONFIG_SCOPE_COMMAND);
@@ -2246,6 +2255,7 @@ static int do_git_config_sequence(struct config_reader *reader,
free(xdg_config);
free(user_config);
free(repo_config);
+ free(worktree_config);
return ret;
}
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index dd7770e85de..e35c203241f 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -299,6 +299,29 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_i18ngrep "does not support --error-unmatch" actual
'
+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_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" &&
+
+ 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 &&
+
+ test_must_fail git ls-files --recurse-submodules 2>err &&
+ grep "bad boolean config value" err
+'
+
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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope
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-23 23:17 ` Victoria Dye via GitGitGadget
2023-05-25 1:29 ` Glen Choo
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
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-23 23:17 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
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'
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'.
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
'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
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.
Signed-off-by: Victoria Dye <vdye@github.com>
---
builtin/config.c | 3 ++-
builtin/worktree.c | 2 +-
config.c | 16 +++++++++++-----
environment.c | 1 -
environment.h | 1 -
repository.c | 1 +
repository.h | 1 +
setup.c | 10 ++++++++--
t/t3007-ls-files-recurse-submodules.sh | 21 ++++++++++++++++-----
worktree.c | 4 ++--
10 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index ff2fe8ef125..5e352117c7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -5,6 +5,7 @@
#include "color.h"
#include "editor.h"
#include "environment.h"
+#include "repository.h"
#include "gettext.h"
#include "ident.h"
#include "parse-options.h"
@@ -713,7 +714,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
given_config_source.scope = CONFIG_SCOPE_LOCAL;
} else if (use_worktree_config) {
struct worktree **worktrees = get_worktrees();
- if (repository_format_worktree_config)
+ if (the_repository->repository_format_worktree_config)
given_config_source.file = git_pathdup("config.worktree");
else if (worktrees[0] && worktrees[1])
die(_("--worktree cannot be used with multiple "
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f3180463be2..60e389aaedb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -483,7 +483,7 @@ static int add_worktree(const char *path, const char *refname,
* values from the current worktree into the new one, that way the
* new worktree behaves the same as this one.
*/
- if (repository_format_worktree_config)
+ if (the_repository->repository_format_worktree_config)
copy_filtered_worktree_config(sb_repo.buf);
strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
diff --git a/config.c b/config.c
index a93f7bfa3aa..9ce2ffff5e1 100644
--- a/config.c
+++ b/config.c
@@ -2193,6 +2193,7 @@ int git_config_system(void)
static int do_git_config_sequence(struct config_reader *reader,
const struct config_options *opts,
+ const struct repository *repo,
config_fn_t fn, void *data)
{
int ret = 0;
@@ -2241,7 +2242,7 @@ static int do_git_config_sequence(struct config_reader *reader,
config_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);
if (!opts->ignore_worktree && worktree_config &&
- repository_format_worktree_config &&
+ repo && repo->repository_format_worktree_config &&
!access_or_die(worktree_config, R_OK, 0)) {
ret += git_config_from_file(fn, worktree_config, data);
}
@@ -2277,7 +2278,7 @@ 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);
/*
@@ -2294,7 +2295,9 @@ 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) {
@@ -2667,11 +2670,14 @@ 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
@@ -2681,7 +2687,7 @@ 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
@@ -3337,7 +3343,7 @@ int repo_config_set_worktree_gently(struct repository *r,
const char *key, const char *value)
{
/* Only use worktree-specific config if it is already enabled. */
- if (repository_format_worktree_config) {
+ if (r->repository_format_worktree_config) {
char *file = repo_git_path(r, "config.worktree");
int ret = git_config_set_multivar_in_file_gently(
file, key, value, NULL, 0);
diff --git a/environment.c b/environment.c
index 28d18eaca8e..6bd001efbde 100644
--- a/environment.c
+++ b/environment.c
@@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */
int warn_ambiguous_refs = 1;
int warn_on_object_refname_ambiguity = 1;
int repository_format_precious_objects;
-int repository_format_worktree_config;
const char *git_commit_encoding;
const char *git_log_output_encoding;
char *apply_default_whitespace;
diff --git a/environment.h b/environment.h
index 30cb7e0fa34..e6668079269 100644
--- a/environment.h
+++ b/environment.h
@@ -197,7 +197,6 @@ extern char *notes_ref_name;
extern int grafts_replace_parents;
extern int repository_format_precious_objects;
-extern int repository_format_worktree_config;
/*
* Create a temporary file rooted in the object database directory, or
diff --git a/repository.c b/repository.c
index c53e480e326..104960f8f59 100644
--- a/repository.c
+++ b/repository.c
@@ -182,6 +182,7 @@ int repo_init(struct repository *repo,
goto error;
repo_set_hash_algo(repo, format.hash_algo);
+ repo->repository_format_worktree_config = format.worktree_config;
/* take ownership of format.partial_clone */
repo->repository_format_partial_clone = format.partial_clone;
diff --git a/repository.h b/repository.h
index 1a13ff28677..74ae26635a4 100644
--- a/repository.h
+++ b/repository.h
@@ -163,6 +163,7 @@ struct repository {
struct promisor_remote_config *promisor_remote_config;
/* Configurations */
+ int repository_format_worktree_config;
/* Indicate if a repository has a different 'commondir' from 'gitdir' */
unsigned different_commondir:1;
diff --git a/setup.c b/setup.c
index 458582207ea..d8663954350 100644
--- a/setup.c
+++ b/setup.c
@@ -650,11 +650,10 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
}
repository_format_precious_objects = candidate->precious_objects;
- repository_format_worktree_config = candidate->worktree_config;
string_list_clear(&candidate->unknown_extensions, 0);
string_list_clear(&candidate->v1_only_extensions, 0);
- if (repository_format_worktree_config) {
+ if (candidate->worktree_config) {
/*
* pick up core.bare and core.worktree from per-worktree
* config if present
@@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,
return -1;
}
+ the_repository->repository_format_worktree_config =
+ candidate.worktree_config;
+
/* take ownership of candidate.partial_clone */
the_repository->repository_format_partial_clone =
candidate.partial_clone;
@@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
}
if (startup_info->have_repository) {
repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
+ the_repository->repository_format_worktree_config =
+ repo_fmt.worktree_config;
/* take ownership of repo_fmt.partial_clone */
the_repository->repository_format_partial_clone =
repo_fmt.partial_clone;
@@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt)
check_repository_format_gently(get_git_dir(), fmt, NULL);
startup_info->have_repository = 1;
repo_set_hash_algo(the_repository, fmt->hash_algo);
+ the_repository->repository_format_worktree_config =
+ fmt->worktree_config;
the_repository->repository_format_partial_clone =
xstrdup_or_null(fmt->partial_clone);
clear_repository_format(&repo_fmt);
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index e35c203241f..6d0bacef4de 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -309,19 +309,30 @@ 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" &&
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 &&
-
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 &&
+
+ git ls-files --recurse-submodules 2>err &&
+ ! grep "bad boolean config value" err
+'
+
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 &&
diff --git a/worktree.c b/worktree.c
index b5ee71c5ebd..c448fecd4b3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -806,7 +806,7 @@ int init_worktree_config(struct repository *r)
* If the extension is already enabled, then we can skip the
* upgrade process.
*/
- if (repository_format_worktree_config)
+ if (r->repository_format_worktree_config)
return 0;
if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
return error(_("failed to set extensions.worktreeConfig setting"));
@@ -846,7 +846,7 @@ int init_worktree_config(struct repository *r)
* Ensure that we use worktree config for the remaining lifetime
* of the current process.
*/
- repository_format_worktree_config = 1;
+ r->repository_format_worktree_config = 1;
cleanup:
git_configset_clear(&cs);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] Fix behavior of worktree config in submodules
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-23 23:17 ` [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
@ 2023-05-24 10:25 ` Junio C Hamano
2023-05-25 19:56 ` Glen Choo
2023-05-26 1:32 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
4 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2023-05-24 10:25 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, derrickstolee, chooglen, Victoria Dye
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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').
OK. So in short, worktreeConfig used to be a singleton, global for
the entire git process, but because "git grep" (and possibly others)
wants to operate across repository boundary when recursing into
submodules, the repository_format_worktree_config needs to be per
repository instance, not a global singleton.
Makes sense.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] config: use gitdir to get worktree config
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
0 siblings, 1 reply; 16+ messages in thread
From: Glen Choo @ 2023-05-25 1:05 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget, git; +Cc: derrickstolee, Victoria Dye
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> Update 'do_git_config_sequence()' to read the worktree config from
> 'config.worktree' in 'opts->git_dir' rather than the gitdir of
> 'the_repository'.
Thanks for the patches! This makes sense. do_git_config_sequence() is
eventually called by repo_config(), which is supposed to read config
into a "struct repository", so any reliance on the_repository's settings
is wrong.
> Note that behavior still isn't ideal
> because 'extensions.worktreeConfig' in the super project[...]
Nit: We typically use "superproject" without the space.
> diff --git a/config.c b/config.c
> index b79baf83e35..a93f7bfa3aa 100644
> --- a/config.c
> +++ b/config.c
> @@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,
> char *xdg_config = NULL;
> char *user_config = NULL;
> char *repo_config;
> + char *worktree_config;
> enum config_scope prev_parsing_scope = reader->parsing_scope;
>
> - if (opts->commondir)
> + /*
> + * Ensure that either:
> + * - the git_dir and commondir are both set, or
> + * - the git_dir and commondir are both NULL
> + */
> + if (!opts->git_dir != !opts->commondir)
> + BUG("only one of commondir and git_dir is non-NULL");
> +
> + if (opts->commondir) {
> repo_config = mkpathdup("%s/config", opts->commondir);
> - else if (opts->git_dir)
> - BUG("git_dir without commondir");
> - else
> + worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
> + } else {
> repo_config = NULL;
> + worktree_config = NULL;
> + }
Makes sense to me. I don't see why we would ever want to set one without
the other.
I looked into whether we could get replace opts->commondir and
opts->git_dir with a "struct repository" arg, but unfortunately
read_early_config() needs to pass these values without touching
"the_repository".
> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> index dd7770e85de..e35c203241f 100755
> --- a/t/t3007-ls-files-recurse-submodules.sh
> +++ b/t/t3007-ls-files-recurse-submodules.sh
> @@ -299,6 +299,29 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
> test_i18ngrep "does not support --error-unmatch" actual
> '
>
> +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_must_fail git ls-files --recurse-submodules 2>err &&
> + grep "bad boolean config value" err
> +'
This test has a few bits that are important but non-obvious. It would be
useful to capture them in either the commit message or a comment.
Firstly, we can't test this using "git config" because that only uses
the_repository, and we specifically need to read config in-core into a
"struct repository" that is a submodule, so we need a command that
recurses into a submodule without using subprocesses. IIRC the only
choices are "git grep" and "git ls-files".
Secondly, when we test that config is read from the submodule the choice
of "feature.experimental" is quite important. The config is read quite
indirectly: "git ls-files" reads from the submodule's index, which
will call prepare_repo_settings() on the submodule, and eventually calls
repo_config_get_bool() on "feature.experimental". Any of the configs in
prepare_repo_settings() should do, though. A tiny suggestion would be to
use "index.sparse" instead of "feature.experimental", since (I presume)
we'll have to add sparse index + submodule tests for "git ls-files"
eventually.
> +test_expect_success '--recurse-submodules parses submodule worktree config' '
> + test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
I believe "test_config -C" will achieve the desired effect.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope
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:13 ` Derrick Stolee
1 sibling, 1 reply; 16+ messages in thread
From: Glen Choo @ 2023-05-25 1:29 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget, git; +Cc: derrickstolee, Victoria Dye
Here's a quick response on the config.c bits, I haven't looked through
the global-removing parts closely yet. Rearranging the hunks for
clarity...
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -2667,11 +2670,14 @@ 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
> @@ -2681,7 +2687,7 @@ 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
I think it would be better to pass a "struct repository" arg to
config_with_options() instead of mocking a config_source to hold a .repo
member. config_with_options() does double duty - it either discovers and
reads the configs for the repo (system, global, worktree, etc), or it
reads the config from just config_source. From this perspective, it
doesn't make sense that the caller can pass config_source but
config_with_options() will still discover and read all configs, and I
think the only reason why this behavior is supported at all is that
builtin/config.c sometimes "reads all config" and sometimes "reads from
a single file", but sloppily passes a non-NULL "config_source" arg
unconditionally.
> diff --git a/config.c b/config.c
> index a93f7bfa3aa..9ce2ffff5e1 100644
> --- a/config.c
> +++ b/config.c
> @@ -2277,7 +2278,7 @@ 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);
>
> /*
The aforemented change would also let us get rid of this, which might
not always be correct. I think there might be cases where the scope is
actually unknown, but I'm not sure if we have any of those situations
in-tree.
> diff --git a/environment.c b/environment.c
> index 28d18eaca8e..6bd001efbde 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */
> int warn_ambiguous_refs = 1;
> int warn_on_object_refname_ambiguity = 1;
> int repository_format_precious_objects;
> -int repository_format_worktree_config;
> const char *git_commit_encoding;
> const char *git_log_output_encoding;
> char *apply_default_whitespace;
As an aside, I'm really happy to lose another global :)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope
2023-05-25 1:29 ` Glen Choo
@ 2023-05-25 16:09 ` Glen Choo
2023-05-25 20:02 ` Victoria Dye
0 siblings, 1 reply; 16+ messages in thread
From: Glen Choo @ 2023-05-25 16:09 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget, git; +Cc: derrickstolee, Victoria Dye
Glen Choo <chooglen@google.com> writes:
> I think it would be better to pass a "struct repository" arg to
> config_with_options() instead of mocking a config_source to hold a .repo
> member.
The flipside is that this would be redundant with an existing use of
git_config_source.repo, so for consistency, we should probably remove
git_config_source.repo. There's only one user of git_config_source.repo
- reading .gitmodules from a blob. It probably made sense to add .repo
the time, but now that we have a second, different use of "struct
repository", accepting an arg is probably better.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] Fix behavior of worktree config in submodules
2023-05-23 23:17 [PATCH 0/2] Fix behavior of worktree config in submodules Victoria Dye via GitGitGadget
` (2 preceding siblings ...)
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 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
4 siblings, 0 replies; 16+ messages in thread
From: Glen Choo @ 2023-05-25 19:56 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget, git; +Cc: derrickstolee, Victoria Dye
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
Thanks sending these patches! This is an obvious bug, and squashing
another global is always helpful.
> I'm
> also hoping this will help (or at least not hurt) the work to avoid use of
> global state in config parsing [2].
Both pieces of work touch different bits I think - yours plumbs and
reads a "struct repository" before the config iterating begins, mine
plumbs config source information after the config interating - so
they don't help each other, but at least the conflicts are only textual.
Junio: There are quite a few conflicts. AFAICT each only has to be
resolved once (IOW the number of conflicts to be fixed is the same with
"git merge" vs "git rebase"). I have a v2 that I'm about to send, and if
you'd prefer (especially since you're on half-vacation), I can base it
off Victoria's topic, which looks pretty straightforward and will
probably get merged pretty quickly.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope
2023-05-25 16:09 ` Glen Choo
@ 2023-05-25 20:02 ` Victoria Dye
0 siblings, 0 replies; 16+ messages in thread
From: Victoria Dye @ 2023-05-25 20:02 UTC (permalink / raw)
To: Glen Choo, Victoria Dye via GitGitGadget, git; +Cc: derrickstolee
Glen Choo wrote:
> Glen Choo <chooglen@google.com> writes:
>
>> I think it would be better to pass a "struct repository" arg to
>> config_with_options() instead of mocking a config_source to hold a .repo
>> member.
>
> The flipside is that this would be redundant with an existing use of
> git_config_source.repo, so for consistency, we should probably remove
> git_config_source.repo. There's only one user of git_config_source.repo
> - reading .gitmodules from a blob. It probably made sense to add .repo
> the time, but now that we have a second, different use of "struct
> repository", accepting an arg is probably better.
Agreed, I'd much prefer having a single 'struct repository' instance used in
the context of config parsing (if they ever had different values, it
probably wouldn't be immediately obvious for debugging). This and all of
your other recommendations seem reasonable to me, I'll re-roll shortly.
Thanks for the review!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] config: use gitdir to get worktree config
2023-05-25 1:05 ` Glen Choo
@ 2023-05-25 20:05 ` Derrick Stolee
0 siblings, 0 replies; 16+ messages in thread
From: Derrick Stolee @ 2023-05-25 20:05 UTC (permalink / raw)
To: Glen Choo, Victoria Dye via GitGitGadget, git; +Cc: Victoria Dye
On 5/24/2023 9:05 PM, Glen Choo wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Victoria Dye <vdye@github.com>
>>
>> Update 'do_git_config_sequence()' to read the worktree config from
>> 'config.worktree' in 'opts->git_dir' rather than the gitdir of
>> 'the_repository'.
>
> Thanks for the patches! This makes sense. do_git_config_sequence() is
> eventually called by repo_config(), which is supposed to read config
> into a "struct repository", so any reliance on the_repository's settings
> is wrong.
>> +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_must_fail git ls-files --recurse-submodules 2>err &&
>> + grep "bad boolean config value" err
>> +'
>
> This test has a few bits that are important but non-obvious. It would be
> useful to capture them in either the commit message or a comment.
>
> Firstly, we can't test this using "git config" because that only uses
> the_repository, and we specifically need to read config in-core into a
> "struct repository" that is a submodule, so we need a command that
> recurses into a submodule without using subprocesses. IIRC the only
> choices are "git grep" and "git ls-files".
>
> Secondly, when we test that config is read from the submodule the choice
> of "feature.experimental" is quite important. The config is read quite
> indirectly: "git ls-files" reads from the submodule's index, which
> will call prepare_repo_settings() on the submodule, and eventually calls
> repo_config_get_bool() on "feature.experimental". Any of the configs in
> prepare_repo_settings() should do, though. A tiny suggestion would be to
> use "index.sparse" instead of "feature.experimental", since (I presume)
> we'll have to add sparse index + submodule tests for "git ls-files"
> eventually.
Some of the points you bring up are definitely subtle, like the choice
of config variable.
I appreciate that there are two tests here: one to verify the test
checks have a similar effect without using the worktree config, and
then a second test to show the same behavior with worktree config.
If I understand correctly, the first test would pass without this
code change, but it is a helpful one to help add confidence in the
second test.
> +test_expect_success '--recurse-submodules parses submodule worktree config' '
>> + test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
>
> I believe "test_config -C" will achieve the desired effect.
This should work, though it requires acting a bit strangely, at least
if we want to replace the 'git config --worktree' command.
test_config treats the positions of the arguments as special, so we
would need to write it as:
test_config -C submodule feature.experimental --worktree "non boolean value"
and that's assuming that 'git -C submodule config feature.experimental
--worktree "non boolean value"' is parsed correctly to use the --worktree
argument. (I haven't tried it.) By using this order, that allows the
test_config helper to run the appropriate 'test_when_finished git config
--unset feature.experimental' command.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope
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 20:13 ` Derrick Stolee
1 sibling, 0 replies; 16+ messages in thread
From: Derrick Stolee @ 2023-05-25 20:13 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget, git; +Cc: chooglen, Victoria Dye
On 5/23/2023 7:17 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> 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'
> 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'.
>
> 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
> '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
>
> 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.
> @@ -2277,7 +2278,7 @@ 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);
This extra condition on config_source->scope surprised me. Could you
elaborate on the reason this is necessary?
> @@ -2667,11 +2670,14 @@ 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 };
This could be...
struct git_config_source config_source = { .repo = repo };
>
> opts.respect_includes = 1;
> opts.commondir = repo->commondir;
> opts.git_dir = repo->gitdir;
>
> + config_source.repo = repo;
> +
...avoiding these lines.
> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> index e35c203241f..6d0bacef4de 100755
> --- a/t/t3007-ls-files-recurse-submodules.sh
> +++ b/t/t3007-ls-files-recurse-submodules.sh
> @@ -309,19 +309,30 @@ 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" &&
>
> 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 &&
> -
> test_must_fail git ls-files --recurse-submodules 2>err &&
> grep "bad boolean config value" err
> '
These are my favorite kind of test updates: deleting extra setup that's no
longer needed.
> +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 &&
> +
> + git ls-files --recurse-submodules 2>err &&
> + ! grep "bad boolean config value" err
> +'
We have the same ways to improve here using 'test_config' as recommended
in patch 1.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/3] Fix behavior of worktree config in submodules
2023-05-23 23:17 [PATCH 0/2] Fix behavior of worktree config in submodules Victoria Dye via GitGitGadget
` (3 preceding siblings ...)
2023-05-25 19:56 ` Glen Choo
@ 2023-05-26 1:32 ` Victoria Dye via GitGitGadget
2023-05-26 1:32 ` [PATCH v2 1/3] config: use gitdir to get worktree config Victoria Dye via GitGitGadget
` (3 more replies)
4 siblings, 4 replies; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-26 1:32 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, gitster, Victoria Dye
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] config: use gitdir to get worktree config
2023-05-26 1:32 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
@ 2023-05-26 1:32 ` 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
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-26 1:32 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, gitster, Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
Update 'do_git_config_sequence()' to read the worktree config from
'config.worktree' in 'opts->git_dir' rather than the gitdir of
'the_repository'.
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 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 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>
---
config.c | 28 +++++++++++++++++---------
t/t3007-ls-files-recurse-submodules.sh | 18 +++++++++++++++++
t/test-lib-functions.sh | 13 ++++++++++--
3 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/config.c b/config.c
index b79baf83e35..a93f7bfa3aa 100644
--- a/config.c
+++ b/config.c
@@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,
char *xdg_config = NULL;
char *user_config = NULL;
char *repo_config;
+ char *worktree_config;
enum config_scope prev_parsing_scope = reader->parsing_scope;
- if (opts->commondir)
+ /*
+ * Ensure that either:
+ * - the git_dir and commondir are both set, or
+ * - the git_dir and commondir are both NULL
+ */
+ if (!opts->git_dir != !opts->commondir)
+ BUG("only one of commondir and git_dir is non-NULL");
+
+ if (opts->commondir) {
repo_config = mkpathdup("%s/config", opts->commondir);
- else if (opts->git_dir)
- BUG("git_dir without commondir");
- else
+ worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
+ } else {
repo_config = NULL;
+ worktree_config = NULL;
+ }
config_reader_set_scope(reader, CONFIG_SCOPE_SYSTEM);
if (git_config_system() && system_config &&
@@ -2230,11 +2240,10 @@ static int do_git_config_sequence(struct config_reader *reader,
ret += git_config_from_file(fn, repo_config, data);
config_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);
- if (!opts->ignore_worktree && repository_format_worktree_config) {
- char *path = git_pathdup("config.worktree");
- if (!access_or_die(path, R_OK, 0))
- ret += git_config_from_file(fn, path, data);
- free(path);
+ if (!opts->ignore_worktree && worktree_config &&
+ repository_format_worktree_config &&
+ !access_or_die(worktree_config, R_OK, 0)) {
+ ret += git_config_from_file(fn, worktree_config, data);
}
config_reader_set_scope(reader, CONFIG_SCOPE_COMMAND);
@@ -2246,6 +2255,7 @@ static int do_git_config_sequence(struct config_reader *reader,
free(xdg_config);
free(user_config);
free(repo_config);
+ free(worktree_config);
return ret;
}
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index dd7770e85de..a3e26751427 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -299,6 +299,24 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_i18ngrep "does not support --error-unmatch" actual
'
+test_expect_success '--recurse-submodules parses submodule repo config' '
+ 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_config -C submodule extensions.worktreeConfig true &&
+ test_config -C submodule --worktree index.sparse "invalid non-boolean value" &&
+
+ # 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_incompatible_with_recurse_submodules () {
test_expect_success "--recurse-submodules and $1 are incompatible" "
test_must_fail git ls-files --recurse-submodules $1 2>actual &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6e19ebc922a..b3864e22e9a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -542,8 +542,17 @@ 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 () {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] config: pass 'repo' directly to 'config_with_options()'
2023-05-26 1:32 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
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 ` 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-26 15:48 ` [PATCH v2 0/3] Fix behavior of worktree config in submodules Derrick Stolee
3 siblings, 0 replies; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-26 1:32 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, gitster, Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
Add a 'struct repository' argument to 'config_with_options()' and remove the
'repo' field from 'struct git_config_source'.
A 'struct repository' instance was originally added to the config source in
e3e8bf046e9 (submodule-config: pass repo upon blob config read, 2021-08-16)
to improve how submodule blob config content was accessed. At the time, this
was the only use for a 'repository' instance, so it was naturally added only
where it was needed: to 'struct git_config_source'. However, in upcoming
patches, 'config_with_options()' will need the repository instance to access
extension information (regardless of whether a 'config_source' exists). To
make the 'struct repository' instance more easily accessible, move it into
the function's arguments.
Update all callers of 'config_with_options()' to pass the appropriate 'repo'
value:
* in 'builtin/config.c', use 'the_repository'
* in 'submodule--config.c', use the 'repo' arg in 'config_from_gitmodules()'
* in 'read_[very_]early_config()' & 'read_protected_config()', set 'repo' to
NULL (repository instances aren't available there)
* in 'populate_remote_urls()', use the repo instance that has been added to
the 'struct config_include_data'
* in 'repo_read_config()', use the given 'repo' arg
Finally, note that this patch eliminates the fallback to 'the_repository'
that previously existed for the 'config_source' repo instance if it was
NULL. The fallback is no longer necessary, as the 'repo' is set explicitly
in all cases where it is needed.
Signed-off-by: Victoria Dye <vdye@github.com>
---
builtin/config.c | 14 +++++++++-----
config.c | 16 +++++++++-------
config.h | 4 ++--
submodule-config.c | 3 +--
4 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index ff2fe8ef125..8fc90288f9e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -375,7 +375,8 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)
}
config_with_options(collect_config, &values,
- &given_config_source, &config_options);
+ &given_config_source, the_repository,
+ &config_options);
if (!values.nr && default_value) {
struct strbuf *item;
@@ -486,7 +487,8 @@ static void get_color(const char *var, const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
config_with_options(git_get_color_config, NULL,
- &given_config_source, &config_options);
+ &given_config_source, the_repository,
+ &config_options);
if (!get_color_found && def_color) {
if (color_parse(def_color, parsed_color) < 0)
@@ -518,7 +520,8 @@ static int get_colorbool(const char *var, int print)
get_diff_color_found = -1;
get_color_ui_found = -1;
config_with_options(git_get_colorbool_config, NULL,
- &given_config_source, &config_options);
+ &given_config_source, the_repository,
+ &config_options);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -607,7 +610,8 @@ static int get_urlmatch(const char *var, const char *url)
}
config_with_options(urlmatch_config_entry, &config,
- &given_config_source, &config_options);
+ &given_config_source, the_repository,
+ &config_options);
ret = !values.nr;
@@ -827,7 +831,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
if (config_with_options(show_all_config, NULL,
- &given_config_source,
+ &given_config_source, the_repository,
&config_options) < 0) {
if (given_config_source.file)
die_errno(_("unable to read config file '%s'"),
diff --git a/config.c b/config.c
index a93f7bfa3aa..67e60e131c2 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,7 @@ struct config_include_data {
void *data;
const struct config_options *opts;
struct git_config_source *config_source;
+ struct repository *repo;
struct config_reader *config_reader;
/*
@@ -415,7 +416,8 @@ static void populate_remote_urls(struct config_include_data *inc)
inc->remote_urls = xmalloc(sizeof(*inc->remote_urls));
string_list_init_dup(inc->remote_urls);
- config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts);
+ config_with_options(add_remote_url, inc->remote_urls,
+ inc->config_source, inc->repo, &opts);
config_reader_set_scope(inc->config_reader, store_scope);
}
@@ -2261,6 +2263,7 @@ static int do_git_config_sequence(struct config_reader *reader,
int config_with_options(config_fn_t fn, void *data,
struct git_config_source *config_source,
+ struct repository *repo,
const struct config_options *opts)
{
struct config_include_data inc = CONFIG_INCLUDE_INIT;
@@ -2271,6 +2274,7 @@ int config_with_options(config_fn_t fn, void *data,
inc.fn = fn;
inc.data = data;
inc.opts = opts;
+ inc.repo = repo;
inc.config_source = config_source;
inc.config_reader = &the_reader;
fn = git_config_include;
@@ -2289,8 +2293,6 @@ int config_with_options(config_fn_t fn, void *data,
} else if (config_source && config_source->file) {
ret = git_config_from_file(fn, config_source->file, data);
} else if (config_source && config_source->blob) {
- struct repository *repo = config_source->repo ?
- config_source->repo : the_repository;
ret = git_config_from_blob_ref(fn, repo, config_source->blob,
data);
} else {
@@ -2353,7 +2355,7 @@ void read_early_config(config_fn_t cb, void *data)
opts.git_dir = gitdir.buf;
}
- config_with_options(cb, data, NULL, &opts);
+ config_with_options(cb, data, NULL, NULL, &opts);
strbuf_release(&commondir);
strbuf_release(&gitdir);
@@ -2373,7 +2375,7 @@ void read_very_early_config(config_fn_t cb, void *data)
opts.ignore_cmdline = 1;
opts.system_gently = 1;
- config_with_options(cb, data, NULL, &opts);
+ config_with_options(cb, data, NULL, NULL, &opts);
}
RESULT_MUST_BE_USED
@@ -2681,7 +2683,7 @@ 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, NULL, repo, &opts) < 0)
/*
* config_with_options() normally returns only
* zero, as most errors are fatal, and
@@ -2825,7 +2827,7 @@ static void read_protected_config(void)
git_configset_init(&protected_config);
data.config_set = &protected_config;
data.config_reader = &the_reader;
- config_with_options(config_set_callback, &data, NULL, &opts);
+ config_with_options(config_set_callback, &data, NULL, NULL, &opts);
}
void git_protected_config(config_fn_t fn, void *data)
diff --git a/config.h b/config.h
index 247b572b37b..d1c5577589e 100644
--- a/config.h
+++ b/config.h
@@ -3,6 +3,7 @@
#include "hashmap.h"
#include "string-list.h"
+#include "repository.h"
/**
@@ -49,8 +50,6 @@ const char *config_scope_name(enum config_scope scope);
struct git_config_source {
unsigned int use_stdin:1;
const char *file;
- /* The repository if blob is not NULL; leave blank for the_repository */
- struct repository *repo;
const char *blob;
enum config_scope scope;
};
@@ -196,6 +195,7 @@ void git_config(config_fn_t fn, void *);
*/
int config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
+ struct repository *repo,
const struct config_options *opts);
/**
diff --git a/submodule-config.c b/submodule-config.c
index 58dfbde9ae5..7eb7a0d88d2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -659,7 +659,6 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
config_source.file = file;
} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||
repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {
- config_source.repo = repo;
config_source.blob = oidstr = xstrdup(oid_to_hex(&oid));
if (repo != the_repository)
add_submodule_odb_by_path(repo->objects->odb->path);
@@ -667,7 +666,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
goto out;
}
- config_with_options(fn, data, &config_source, &opts);
+ config_with_options(fn, data, &config_source, repo, &opts);
out:
free(oidstr);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope
2023-05-26 1:32 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
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 ` Victoria Dye via GitGitGadget
2023-05-26 15:48 ` [PATCH v2 0/3] Fix behavior of worktree config in submodules Derrick Stolee
3 siblings, 0 replies; 16+ messages in thread
From: Victoria Dye via GitGitGadget @ 2023-05-26 1:33 UTC (permalink / raw)
To: git; +Cc: derrickstolee, chooglen, gitster, Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
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 it to the 'repository'
struct and updating 'setup.c' & 'repository.c' functions to assign the value
appropriately.
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, 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
superprojects and submodules.
Signed-off-by: Victoria Dye <vdye@github.com>
---
builtin/config.c | 3 ++-
builtin/worktree.c | 2 +-
config.c | 7 ++++---
environment.c | 1 -
environment.h | 1 -
repository.c | 1 +
repository.h | 1 +
setup.c | 10 ++++++++--
t/t3007-ls-files-recurse-submodules.sh | 23 +++++++++++++++++++----
worktree.c | 4 ++--
10 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8fc90288f9e..d40fddb042a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -5,6 +5,7 @@
#include "color.h"
#include "editor.h"
#include "environment.h"
+#include "repository.h"
#include "gettext.h"
#include "ident.h"
#include "parse-options.h"
@@ -717,7 +718,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
given_config_source.scope = CONFIG_SCOPE_LOCAL;
} else if (use_worktree_config) {
struct worktree **worktrees = get_worktrees();
- if (repository_format_worktree_config)
+ if (the_repository->repository_format_worktree_config)
given_config_source.file = git_pathdup("config.worktree");
else if (worktrees[0] && worktrees[1])
die(_("--worktree cannot be used with multiple "
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f3180463be2..60e389aaedb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -483,7 +483,7 @@ static int add_worktree(const char *path, const char *refname,
* values from the current worktree into the new one, that way the
* new worktree behaves the same as this one.
*/
- if (repository_format_worktree_config)
+ if (the_repository->repository_format_worktree_config)
copy_filtered_worktree_config(sb_repo.buf);
strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
diff --git a/config.c b/config.c
index 67e60e131c2..f5bdac0aeed 100644
--- a/config.c
+++ b/config.c
@@ -2195,6 +2195,7 @@ int git_config_system(void)
static int do_git_config_sequence(struct config_reader *reader,
const struct config_options *opts,
+ const struct repository *repo,
config_fn_t fn, void *data)
{
int ret = 0;
@@ -2243,7 +2244,7 @@ static int do_git_config_sequence(struct config_reader *reader,
config_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);
if (!opts->ignore_worktree && worktree_config &&
- repository_format_worktree_config &&
+ repo && repo->repository_format_worktree_config &&
!access_or_die(worktree_config, R_OK, 0)) {
ret += git_config_from_file(fn, worktree_config, data);
}
@@ -2296,7 +2297,7 @@ 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);
+ ret = do_git_config_sequence(&the_reader, opts, repo, fn, data);
}
if (inc.remote_urls) {
@@ -3339,7 +3340,7 @@ int repo_config_set_worktree_gently(struct repository *r,
const char *key, const char *value)
{
/* Only use worktree-specific config if it is already enabled. */
- if (repository_format_worktree_config) {
+ if (r->repository_format_worktree_config) {
char *file = repo_git_path(r, "config.worktree");
int ret = git_config_set_multivar_in_file_gently(
file, key, value, NULL, 0);
diff --git a/environment.c b/environment.c
index 28d18eaca8e..6bd001efbde 100644
--- a/environment.c
+++ b/environment.c
@@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */
int warn_ambiguous_refs = 1;
int warn_on_object_refname_ambiguity = 1;
int repository_format_precious_objects;
-int repository_format_worktree_config;
const char *git_commit_encoding;
const char *git_log_output_encoding;
char *apply_default_whitespace;
diff --git a/environment.h b/environment.h
index 30cb7e0fa34..e6668079269 100644
--- a/environment.h
+++ b/environment.h
@@ -197,7 +197,6 @@ extern char *notes_ref_name;
extern int grafts_replace_parents;
extern int repository_format_precious_objects;
-extern int repository_format_worktree_config;
/*
* Create a temporary file rooted in the object database directory, or
diff --git a/repository.c b/repository.c
index c53e480e326..104960f8f59 100644
--- a/repository.c
+++ b/repository.c
@@ -182,6 +182,7 @@ int repo_init(struct repository *repo,
goto error;
repo_set_hash_algo(repo, format.hash_algo);
+ repo->repository_format_worktree_config = format.worktree_config;
/* take ownership of format.partial_clone */
repo->repository_format_partial_clone = format.partial_clone;
diff --git a/repository.h b/repository.h
index 1a13ff28677..74ae26635a4 100644
--- a/repository.h
+++ b/repository.h
@@ -163,6 +163,7 @@ struct repository {
struct promisor_remote_config *promisor_remote_config;
/* Configurations */
+ int repository_format_worktree_config;
/* Indicate if a repository has a different 'commondir' from 'gitdir' */
unsigned different_commondir:1;
diff --git a/setup.c b/setup.c
index 458582207ea..d8663954350 100644
--- a/setup.c
+++ b/setup.c
@@ -650,11 +650,10 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
}
repository_format_precious_objects = candidate->precious_objects;
- repository_format_worktree_config = candidate->worktree_config;
string_list_clear(&candidate->unknown_extensions, 0);
string_list_clear(&candidate->v1_only_extensions, 0);
- if (repository_format_worktree_config) {
+ if (candidate->worktree_config) {
/*
* pick up core.bare and core.worktree from per-worktree
* config if present
@@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,
return -1;
}
+ the_repository->repository_format_worktree_config =
+ candidate.worktree_config;
+
/* take ownership of candidate.partial_clone */
the_repository->repository_format_partial_clone =
candidate.partial_clone;
@@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
}
if (startup_info->have_repository) {
repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
+ the_repository->repository_format_worktree_config =
+ repo_fmt.worktree_config;
/* take ownership of repo_fmt.partial_clone */
the_repository->repository_format_partial_clone =
repo_fmt.partial_clone;
@@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt)
check_repository_format_gently(get_git_dir(), fmt, NULL);
startup_info->have_repository = 1;
repo_set_hash_algo(the_repository, fmt->hash_algo);
+ the_repository->repository_format_worktree_config =
+ fmt->worktree_config;
the_repository->repository_format_partial_clone =
xstrdup_or_null(fmt->partial_clone);
clear_repository_format(&repo_fmt);
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index a3e26751427..7308a3d4e25 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -309,14 +309,29 @@ 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" &&
- # 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' '
+ # Enable worktree config in both super project & submodule, set an
+ # 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
+'
+
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 &&
diff --git a/worktree.c b/worktree.c
index b5ee71c5ebd..c448fecd4b3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -806,7 +806,7 @@ int init_worktree_config(struct repository *r)
* If the extension is already enabled, then we can skip the
* upgrade process.
*/
- if (repository_format_worktree_config)
+ if (r->repository_format_worktree_config)
return 0;
if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
return error(_("failed to set extensions.worktreeConfig setting"));
@@ -846,7 +846,7 @@ int init_worktree_config(struct repository *r)
* Ensure that we use worktree config for the remaining lifetime
* of the current process.
*/
- repository_format_worktree_config = 1;
+ r->repository_format_worktree_config = 1;
cleanup:
git_configset_clear(&cs);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Fix behavior of worktree config in submodules
2023-05-26 1:32 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
` (2 preceding siblings ...)
2023-05-26 1:33 ` [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope Victoria Dye via GitGitGadget
@ 2023-05-26 15:48 ` Derrick Stolee
3 siblings, 0 replies; 16+ messages in thread
From: Derrick Stolee @ 2023-05-26 15:48 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget, git; +Cc: chooglen, gitster, Victoria Dye
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-05-26 15:48 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH v2 0/3] " Victoria Dye via GitGitGadget
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-26 15:48 ` [PATCH v2 0/3] Fix behavior of worktree config in submodules Derrick Stolee
Code repositories for project(s) associated with this public inbox
https://80x24.org/pub/scm/git/git.git/
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).