{"thread":{"id":"59785","subject":"[PATCH 0/2] Fix behavior of worktree config in submodules","startedAt":"2023-05-23T23:17:59Z","lastAt":"2023-06-13T22:17:35Z","messageCount":29,"participants":["Victoria Dye via GitGitGadget","Junio C Hamano","Glen Choo","Victoria Dye","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"477678","messageId":"pull.1536.git.1684883872.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":null,"subject":"[PATCH 0/2] Fix behavior of worktree config in submodules","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-23T23:17:50Z","receivedAt":"2023-05-23T23:17:59Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"About a year ago, discussion on the sparse index integration of 'git grep'\nsurfaced larger incompatibilities between sparse-checkout and submodules\n[1]. This series fixes one of the underlying issues to that incompatibility,\nwhich is that the worktree config of the submodule (where\n'core.sparseCheckout', 'core.sparseCheckoutCone', and 'index.sparse' are\nset) is not used when operating on the submodule from its super project\n(e.g., in a command with '--recurse-submodules').\n\nThe outcome of this series is that 'extensions.worktreeConfig' and the\ncontents of the repository's worktree config are read and applied to (and\nonly to) the relevant repo when working in a super project/submodule setup.\nThis alone doesn't fix sparse-checkout/submodule interoperability; the\nadditional changes needed for that will be submitted in a later series. I'm\nalso hoping this will help (or at least not hurt) the work to avoid use of\nglobal state in config parsing [2].\n\nThanks!\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/093827ae-41ef-5f7c-7829-647536ce1305@github.com/\n[2]\nhttps://lore.kernel.org/git/pull.1497.git.git.1682104398.gitgitgadget@gmail.com/\n\nVictoria Dye (2):\n config: use gitdir to get worktree config\n repository: move 'repository_format_worktree_config' to repo scope\n\n builtin/config.c | 3 +-\n builtin/worktree.c | 2 +-\n config.c | 42 ++++++++++++++++++--------\n environment.c | 1 -\n environment.h | 1 -\n repository.c | 1 +\n repository.h | 1 +\n setup.c | 10 ++++--\n t/t3007-ls-files-recurse-submodules.sh | 34 +++++++++++++++++++++\n worktree.c | 4 +--\n 10 files changed, 78 insertions(+), 21 deletions(-)\n\n\nbase-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1536%2Fvdye%2Fvdye%2Fsubmodule-worktree-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1536/vdye/vdye/submodule-worktree-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1536\n-- \ngitgitgadget\n"},{"id":"477679","messageId":"aead2fe1ce162949fb313a92fe960e5a64512f60.1684883872.git.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":"pull.1536.git.1684883872.gitgitgadget@gmail.com","subject":"[PATCH 1/2] config: use gitdir to get worktree config","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-23T23:17:51Z","receivedAt":"2023-05-23T23:18:00Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate 'do_git_config_sequence()' to read the worktree config from\n'config.worktree' in 'opts->git_dir' rather than the gitdir of\n'the_repository'.\n\nThe worktree config is loaded from the path returned by\n'git_pathdup(\"config.worktree\")', the 'config.worktree' relative to the\ngitdir of 'the_repository'. If loading the config for a submodule, this path\nis incorrect, since 'the_repository' is the super project. Conversely,\n'opts->git_dir' is the gitdir of the submodule being configured, so the\nconfig file in that location should be read instead.\n\nTo ensure the use of 'opts->git_dir' is safe, require that 'opts->git_dir'\nis set if-and-only-if 'opts->commondir' is set (rather than \"only-if\" as it\nis now). In all current usage of 'config_options', these values are set\ntogether, so the stricter check does not change any behavior.\n\nFinally, add tests to 't3007-ls-files-recurse-submodules.sh' to demonstrate\nthe corrected config loading behavior. Note that behavior still isn't ideal\nbecause 'extensions.worktreeConfig' in the super project controls whether or\nnot the worktree config is used in the submodule. This will be fixed in a\nlater patch.\n\nSigned-off-by: Victoria Dye \n---\n config.c | 28 +++++++++++++++++---------\n t/t3007-ls-files-recurse-submodules.sh | 23 +++++++++++++++++++++\n 2 files changed, 42 insertions(+), 9 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b79baf83e35..a93f7bfa3aa 100644\n--- a/config.c\n+++ b/config.c\n@@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,\n \tchar *xdg_config = NULL;\n \tchar *user_config = NULL;\n \tchar *repo_config;\n+\tchar *worktree_config;\n \tenum config_scope prev_parsing_scope = reader->parsing_scope;\n \n-\tif (opts->commondir)\n+\t/*\n+\t * Ensure that either:\n+\t * - the git_dir and commondir are both set, or\n+\t * - the git_dir and commondir are both NULL\n+\t */\n+\tif (!opts->git_dir != !opts->commondir)\n+\t\tBUG(\"only one of commondir and git_dir is non-NULL\");\n+\n+\tif (opts->commondir) {\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n-\telse if (opts->git_dir)\n-\t\tBUG(\"git_dir without commondir\");\n-\telse\n+\t\tworktree_config = mkpathdup(\"%s/config.worktree\", opts->git_dir);\n+\t} else {\n \t\trepo_config = NULL;\n+\t\tworktree_config = NULL;\n+\t}\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_SYSTEM);\n \tif (git_config_system() && system_config &&\n@@ -2230,11 +2240,10 @@ static int do_git_config_sequence(struct config_reader *reader,\n \t\tret += git_config_from_file(fn, repo_config, data);\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);\n-\tif (!opts->ignore_worktree && repository_format_worktree_config) {\n-\t\tchar *path = git_pathdup(\"config.worktree\");\n-\t\tif (!access_or_die(path, R_OK, 0))\n-\t\t\tret += git_config_from_file(fn, path, data);\n-\t\tfree(path);\n+\tif (!opts->ignore_worktree && worktree_config &&\n+\t repository_format_worktree_config &&\n+\t !access_or_die(worktree_config, R_OK, 0)) {\n+\t\tret += git_config_from_file(fn, worktree_config, data);\n \t}\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_COMMAND);\n@@ -2246,6 +2255,7 @@ static int do_git_config_sequence(struct config_reader *reader,\n \tfree(xdg_config);\n \tfree(user_config);\n \tfree(repo_config);\n+\tfree(worktree_config);\n \treturn ret;\n }\n \ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex dd7770e85de..e35c203241f 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -299,6 +299,29 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '\n \ttest_i18ngrep \"does not support --error-unmatch\" actual\n '\n \n+test_expect_success '--recurse-submodules parses submodule repo config' '\n+\ttest_when_finished \"git -C submodule config --unset feature.experimental\" &&\n+\tgit -C submodule config feature.experimental \"invalid non-boolean value\" &&\n+\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n+\tgrep \"bad boolean config value\" err\n+'\n+\n+test_expect_success '--recurse-submodules parses submodule worktree config' '\n+\ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n+\ttest_when_finished \"git -C submodule config --worktree --unset feature.experimental\" &&\n+\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n+\n+\tgit -C submodule config extensions.worktreeConfig true &&\n+\tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n+\n+\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on super\n+\t# project, so we need to enable it in the super project.\n+\tgit config extensions.worktreeConfig true &&\n+\n+\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n+\tgrep \"bad boolean config value\" err\n+'\n+\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n-- \ngitgitgadget\n\n"},{"id":"477680","messageId":"5ed9100a7707a529b309005419244d083cdc85ba.1684883872.git.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":"pull.1536.git.1684883872.gitgitgadget@gmail.com","subject":"[PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-23T23:17:52Z","receivedAt":"2023-05-23T23:18:07Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nMove 'repository_format_worktree_config' out of the global scope and into\nthe 'repository' struct. This change is similar to how\n'repository_format_partial_clone' was moved in ebaf3bcf1ae (repository: move\nglobal r_f_p_c to repo struct, 2021-06-17), adding to the 'repository'\nstruct and updating 'setup.c' & 'repository.c' functions to assign the value\nappropriately. In addition, update usage of the setting to reference the\nrelevant context's repo or, as a fallback, 'the_repository'.\n\nThe primary goal of this change is to be able to load worktree config for a\nsubmodule depending on whether that submodule - not the super project - has\n'extensions.worktreeConfig' enabled. To ensure 'do_git_config_sequence()'\nhas access to the newly repo-scoped configuration:\n\n- update 'repo_read_config()' to create a 'config_source' to hold the\n repo instance\n- add a 'repo' argument to 'do_git_config_sequence()'\n- update 'config_with_options' to call 'do_git_config_sequence()' with\n 'config_source.repo', or 'the_repository' as a fallback\n\nFinally, add/update tests in 't3007-ls-files-recurse-submodules.sh' to\nverify 'extensions.worktreeConfig' is read an used independently by super\nprojects and submodules.\n\nSigned-off-by: Victoria Dye \n---\n builtin/config.c | 3 ++-\n builtin/worktree.c | 2 +-\n config.c | 16 +++++++++++-----\n environment.c | 1 -\n environment.h | 1 -\n repository.c | 1 +\n repository.h | 1 +\n setup.c | 10 ++++++++--\n t/t3007-ls-files-recurse-submodules.sh | 21 ++++++++++++++++-----\n worktree.c | 4 ++--\n 10 files changed, 42 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ff2fe8ef125..5e352117c7a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -5,6 +5,7 @@\n #include \"color.h\"\n #include \"editor.h\"\n #include \"environment.h\"\n+#include \"repository.h\"\n #include \"gettext.h\"\n #include \"ident.h\"\n #include \"parse-options.h\"\n@@ -713,7 +714,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.scope = CONFIG_SCOPE_LOCAL;\n \t} else if (use_worktree_config) {\n \t\tstruct worktree **worktrees = get_worktrees();\n-\t\tif (repository_format_worktree_config)\n+\t\tif (the_repository->repository_format_worktree_config)\n \t\t\tgiven_config_source.file = git_pathdup(\"config.worktree\");\n \t\telse if (worktrees[0] && worktrees[1])\n \t\t\tdie(_(\"--worktree cannot be used with multiple \"\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex f3180463be2..60e389aaedb 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -483,7 +483,7 @@ static int add_worktree(const char *path, const char *refname,\n \t * values from the current worktree into the new one, that way the\n \t * new worktree behaves the same as this one.\n \t */\n-\tif (repository_format_worktree_config)\n+\tif (the_repository->repository_format_worktree_config)\n \t\tcopy_filtered_worktree_config(sb_repo.buf);\n \n \tstrvec_pushf(&child_env, \"%s=%s\", GIT_DIR_ENVIRONMENT, sb_git.buf);\ndiff --git a/config.c b/config.c\nindex a93f7bfa3aa..9ce2ffff5e1 100644\n--- a/config.c\n+++ b/config.c\n@@ -2193,6 +2193,7 @@ int git_config_system(void)\n \n static int do_git_config_sequence(struct config_reader *reader,\n \t\t\t\t const struct config_options *opts,\n+\t\t\t\t const struct repository *repo,\n \t\t\t\t config_fn_t fn, void *data)\n {\n \tint ret = 0;\n@@ -2241,7 +2242,7 @@ static int do_git_config_sequence(struct config_reader *reader,\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);\n \tif (!opts->ignore_worktree && worktree_config &&\n-\t repository_format_worktree_config &&\n+\t repo && repo->repository_format_worktree_config &&\n \t !access_or_die(worktree_config, R_OK, 0)) {\n \t\tret += git_config_from_file(fn, worktree_config, data);\n \t}\n@@ -2277,7 +2278,7 @@ int config_with_options(config_fn_t fn, void *data,\n \t\tdata = &inc;\n \t}\n \n-\tif (config_source)\n+\tif (config_source && config_source->scope != CONFIG_SCOPE_UNKNOWN)\n \t\tconfig_reader_set_scope(&the_reader, config_source->scope);\n \n \t/*\n@@ -2294,7 +2295,9 @@ int config_with_options(config_fn_t fn, void *data,\n \t\tret = git_config_from_blob_ref(fn, repo, config_source->blob,\n \t\t\t\t\t\tdata);\n \t} else {\n-\t\tret = do_git_config_sequence(&the_reader, opts, fn, data);\n+\t\tstruct repository *repo = config_source && config_source->repo ?\n+\t\t\tconfig_source->repo : the_repository;\n+\t\tret = do_git_config_sequence(&the_reader, opts, repo, fn, data);\n \t}\n \n \tif (inc.remote_urls) {\n@@ -2667,11 +2670,14 @@ static void repo_read_config(struct repository *repo)\n {\n \tstruct config_options opts = { 0 };\n \tstruct configset_add_data data = CONFIGSET_ADD_INIT;\n+\tstruct git_config_source config_source = { 0 };\n \n \topts.respect_includes = 1;\n \topts.commondir = repo->commondir;\n \topts.git_dir = repo->gitdir;\n \n+\tconfig_source.repo = repo;\n+\n \tif (!repo->config)\n \t\tCALLOC_ARRAY(repo->config, 1);\n \telse\n@@ -2681,7 +2687,7 @@ static void repo_read_config(struct repository *repo)\n \tdata.config_set = repo->config;\n \tdata.config_reader = &the_reader;\n \n-\tif (config_with_options(config_set_callback, &data, NULL, &opts) < 0)\n+\tif (config_with_options(config_set_callback, &data, &config_source, &opts) < 0)\n \t\t/*\n \t\t * config_with_options() normally returns only\n \t\t * zero, as most errors are fatal, and\n@@ -3337,7 +3343,7 @@ int repo_config_set_worktree_gently(struct repository *r,\n \t\t\t\t const char *key, const char *value)\n {\n \t/* Only use worktree-specific config if it is already enabled. */\n-\tif (repository_format_worktree_config) {\n+\tif (r->repository_format_worktree_config) {\n \t\tchar *file = repo_git_path(r, \"config.worktree\");\n \t\tint ret = git_config_set_multivar_in_file_gently(\n \t\t\t\t\tfile, key, value, NULL, 0);\ndiff --git a/environment.c b/environment.c\nindex 28d18eaca8e..6bd001efbde 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int repository_format_precious_objects;\n-int repository_format_worktree_config;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n char *apply_default_whitespace;\ndiff --git a/environment.h b/environment.h\nindex 30cb7e0fa34..e6668079269 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -197,7 +197,6 @@ extern char *notes_ref_name;\n extern int grafts_replace_parents;\n \n extern int repository_format_precious_objects;\n-extern int repository_format_worktree_config;\n \n /*\n * Create a temporary file rooted in the object database directory, or\ndiff --git a/repository.c b/repository.c\nindex c53e480e326..104960f8f59 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -182,6 +182,7 @@ int repo_init(struct repository *repo,\n \t\tgoto error;\n \n \trepo_set_hash_algo(repo, format.hash_algo);\n+\trepo->repository_format_worktree_config = format.worktree_config;\n \n \t/* take ownership of format.partial_clone */\n \trepo->repository_format_partial_clone = format.partial_clone;\ndiff --git a/repository.h b/repository.h\nindex 1a13ff28677..74ae26635a4 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -163,6 +163,7 @@ struct repository {\n \tstruct promisor_remote_config *promisor_remote_config;\n \n \t/* Configurations */\n+\tint repository_format_worktree_config;\n \n \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\n \tunsigned different_commondir:1;\ndiff --git a/setup.c b/setup.c\nindex 458582207ea..d8663954350 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -650,11 +650,10 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t}\n \n \trepository_format_precious_objects = candidate->precious_objects;\n-\trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \tstring_list_clear(&candidate->v1_only_extensions, 0);\n \n-\tif (repository_format_worktree_config) {\n+\tif (candidate->worktree_config) {\n \t\t/*\n \t\t * pick up core.bare and core.worktree from per-worktree\n \t\t * config if present\n@@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,\n \t\treturn -1;\n \t}\n \n+\tthe_repository->repository_format_worktree_config =\n+\t\tcandidate.worktree_config;\n+\n \t/* take ownership of candidate.partial_clone */\n \tthe_repository->repository_format_partial_clone =\n \t\tcandidate.partial_clone;\n@@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t}\n \t\tif (startup_info->have_repository) {\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n+\t\t\tthe_repository->repository_format_worktree_config =\n+\t\t\t\trepo_fmt.worktree_config;\n \t\t\t/* take ownership of repo_fmt.partial_clone */\n \t\t\tthe_repository->repository_format_partial_clone =\n \t\t\t\trepo_fmt.partial_clone;\n@@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt)\n \tcheck_repository_format_gently(get_git_dir(), fmt, NULL);\n \tstartup_info->have_repository = 1;\n \trepo_set_hash_algo(the_repository, fmt->hash_algo);\n+\tthe_repository->repository_format_worktree_config =\n+\t\tfmt->worktree_config;\n \tthe_repository->repository_format_partial_clone =\n \t\txstrdup_or_null(fmt->partial_clone);\n \tclear_repository_format(&repo_fmt);\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex e35c203241f..6d0bacef4de 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -309,19 +309,30 @@ test_expect_success '--recurse-submodules parses submodule repo config' '\n test_expect_success '--recurse-submodules parses submodule worktree config' '\n \ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n \ttest_when_finished \"git -C submodule config --worktree --unset feature.experimental\" &&\n-\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n \n \tgit -C submodule config extensions.worktreeConfig true &&\n \tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n \n-\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on super\n-\t# project, so we need to enable it in the super project.\n-\tgit config extensions.worktreeConfig true &&\n-\n \ttest_must_fail git ls-files --recurse-submodules 2>err &&\n \tgrep \"bad boolean config value\" err\n '\n \n+test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' '\n+\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n+\n+\t# Enable worktree config in both super project & submodule, set an\n+\t# invalid config in the submodule worktree config, then disable worktree\n+\t# config in the submodule. The invalid worktree config should not be\n+\t# picked up.\n+\tgit config extensions.worktreeConfig true &&\n+\tgit -C submodule config extensions.worktreeConfig true &&\n+\tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n+\tgit -C submodule config --unset extensions.worktreeConfig &&\n+\n+\tgit ls-files --recurse-submodules 2>err &&\n+\t! grep \"bad boolean config value\" err\n+'\n+\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\ndiff --git a/worktree.c b/worktree.c\nindex b5ee71c5ebd..c448fecd4b3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -806,7 +806,7 @@ int init_worktree_config(struct repository *r)\n \t * If the extension is already enabled, then we can skip the\n \t * upgrade process.\n \t */\n-\tif (repository_format_worktree_config)\n+\tif (r->repository_format_worktree_config)\n \t\treturn 0;\n \tif ((res = git_config_set_gently(\"extensions.worktreeConfig\", \"true\")))\n \t\treturn error(_(\"failed to set extensions.worktreeConfig setting\"));\n@@ -846,7 +846,7 @@ int init_worktree_config(struct repository *r)\n \t * Ensure that we use worktree config for the remaining lifetime\n \t * of the current process.\n \t */\n-\trepository_format_worktree_config = 1;\n+\tr->repository_format_worktree_config = 1;\n \n cleanup:\n \tgit_configset_clear(&cs);\n-- \ngitgitgadget\n"},{"id":"477686","messageId":"xmqq4jo2gh5l.fsf@gitster.g","threadId":"59785","inReplyTo":"pull.1536.git.1684883872.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Fix behavior of worktree config in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-24T10:25:42Z","receivedAt":"2023-05-24T10:26:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> About a year ago, discussion on the sparse index integration of 'git grep'\n> surfaced larger incompatibilities between sparse-checkout and submodules\n> [1]. This series fixes one of the underlying issues to that incompatibility,\n> which is that the worktree config of the submodule (where\n> 'core.sparseCheckout', 'core.sparseCheckoutCone', and 'index.sparse' are\n> set) is not used when operating on the submodule from its super project\n> (e.g., in a command with '--recurse-submodules').\n\nOK. So in short, worktreeConfig used to be a singleton, global for\nthe entire git process, but because \"git grep\" (and possibly others)\nwants to operate across repository boundary when recursing into\nsubmodules, the repository_format_worktree_config needs to be per\nrepository instance, not a global singleton.\n\nMakes sense.\n"},{"id":"477707","messageId":"kl6ly1ldxlsv.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"aead2fe1ce162949fb313a92fe960e5a64512f60.1684883872.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] config: use gitdir to get worktree config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-25T01:05:36Z","receivedAt":"2023-05-25T01:05:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Update 'do_git_config_sequence()' to read the worktree config from\n> 'config.worktree' in 'opts->git_dir' rather than the gitdir of\n> 'the_repository'.\n\nThanks for the patches! This makes sense. do_git_config_sequence() is\neventually called by repo_config(), which is supposed to read config\ninto a \"struct repository\", so any reliance on the_repository's settings\nis wrong.\n\n> Note that behavior still isn't ideal\n> because 'extensions.worktreeConfig' in the super project[...]\n\nNit: We typically use \"superproject\" without the space.\n\n> diff --git a/config.c b/config.c\n> index b79baf83e35..a93f7bfa3aa 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,\n> \tchar *xdg_config = NULL;\n> \tchar *user_config = NULL;\n> \tchar *repo_config;\n> +\tchar *worktree_config;\n> \tenum config_scope prev_parsing_scope = reader->parsing_scope;\n> \n> -\tif (opts->commondir)\n> +\t/*\n> +\t * Ensure that either:\n> +\t * - the git_dir and commondir are both set, or\n> +\t * - the git_dir and commondir are both NULL\n> +\t */\n> +\tif (!opts->git_dir != !opts->commondir)\n> +\t\tBUG(\"only one of commondir and git_dir is non-NULL\");\n> +\n> +\tif (opts->commondir) {\n> \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n> -\telse if (opts->git_dir)\n> -\t\tBUG(\"git_dir without commondir\");\n> -\telse\n> +\t\tworktree_config = mkpathdup(\"%s/config.worktree\", opts->git_dir);\n> +\t} else {\n> \t\trepo_config = NULL;\n> +\t\tworktree_config = NULL;\n> +\t}\n\nMakes sense to me. I don't see why we would ever want to set one without\nthe other.\n\nI looked into whether we could get replace opts->commondir and\nopts->git_dir with a \"struct repository\" arg, but unfortunately\nread_early_config() needs to pass these values without touching\n\"the_repository\".\n\n> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n> index dd7770e85de..e35c203241f 100755\n> --- a/t/t3007-ls-files-recurse-submodules.sh\n> +++ b/t/t3007-ls-files-recurse-submodules.sh\n> @@ -299,6 +299,29 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '\n> \ttest_i18ngrep \"does not support --error-unmatch\" actual\n> '\n> \n> +test_expect_success '--recurse-submodules parses submodule repo config' '\n> +\ttest_when_finished \"git -C submodule config --unset feature.experimental\" &&\n> +\tgit -C submodule config feature.experimental \"invalid non-boolean value\" &&\n> +\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n> +\tgrep \"bad boolean config value\" err\n> +'\n\nThis test has a few bits that are important but non-obvious. It would be\nuseful to capture them in either the commit message or a comment.\n\nFirstly, we can't test this using \"git config\" because that only uses\nthe_repository, and we specifically need to read config in-core into a\n\"struct repository\" that is a submodule, so we need a command that\nrecurses into a submodule without using subprocesses. IIRC the only\nchoices are \"git grep\" and \"git ls-files\".\n\nSecondly, when we test that config is read from the submodule the choice\nof \"feature.experimental\" is quite important. The config is read quite\nindirectly: \"git ls-files\" reads from the submodule's index, which\nwill call prepare_repo_settings() on the submodule, and eventually calls\nrepo_config_get_bool() on \"feature.experimental\". Any of the configs in\nprepare_repo_settings() should do, though. A tiny suggestion would be to\nuse \"index.sparse\" instead of \"feature.experimental\", since (I presume)\nwe'll have to add sparse index + submodule tests for \"git ls-files\"\neventually.\n\n> +test_expect_success '--recurse-submodules parses submodule worktree config' '\n> +\ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n\nI believe \"test_config -C\" will achieve the desired effect.\n"},{"id":"477708","messageId":"kl6lv8ghxkpc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"5ed9100a7707a529b309005419244d083cdc85ba.1684883872.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-25T01:29:19Z","receivedAt":"2023-05-25T01:29:25Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Here's a quick response on the config.c bits, I haven't looked through\nthe global-removing parts closely yet. Rearranging the hunks for\nclarity...\n\n\"Victoria Dye via GitGitGadget\" writes:\n\n\n> @@ -2667,11 +2670,14 @@ static void repo_read_config(struct repository *repo)\n> {\n> \tstruct config_options opts = { 0 };\n> \tstruct configset_add_data data = CONFIGSET_ADD_INIT;\n> +\tstruct git_config_source config_source = { 0 };\n> \n> \topts.respect_includes = 1;\n> \topts.commondir = repo->commondir;\n> \topts.git_dir = repo->gitdir;\n> \n> +\tconfig_source.repo = repo;\n> +\n> \tif (!repo->config)\n> \t\tCALLOC_ARRAY(repo->config, 1);\n> \telse\n> @@ -2681,7 +2687,7 @@ static void repo_read_config(struct repository *repo)\n> \tdata.config_set = repo->config;\n> \tdata.config_reader = &the_reader;\n> \n> -\tif (config_with_options(config_set_callback, &data, NULL, &opts) < 0)\n> +\tif (config_with_options(config_set_callback, &data, &config_source, &opts) < 0)\n> \t\t/*\n> \t\t * config_with_options() normally returns only\n> \t\t * zero, as most errors are fatal, and\n\nI think it would be better to pass a \"struct repository\" arg to\nconfig_with_options() instead of mocking a config_source to hold a .repo\nmember. config_with_options() does double duty - it either discovers and\nreads the configs for the repo (system, global, worktree, etc), or it\nreads the config from just config_source. From this perspective, it\ndoesn't make sense that the caller can pass config_source but\nconfig_with_options() will still discover and read all configs, and I\nthink the only reason why this behavior is supported at all is that\nbuiltin/config.c sometimes \"reads all config\" and sometimes \"reads from\na single file\", but sloppily passes a non-NULL \"config_source\" arg\nunconditionally.\n\n> diff --git a/config.c b/config.c\n> index a93f7bfa3aa..9ce2ffff5e1 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2277,7 +2278,7 @@ int config_with_options(config_fn_t fn, void *data,\n> \t\tdata = &inc;\n> \t}\n> \n> -\tif (config_source)\n> +\tif (config_source && config_source->scope != CONFIG_SCOPE_UNKNOWN)\n> \t\tconfig_reader_set_scope(&the_reader, config_source->scope);\n> \n> \t/*\n\nThe aforemented change would also let us get rid of this, which might\nnot always be correct. I think there might be cases where the scope is\nactually unknown, but I'm not sure if we have any of those situations\nin-tree.\n\n> diff --git a/environment.c b/environment.c\n> index 28d18eaca8e..6bd001efbde 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */\n> int warn_ambiguous_refs = 1;\n> int warn_on_object_refname_ambiguity = 1;\n> int repository_format_precious_objects;\n> -int repository_format_worktree_config;\n> const char *git_commit_encoding;\n> const char *git_log_output_encoding;\n> char *apply_default_whitespace;\n\nAs an aside, I'm really happy to lose another global :)\n\n"},{"id":"477715","messageId":"kl6lsfbkxuje.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"kl6lv8ghxkpc.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-25T16:09:09Z","receivedAt":"2023-05-25T16:10:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Glen Choo writes:\n\n> I think it would be better to pass a \"struct repository\" arg to\n> config_with_options() instead of mocking a config_source to hold a .repo\n> member.\n\nThe flipside is that this would be redundant with an existing use of\ngit_config_source.repo, so for consistency, we should probably remove\ngit_config_source.repo. There's only one user of git_config_source.repo\n- reading .gitmodules from a blob. It probably made sense to add .repo\nthe time, but now that we have a second, different use of \"struct\nrepository\", accepting an arg is probably better.\n"},{"id":"477720","messageId":"kl6lpm6oxk09.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"pull.1536.git.1684883872.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Fix behavior of worktree config in submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-25T19:56:38Z","receivedAt":"2023-05-25T19:57:00Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> The outcome of this series is that 'extensions.worktreeConfig' and the\n> contents of the repository's worktree config are read and applied to (and\n> only to) the relevant repo when working in a super project/submodule setup.\n\nThanks sending these patches! This is an obvious bug, and squashing\nanother global is always helpful.\n\n> I'm\n> also hoping this will help (or at least not hurt) the work to avoid use of\n> global state in config parsing [2].\n\nBoth pieces of work touch different bits I think - yours plumbs and\nreads a \"struct repository\" before the config iterating begins, mine\nplumbs config source information after the config interating - so\nthey don't help each other, but at least the conflicts are only textual.\n\nJunio: There are quite a few conflicts. AFAICT each only has to be\nresolved once (IOW the number of conflicts to be fixed is the same with\n\"git merge\" vs \"git rebase\"). I have a v2 that I'm about to send, and if\nyou'd prefer (especially since you're on half-vacation), I can base it\noff Victoria's topic, which looks pretty straightforward and will\nprobably get merged pretty quickly.\n"},{"id":"477721","messageId":"9d1e5afd-e29a-0088-6151-251796277ef3@github.com","threadId":"59785","inReplyTo":"kl6lsfbkxuje.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-05-25T20:02:45Z","receivedAt":"2023-05-25T20:02:55Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Glen Choo wrote:\n> Glen Choo writes:\n> \n>> I think it would be better to pass a \"struct repository\" arg to\n>> config_with_options() instead of mocking a config_source to hold a .repo\n>> member.\n> \n> The flipside is that this would be redundant with an existing use of\n> git_config_source.repo, so for consistency, we should probably remove\n> git_config_source.repo. There's only one user of git_config_source.repo\n> - reading .gitmodules from a blob. It probably made sense to add .repo\n> the time, but now that we have a second, different use of \"struct\n> repository\", accepting an arg is probably better.\n\nAgreed, I'd much prefer having a single 'struct repository' instance used in\nthe context of config parsing (if they ever had different values, it\nprobably wouldn't be immediately obvious for debugging). This and all of\nyour other recommendations seem reasonable to me, I'll re-roll shortly.\n\nThanks for the review!\n"},{"id":"477722","messageId":"2b52aac0-46c3-6780-9411-d35c819f3673@github.com","threadId":"59785","inReplyTo":"kl6ly1ldxlsv.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 1/2] config: use gitdir to get worktree config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-25T20:05:28Z","receivedAt":"2023-05-25T20:05:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2023 9:05 PM, Glen Choo wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> From: Victoria Dye \n>>\n>> Update 'do_git_config_sequence()' to read the worktree config from\n>> 'config.worktree' in 'opts->git_dir' rather than the gitdir of\n>> 'the_repository'.\n> \n> Thanks for the patches! This makes sense. do_git_config_sequence() is\n> eventually called by repo_config(), which is supposed to read config\n> into a \"struct repository\", so any reliance on the_repository's settings\n> is wrong.\n\n>> +test_expect_success '--recurse-submodules parses submodule repo config' '\n>> +\ttest_when_finished \"git -C submodule config --unset feature.experimental\" &&\n>> +\tgit -C submodule config feature.experimental \"invalid non-boolean value\" &&\n>> +\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n>> +\tgrep \"bad boolean config value\" err\n>> +'\n> \n> This test has a few bits that are important but non-obvious. It would be\n> useful to capture them in either the commit message or a comment.\n> \n> Firstly, we can't test this using \"git config\" because that only uses\n> the_repository, and we specifically need to read config in-core into a\n> \"struct repository\" that is a submodule, so we need a command that\n> recurses into a submodule without using subprocesses. IIRC the only\n> choices are \"git grep\" and \"git ls-files\".\n> \n> Secondly, when we test that config is read from the submodule the choice\n> of \"feature.experimental\" is quite important. The config is read quite\n> indirectly: \"git ls-files\" reads from the submodule's index, which\n> will call prepare_repo_settings() on the submodule, and eventually calls\n> repo_config_get_bool() on \"feature.experimental\". Any of the configs in\n> prepare_repo_settings() should do, though. A tiny suggestion would be to\n> use \"index.sparse\" instead of \"feature.experimental\", since (I presume)\n> we'll have to add sparse index + submodule tests for \"git ls-files\"\n> eventually.\n\nSome of the points you bring up are definitely subtle, like the choice\nof config variable.\n\nI appreciate that there are two tests here: one to verify the test\nchecks have a similar effect without using the worktree config, and\nthen a second test to show the same behavior with worktree config.\n\nIf I understand correctly, the first test would pass without this\ncode change, but it is a helpful one to help add confidence in the\nsecond test.\n \n> +test_expect_success '--recurse-submodules parses submodule worktree config' '\n>> +\ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n> \n> I believe \"test_config -C\" will achieve the desired effect.\n\nThis should work, though it requires acting a bit strangely, at least\nif we want to replace the 'git config --worktree' command.\n\ntest_config treats the positions of the arguments as special, so we\nwould need to write it as:\n\n test_config -C submodule feature.experimental --worktree \"non boolean value\"\n\nand that's assuming that 'git -C submodule config feature.experimental\n--worktree \"non boolean value\"' is parsed correctly to use the --worktree\nargument. (I haven't tried it.) By using this order, that allows the\ntest_config helper to run the appropriate 'test_when_finished git config\n--unset feature.experimental' command.\n\nThanks,\n-Stolee\n"},{"id":"477723","messageId":"4d8907ba-3fe5-5bc1-e7bd-237edec31261@github.com","threadId":"59785","inReplyTo":"5ed9100a7707a529b309005419244d083cdc85ba.1684883872.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-25T20:13:44Z","receivedAt":"2023-05-25T20:13:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/23/2023 7:17 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Move 'repository_format_worktree_config' out of the global scope and into\n> the 'repository' struct. This change is similar to how\n> 'repository_format_partial_clone' was moved in ebaf3bcf1ae (repository: move\n> global r_f_p_c to repo struct, 2021-06-17), adding to the 'repository'\n> struct and updating 'setup.c' & 'repository.c' functions to assign the value\n> appropriately. In addition, update usage of the setting to reference the\n> relevant context's repo or, as a fallback, 'the_repository'.\n> \n> The primary goal of this change is to be able to load worktree config for a\n> submodule depending on whether that submodule - not the super project - has\n> 'extensions.worktreeConfig' enabled. To ensure 'do_git_config_sequence()'\n> has access to the newly repo-scoped configuration:\n> \n> - update 'repo_read_config()' to create a 'config_source' to hold the\n> repo instance\n> - add a 'repo' argument to 'do_git_config_sequence()'\n> - update 'config_with_options' to call 'do_git_config_sequence()' with\n> 'config_source.repo', or 'the_repository' as a fallback\n> \n> Finally, add/update tests in 't3007-ls-files-recurse-submodules.sh' to\n> verify 'extensions.worktreeConfig' is read an used independently by super\n> projects and submodules.\n \n> @@ -2277,7 +2278,7 @@ int config_with_options(config_fn_t fn, void *data,\n> \t\tdata = &inc;\n> \t}\n> \n> -\tif (config_source)\n> +\tif (config_source && config_source->scope != CONFIG_SCOPE_UNKNOWN)\n> \t\tconfig_reader_set_scope(&the_reader, config_source->scope);\n\nThis extra condition on config_source->scope surprised me. Could you\nelaborate on the reason this is necessary?\n\n> @@ -2667,11 +2670,14 @@ static void repo_read_config(struct repository *repo)\n> {\n> \tstruct config_options opts = { 0 };\n> \tstruct configset_add_data data = CONFIGSET_ADD_INIT;\n> +\tstruct git_config_source config_source = { 0 };\n\nThis could be...\n\n\tstruct git_config_source config_source = { .repo = repo };\n\n> \n> \topts.respect_includes = 1;\n> \topts.commondir = repo->commondir;\n> \topts.git_dir = repo->gitdir;\n> \n> +\tconfig_source.repo = repo;\n> +\n\n...avoiding these lines.\n\n> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n> index e35c203241f..6d0bacef4de 100755\n> --- a/t/t3007-ls-files-recurse-submodules.sh\n> +++ b/t/t3007-ls-files-recurse-submodules.sh\n> @@ -309,19 +309,30 @@ test_expect_success '--recurse-submodules parses submodule repo config' '\n> test_expect_success '--recurse-submodules parses submodule worktree config' '\n> \ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n> \ttest_when_finished \"git -C submodule config --worktree --unset feature.experimental\" &&\n> -\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n> \n> \tgit -C submodule config extensions.worktreeConfig true &&\n> \tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n> \n> -\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on super\n> -\t# project, so we need to enable it in the super project.\n> -\tgit config extensions.worktreeConfig true &&\n> -\n> \ttest_must_fail git ls-files --recurse-submodules 2>err &&\n> \tgrep \"bad boolean config value\" err\n> '\n\nThese are my favorite kind of test updates: deleting extra setup that's no\nlonger needed.\n\n> +test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' '\n> +\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n> +\n> +\t# Enable worktree config in both super project & submodule, set an\n> +\t# invalid config in the submodule worktree config, then disable worktree\n> +\t# config in the submodule. The invalid worktree config should not be\n> +\t# picked up.\n> +\tgit config extensions.worktreeConfig true &&\n> +\tgit -C submodule config extensions.worktreeConfig true &&\n> +\tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n> +\tgit -C submodule config --unset extensions.worktreeConfig &&\n> +\n> +\tgit ls-files --recurse-submodules 2>err &&\n> +\t! grep \"bad boolean config value\" err\n> +'\n\nWe have the same ways to improve here using 'test_config' as recommended\nin patch 1.\n\nThanks,\n-Stolee\n"},{"id":"477724","messageId":"fb597cdfeb033478103c81143e1b16dec957e0a4.1685064781.git.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":"pull.1536.v2.git.1685064781.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] config: use gitdir to get worktree config","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-26T01:32:58Z","receivedAt":"2023-05-26T01:33:10Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate 'do_git_config_sequence()' to read the worktree config from\n'config.worktree' in 'opts->git_dir' rather than the gitdir of\n'the_repository'.\n\nThe worktree config is loaded from the path returned by\n'git_pathdup(\"config.worktree\")', the 'config.worktree' relative to the\ngitdir of 'the_repository'. If loading the config for a submodule, this path\nis incorrect, since 'the_repository' is the superproject. 'opts->git_dir' is\nthe gitdir of the submodule being configured, so the config file in that\nlocation should be read instead.\n\nTo ensure the use of 'opts->git_dir' is safe, require that 'opts->git_dir'\nis set if-and-only-if 'opts->commondir' is set (rather than \"only-if\" as it\nis now). In all current usage of 'config_options', these values are set\ntogether, so the stricter check does not change any behavior.\n\nFinally, add tests to 't3007-ls-files-recurse-submodules.sh' to verify the\ncorrected config is loaded. Use 'ls-files' to test this because, unlike some\nother '--recurse-submodules' commands, 'ls-files' parses the config of the\nsubmodule in the same process as the superproject (via 'show_submodule()' ->\n'repo_read_index()' -> 'prepare_repo_settings()'). As a result,\n'the_repository' points to the config of the superproject but the\ncommondir/gitdir in the config sequence will be that of the submodule,\nproviding the exact scenario needed to verify this patch.\n\nThe first test ('--recurse-submodules parses submodule repo config') checks\nthat the submodule's *repo* config is read when running 'ls-files' on the\nsuperproject; this confirms already-working behavior, serving as a reference\nfor how worktree config parsing should behave. The second test\n('--recurse-submodules parses submodule worktree config') tests the same\nscenario as the previous but instead using the *worktree* config,\ndemonstrating the corrected behavior. The 'test_config' helper is extended\nfor this case so that it properly applies the '--worktree' option to the\nconfigure/unconfigure operations it performs.\n\nNote that, although the submodule worktree config is now parsed instead of\nthe superproject's, 'extensions.worktreeConfig' in the superproject still\ncontrols whether or not the worktree config is enabled at all in the\nsubmodule. This will be fixed in a later patch.\n\nSigned-off-by: Victoria Dye \n---\n config.c | 28 +++++++++++++++++---------\n t/t3007-ls-files-recurse-submodules.sh | 18 +++++++++++++++++\n t/test-lib-functions.sh | 13 ++++++++++--\n 3 files changed, 48 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b79baf83e35..a93f7bfa3aa 100644\n--- a/config.c\n+++ b/config.c\n@@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,\n \tchar *xdg_config = NULL;\n \tchar *user_config = NULL;\n \tchar *repo_config;\n+\tchar *worktree_config;\n \tenum config_scope prev_parsing_scope = reader->parsing_scope;\n \n-\tif (opts->commondir)\n+\t/*\n+\t * Ensure that either:\n+\t * - the git_dir and commondir are both set, or\n+\t * - the git_dir and commondir are both NULL\n+\t */\n+\tif (!opts->git_dir != !opts->commondir)\n+\t\tBUG(\"only one of commondir and git_dir is non-NULL\");\n+\n+\tif (opts->commondir) {\n \t\trepo_config = mkpathdup(\"%s/config\", opts->commondir);\n-\telse if (opts->git_dir)\n-\t\tBUG(\"git_dir without commondir\");\n-\telse\n+\t\tworktree_config = mkpathdup(\"%s/config.worktree\", opts->git_dir);\n+\t} else {\n \t\trepo_config = NULL;\n+\t\tworktree_config = NULL;\n+\t}\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_SYSTEM);\n \tif (git_config_system() && system_config &&\n@@ -2230,11 +2240,10 @@ static int do_git_config_sequence(struct config_reader *reader,\n \t\tret += git_config_from_file(fn, repo_config, data);\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);\n-\tif (!opts->ignore_worktree && repository_format_worktree_config) {\n-\t\tchar *path = git_pathdup(\"config.worktree\");\n-\t\tif (!access_or_die(path, R_OK, 0))\n-\t\t\tret += git_config_from_file(fn, path, data);\n-\t\tfree(path);\n+\tif (!opts->ignore_worktree && worktree_config &&\n+\t repository_format_worktree_config &&\n+\t !access_or_die(worktree_config, R_OK, 0)) {\n+\t\tret += git_config_from_file(fn, worktree_config, data);\n \t}\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_COMMAND);\n@@ -2246,6 +2255,7 @@ static int do_git_config_sequence(struct config_reader *reader,\n \tfree(xdg_config);\n \tfree(user_config);\n \tfree(repo_config);\n+\tfree(worktree_config);\n \treturn ret;\n }\n \ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex dd7770e85de..a3e26751427 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -299,6 +299,24 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '\n \ttest_i18ngrep \"does not support --error-unmatch\" actual\n '\n \n+test_expect_success '--recurse-submodules parses submodule repo config' '\n+\ttest_config -C submodule index.sparse \"invalid non-boolean value\" &&\n+\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n+\tgrep \"bad boolean config value\" err\n+'\n+\n+test_expect_success '--recurse-submodules parses submodule worktree config' '\n+\ttest_config -C submodule extensions.worktreeConfig true &&\n+\ttest_config -C submodule --worktree index.sparse \"invalid non-boolean value\" &&\n+\n+\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on\n+\t# superproject, so we need to enable it in the superproject.\n+\ttest_config extensions.worktreeConfig true &&\n+\n+\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n+\tgrep \"bad boolean config value\" err\n+'\n+\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 6e19ebc922a..b3864e22e9a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -542,8 +542,17 @@ test_config () {\n \t\tconfig_dir=$1\n \t\tshift\n \tfi\n-\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$1'\" &&\n-\tgit ${config_dir:+-C \"$config_dir\"} config \"$@\"\n+\n+\t# If --worktree is provided, use it to configure/unconfigure\n+\tis_worktree=\n+\tif test \"$1\" = --worktree\n+\tthen\n+\t\tis_worktree=1\n+\t\tshift\n+\tfi\n+\n+\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} ${is_worktree:+--worktree} '$1'\" &&\n+\tgit ${config_dir:+-C \"$config_dir\"} config ${is_worktree:+--worktree} \"$@\"\n }\n \n test_config_global () {\n-- \ngitgitgadget\n\n"},{"id":"477725","messageId":"pull.1536.v2.git.1685064781.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":"pull.1536.git.1684883872.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix behavior of worktree config in submodules","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-26T01:32:57Z","receivedAt":"2023-05-26T01:33:12Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"About a year ago, discussion on the sparse index integration of 'git grep'\nsurfaced larger incompatibilities between sparse-checkout and submodules\n[1]. This series fixes one of the underlying issues to that incompatibility,\nwhich is that the worktree config of the submodule (where\n'core.sparseCheckout', 'core.sparseCheckoutCone', and 'index.sparse' are\nset) is not used when operating on the submodule from its super project\n(e.g., in a command with '--recurse-submodules').\n\nThe outcome of this series is that 'extensions.worktreeConfig' and the\ncontents of the repository's worktree config are read and applied to (and\nonly to) the relevant repo when working in a super project/submodule setup.\nThis alone doesn't fix sparse-checkout/submodule interoperability; the\nadditional changes needed for that will be submitted in a later series. I'm\nalso hoping this will help (or at least not hurt) the work to avoid use of\nglobal state in config parsing [2].\n\n\nChanges since V1\n================\n\n * In 't3007', replaced manual 'git config'/'test_when_finished \"git config\n --unset\"' pairs with 'test_config' helper. Updated 'test_config' to\n handle the '--worktree' option.\n * Updated commit messages & test comments to better explain the purpose and\n more subtle functionality details to the new tests\n * Added a commit to move 'struct repository' out of 'git_config_source',\n rather than creating a dummy 'config_source' just to hold a repository\n instance.\n * Changed the config setting in the new tests from 'feature.experimental'\n to 'index.sparse' to tie these changes to their intended use case.\n * \"super project\" -> \"superproject\"\n\nThanks!\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/093827ae-41ef-5f7c-7829-647536ce1305@github.com/\n[2]\nhttps://lore.kernel.org/git/pull.1497.git.git.1682104398.gitgitgadget@gmail.com/\n\nVictoria Dye (3):\n config: use gitdir to get worktree config\n config: pass 'repo' directly to 'config_with_options()'\n repository: move 'repository_format_worktree_config' to repo scope\n\n builtin/config.c | 17 +++++----\n builtin/worktree.c | 2 +-\n config.c | 49 ++++++++++++++++----------\n config.h | 4 +--\n environment.c | 1 -\n environment.h | 1 -\n repository.c | 1 +\n repository.h | 1 +\n setup.c | 10 ++++--\n submodule-config.c | 3 +-\n t/t3007-ls-files-recurse-submodules.sh | 33 +++++++++++++++++\n t/test-lib-functions.sh | 13 +++++--\n worktree.c | 4 +--\n 13 files changed, 102 insertions(+), 37 deletions(-)\n\n\nbase-commit: 4a714b37029a4b63dbd22f7d7ed81f7a0d693680\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1536%2Fvdye%2Fvdye%2Fsubmodule-worktree-config-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1536/vdye/vdye/submodule-worktree-config-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1536\n\nRange-diff vs v1:\n\n 1: aead2fe1ce1 ! 1: fb597cdfeb0 config: use gitdir to get worktree config\n @@ Commit message\n The worktree config is loaded from the path returned by\n 'git_pathdup(\"config.worktree\")', the 'config.worktree' relative to the\n gitdir of 'the_repository'. If loading the config for a submodule, this path\n - is incorrect, since 'the_repository' is the super project. Conversely,\n - 'opts->git_dir' is the gitdir of the submodule being configured, so the\n - config file in that location should be read instead.\n + is incorrect, since 'the_repository' is the superproject. 'opts->git_dir' is\n + the gitdir of the submodule being configured, so the config file in that\n + location should be read instead.\n \n To ensure the use of 'opts->git_dir' is safe, require that 'opts->git_dir'\n is set if-and-only-if 'opts->commondir' is set (rather than \"only-if\" as it\n is now). In all current usage of 'config_options', these values are set\n together, so the stricter check does not change any behavior.\n \n - Finally, add tests to 't3007-ls-files-recurse-submodules.sh' to demonstrate\n - the corrected config loading behavior. Note that behavior still isn't ideal\n - because 'extensions.worktreeConfig' in the super project controls whether or\n - not the worktree config is used in the submodule. This will be fixed in a\n - later patch.\n + Finally, add tests to 't3007-ls-files-recurse-submodules.sh' to verify the\n + corrected config is loaded. Use 'ls-files' to test this because, unlike some\n + other '--recurse-submodules' commands, 'ls-files' parses the config of the\n + submodule in the same process as the superproject (via 'show_submodule()' ->\n + 'repo_read_index()' -> 'prepare_repo_settings()'). As a result,\n + 'the_repository' points to the config of the superproject but the\n + commondir/gitdir in the config sequence will be that of the submodule,\n + providing the exact scenario needed to verify this patch.\n +\n + The first test ('--recurse-submodules parses submodule repo config') checks\n + that the submodule's *repo* config is read when running 'ls-files' on the\n + superproject; this confirms already-working behavior, serving as a reference\n + for how worktree config parsing should behave. The second test\n + ('--recurse-submodules parses submodule worktree config') tests the same\n + scenario as the previous but instead using the *worktree* config,\n + demonstrating the corrected behavior. The 'test_config' helper is extended\n + for this case so that it properly applies the '--worktree' option to the\n + configure/unconfigure operations it performs.\n +\n + Note that, although the submodule worktree config is now parsed instead of\n + the superproject's, 'extensions.worktreeConfig' in the superproject still\n + controls whether or not the worktree config is enabled at all in the\n + submodule. This will be fixed in a later patch.\n \n Signed-off-by: Victoria Dye \n \n @@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodule\n '\n \n +test_expect_success '--recurse-submodules parses submodule repo config' '\n -+\ttest_when_finished \"git -C submodule config --unset feature.experimental\" &&\n -+\tgit -C submodule config feature.experimental \"invalid non-boolean value\" &&\n ++\ttest_config -C submodule index.sparse \"invalid non-boolean value\" &&\n +\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n +\tgrep \"bad boolean config value\" err\n +'\n +\n +test_expect_success '--recurse-submodules parses submodule worktree config' '\n -+\ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n -+\ttest_when_finished \"git -C submodule config --worktree --unset feature.experimental\" &&\n -+\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n ++\ttest_config -C submodule extensions.worktreeConfig true &&\n ++\ttest_config -C submodule --worktree index.sparse \"invalid non-boolean value\" &&\n +\n -+\tgit -C submodule config extensions.worktreeConfig true &&\n -+\tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n -+\n -+\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on super\n -+\t# project, so we need to enable it in the super project.\n -+\tgit config extensions.worktreeConfig true &&\n ++\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on\n ++\t# superproject, so we need to enable it in the superproject.\n ++\ttest_config extensions.worktreeConfig true &&\n +\n +\ttest_must_fail git ls-files --recurse-submodules 2>err &&\n +\tgrep \"bad boolean config value\" err\n @@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodule\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n +\n + ## t/test-lib-functions.sh ##\n +@@ t/test-lib-functions.sh: test_config () {\n + \t\tconfig_dir=$1\n + \t\tshift\n + \tfi\n +-\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$1'\" &&\n +-\tgit ${config_dir:+-C \"$config_dir\"} config \"$@\"\n ++\n ++\t# If --worktree is provided, use it to configure/unconfigure\n ++\tis_worktree=\n ++\tif test \"$1\" = --worktree\n ++\tthen\n ++\t\tis_worktree=1\n ++\t\tshift\n ++\tfi\n ++\n ++\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} ${is_worktree:+--worktree} '$1'\" &&\n ++\tgit ${config_dir:+-C \"$config_dir\"} config ${is_worktree:+--worktree} \"$@\"\n + }\n + \n + test_config_global () {\n -: ----------- > 2: 26a36423a8a config: pass 'repo' directly to 'config_with_options()'\n 2: 5ed9100a770 ! 3: 506a2cf8c73 repository: move 'repository_format_worktree_config' to repo scope\n @@ Commit message\n Move 'repository_format_worktree_config' out of the global scope and into\n the 'repository' struct. This change is similar to how\n 'repository_format_partial_clone' was moved in ebaf3bcf1ae (repository: move\n - global r_f_p_c to repo struct, 2021-06-17), adding to the 'repository'\n + global r_f_p_c to repo struct, 2021-06-17), adding it to the 'repository'\n struct and updating 'setup.c' & 'repository.c' functions to assign the value\n - appropriately. In addition, update usage of the setting to reference the\n - relevant context's repo or, as a fallback, 'the_repository'.\n + appropriately.\n \n - The primary goal of this change is to be able to load worktree config for a\n - submodule depending on whether that submodule - not the super project - has\n + The primary goal of this change is to be able to load the worktree config of\n + a submodule depending on whether that submodule - not its superproject - has\n 'extensions.worktreeConfig' enabled. To ensure 'do_git_config_sequence()'\n - has access to the newly repo-scoped configuration:\n -\n - - update 'repo_read_config()' to create a 'config_source' to hold the\n - repo instance\n - - add a 'repo' argument to 'do_git_config_sequence()'\n - - update 'config_with_options' to call 'do_git_config_sequence()' with\n - 'config_source.repo', or 'the_repository' as a fallback\n + has access to the newly repo-scoped configuration, add a 'struct repository'\n + argument to 'do_git_config_sequence()' and pass it the 'repo' value from\n + 'config_with_options()'.\n \n Finally, add/update tests in 't3007-ls-files-recurse-submodules.sh' to\n - verify 'extensions.worktreeConfig' is read an used independently by super\n - projects and submodules.\n + verify 'extensions.worktreeConfig' is read an used independently by\n + superprojects and submodules.\n \n Signed-off-by: Victoria Dye \n \n @@ config.c: static int do_git_config_sequence(struct config_reader *reader,\n \t !access_or_die(worktree_config, R_OK, 0)) {\n \t\tret += git_config_from_file(fn, worktree_config, data);\n \t}\n -@@ config.c: int config_with_options(config_fn_t fn, void *data,\n - \t\tdata = &inc;\n - \t}\n - \n --\tif (config_source)\n -+\tif (config_source && config_source->scope != CONFIG_SCOPE_UNKNOWN)\n - \t\tconfig_reader_set_scope(&the_reader, config_source->scope);\n - \n - \t/*\n @@ config.c: int config_with_options(config_fn_t fn, void *data,\n \t\tret = git_config_from_blob_ref(fn, repo, config_source->blob,\n \t\t\t\t\t\tdata);\n \t} else {\n -\t\tret = do_git_config_sequence(&the_reader, opts, fn, data);\n -+\t\tstruct repository *repo = config_source && config_source->repo ?\n -+\t\t\tconfig_source->repo : the_repository;\n +\t\tret = do_git_config_sequence(&the_reader, opts, repo, fn, data);\n \t}\n \n \tif (inc.remote_urls) {\n -@@ config.c: static void repo_read_config(struct repository *repo)\n - {\n - \tstruct config_options opts = { 0 };\n - \tstruct configset_add_data data = CONFIGSET_ADD_INIT;\n -+\tstruct git_config_source config_source = { 0 };\n - \n - \topts.respect_includes = 1;\n - \topts.commondir = repo->commondir;\n - \topts.git_dir = repo->gitdir;\n - \n -+\tconfig_source.repo = repo;\n -+\n - \tif (!repo->config)\n - \t\tCALLOC_ARRAY(repo->config, 1);\n - \telse\n -@@ config.c: static void repo_read_config(struct repository *repo)\n - \tdata.config_set = repo->config;\n - \tdata.config_reader = &the_reader;\n - \n --\tif (config_with_options(config_set_callback, &data, NULL, &opts) < 0)\n -+\tif (config_with_options(config_set_callback, &data, &config_source, &opts) < 0)\n - \t\t/*\n - \t\t * config_with_options() normally returns only\n - \t\t * zero, as most errors are fatal, and\n @@ config.c: int repo_config_set_worktree_gently(struct repository *r,\n \t\t\t\t const char *key, const char *value)\n {\n @@ setup.c: void check_repository_format(struct repository_format *fmt)\n \tclear_repository_format(&repo_fmt);\n \n ## t/t3007-ls-files-recurse-submodules.sh ##\n -@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodules parses submodule repo config' '\n - test_expect_success '--recurse-submodules parses submodule worktree config' '\n - \ttest_when_finished \"git -C submodule config --unset extensions.worktreeConfig\" &&\n - \ttest_when_finished \"git -C submodule config --worktree --unset feature.experimental\" &&\n --\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n +@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodules parses submodule worktree config' '\n + \ttest_config -C submodule extensions.worktreeConfig true &&\n + \ttest_config -C submodule --worktree index.sparse \"invalid non-boolean value\" &&\n \n - \tgit -C submodule config extensions.worktreeConfig true &&\n - \tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n - \n --\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on super\n --\t# project, so we need to enable it in the super project.\n --\tgit config extensions.worktreeConfig true &&\n +-\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on\n +-\t# superproject, so we need to enable it in the superproject.\n +-\ttest_config extensions.worktreeConfig true &&\n -\n \ttest_must_fail git ls-files --recurse-submodules 2>err &&\n \tgrep \"bad boolean config value\" err\n '\n \n +test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' '\n -+\ttest_when_finished \"git config --unset extensions.worktreeConfig\" &&\n -+\n +\t# Enable worktree config in both super project & submodule, set an\n -+\t# invalid config in the submodule worktree config, then disable worktree\n -+\t# config in the submodule. The invalid worktree config should not be\n -+\t# picked up.\n -+\tgit config extensions.worktreeConfig true &&\n -+\tgit -C submodule config extensions.worktreeConfig true &&\n -+\tgit -C submodule config --worktree feature.experimental \"invalid non-boolean value\" &&\n -+\tgit -C submodule config --unset extensions.worktreeConfig &&\n ++\t# invalid config in the submodule worktree config\n ++\ttest_config extensions.worktreeConfig true &&\n ++\ttest_config -C submodule extensions.worktreeConfig true &&\n ++\ttest_config -C submodule --worktree index.sparse \"invalid non-boolean value\" &&\n ++\n ++\t# Now, disable the worktree config in the submodule. Note that we need\n ++\t# to manually re-enable extensions.worktreeConfig when the test is\n ++\t# finished, otherwise the test_unconfig of index.sparse will not work.\n ++\ttest_unconfig -C submodule extensions.worktreeConfig &&\n ++\ttest_when_finished \"git -C submodule config extensions.worktreeConfig true\" &&\n +\n ++\t# With extensions.worktreeConfig disabled in the submodule, the invalid\n ++\t# worktree config is not picked up.\n +\tgit ls-files --recurse-submodules 2>err &&\n +\t! grep \"bad boolean config value\" err\n +'\n\n-- \ngitgitgadget\n"},{"id":"477726","messageId":"26a36423a8a8449871a0695e835e7a2616487251.1685064781.git.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":"pull.1536.v2.git.1685064781.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] config: pass 'repo' directly to 'config_with_options()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-26T01:32:59Z","receivedAt":"2023-05-26T01:33:15Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a 'struct repository' argument to 'config_with_options()' and remove the\n'repo' field from 'struct git_config_source'.\n\nA 'struct repository' instance was originally added to the config source in\ne3e8bf046e9 (submodule-config: pass repo upon blob config read, 2021-08-16)\nto improve how submodule blob config content was accessed. At the time, this\nwas the only use for a 'repository' instance, so it was naturally added only\nwhere it was needed: to 'struct git_config_source'. However, in upcoming\npatches, 'config_with_options()' will need the repository instance to access\nextension information (regardless of whether a 'config_source' exists). To\nmake the 'struct repository' instance more easily accessible, move it into\nthe function's arguments.\n\nUpdate all callers of 'config_with_options()' to pass the appropriate 'repo'\nvalue:\n\n* in 'builtin/config.c', use 'the_repository'\n* in 'submodule--config.c', use the 'repo' arg in 'config_from_gitmodules()'\n* in 'read_[very_]early_config()' & 'read_protected_config()', set 'repo' to\n NULL (repository instances aren't available there)\n* in 'populate_remote_urls()', use the repo instance that has been added to\n the 'struct config_include_data'\n* in 'repo_read_config()', use the given 'repo' arg\n\nFinally, note that this patch eliminates the fallback to 'the_repository'\nthat previously existed for the 'config_source' repo instance if it was\nNULL. The fallback is no longer necessary, as the 'repo' is set explicitly\nin all cases where it is needed.\n\nSigned-off-by: Victoria Dye \n---\n builtin/config.c | 14 +++++++++-----\n config.c | 16 +++++++++-------\n config.h | 4 ++--\n submodule-config.c | 3 +--\n 4 files changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ff2fe8ef125..8fc90288f9e 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -375,7 +375,8 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)\n \t}\n \n \tconfig_with_options(collect_config, &values,\n-\t\t\t &given_config_source, &config_options);\n+\t\t\t &given_config_source, the_repository,\n+\t\t\t &config_options);\n \n \tif (!values.nr && default_value) {\n \t\tstruct strbuf *item;\n@@ -486,7 +487,8 @@ static void get_color(const char *var, const char *def_color)\n \tget_color_found = 0;\n \tparsed_color[0] = '\\0';\n \tconfig_with_options(git_get_color_config, NULL,\n-\t\t\t &given_config_source, &config_options);\n+\t\t\t &given_config_source, the_repository,\n+\t\t\t &config_options);\n \n \tif (!get_color_found && def_color) {\n \t\tif (color_parse(def_color, parsed_color) < 0)\n@@ -518,7 +520,8 @@ static int get_colorbool(const char *var, int print)\n \tget_diff_color_found = -1;\n \tget_color_ui_found = -1;\n \tconfig_with_options(git_get_colorbool_config, NULL,\n-\t\t\t &given_config_source, &config_options);\n+\t\t\t &given_config_source, the_repository,\n+\t\t\t &config_options);\n \n \tif (get_colorbool_found < 0) {\n \t\tif (!strcmp(get_colorbool_slot, \"color.diff\"))\n@@ -607,7 +610,8 @@ static int get_urlmatch(const char *var, const char *url)\n \t}\n \n \tconfig_with_options(urlmatch_config_entry, &config,\n-\t\t\t &given_config_source, &config_options);\n+\t\t\t &given_config_source, the_repository,\n+\t\t\t &config_options);\n \n \tret = !values.nr;\n \n@@ -827,7 +831,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (config_with_options(show_all_config, NULL,\n-\t\t\t\t\t&given_config_source,\n+\t\t\t\t\t&given_config_source, the_repository,\n \t\t\t\t\t&config_options) < 0) {\n \t\t\tif (given_config_source.file)\n \t\t\t\tdie_errno(_(\"unable to read config file '%s'\"),\ndiff --git a/config.c b/config.c\nindex a93f7bfa3aa..67e60e131c2 100644\n--- a/config.c\n+++ b/config.c\n@@ -199,6 +199,7 @@ struct config_include_data {\n \tvoid *data;\n \tconst struct config_options *opts;\n \tstruct git_config_source *config_source;\n+\tstruct repository *repo;\n \tstruct config_reader *config_reader;\n \n \t/*\n@@ -415,7 +416,8 @@ static void populate_remote_urls(struct config_include_data *inc)\n \n \tinc->remote_urls = xmalloc(sizeof(*inc->remote_urls));\n \tstring_list_init_dup(inc->remote_urls);\n-\tconfig_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts);\n+\tconfig_with_options(add_remote_url, inc->remote_urls,\n+\t\t\t inc->config_source, inc->repo, &opts);\n \n \tconfig_reader_set_scope(inc->config_reader, store_scope);\n }\n@@ -2261,6 +2263,7 @@ static int do_git_config_sequence(struct config_reader *reader,\n \n int config_with_options(config_fn_t fn, void *data,\n \t\t\tstruct git_config_source *config_source,\n+\t\t\tstruct repository *repo,\n \t\t\tconst struct config_options *opts)\n {\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n@@ -2271,6 +2274,7 @@ int config_with_options(config_fn_t fn, void *data,\n \t\tinc.fn = fn;\n \t\tinc.data = data;\n \t\tinc.opts = opts;\n+\t\tinc.repo = repo;\n \t\tinc.config_source = config_source;\n \t\tinc.config_reader = &the_reader;\n \t\tfn = git_config_include;\n@@ -2289,8 +2293,6 @@ int config_with_options(config_fn_t fn, void *data,\n \t} else if (config_source && config_source->file) {\n \t\tret = git_config_from_file(fn, config_source->file, data);\n \t} else if (config_source && config_source->blob) {\n-\t\tstruct repository *repo = config_source->repo ?\n-\t\t\tconfig_source->repo : the_repository;\n \t\tret = git_config_from_blob_ref(fn, repo, config_source->blob,\n \t\t\t\t\t\tdata);\n \t} else {\n@@ -2353,7 +2355,7 @@ void read_early_config(config_fn_t cb, void *data)\n \t\topts.git_dir = gitdir.buf;\n \t}\n \n-\tconfig_with_options(cb, data, NULL, &opts);\n+\tconfig_with_options(cb, data, NULL, NULL, &opts);\n \n \tstrbuf_release(&commondir);\n \tstrbuf_release(&gitdir);\n@@ -2373,7 +2375,7 @@ void read_very_early_config(config_fn_t cb, void *data)\n \topts.ignore_cmdline = 1;\n \topts.system_gently = 1;\n \n-\tconfig_with_options(cb, data, NULL, &opts);\n+\tconfig_with_options(cb, data, NULL, NULL, &opts);\n }\n \n RESULT_MUST_BE_USED\n@@ -2681,7 +2683,7 @@ static void repo_read_config(struct repository *repo)\n \tdata.config_set = repo->config;\n \tdata.config_reader = &the_reader;\n \n-\tif (config_with_options(config_set_callback, &data, NULL, &opts) < 0)\n+\tif (config_with_options(config_set_callback, &data, NULL, repo, &opts) < 0)\n \t\t/*\n \t\t * config_with_options() normally returns only\n \t\t * zero, as most errors are fatal, and\n@@ -2825,7 +2827,7 @@ static void read_protected_config(void)\n \tgit_configset_init(&protected_config);\n \tdata.config_set = &protected_config;\n \tdata.config_reader = &the_reader;\n-\tconfig_with_options(config_set_callback, &data, NULL, &opts);\n+\tconfig_with_options(config_set_callback, &data, NULL, NULL, &opts);\n }\n \n void git_protected_config(config_fn_t fn, void *data)\ndiff --git a/config.h b/config.h\nindex 247b572b37b..d1c5577589e 100644\n--- a/config.h\n+++ b/config.h\n@@ -3,6 +3,7 @@\n \n #include \"hashmap.h\"\n #include \"string-list.h\"\n+#include \"repository.h\"\n \n \n /**\n@@ -49,8 +50,6 @@ const char *config_scope_name(enum config_scope scope);\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\n-\t/* The repository if blob is not NULL; leave blank for the_repository */\n-\tstruct repository *repo;\n \tconst char *blob;\n \tenum config_scope scope;\n };\n@@ -196,6 +195,7 @@ void git_config(config_fn_t fn, void *);\n */\n int config_with_options(config_fn_t fn, void *,\n \t\t\tstruct git_config_source *config_source,\n+\t\t\tstruct repository *repo,\n \t\t\tconst struct config_options *opts);\n \n /**\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 58dfbde9ae5..7eb7a0d88d2 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -659,7 +659,6 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\t\tconfig_source.file = file;\n \t\t} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n \t\t\t repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n-\t\t\tconfig_source.repo = repo;\n \t\t\tconfig_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n \t\t\tif (repo != the_repository)\n \t\t\t\tadd_submodule_odb_by_path(repo->objects->odb->path);\n@@ -667,7 +666,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\t\tgoto out;\n \t\t}\n \n-\t\tconfig_with_options(fn, data, &config_source, &opts);\n+\t\tconfig_with_options(fn, data, &config_source, repo, &opts);\n \n out:\n \t\tfree(oidstr);\n-- \ngitgitgadget\n\n"},{"id":"477727","messageId":"506a2cf8c73549bc8f9761b56532ef08ed220da4.1685064781.git.gitgitgadget@gmail.com","threadId":"59785","inReplyTo":"pull.1536.v2.git.1685064781.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-26T01:33:00Z","receivedAt":"2023-05-26T01:33:17Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nMove 'repository_format_worktree_config' out of the global scope and into\nthe 'repository' struct. This change is similar to how\n'repository_format_partial_clone' was moved in ebaf3bcf1ae (repository: move\nglobal r_f_p_c to repo struct, 2021-06-17), adding it to the 'repository'\nstruct and updating 'setup.c' & 'repository.c' functions to assign the value\nappropriately.\n\nThe primary goal of this change is to be able to load the worktree config of\na submodule depending on whether that submodule - not its superproject - has\n'extensions.worktreeConfig' enabled. To ensure 'do_git_config_sequence()'\nhas access to the newly repo-scoped configuration, add a 'struct repository'\nargument to 'do_git_config_sequence()' and pass it the 'repo' value from\n'config_with_options()'.\n\nFinally, add/update tests in 't3007-ls-files-recurse-submodules.sh' to\nverify 'extensions.worktreeConfig' is read an used independently by\nsuperprojects and submodules.\n\nSigned-off-by: Victoria Dye \n---\n builtin/config.c | 3 ++-\n builtin/worktree.c | 2 +-\n config.c | 7 ++++---\n environment.c | 1 -\n environment.h | 1 -\n repository.c | 1 +\n repository.h | 1 +\n setup.c | 10 ++++++++--\n t/t3007-ls-files-recurse-submodules.sh | 23 +++++++++++++++++++----\n worktree.c | 4 ++--\n 10 files changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8fc90288f9e..d40fddb042a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -5,6 +5,7 @@\n #include \"color.h\"\n #include \"editor.h\"\n #include \"environment.h\"\n+#include \"repository.h\"\n #include \"gettext.h\"\n #include \"ident.h\"\n #include \"parse-options.h\"\n@@ -717,7 +718,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.scope = CONFIG_SCOPE_LOCAL;\n \t} else if (use_worktree_config) {\n \t\tstruct worktree **worktrees = get_worktrees();\n-\t\tif (repository_format_worktree_config)\n+\t\tif (the_repository->repository_format_worktree_config)\n \t\t\tgiven_config_source.file = git_pathdup(\"config.worktree\");\n \t\telse if (worktrees[0] && worktrees[1])\n \t\t\tdie(_(\"--worktree cannot be used with multiple \"\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex f3180463be2..60e389aaedb 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -483,7 +483,7 @@ static int add_worktree(const char *path, const char *refname,\n \t * values from the current worktree into the new one, that way the\n \t * new worktree behaves the same as this one.\n \t */\n-\tif (repository_format_worktree_config)\n+\tif (the_repository->repository_format_worktree_config)\n \t\tcopy_filtered_worktree_config(sb_repo.buf);\n \n \tstrvec_pushf(&child_env, \"%s=%s\", GIT_DIR_ENVIRONMENT, sb_git.buf);\ndiff --git a/config.c b/config.c\nindex 67e60e131c2..f5bdac0aeed 100644\n--- a/config.c\n+++ b/config.c\n@@ -2195,6 +2195,7 @@ int git_config_system(void)\n \n static int do_git_config_sequence(struct config_reader *reader,\n \t\t\t\t const struct config_options *opts,\n+\t\t\t\t const struct repository *repo,\n \t\t\t\t config_fn_t fn, void *data)\n {\n \tint ret = 0;\n@@ -2243,7 +2244,7 @@ static int do_git_config_sequence(struct config_reader *reader,\n \n \tconfig_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);\n \tif (!opts->ignore_worktree && worktree_config &&\n-\t repository_format_worktree_config &&\n+\t repo && repo->repository_format_worktree_config &&\n \t !access_or_die(worktree_config, R_OK, 0)) {\n \t\tret += git_config_from_file(fn, worktree_config, data);\n \t}\n@@ -2296,7 +2297,7 @@ int config_with_options(config_fn_t fn, void *data,\n \t\tret = git_config_from_blob_ref(fn, repo, config_source->blob,\n \t\t\t\t\t\tdata);\n \t} else {\n-\t\tret = do_git_config_sequence(&the_reader, opts, fn, data);\n+\t\tret = do_git_config_sequence(&the_reader, opts, repo, fn, data);\n \t}\n \n \tif (inc.remote_urls) {\n@@ -3339,7 +3340,7 @@ int repo_config_set_worktree_gently(struct repository *r,\n \t\t\t\t const char *key, const char *value)\n {\n \t/* Only use worktree-specific config if it is already enabled. */\n-\tif (repository_format_worktree_config) {\n+\tif (r->repository_format_worktree_config) {\n \t\tchar *file = repo_git_path(r, \"config.worktree\");\n \t\tint ret = git_config_set_multivar_in_file_gently(\n \t\t\t\t\tfile, key, value, NULL, 0);\ndiff --git a/environment.c b/environment.c\nindex 28d18eaca8e..6bd001efbde 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int repository_format_precious_objects;\n-int repository_format_worktree_config;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n char *apply_default_whitespace;\ndiff --git a/environment.h b/environment.h\nindex 30cb7e0fa34..e6668079269 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -197,7 +197,6 @@ extern char *notes_ref_name;\n extern int grafts_replace_parents;\n \n extern int repository_format_precious_objects;\n-extern int repository_format_worktree_config;\n \n /*\n * Create a temporary file rooted in the object database directory, or\ndiff --git a/repository.c b/repository.c\nindex c53e480e326..104960f8f59 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -182,6 +182,7 @@ int repo_init(struct repository *repo,\n \t\tgoto error;\n \n \trepo_set_hash_algo(repo, format.hash_algo);\n+\trepo->repository_format_worktree_config = format.worktree_config;\n \n \t/* take ownership of format.partial_clone */\n \trepo->repository_format_partial_clone = format.partial_clone;\ndiff --git a/repository.h b/repository.h\nindex 1a13ff28677..74ae26635a4 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -163,6 +163,7 @@ struct repository {\n \tstruct promisor_remote_config *promisor_remote_config;\n \n \t/* Configurations */\n+\tint repository_format_worktree_config;\n \n \t/* Indicate if a repository has a different 'commondir' from 'gitdir' */\n \tunsigned different_commondir:1;\ndiff --git a/setup.c b/setup.c\nindex 458582207ea..d8663954350 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -650,11 +650,10 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t}\n \n \trepository_format_precious_objects = candidate->precious_objects;\n-\trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \tstring_list_clear(&candidate->v1_only_extensions, 0);\n \n-\tif (repository_format_worktree_config) {\n+\tif (candidate->worktree_config) {\n \t\t/*\n \t\t * pick up core.bare and core.worktree from per-worktree\n \t\t * config if present\n@@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,\n \t\treturn -1;\n \t}\n \n+\tthe_repository->repository_format_worktree_config =\n+\t\tcandidate.worktree_config;\n+\n \t/* take ownership of candidate.partial_clone */\n \tthe_repository->repository_format_partial_clone =\n \t\tcandidate.partial_clone;\n@@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t}\n \t\tif (startup_info->have_repository) {\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n+\t\t\tthe_repository->repository_format_worktree_config =\n+\t\t\t\trepo_fmt.worktree_config;\n \t\t\t/* take ownership of repo_fmt.partial_clone */\n \t\t\tthe_repository->repository_format_partial_clone =\n \t\t\t\trepo_fmt.partial_clone;\n@@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt)\n \tcheck_repository_format_gently(get_git_dir(), fmt, NULL);\n \tstartup_info->have_repository = 1;\n \trepo_set_hash_algo(the_repository, fmt->hash_algo);\n+\tthe_repository->repository_format_worktree_config =\n+\t\tfmt->worktree_config;\n \tthe_repository->repository_format_partial_clone =\n \t\txstrdup_or_null(fmt->partial_clone);\n \tclear_repository_format(&repo_fmt);\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex a3e26751427..7308a3d4e25 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -309,14 +309,29 @@ test_expect_success '--recurse-submodules parses submodule worktree config' '\n \ttest_config -C submodule extensions.worktreeConfig true &&\n \ttest_config -C submodule --worktree index.sparse \"invalid non-boolean value\" &&\n \n-\t# NEEDSWORK: the extensions.worktreeConfig is set globally based on\n-\t# superproject, so we need to enable it in the superproject.\n-\ttest_config extensions.worktreeConfig true &&\n-\n \ttest_must_fail git ls-files --recurse-submodules 2>err &&\n \tgrep \"bad boolean config value\" err\n '\n \n+test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' '\n+\t# Enable worktree config in both super project & submodule, set an\n+\t# invalid config in the submodule worktree config\n+\ttest_config extensions.worktreeConfig true &&\n+\ttest_config -C submodule extensions.worktreeConfig true &&\n+\ttest_config -C submodule --worktree index.sparse \"invalid non-boolean value\" &&\n+\n+\t# Now, disable the worktree config in the submodule. Note that we need\n+\t# to manually re-enable extensions.worktreeConfig when the test is\n+\t# finished, otherwise the test_unconfig of index.sparse will not work.\n+\ttest_unconfig -C submodule extensions.worktreeConfig &&\n+\ttest_when_finished \"git -C submodule config extensions.worktreeConfig true\" &&\n+\n+\t# With extensions.worktreeConfig disabled in the submodule, the invalid\n+\t# worktree config is not picked up.\n+\tgit ls-files --recurse-submodules 2>err &&\n+\t! grep \"bad boolean config value\" err\n+'\n+\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\ndiff --git a/worktree.c b/worktree.c\nindex b5ee71c5ebd..c448fecd4b3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -806,7 +806,7 @@ int init_worktree_config(struct repository *r)\n \t * If the extension is already enabled, then we can skip the\n \t * upgrade process.\n \t */\n-\tif (repository_format_worktree_config)\n+\tif (r->repository_format_worktree_config)\n \t\treturn 0;\n \tif ((res = git_config_set_gently(\"extensions.worktreeConfig\", \"true\")))\n \t\treturn error(_(\"failed to set extensions.worktreeConfig setting\"));\n@@ -846,7 +846,7 @@ int init_worktree_config(struct repository *r)\n \t * Ensure that we use worktree config for the remaining lifetime\n \t * of the current process.\n \t */\n-\trepository_format_worktree_config = 1;\n+\tr->repository_format_worktree_config = 1;\n \n cleanup:\n \tgit_configset_clear(&cs);\n-- \ngitgitgadget\n"},{"id":"477736","messageId":"3145f4f3-7bd4-8a1b-4943-11b7d22b60c6@github.com","threadId":"59785","inReplyTo":"pull.1536.v2.git.1685064781.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Fix behavior of worktree config in submodules","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-26T15:48:06Z","receivedAt":"2023-05-26T15:48:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/25/2023 9:32 PM, Victoria Dye via GitGitGadget wrote:\n> About a year ago, discussion on the sparse index integration of 'git grep'\n> surfaced larger incompatibilities between sparse-checkout and submodules\n> [1]. This series fixes one of the underlying issues to that incompatibility,\n> which is that the worktree config of the submodule (where\n> 'core.sparseCheckout', 'core.sparseCheckoutCone', and 'index.sparse' are\n> set) is not used when operating on the submodule from its super project\n> (e.g., in a command with '--recurse-submodules').\n> \n> The outcome of this series is that 'extensions.worktreeConfig' and the\n> contents of the repository's worktree config are read and applied to (and\n> only to) the relevant repo when working in a super project/submodule setup.\n> This alone doesn't fix sparse-checkout/submodule interoperability; the\n> additional changes needed for that will be submitted in a later series. I'm\n> also hoping this will help (or at least not hurt) the work to avoid use of\n> global state in config parsing [2].\n> \n> \n> Changes since V1\n> ================\n> \n> * In 't3007', replaced manual 'git config'/'test_when_finished \"git config\n> --unset\"' pairs with 'test_config' helper. Updated 'test_config' to\n> handle the '--worktree' option.\n> * Updated commit messages & test comments to better explain the purpose and\n> more subtle functionality details to the new tests\n> * Added a commit to move 'struct repository' out of 'git_config_source',\n> rather than creating a dummy 'config_source' just to hold a repository\n> instance.\n> * Changed the config setting in the new tests from 'feature.experimental'\n> to 'index.sparse' to tie these changes to their intended use case.\n> * \"super project\" -> \"superproject\"\n\nThanks for these updates. I'm happy with this version.\n\nThanks,\n-Stolee\n"},{"id":"477838","messageId":"kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"506a2cf8c73549bc8f9761b56532ef08ed220da4.1685064781.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-31T22:17:01Z","receivedAt":"2023-05-31T22:17:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"The changes that replace repository_format_worktree_config with \"struct\nrepository\".worktree_config look trivially good. The \"struct\nrepository_format\" bits track ebaf3bcf1ae (repository: move global\nr_f_p_c to repo struct, 2021-06-17) so it preserves the status quo, but\nI have some questions about ebaf3bcf1ae. Cc-ing the author (Jonathan\nTan) for context.\n\nRearranging the hunks for clarity,\n\n\"Victoria Dye via GitGitGadget\" writes:\n\n> @@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t\t}\n> \t\tif (startup_info->have_repository) {\n> \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n> +\t\t\tthe_repository->repository_format_worktree_config =\n> +\t\t\t\trepo_fmt.worktree_config;\n> \t\t\t/* take ownership of repo_fmt.partial_clone */\n> \t\t\tthe_repository->repository_format_partial_clone =\n> \t\t\t\trepo_fmt.partial_clone;\n\n[snip]\n\n> @@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt)\n> \tcheck_repository_format_gently(get_git_dir(), fmt, NULL);\n> \tstartup_info->have_repository = 1;\n> \trepo_set_hash_algo(the_repository, fmt->hash_algo);\n> +\tthe_repository->repository_format_worktree_config =\n> +\t\tfmt->worktree_config;\n> \tthe_repository->repository_format_partial_clone =\n> \t\txstrdup_or_null(fmt->partial_clone);\n> \tclear_repository_format(&repo_fmt);\n\n[snip]\n\n> diff --git a/repository.c b/repository.c\n> index c53e480e326..104960f8f59 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -182,6 +182,7 @@ int repo_init(struct repository *repo,\n> \t\tgoto error;\n> \n> \trepo_set_hash_algo(repo, format.hash_algo);\n> +\trepo->repository_format_worktree_config = format.worktree_config;\n> \n> \t/* take ownership of format.partial_clone */\n> \trepo->repository_format_partial_clone = format.partial_clone;\n\nThis patch adds another instance of copying fields from \"struct\nrepository_format\" to \"struct repository\", so I think that we should\nstart doing this with a helper function instead of copy-pasting the\nlogic.\n\nAs for what should be in the helper function, the above hunks suggest\nthat we should copy .hash_algo, .partial_clone, and .worktree_config.\nHowever...\n\n> @@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,\n> \t\treturn -1;\n> \t}\n> \n> +\tthe_repository->repository_format_worktree_config =\n> +\t\tcandidate.worktree_config;\n> +\n> \t/* take ownership of candidate.partial_clone */\n> \tthe_repository->repository_format_partial_clone =\n> \t\tcandidate.partial_clone;\n\nThis hunk does not copy .hash_algo. I initially wondered if it is safe\nto just copy .hash_algo here too, but I now suspect that we shouldn't\nhave done the_repository setup in discover_git_directory() in the first\nplace. It isn't used by the setup.c machinery - its one caller in \"git\"\n(it's used by \"scalar\") is read_early_config(), which is supposed to\nwork without a fully set up repository, and bears a comment saying that\n\"no global state is changed\" by calling discover_git_directory() (which\nstopped being true in ebaf3bcf1ae). It looks like\ndiscover_git_directory() is just a lightweight entrypoint into the\nsetup.c machinery. 16ac8b8db6 (setup: introduce the\ndiscover_git_directory() function, 2017-03-13)) says \"Let's just provide\na convenient wrapper function with an easier signature that *just*\ndiscovers the .git/ directory. We will use it in a subsequent patch to\nfix the early config.\"\n\nIf I'm wrong and we _should_ be doing the_repository setup, then I'm\nguessing it's safe to copy .hash_algo here too. So either way, I think\nwe should introduce a helper function to do the copying, especially\nbecause we will probably need to repeat this process yet again for\n\"repository_format_precious_objects\".\n"},{"id":"477848","messageId":"xmqqedmveqs2.fsf@gitster.g","threadId":"59785","inReplyTo":"kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-01T04:43:25Z","receivedAt":"2023-06-01T04:43:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n>> @@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir,\n>> \t\treturn -1;\n>> \t}\n>> \n>> +\tthe_repository->repository_format_worktree_config =\n>> +\t\tcandidate.worktree_config;\n>> +\n>> \t/* take ownership of candidate.partial_clone */\n>> \tthe_repository->repository_format_partial_clone =\n>> \t\tcandidate.partial_clone;\n>\n> This hunk does not copy .hash_algo. I initially wondered if it is safe\n> to just copy .hash_algo here too, but I now suspect that we shouldn't\n> have done the_repository setup in discover_git_directory() in the first\n> place.\n\nThat's quite a departure from the established practice, isn't it?\nDue to recent and not so recent header shuffling (moving everything\nout of cache.h, dropping \"extern\", etc.), \"git blame\" is a bit hard\nto follow, but ever since 16ac8b8d (setup: introduce the\ndiscover_git_directory() function, 2017-03-13) added the function,\nwe do execute the \"setup\" when we know we are in a repository.\n\nIt would probably be worth mentioning that the \"global state\" Dscho\nrefers to in that commit is primarily about the current directory of\nthe Git process. During the discovery, we used to go up one level\nat a time and tried to see if the current directory is either the\ntop of the working tree (i.e. has \".git/\" that is a git repository)\nor the top of a GIT_DIR-looking directory. That was changed in\nce9b8aab (setup_git_directory_1(): avoid changing global state,\n2017-03-13) in the same series and discusses what \"global state\" the\nseries addresses.\n\nIf a relatively recent and oddball caller calls the function when it\ndoes not want any of the setup donw after finding out that we could\nuse the directory as a repository, a new early \"pure discovery\" part\nshould be split out of the function, and both the function itself\nand the oddball caller should be taught to call that pure-discovery\nhelper, I think.\n\n> If I'm wrong and we _should_ be doing the_repository setup, then I'm\n> guessing it's safe to copy .hash_algo here too. So either way, I think\n> we should introduce a helper function to do the copying, especially\n> because we will probably need to repeat this process yet again for\n> \"repository_format_precious_objects\".\n\nI do not know (or care in the context of this thread) about the\n\"precious objects\" bit, but .worktree-config is the third one on top\nof .hash_algo and .partial_clone, and it generally is a good time to\nrefactor when you find yourself adding the third instance of\nrepetitive code. So I agree with you that it is time to introduce a\nhelper function to copy from a \"struct repository_format\" to the\nrepository instance.\n\n"},{"id":"478154","messageId":"49509708-c0a1-2439-a551-cab05d944b66@github.com","threadId":"59785","inReplyTo":"kl6lr0qwno2q.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-06-07T22:29:24Z","receivedAt":"2023-06-07T22:30:11Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Glen Choo wrote:\n> This patch adds another instance of copying fields from \"struct\n> repository_format\" to \"struct repository\", so I think that we should\n> start doing this with a helper function instead of copy-pasting the\n> logic.\n> \n> As for what should be in the helper function, the above hunks suggest\n> that we should copy .hash_algo, .partial_clone, and .worktree_config.\n> However...\n> \n\n...\n\n> \n> This hunk does not copy .hash_algo. I initially wondered if it is safe\n> to just copy .hash_algo here too, but I now suspect that we shouldn't\n> have done the_repository setup in discover_git_directory() in the first\n> place. It isn't used by the setup.c machinery - its one caller in \"git\"\n> (it's used by \"scalar\") is read_early_config(), which is supposed to\n> work without a fully set up repository, and bears a comment saying that\n> \"no global state is changed\" by calling discover_git_directory() (which\n> stopped being true in ebaf3bcf1ae). It looks like\n> discover_git_directory() is just a lightweight entrypoint into the\n> setup.c machinery. 16ac8b8db6 (setup: introduce the\n> discover_git_directory() function, 2017-03-13)) says \"Let's just provide\n> a convenient wrapper function with an easier signature that *just*\n> discovers the .git/ directory. We will use it in a subsequent patch to\n> fix the early config.\"\n> \n> If I'm wrong and we _should_ be doing the_repository setup, then I'm\n> guessing it's safe to copy .hash_algo here too. So either way, I think\n> we should introduce a helper function to do the copying, especially\n> because we will probably need to repeat this process yet again for\n> \"repository_format_precious_objects\".\n\nThanks for pointing this out & sharing your findings! \n\nI agree with the desire to reduce code duplication, but the reason I avoided\nthat refactor when putting these patches together is because of the subtle\ndifferences across the different repository format assignment blocks.\n\nFor example, in addition to what you mentioned here w.r.t. '.hash_algo',\nthere are also differences in how 'repository_format_partial_clone' is\nassigned: it's deep-copied in 'check_repository_format', but shallow-copied\n(then subsequently NULL'd in the 'struct repository_format' to avoid freeing\nthe pointer when the struct is disposed of) in 'discover_git_directory()' &\n'setup_git_directory_gently()'. \n\nIf we were to settle on a single \"copy repository format settings\" function,\nit's not obvious what the \"right\" approach is. We could change\n'check_repository_format()' to the shallow-copy-then-null like the others:\nits two callers (in 'init-db.c' and 'path.c') don't use the value of\n'repository_format_partial_clone' in 'struct repository_format' after\ncalling 'check_repository_format()'. But, if we did that, it'd introduce a\nside effect to the input 'struct repository_format', which IMO would be\nsurprising behavior for a function called 'check_()'. Conversely,\nunifying on a deep copy or adding a flag to toggle deep vs. shallow copy\nfeels like unnecessary complexity if we don't actually need a deep copy.\n\nBeyond the smaller subtleties, there's the larger question (that you sort of\nget at with the questions around 'discover_git_directory()') as to whether\nwe should more heavily refactor or consolidate these setup functions. The\nsimilar code implies \"yes\", but such a refactor feels firmly out-of-scope\nfor this series. A smaller change (e.g. just moving the assignments into\ntheir own function) could be less of a diversion, but any benefit seems like\nit'd be outweighed by the added churn/complexity of a new function.\n\nIn any case, sorry for the long-winded response. I'd initially tried to\nimplement your feedback, but every time I did I'd get stopped up on the\nthings I mentioned above. So, rather than continue to put off responding to\nthis thread, I tried to capture what kept stopping me from moving forward -\nhopefully it makes (at least a little bit of) sense!\n\n"},{"id":"478291","messageId":"kl6lttvcft59.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"49509708-c0a1-2439-a551-cab05d944b66@github.com","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-12T18:10:58Z","receivedAt":"2023-06-12T18:11:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Victoria Dye writes:\n\n> For example, in addition to what you mentioned here w.r.t. '.hash_algo',\n> there are also differences in how 'repository_format_partial_clone' is\n> assigned: it's deep-copied in 'check_repository_format', but shallow-copied\n> (then subsequently NULL'd in the 'struct repository_format' to avoid freeing\n> the pointer when the struct is disposed of) in 'discover_git_directory()' &\n> 'setup_git_directory_gently()'. \n\nThanks for the analysis and explanation. It's quite a pain that the\nvarious sites are similar but subtly different.\n\n> If we were to settle on a single \"copy repository format settings\" function,\n> it's not obvious what the \"right\" approach is. We could change\n> 'check_repository_format()' to the shallow-copy-then-null like the others:\n> its two callers (in 'init-db.c' and 'path.c') don't use the value of\n> 'repository_format_partial_clone' in 'struct repository_format' after\n> calling 'check_repository_format()'. But, if we did that, it'd introduce a\n> side effect to the input 'struct repository_format', which IMO would be\n> surprising behavior for a function called 'check_()'. Conversely,\n> unifying on a deep copy or adding a flag to toggle deep vs. shallow copy\n> feels like unnecessary complexity if we don't actually need a deep copy.\n>\n> Beyond the smaller subtleties, there's the larger question (that you sort of\n> get at with the questions around 'discover_git_directory()') as to whether\n> we should more heavily refactor or consolidate these setup functions. The\n> similar code implies \"yes\", but such a refactor feels firmly out-of-scope\n> for this series. A smaller change (e.g. just moving the assignments into\n> their own function) could be less of a diversion, but any benefit seems like\n> it'd be outweighed by the added churn/complexity of a new function.\n\nI don't agree that this refactor is out of scope. I think we agree that\nthe refactor is desirable, but if we apply the same heuristics in the\nfuture, the next author to copy a member from 'repository_format' to\n'repository' could do the same and we'd never end up with the refactor\nwe wanted. I strongly feel that if we don't put in a concerted effort\ninto such refactors along the way, we end up creating more of the churn\nthat made our lives harder in the first place.\n\nI sympathize with the 'out-of-scope' sentiment, though, and I find it\nfrustrating when a simple change starts growing in scope because a\nreviewer suggests fixing oddities in the codebase that I didn't think\nwere in scope. In that vein, I think the helper function can simplify\nthe in-scope things even if we punt on the difficult-to-reason-about\nparts.\n\nE.g. we could support both deep and shallow copying, like:\n\n /*\n * Copy members from a repository_format to repository.\n *\n * If 'src' will no longer be read after copying (e.g. it will be\n * cleared soon), pass a nonzero value so that pointer members will be\n * moved to 'dest' (NULL-ed and shallow copied) instead of being deep\n * copied.\n */\n void copy_repository_format(struct repository *dest,\n struct repository_format *src,\n int take_ownership);\n\nAnd in discover_git_directory(), where we don't copy .hash_algo, we\ncould leave the code as-is and put a FIXME to figure out if we should\nuse the helper function or drop the copying entirely.\n\n(I'm somewhat convinced that we can just do shallow copying, though.\nInspecting check_repository_format() shows that it calls\nclear_repository_format() right afterwards, so we really don't need the\ndeep copy there. Using shallow copying seems to work just fine here [1].\nI'll ping Jonathan Tan to see if there was a good reason to deep copy.)\n\n[1] https://github.com/chooglen/git/actions/runs/5246795137/jobs/9476098535\n\n> In any case, sorry for the long-winded response. I'd initially tried to\n> implement your feedback, but every time I did I'd get stopped up on the\n> things I mentioned above. So, rather than continue to put off responding to\n> this thread, I tried to capture what kept stopping me from moving forward -\n> hopefully it makes (at least a little bit of) sense!\n\nThanks for being receptive to the feedback in the first round. I really\nappreciate the response, and I agree that discussing this was a better\nway forward than being stuck.\n"},{"id":"478295","messageId":"dd21767c-7c66-cf42-1a64-954a069dc466@github.com","threadId":"59785","inReplyTo":"kl6lttvcft59.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-06-12T19:45:26Z","receivedAt":"2023-06-12T19:45:35Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Glen Choo wrote:\n> Victoria Dye writes:\n>> If we were to settle on a single \"copy repository format settings\" function,\n>> it's not obvious what the \"right\" approach is. We could change\n>> 'check_repository_format()' to the shallow-copy-then-null like the others:\n>> its two callers (in 'init-db.c' and 'path.c') don't use the value of\n>> 'repository_format_partial_clone' in 'struct repository_format' after\n>> calling 'check_repository_format()'. But, if we did that, it'd introduce a\n>> side effect to the input 'struct repository_format', which IMO would be\n>> surprising behavior for a function called 'check_()'. Conversely,\n>> unifying on a deep copy or adding a flag to toggle deep vs. shallow copy\n>> feels like unnecessary complexity if we don't actually need a deep copy.\n>>\n>> Beyond the smaller subtleties, there's the larger question (that you sort of\n>> get at with the questions around 'discover_git_directory()') as to whether\n>> we should more heavily refactor or consolidate these setup functions. The\n>> similar code implies \"yes\", but such a refactor feels firmly out-of-scope\n>> for this series. A smaller change (e.g. just moving the assignments into\n>> their own function) could be less of a diversion, but any benefit seems like\n>> it'd be outweighed by the added churn/complexity of a new function.\n> \n> I don't agree that this refactor is out of scope. I think we agree that\n> the refactor is desirable, but if we apply the same heuristics in the\n> future, the next author to copy a member from 'repository_format' to\n> 'repository' could do the same and we'd never end up with the refactor\n> we wanted. I strongly feel that if we don't put in a concerted effort\n> into such refactors along the way, we end up creating more of the churn\n> that made our lives harder in the first place.\n\nI don't actually agree that *this* refactor (that is, moving all the\n\"repository_format -> repository\" assignments into a helper function) is\ndesirable, though, even if we extrapolate to future updates. \n\nEven if we updated the only other 'repository_format' value\n('repository_format_precious_objects') to be copied the same way, the\nbenefit we'd get from eliminating a couple of lines of code duplication\nwouldn't necessarily outweigh the the extra complexity of a new abstraction\n- which may or may not need special-casing based on who's calling it -\nand/or the risk associated with changing behavior if we want to eliminate\nthose special cases. IOW, I don't feel it's a definitive net improvement in\nthis situation.\n\nWhat I do feel is desirable (although, to be honest, not particularly\nstrongly) is a larger refactor of 'setup.c' - including a deeper analysis\ninto the all the different setup functions we have, how they're used, why\nthey differ, etc. - to create a more unified setup process across all of\nGit. That might include a helper function like what you've described, but it\nmight not need one at all, hence my comment about \"added churn\" (it's just\nanother thing a larger refactor would need to work around). \n\n*That* refactor is what I was referring to as \"out-of-scope\" for this series\n(sorry I wasn't clearer earlier!), which - considering the goal is \"make\nsubmodules work with worktree config\" - doesn't seem unreasonable to me.\n\n> E.g. we could support both deep and shallow copying, like:\n> \n> /*\n> * Copy members from a repository_format to repository.\n> *\n> * If 'src' will no longer be read after copying (e.g. it will be\n> * cleared soon), pass a nonzero value so that pointer members will be\n> * moved to 'dest' (NULL-ed and shallow copied) instead of being deep\n> * copied.\n> */\n> void copy_repository_format(struct repository *dest,\n> struct repository_format *src,\n> int take_ownership);\n\nUnless we find that we *need* to support both, this approach would be more\nharmful than helpful. If it doesn't matter whether the copy is shallow or\ndeep, this design proliferates that meaningless distinction in a way that\ncan easily confuse developers (or at least create more work for them as try\nto try to understand it) if they ever want to change or use the function.\n\n"},{"id":"478297","messageId":"kl6lr0qgfmzo.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"dd21767c-7c66-cf42-1a64-954a069dc466@github.com","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-12T20:23:55Z","receivedAt":"2023-06-12T20:24:00Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Victoria Dye writes:\n\n> Even if we updated the only other 'repository_format' value\n> ('repository_format_precious_objects') to be copied the same way, the\n> benefit we'd get from eliminating a couple of lines of code duplication\n> wouldn't necessarily outweigh the the extra complexity of a new abstraction\n> - which may or may not need special-casing based on who's calling it -\n> and/or the risk associated with changing behavior if we want to eliminate\n> those special cases. IOW, I don't feel it's a definitive net improvement in\n> this situation.\n\nI see. In the process of doing this digging, I've become quite convinced\nthat the risk is minimal. I definitely want the refactor to happen, but\nI suppose it's not reasonable for you to bear the risk.\n\nI'll send a follow up patch on top of your series that implements the\ncleanup I hope to see, and I'd be happy to give _that_ series a\nReviewed-by (though it's a bit weird since one of the patches will be\nmine). It'll touch the same lines twice, but at least the patches will\nbe owned by the people who care about them the most.\n\n>> E.g. we could support both deep and shallow copying, like:\n>> \n>> /*\n>> * Copy members from a repository_format to repository.\n>> *\n>> * If 'src' will no longer be read after copying (e.g. it will be\n>> * cleared soon), pass a nonzero value so that pointer members will be\n>> * moved to 'dest' (NULL-ed and shallow copied) instead of being deep\n>> * copied.\n>> */\n>> void copy_repository_format(struct repository *dest,\n>> struct repository_format *src,\n>> int take_ownership);\n>\n> Unless we find that we *need* to support both, this approach would be more\n> harmful than helpful. If it doesn't matter whether the copy is shallow or\n> deep, this design proliferates that meaningless distinction in a way that\n> can easily confuse developers (or at least create more work for them as try\n> to try to understand it) if they ever want to change or use the function.\n\nFair enough. I agree we're better off figuring out if the need exists\nbefore trying to support it.\n"},{"id":"478310","messageId":"kl6lo7lkfjk9.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"xmqqedmveqs2.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-12T21:37:58Z","receivedAt":"2023-06-12T21:38:06Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> That's quite a departure from the established practice, isn't it?\n> Due to recent and not so recent header shuffling (moving everything\n> out of cache.h, dropping \"extern\", etc.), \"git blame\" is a bit hard\n> to follow, but ever since 16ac8b8d (setup: introduce the\n> discover_git_directory() function, 2017-03-13) added the function,\n> we do execute the \"setup\" when we know we are in a repository.\n>\n> It would probably be worth mentioning that the \"global state\" Dscho\n> refers to in that commit is primarily about the current directory of\n> the Git process. During the discovery, we used to go up one level\n> at a time and tried to see if the current directory is either the\n> top of the working tree (i.e. has \".git/\" that is a git repository)\n> or the top of a GIT_DIR-looking directory. That was changed in\n> ce9b8aab (setup_git_directory_1(): avoid changing global state,\n> 2017-03-13) in the same series and discusses what \"global state\" the\n> series addresses.\n>\n> If a relatively recent and oddball caller calls the function when it\n> does not want any of the setup donw after finding out that we could\n> use the directory as a repository, a new early \"pure discovery\" part\n> should be split out of the function, and both the function itself\n> and the oddball caller should be taught to call that pure-discovery\n> helper, I think.\n\nHm, isn't discover_git_directory() that pure-discovery helper? In the\nce9b8aab, Dscho created stateless machinery (setup_git_directory_1()),\nand in 16ac8b8d, he used that stateless machinery to create a\npure-discovery helper (discover_git_directory()).\n\nIt's true that the global state is primarily the cwd, but the spirit of\nthe change is the same - setup_git_directory_1() should have no global\nside effects and neither should discover_git_diretory() (since it's\nsupposed to only do discovery).\n"},{"id":"478317","messageId":"20230612230453.70864-1-chooglen@google.com","threadId":"59785","inReplyTo":"kl6lr0qgfmzo.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"[PATCH] setup: copy repository_format using helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-12T23:04:48Z","receivedAt":"2023-06-12T23:05:20Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"In several parts of the setup machinery, we set up a repository_format\nand then use it to set up the_repository in nearly the exact same way,\nsuggesting that we might be able to use a helper function to standardize\nthe behavior and make future modifications easier. Create this helper\nfunction, setup_repository_from_format(), thus standardizing this\nbehavior.\n\nTo determine what the 'standardized behavior' should be, we can compare\nthe candidate call sites in repo_init(), setup_git_directory_gently(),\ncheck_repository_format() and discover_git_directory(),\n\n- All of them copy .worktree_config.\n\n- All of them 'copy' .partial_clone. Most perform a shallow copy of the\n pointer, then set the .partial_clone = NULL so that it doesn't get\n cleared by clear_repository_format(). However,\n check_repository_format() copies the string deeply because the\n repository_format is sometimes read back (it is an \"out\" parameter).\n To accomodate both shallow copying and deep copying, toggle this\n behavior using the \"modify_fmt_ok\" parameter.\n\n- Most of them set up repository.hash_algo, except\n discover_git_directory(). Our helper function unconditionally sets up\n .hash_algo because it turns out that discover_git_directory() probably\n doesn't need to set up \"struct repository\" at all!\n discover_git_directory() isn't actually used in the setup process - its\n only caller in the Git binary is read_early_config(). As explained by\n 16ac8b8db6 (setup: introduce the discover_git_directory() function,\n 2017-03-13), it is supposed to be an entrypoint into setup.c machinery\n that allows the Git directory to be discovered without side effects,\n in other words, we shouldn't have introduced side effects in\n ebaf3bcf1ae (repository: move global r_f_p_c to repo struct,\n 2021-06-17). Fortunately, we didn't start to rely on this unintended\n behavior between then and now, so we can just drop it.\n\nSigned-off-by: Glen Choo \n---\nHere's the helper function I had in mind. I was initially mistaken and\nit turns out that we need to support deep copying, but fortunately,\nt0001 is extremely thorough and will catch virtually any mistake in the\nsetup process. CI seems to pass, though it appears to be a little flaky\ntoday and sometimes cancels jobs\n(https://github.com/chooglen/git/actions/runs/5249029150).\n\nIf you're comfortable with it, I would prefer for you to squash this\ninto your patches so that we don't just end up changing the same few\nlines. If not, I'll Reviewed-by your patches (if I don't find any other\nconcerns on a re-read) and send this as a 1-patch on top.\n\n repository.c | 7 +------\n setup.c | 31 +++++++++++++++++++------------\n setup.h | 10 ++++++++++\n 3 files changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/repository.c b/repository.c\nindex 104960f8f5..50f0b26a6c 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -181,12 +181,7 @@ int repo_init(struct repository *repo,\n \tif (read_and_verify_repository_format(&format, repo->commondir))\n \t\tgoto error;\n \n-\trepo_set_hash_algo(repo, format.hash_algo);\n-\trepo->repository_format_worktree_config = format.worktree_config;\n-\n-\t/* take ownership of format.partial_clone */\n-\trepo->repository_format_partial_clone = format.partial_clone;\n-\tformat.partial_clone = NULL;\n+\tsetup_repository_from_format(repo, &format, 1);\n \n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\ndiff --git a/setup.c b/setup.c\nindex d866395435..33ce58676f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1561,13 +1561,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\tsetup_git_env(gitdir);\n \t\t}\n \t\tif (startup_info->have_repository) {\n-\t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n-\t\t\tthe_repository->repository_format_worktree_config =\n-\t\t\t\trepo_fmt.worktree_config;\n-\t\t\t/* take ownership of repo_fmt.partial_clone */\n-\t\t\tthe_repository->repository_format_partial_clone =\n-\t\t\t\trepo_fmt.partial_clone;\n-\t\t\trepo_fmt.partial_clone = NULL;\n+\t\t\tsetup_repository_from_format(the_repository,\n+\t\t\t\t\t\t &repo_fmt, 1);\n \t\t}\n \t}\n \t/*\n@@ -1654,14 +1649,26 @@ void check_repository_format(struct repository_format *fmt)\n \t\tfmt = &repo_fmt;\n \tcheck_repository_format_gently(get_git_dir(), fmt, NULL);\n \tstartup_info->have_repository = 1;\n-\trepo_set_hash_algo(the_repository, fmt->hash_algo);\n-\tthe_repository->repository_format_worktree_config =\n-\t\tfmt->worktree_config;\n-\tthe_repository->repository_format_partial_clone =\n-\t\txstrdup_or_null(fmt->partial_clone);\n+\tsetup_repository_from_format(the_repository, fmt, 0);\n \tclear_repository_format(&repo_fmt);\n }\n \n+void setup_repository_from_format(struct repository *repo,\n+\t\t\t\t struct repository_format *fmt,\n+\t\t\t\t int modify_fmt_ok)\n+{\n+\trepo_set_hash_algo(repo, fmt->hash_algo);\n+\trepo->repository_format_worktree_config = fmt->worktree_config;\n+\tif (modify_fmt_ok) {\n+\t\trepo->repository_format_partial_clone =\n+\t\t\tfmt->partial_clone;\n+\t\tfmt->partial_clone = NULL;\n+\t} else {\n+\t\trepo->repository_format_partial_clone =\n+\t\t\txstrdup_or_null(fmt->partial_clone);\n+\t}\n+}\n+\n /*\n * Returns the \"prefix\", a path to the current working directory\n * relative to the work tree root, or NULL, if the current working\ndiff --git a/setup.h b/setup.h\nindex 4c1ca9d0c9..ed39aa38e0 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -140,6 +140,16 @@ int verify_repository_format(const struct repository_format *format,\n */\n void check_repository_format(struct repository_format *fmt);\n \n+/*\n+ * Setup a \"struct repository\" from the fields from the repository format.\n+ * If \"modify_fmt_ok\" is nonzero, pointer members in \"fmt\" will be shallowly\n+ * copied to repo and set to NULL (so that it's safe to clear \"fmt\").\n+ */\n+struct repository;\n+void setup_repository_from_format(struct repository *repo,\n+\t\t\t\t struct repository_format *fmt,\n+\t\t\t\t int modify_fmt_ok);\n+\n /*\n * NOTE NOTE NOTE!!\n *\n-- \n2.41.0.162.gfafddb0af9-goog\n\n"},{"id":"478319","messageId":"9fb6d7b1-00b6-93ee-efec-9dd0ab91a66d@github.com","threadId":"59785","inReplyTo":"20230612230453.70864-1-chooglen@google.com","subject":"Re: [PATCH] setup: copy repository_format using helper","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-06-13T00:03:17Z","receivedAt":"2023-06-13T00:03:24Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Glen Choo wrote:\n> In several parts of the setup machinery, we set up a repository_format\n> and then use it to set up the_repository in nearly the exact same way,\n> suggesting that we might be able to use a helper function to standardize\n> the behavior and make future modifications easier. Create this helper\n> function, setup_repository_from_format(), thus standardizing this\n> behavior.\n> \n> To determine what the 'standardized behavior' should be, we can compare\n> the candidate call sites in repo_init(), setup_git_directory_gently(),\n> check_repository_format() and discover_git_directory(),\n> \n> - All of them copy .worktree_config.\n> \n> - All of them 'copy' .partial_clone. Most perform a shallow copy of the\n> pointer, then set the .partial_clone = NULL so that it doesn't get\n> cleared by clear_repository_format(). However,\n> check_repository_format() copies the string deeply because the\n> repository_format is sometimes read back (it is an \"out\" parameter).\n> To accomodate both shallow copying and deep copying, toggle this\n> behavior using the \"modify_fmt_ok\" parameter.\n\nDo you have a specific example of this happening? I see two uses of\n'check_repository_format()' in the codebase:\n\n1. in 'enter_repo()' ('path.c')\n2. in 'init_db()' ('init-db.c')\n\nThe first one calls 'check_repository_format()' with 'NULL', which causes\nthe function to create a temporary 'struct repository_format' that is then\ndiscarded at the end of the function - no need to worry about the value\nbeing cleared there.\n\nThe second one does call 'check_repository_format()' with a 'struct\nrepository_format' instance, but the 'partial_clone' field field is not\naccessed again after that. The only subsequent usages of the 'repo_fmt'\nvariable in 'init_db()' are:\n\n- in 'validate_hash_algorithm()', where only the 'version' and 'hash_algo'\n fields are accessed.\n- in 'create_default_files()', where only 'hash_algo' is accessed.\n\nSo, shouldn't it be safe to shallow-copy-and-NULL? But as I noted earlier\n[1], if you do that it'll make the name 'check_repository_format()' a bit\nmisleading (since it's actually modifying its arg in place). So, if you\nupdate to always shallow copy, 'check_repository_format()' should be renamed\nto reflect its side effects.\n\n[1] https://lore.kernel.org/git/49509708-c0a1-2439-a551-cab05d944b66@github.com/\n\n> \n> - Most of them set up repository.hash_algo, except\n> discover_git_directory(). Our helper function unconditionally sets up\n> .hash_algo because it turns out that discover_git_directory() probably\n> doesn't need to set up \"struct repository\" at all!\n\nIf that's the case, shouldn't the 'repository_format' assignments in\n'discover_git_directory()' be removed altogether? \n\n> discover_git_directory() isn't actually used in the setup process - its\n> only caller in the Git binary is read_early_config(). As explained by\n> 16ac8b8db6 (setup: introduce the discover_git_directory() function,\n> 2017-03-13), it is supposed to be an entrypoint into setup.c machinery\n> that allows the Git directory to be discovered without side effects,\n> in other words, we shouldn't have introduced side effects in\n> ebaf3bcf1ae (repository: move global r_f_p_c to repo struct,\n> 2021-06-17). Fortunately, we didn't start to rely on this unintended\n> behavior between then and now, so we can just drop it.\n> \n> Signed-off-by: Glen Choo \n> ---\n> Here's the helper function I had in mind. I was initially mistaken and\n> it turns out that we need to support deep copying, but fortunately,\n> t0001 is extremely thorough and will catch virtually any mistake in the\n> setup process. CI seems to pass, though it appears to be a little flaky\n> today and sometimes cancels jobs\n> (https://github.com/chooglen/git/actions/runs/5249029150).\n> \n> If you're comfortable with it, I would prefer for you to squash this\n> into your patches so that we don't just end up changing the same few\n> lines. If not, I'll Reviewed-by your patches (if I don't find any other\n> concerns on a re-read) and send this as a 1-patch on top.\n\nReading through the commit message & patch, I'm still not convinced this\nrefactor is a good idea - to me, it doesn't leave the code in a clearly\nbetter state. If you feel strongly that it does, though, I'm happy to leave\nit to others to review/decide but I would prefer that you keep it a separate\npatch submission on top.\n\nThanks!\n\n> diff --git a/repository.c b/repository.c\n> index 104960f8f5..50f0b26a6c 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -181,12 +181,7 @@ int repo_init(struct repository *repo,\n> \tif (read_and_verify_repository_format(&format, repo->commondir))\n> \t\tgoto error;\n> \n> -\trepo_set_hash_algo(repo, format.hash_algo);\n> -\trepo->repository_format_worktree_config = format.worktree_config;\n> -\n> -\t/* take ownership of format.partial_clone */\n> -\trepo->repository_format_partial_clone = format.partial_clone;\n> -\tformat.partial_clone = NULL;\n> +\tsetup_repository_from_format(repo, &format, 1);\n> \n> \tif (worktree)\n> \t\trepo_set_worktree(repo, worktree);\n> diff --git a/setup.c b/setup.c\n> index d866395435..33ce58676f 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1561,13 +1561,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t\t\tsetup_git_env(gitdir);\n> \t\t}\n> \t\tif (startup_info->have_repository) {\n> -\t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n> -\t\t\tthe_repository->repository_format_worktree_config =\n> -\t\t\t\trepo_fmt.worktree_config;\n> -\t\t\t/* take ownership of repo_fmt.partial_clone */\n> -\t\t\tthe_repository->repository_format_partial_clone =\n> -\t\t\t\trepo_fmt.partial_clone;\n> -\t\t\trepo_fmt.partial_clone = NULL;\n> +\t\t\tsetup_repository_from_format(the_repository,\n> +\t\t\t\t\t\t &repo_fmt, 1);\n> \t\t}\n> \t}\n> \t/*\n> @@ -1654,14 +1649,26 @@ void check_repository_format(struct repository_format *fmt)\n> \t\tfmt = &repo_fmt;\n> \tcheck_repository_format_gently(get_git_dir(), fmt, NULL);\n> \tstartup_info->have_repository = 1;\n> -\trepo_set_hash_algo(the_repository, fmt->hash_algo);\n> -\tthe_repository->repository_format_worktree_config =\n> -\t\tfmt->worktree_config;\n> -\tthe_repository->repository_format_partial_clone =\n> -\t\txstrdup_or_null(fmt->partial_clone);\n> +\tsetup_repository_from_format(the_repository, fmt, 0);\n> \tclear_repository_format(&repo_fmt);\n> }\n> \n\nI think you may be missing changes to 'discover_git_directory()'? Like I\nmentioned above, though, if you don't think 'discover_git_directory()' needs\nto set up 'the_repository', then those assignments should just be removed\n(not replaced with 'setup_repository_from_format()').\n\n"},{"id":"478331","messageId":"kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"9fb6d7b1-00b6-93ee-efec-9dd0ab91a66d@github.com","subject":"Re: [PATCH] setup: copy repository_format using helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-13T18:25:51Z","receivedAt":"2023-06-13T18:25:58Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Victoria Dye writes:\n\n>> - All of them 'copy' .partial_clone. Most perform a shallow copy of the\n>> pointer, then set the .partial_clone = NULL so that it doesn't get\n>> cleared by clear_repository_format(). However,\n>> check_repository_format() copies the string deeply because the\n>> repository_format is sometimes read back (it is an \"out\" parameter).\n>> To accomodate both shallow copying and deep copying, toggle this\n>> behavior using the \"modify_fmt_ok\" parameter.\n>\n> Do you have a specific example of this happening? I see two uses of\n> 'check_repository_format()' in the codebase:\n>\n> 1. in 'enter_repo()' ('path.c')\n> 2. in 'init_db()' ('init-db.c')\n>\n> The first one calls 'check_repository_format()' with 'NULL', which causes\n> the function to create a temporary 'struct repository_format' that is then\n> discarded at the end of the function - no need to worry about the value\n> being cleared there.\n>\n> The second one does call 'check_repository_format()' with a 'struct\n> repository_format' instance, but the 'partial_clone' field field is not\n> accessed again after that. The only subsequent usages of the 'repo_fmt'\n> variable in 'init_db()' are:\n>\n> - in 'validate_hash_algorithm()', where only the 'version' and 'hash_algo'\n> fields are accessed.\n> - in 'create_default_files()', where only 'hash_algo' is accessed.\n>\n> So, shouldn't it be safe to shallow-copy-and-NULL? But as I noted earlier\n> [1], if you do that it'll make the name 'check_repository_format()' a bit\n> misleading (since it's actually modifying its arg in place). So, if you\n> update to always shallow copy, 'check_repository_format()' should be renamed\n> to reflect its side effects.\n\nMy understanding of check_repository_format() is that it serves double\nduty of doing a) setup of the_repository and b) populating an \"out\"\nparameter with the appropriate values. IMO a) is the side effect that\ncould warrant the rename, and b) is the expected, \"read-only\" use case.\nFrom that perspective, doing a shallow copy here isn't really\nintroducing a weird side-effect (because the arg to an \"out\" parameter\nshould be zero-ed out to begin with), but it's returning a 'wrong'\nvalue. You're right that it's safe because the NULL-ed value isn't read\nback right now, but it's not any good if this function gains more\ncallers.\n\nYour point about not having side effects in check_*() is a good one\nthough, and I'm starting to feel doubtful that we should be doing setup\nthere either....\n\n>> If you're comfortable with it, I would prefer for you to squash this\n>> into your patches so that we don't just end up changing the same few\n>> lines. If not, I'll Reviewed-by your patches (if I don't find any other\n>> concerns on a re-read) and send this as a 1-patch on top.\n>\n> Reading through the commit message & patch, I'm still not convinced this\n> refactor is a good idea - to me, it doesn't leave the code in a clearly\n> better state. If you feel strongly that it does, though, I'm happy to leave\n> it to others to review/decide but I would prefer that you keep it a separate\n> patch submission on top.\n\nOkay. Given how weird check_repository_format() and\ndiscover_git_directory() are, I think we haven't done enough\ninvestigation to properly consolidate this logic, and doing that\nintroduces quite a lot of scope creep. It feels very unsatisfactory that\nwe are propagating a pattern that is suspicious in some places and\noutright wrong in others instead of cleaning up as we go and leaving it\nin a better state for future authors, but this series does leave some\n_other_ parts in a better state (removing the global), and I think it's\nstill a net positive.\n\nThe helper function might not be a good idea yet, but I'm convinced that\nremoving the setup from discover_git_directory() is a good idea. I think\nthis series would be in a better state if we get rid of the wrong\npattern instead of extending it. Unfortunately, I forgot to include that\nchange in the patch I sent (ugh) but here's a patch that _just_ includes\nthe discover_git_directory() change that I hope you can squash into your\nseries (and you can use whatever bits of my commit message you see fit).\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\n diff --git a/setup.c b/setup.c\n index 33ce58676f..b172ffd48a 100644\n --- a/setup.c\n +++ b/setup.c\n @@ -1422,14 +1422,6 @@ int discover_git_directory(struct strbuf *commondir,\n return -1;\n }\n\n -\tthe_repository->repository_format_worktree_config =\n -\t\tcandidate.worktree_config;\n -\n -\t/* take ownership of candidate.partial_clone */\n -\tthe_repository->repository_format_partial_clone =\n -\t\tcandidate.partial_clone;\n -\tcandidate.partial_clone = NULL;\n -\n clear_repository_format(&candidate);\n return 0;\n }\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\nYou can see that this patch based on top of yours passes CI\n\n https://github.com/git/git/commit/9469fe3a6b0efbe89d26ef096a2eebabea59c55f\n https://github.com/chooglen/git/actions/runs/5258672473\n\n> I think you may be missing changes to 'discover_git_directory()'? Like I\n> mentioned above, though, if you don't think 'discover_git_directory()' needs\n> to set up 'the_repository', then those assignments should just be removed\n> (not replaced with 'setup_repository_from_format()').\n\nAh sorry, yes they were meant to be removed. I somehow missed those as I\nwas preparing the patch.\n\n"},{"id":"478338","messageId":"xmqqpm5z404p.fsf@gitster.g","threadId":"59785","inReplyTo":"kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] setup: copy repository_format using helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-13T19:45:26Z","receivedAt":"2023-06-13T19:45:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Victoria Dye writes:\n>\n>> So, shouldn't it be safe to shallow-copy-and-NULL? But as I noted earlier\n>> [1], if you do that it'll make the name 'check_repository_format()' a bit\n>> misleading (since it's actually modifying its arg in place). So, if you\n>> update to always shallow copy, 'check_repository_format()' should be renamed\n>> to reflect its side effects.\n>\n> My understanding of check_repository_format() is that it serves double\n> duty of doing a) setup of the_repository and b) populating an \"out\"\n> parameter with the appropriate values. IMO a) is the side effect that\n> could warrant the rename, and b) is the expected, \"read-only\" use case.\n>\n> From that perspective, doing a shallow copy here isn't really\n> introducing a weird side-effect (because the arg to an \"out\" parameter\n> should be zero-ed out to begin with), but it's returning a 'wrong'\n> value. You're right that it's safe because the NULL-ed value isn't read\n> back right now, but it's not any good if this function gains more\n> callers.\n\nThanks for having this discussion. The above makes perfect sense to\nme.\n\n> The helper function might not be a good idea yet, but I'm convinced that\n> removing the setup from discover_git_directory() is a good idea. I think\n> this series would be in a better state if we get rid of the wrong\n> pattern instead of extending it.\n> ...\n>> I think you may be missing changes to 'discover_git_directory()'? Like I\n>> mentioned above, though, if you don't think 'discover_git_directory()' needs\n>> to set up 'the_repository', then those assignments should just be removed\n>> (not replaced with 'setup_repository_from_format()').\n>\n> Ah sorry, yes they were meant to be removed. I somehow missed those as I\n> was preparing the patch.\n\nIt looks like you two are in agreement at the end. It does feel\nthat the change to make discover purely about discovering extends\nthe scope a bit too much, but it would be a good direction to go in\nthe longer term.\n\nThanks.\n"},{"id":"478340","messageId":"kl6lfs6v815k.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59785","inReplyTo":"pull.1536.v2.git.1685064781.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Fix behavior of worktree config in submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-13T22:09:43Z","receivedAt":"2023-06-13T22:10:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> * Added a commit to move 'struct repository' out of 'git_config_source',\n> rather than creating a dummy 'config_source' just to hold a repository\n> instance.\n> * Changed the config setting in the new tests from 'feature.experimental'\n> to 'index.sparse' to tie these changes to their intended use case.\n> * \"super project\" -> \"superproject\"\n\nThanks! Discounting the discussions on the side thread (which we've\ndecided are mostly out of scope) I think this version is good enough to\nmerge as-is.\n\nIn\n\n https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com\n\nI said that this series is better if we squash in a patch to drop the\nsetup code from discover_git_directory(), but on hindsight, I think it\nalso makes perfect sense for me to send that as a standalone patch. Let\nme know if you plan to squash it in or not so I'll know whether to send\nit :)\n"},{"id":"478341","messageId":"49b82157-0ecb-ad7b-be40-6ea10deec2fe@github.com","threadId":"59785","inReplyTo":"kl6lfs6v815k.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 0/3] Fix behavior of worktree config in submodules","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-06-13T22:17:28Z","receivedAt":"2023-06-13T22:17:35Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Glen Choo wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> * Added a commit to move 'struct repository' out of 'git_config_source',\n>> rather than creating a dummy 'config_source' just to hold a repository\n>> instance.\n>> * Changed the config setting in the new tests from 'feature.experimental'\n>> to 'index.sparse' to tie these changes to their intended use case.\n>> * \"super project\" -> \"superproject\"\n> \n> Thanks! Discounting the discussions on the side thread (which we've\n> decided are mostly out of scope) I think this version is good enough to\n> merge as-is.\n> \n> In\n> \n> https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com\n> \n> I said that this series is better if we squash in a patch to drop the\n> setup code from discover_git_directory(), but on hindsight, I think it\n> also makes perfect sense for me to send that as a standalone patch. Let\n> me know if you plan to squash it in or not so I'll know whether to send\n> it :)\n\nThanks for the re-review! This series was just merged to 'next', so I think\nsending the new patch separately would be the least disruptive option.\n"}]}