{"thread":{"id":"58615","subject":"[PATCH 0/2] [2.38 regression] config: respect includes in protected config","startedAt":"2022-10-12T19:43:41Z","lastAt":"2022-10-18T15:22:11Z","messageCount":13,"participants":["Glen Choo via GitGitGadget","Glen Choo","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"464746","messageId":"pull.1360.git.git.1665603814.gitgitgadget@gmail.com","threadId":"58615","inReplyTo":null,"subject":"[PATCH 0/2] [2.38 regression] config: respect includes in protected config","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-12T19:43:32Z","receivedAt":"2022-10-12T19:43:41Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Here's a fix for the regression reported in [1]. I'm quite confident that\nthe test suite is extensive enough to catch any unintended behavioral\nchanges, but I'd appreciate a second opinion.\n\nCc: Derrick Stolee derrickstolee@github.com, Taylor Blau me@ttaylorr.com,\nJohannes Schindelin Johannes.Schindelin@gmx.de, Junio C Hamano\ngitster@pobox.com\n\n[1]\nhttps://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com\n\nGlen Choo (2):\n t0033, t0035: test for included config\n config: respect includes in protected config\n\n config.c | 30 ++++++++----------------------\n t/t0033-safe-directory.sh | 9 +++++++++\n t/t0035-safe-bare-repository.sh | 9 +++++++++\n 3 files changed, 26 insertions(+), 22 deletions(-)\n\n\nbase-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v1\nPull-Request: https://github.com/git/git/pull/1360\n-- \ngitgitgadget\n"},{"id":"464747","messageId":"8c0f40aed7e7f7044b9233053ce3ab258f9b634f.1665603814.git.gitgitgadget@gmail.com","threadId":"58615","inReplyTo":"pull.1360.git.git.1665603814.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t0033, t0035: test for included config","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-12T19:43:33Z","receivedAt":"2022-10-12T19:43:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nProtected config should consider [include]-s. Add failing tests that\ndescribe the behavior we want; they will pass in the next commit.\n\nSigned-off-by: Glen Choo \n---\n t/t0033-safe-directory.sh | 9 +++++++++\n t/t0035-safe-bare-repository.sh | 9 +++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex aecb308cf66..720d6cdd60b 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '\n \texpect_rejected_dir\n '\n \n+test_expect_failure 'safe.directory in included file' '\n+\tcat >gitconfig-include <<-EOF &&\n+\t[safe]\n+\t\tdirectory = \"$(pwd)\"\n+\tEOF\n+\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n+\tgit status\n+'\n+\n test_done\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex ecbdc8238db..aa6a6a8c3fd 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -51,4 +51,13 @@ test_expect_success 'safe.bareRepository on the command line' '\n \t\t-c safe.bareRepository=all\n '\n \n+test_expect_failure 'safe.bareRepository in included file' '\n+\tcat >gitconfig-include <<-EOF &&\n+\t[safe]\n+\t\tbareRepository = explicit\n+\tEOF\n+\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n+\texpect_rejected -C outer-repo/bare-repo\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"464748","messageId":"0ff5b5741a519c63e65ef57d7d0b3148c38c1a52.1665603814.git.gitgitgadget@gmail.com","threadId":"58615","inReplyTo":"pull.1360.git.git.1665603814.gitgitgadget@gmail.com","subject":"[PATCH 2/2] config: respect includes in protected config","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-12T19:43:34Z","receivedAt":"2022-10-12T19:43:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nProtected config is implemented by reading a fixed set of paths,\nwhich ignores config [include]-s. Replace this implementation with a\ncall to config_with_options(), which handles [include]-s and saves us\nfrom duplicating the logic of 1) identifying which paths to read and 2)\nreading command line config.\n\nAs a result, git_configset_add_parameters() is unused, so remove it. It\nwas introduced alongside protected config in 5b3c650777 (config: learn\n`git_protected_config()`, 2022-07-14) as a way to handle command line\nconfig.\n\nSigned-off-by: Glen Choo \n---\n config.c | 30 ++++++++----------------------\n t/t0033-safe-directory.sh | 2 +-\n t/t0035-safe-bare-repository.sh | 2 +-\n 3 files changed, 10 insertions(+), 24 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..c157fb5ae3f 100644\n--- a/config.c\n+++ b/config.c\n@@ -2392,11 +2392,6 @@ int git_configset_add_file(struct config_set *cs, const char *filename)\n \treturn git_config_from_file(config_set_callback, filename, cs);\n }\n \n-int git_configset_add_parameters(struct config_set *cs)\n-{\n-\treturn git_config_from_parameters(config_set_callback, cs);\n-}\n-\n int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n {\n \tconst struct string_list *values = NULL;\n@@ -2641,24 +2636,15 @@ int repo_config_get_pathname(struct repository *repo,\n /* Read values into protected_config. */\n static void read_protected_config(void)\n {\n-\tchar *xdg_config = NULL, *user_config = NULL, *system_config = NULL;\n-\n+\tstruct config_options opts = {\n+\t\t.respect_includes = 1,\n+\t\t.ignore_repo = 1,\n+\t\t.ignore_worktree = 1,\n+\t\t.system_gently = 1,\n+\t};\n \tgit_configset_init(&protected_config);\n-\n-\tsystem_config = git_system_config();\n-\tgit_global_config(&user_config, &xdg_config);\n-\n-\tif (system_config)\n-\t\tgit_configset_add_file(&protected_config, system_config);\n-\tif (xdg_config)\n-\t\tgit_configset_add_file(&protected_config, xdg_config);\n-\tif (user_config)\n-\t\tgit_configset_add_file(&protected_config, user_config);\n-\tgit_configset_add_parameters(&protected_config);\n-\n-\tfree(system_config);\n-\tfree(xdg_config);\n-\tfree(user_config);\n+\tconfig_with_options(config_set_callback, &protected_config,\n+\t\t\t NULL, &opts);\n }\n \n void git_protected_config(config_fn_t fn, void *data)\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex 720d6cdd60b..dc3496897ab 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -71,7 +71,7 @@ test_expect_success 'safe.directory=*, but is reset' '\n \texpect_rejected_dir\n '\n \n-test_expect_failure 'safe.directory in included file' '\n+test_expect_success 'safe.directory in included file' '\n \tcat >gitconfig-include <<-EOF &&\n \t[safe]\n \t\tdirectory = \"$(pwd)\"\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex aa6a6a8c3fd..fa33839704b 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -51,7 +51,7 @@ test_expect_success 'safe.bareRepository on the command line' '\n \t\t-c safe.bareRepository=all\n '\n \n-test_expect_failure 'safe.bareRepository in included file' '\n+test_expect_success 'safe.bareRepository in included file' '\n \tcat >gitconfig-include <<-EOF &&\n \t[safe]\n \t\tbareRepository = explicit\n-- \ngitgitgadget\n"},{"id":"464749","messageId":"kl6l7d142593.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58615","inReplyTo":"pull.1360.git.git.1665603814.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] [2.38 regression] config: respect includes in protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-12T19:47:52Z","receivedAt":"2022-10-12T19:48:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Cc-ing folks again. Looks like I made a mistake initially, sorry for the\nnoise.\n\n\"Glen Choo via GitGitGadget\" writes:\n\n> Here's a fix for the regression reported in [1]. I'm quite confident that\n> the test suite is extensive enough to catch any unintended behavioral\n> changes, but I'd appreciate a second opinion.\n>\n> Cc: Derrick Stolee derrickstolee@github.com, Taylor Blau me@ttaylorr.com,\n> Johannes Schindelin Johannes.Schindelin@gmx.de, Junio C Hamano\n> gitster@pobox.com\n>\n> [1]\n> https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com\n>\n> Glen Choo (2):\n> t0033, t0035: test for included config\n> config: respect includes in protected config\n>\n> config.c | 30 ++++++++----------------------\n> t/t0033-safe-directory.sh | 9 +++++++++\n> t/t0035-safe-bare-repository.sh | 9 +++++++++\n> 3 files changed, 26 insertions(+), 22 deletions(-)\n>\n>\n> base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v1\n> Pull-Request: https://github.com/git/git/pull/1360\n> -- \n> gitgitgadget\n"},{"id":"464752","messageId":"xmqqv8ookbua.fsf@gitster.g","threadId":"58615","inReplyTo":"0ff5b5741a519c63e65ef57d7d0b3148c38c1a52.1665603814.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] config: respect includes in protected config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-12T20:48:13Z","receivedAt":"2022-10-12T20:48:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\nNot commenting on the code yet as I am in the middle of today's\nintegration run, but as I notice a bad pattern being followed, let\nme comment before it spreads too widely.\n\nThe \"add failing test first and then fix the code with flipping the\ntest to success\" is very much unwelcome. For whoever gets curious\nenough (me included when accepting posted patch), it is easy to\nrevert only the part of the commit outside t/ tentatively to see how\nthe original code breaks. Keeping the fix and protection of the fix\ntogether will avoid mistakes. The post context of the hunk that\nchanges test_expect_failure to test_expect_success does not cover\nthe test script, thereby hiding the body of the test that changes\nbehaviour while reviewing the patch text, which is another downside.\n\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index 720d6cdd60b..dc3496897ab 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -71,7 +71,7 @@ test_expect_success 'safe.directory=*, but is reset' '\n> \texpect_rejected_dir\n> '\n> \n> -test_expect_failure 'safe.directory in included file' '\n> +test_expect_success 'safe.directory in included file' '\n> \tcat >gitconfig-include <<-EOF &&\n> \t[safe]\n> \t\tdirectory = \"$(pwd)\"\n> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> index aa6a6a8c3fd..fa33839704b 100755\n> --- a/t/t0035-safe-bare-repository.sh\n> +++ b/t/t0035-safe-bare-repository.sh\n> @@ -51,7 +51,7 @@ test_expect_success 'safe.bareRepository on the command line' '\n> \t\t-c safe.bareRepository=all\n> '\n> \n> -test_expect_failure 'safe.bareRepository in included file' '\n> +test_expect_success 'safe.bareRepository in included file' '\n> \tcat >gitconfig-include <<-EOF &&\n> \t[safe]\n> \t\tbareRepository = explicit\n"},{"id":"464753","messageId":"xmqqmta0kbkh.fsf@gitster.g","threadId":"58615","inReplyTo":"pull.1360.git.git.1665603814.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] [2.38 regression] config: respect includes in protected config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-12T20:54:06Z","receivedAt":"2022-10-12T20:54:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> Here's a fix for the regression reported in [1]. I'm quite confident that\n> the test suite is extensive enough to catch any unintended behavioral\n> changes, but I'd appreciate a second opinion.\n\nThis hooks onto safe.directory that dates back to 2.30.4 or so, but\nthe topic gc/bare-repo-discovery appears only in 2.38 so we do not\nhave to backport anything beyond the current maintenance track.\nWhew ;-)\n\n> Cc: Derrick Stolee derrickstolee@github.com, Taylor Blau me@ttaylorr.com,\n> Johannes Schindelin Johannes.Schindelin@gmx.de, Junio C Hamano\n> gitster@pobox.com\n>\n> [1]\n> https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com\n>\n> Glen Choo (2):\n> t0033, t0035: test for included config\n> config: respect includes in protected config\n>\n> config.c | 30 ++++++++----------------------\n> t/t0033-safe-directory.sh | 9 +++++++++\n> t/t0035-safe-bare-repository.sh | 9 +++++++++\n> 3 files changed, 26 insertions(+), 22 deletions(-)\n>\n>\n> base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v1\n> Pull-Request: https://github.com/git/git/pull/1360\n"},{"id":"464770","messageId":"xmqqilkokaxj.fsf@gitster.g","threadId":"58615","inReplyTo":"0ff5b5741a519c63e65ef57d7d0b3148c38c1a52.1665603814.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] config: respect includes in protected config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-12T21:07:52Z","receivedAt":"2022-10-12T21:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> -int git_configset_add_parameters(struct config_set *cs)\n> -{\n> -\treturn git_config_from_parameters(config_set_callback, cs);\n> -}\n> -\n> int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n> {\n> \tconst struct string_list *values = NULL;\n> @@ -2641,24 +2636,15 @@ int repo_config_get_pathname(struct repository *repo,\n> /* Read values into protected_config. */\n> static void read_protected_config(void)\n> {\n> -\tchar *xdg_config = NULL, *user_config = NULL, *system_config = NULL;\n> -\n> +\tstruct config_options opts = {\n> +\t\t.respect_includes = 1,\n> +\t\t.ignore_repo = 1,\n> +\t\t.ignore_worktree = 1,\n> +\t\t.system_gently = 1,\n> +\t};\n> \tgit_configset_init(&protected_config);\n> -\n> -\tsystem_config = git_system_config();\n> -\tgit_global_config(&user_config, &xdg_config);\n> -\n> -\tif (system_config)\n> -\t\tgit_configset_add_file(&protected_config, system_config);\n> -\tif (xdg_config)\n> -\t\tgit_configset_add_file(&protected_config, xdg_config);\n> -\tif (user_config)\n> -\t\tgit_configset_add_file(&protected_config, user_config);\n> -\tgit_configset_add_parameters(&protected_config);\n> -\tfree(system_config);\n> -\tfree(xdg_config);\n> -\tfree(user_config);\n\nCompared to the above hand-crafted sequence, we seem to be a lot\nmore careful in config.c::do_git_config_sequence() with respect to\nerror checking, which is good. We can lose the custom helper to\nread from command line parameters, which is also nice.\n\n> +\tconfig_with_options(config_set_callback, &protected_config,\n> +\t\t\t NULL, &opts);\n> }\n\nVery nice, code reduction with an additional feature. I wish all\nfixes were like this.\n\n> config.c | 30 ++++++++----------------------\n> t/t0033-safe-directory.sh | 2 +-\n> t/t0035-safe-bare-repository.sh | 2 +-\n> 3 files changed, 10 insertions(+), 24 deletions(-)\n"},{"id":"464787","messageId":"kl6l4jw81yor.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58615","inReplyTo":"xmqqv8ookbua.fsf@gitster.g","subject":"Re: [PATCH 2/2] config: respect includes in protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-12T22:09:40Z","receivedAt":"2022-10-12T22:11:19Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> \"Glen Choo via GitGitGadget\" writes:\n>\n> Not commenting on the code yet as I am in the middle of today's\n> integration run, but as I notice a bad pattern being followed, let\n> me comment before it spreads too widely.\n>\n> The \"add failing test first and then fix the code with flipping the\n> test to success\" is very much unwelcome. For whoever gets curious\n> enough (me included when accepting posted patch), it is easy to\n> revert only the part of the commit outside t/ tentatively to see how\n> the original code breaks. Keeping the fix and protection of the fix\n> together will avoid mistakes. The post context of the hunk that\n> changes test_expect_failure to test_expect_success does not cover\n> the test script, thereby hiding the body of the test that changes\n> behaviour while reviewing the patch text, which is another downside.\n\nThanks for voicing this, and sorry. I tried this pattern specifically\nbecause I thought it make it easier to review for folks who don't touch\nt/, but I hadn't considered that the reverse might be preferred.\n"},{"id":"464788","messageId":"221013.86a660n08s.gmgdl@evledraar.gmail.com","threadId":"58615","inReplyTo":"8c0f40aed7e7f7044b9233053ce3ab258f9b634f.1665603814.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t0033, t0035: test for included config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-12T22:28:09Z","receivedAt":"2022-10-12T22:30:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 12 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> Protected config should consider [include]-s. Add failing tests that\n> describe the behavior we want; they will pass in the next commit.\n>\n> Signed-off-by: Glen Choo \n> ---\n> t/t0033-safe-directory.sh | 9 +++++++++\n> t/t0035-safe-bare-repository.sh | 9 +++++++++\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index aecb308cf66..720d6cdd60b 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '\n> \texpect_rejected_dir\n> '\n> \n> +test_expect_failure 'safe.directory in included file' '\n> +\tcat >gitconfig-include <<-EOF &&\n> +\t[safe]\n> +\t\tdirectory = \"$(pwd)\"\n\nHere you use $, so <<-EOF, not <<-\\EOF, Okey.\n\n> +test_expect_failure 'safe.bareRepository in included file' '\n> +\tcat >gitconfig-include <<-EOF &&\n> +\t[safe]\n> +\t\tbareRepository = explicit\n\nBut this one should use <<-\\EOF\n"},{"id":"464873","messageId":"pull.1360.v2.git.git.1665683027912.gitgitgadget@gmail.com","threadId":"58615","inReplyTo":"pull.1360.git.git.1665603814.gitgitgadget@gmail.com","subject":"[PATCH v2] config: respect includes in protected config","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-13T17:43:47Z","receivedAt":"2022-10-13T17:43:55Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nProtected config is implemented by reading a fixed set of paths,\nwhich ignores config [include]-s. Replace this implementation with a\ncall to config_with_options(), which handles [include]-s and saves us\nfrom duplicating the logic of 1) identifying which paths to read and 2)\nreading command line config.\n\nAs a result, git_configset_add_parameters() is unused, so remove it. It\nwas introduced alongside protected config in 5b3c650777 (config: learn\n`git_protected_config()`, 2022-07-14) as a way to handle command line\nconfig.\n\nSigned-off-by: Glen Choo \n---\n [2.38 regression] config: respect includes in protected config\n \n Thanks for the quick response, all :)\n \n Changes in v2:\n \n * Squash patches together (per Junio's comments)\n * Use <<-\\EOF (per Ævar's comments)\n \n Cc: Derrick Stolee derrickstolee@github.com Cc: Taylor Blau\n me@ttaylorr.com Cc: Johannes Schindelin Johannes.Schindelin@gmx.de Cc:\n Junio C Hamano gitster@pobox.com Cc: Ævar Arnfjörð Bjarmason\n avarab@gmail.com\n \n [1]\n https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v2\nPull-Request: https://github.com/git/git/pull/1360\n\nRange-diff vs v1:\n\n 1: 8c0f40aed7e < -: ----------- t0033, t0035: test for included config\n 2: 0ff5b5741a5 ! 1: 5c398a7f72a config: respect includes in protected config\n @@ t/t0033-safe-directory.sh: test_expect_success 'safe.directory=*, but is reset'\n \texpect_rejected_dir\n '\n \n --test_expect_failure 'safe.directory in included file' '\n +test_expect_success 'safe.directory in included file' '\n - \tcat >gitconfig-include <<-EOF &&\n - \t[safe]\n - \t\tdirectory = \"$(pwd)\"\n ++\tcat >gitconfig-include <<-EOF &&\n ++\t[safe]\n ++\t\tdirectory = \"$(pwd)\"\n ++\tEOF\n ++\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n ++\tgit status\n ++'\n ++\n + test_done\n \n ## t/t0035-safe-bare-repository.sh ##\n @@ t/t0035-safe-bare-repository.sh: test_expect_success 'safe.bareRepository on the command line' '\n \t\t-c safe.bareRepository=all\n '\n \n --test_expect_failure 'safe.bareRepository in included file' '\n +test_expect_success 'safe.bareRepository in included file' '\n - \tcat >gitconfig-include <<-EOF &&\n - \t[safe]\n - \t\tbareRepository = explicit\n ++\tcat >gitconfig-include <<-\\EOF &&\n ++\t[safe]\n ++\t\tbareRepository = explicit\n ++\tEOF\n ++\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n ++\texpect_rejected -C outer-repo/bare-repo\n ++'\n ++\n + test_done\n\n\n config.c | 30 ++++++++----------------------\n t/t0033-safe-directory.sh | 9 +++++++++\n t/t0035-safe-bare-repository.sh | 9 +++++++++\n 3 files changed, 26 insertions(+), 22 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..c157fb5ae3f 100644\n--- a/config.c\n+++ b/config.c\n@@ -2392,11 +2392,6 @@ int git_configset_add_file(struct config_set *cs, const char *filename)\n \treturn git_config_from_file(config_set_callback, filename, cs);\n }\n \n-int git_configset_add_parameters(struct config_set *cs)\n-{\n-\treturn git_config_from_parameters(config_set_callback, cs);\n-}\n-\n int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n {\n \tconst struct string_list *values = NULL;\n@@ -2641,24 +2636,15 @@ int repo_config_get_pathname(struct repository *repo,\n /* Read values into protected_config. */\n static void read_protected_config(void)\n {\n-\tchar *xdg_config = NULL, *user_config = NULL, *system_config = NULL;\n-\n+\tstruct config_options opts = {\n+\t\t.respect_includes = 1,\n+\t\t.ignore_repo = 1,\n+\t\t.ignore_worktree = 1,\n+\t\t.system_gently = 1,\n+\t};\n \tgit_configset_init(&protected_config);\n-\n-\tsystem_config = git_system_config();\n-\tgit_global_config(&user_config, &xdg_config);\n-\n-\tif (system_config)\n-\t\tgit_configset_add_file(&protected_config, system_config);\n-\tif (xdg_config)\n-\t\tgit_configset_add_file(&protected_config, xdg_config);\n-\tif (user_config)\n-\t\tgit_configset_add_file(&protected_config, user_config);\n-\tgit_configset_add_parameters(&protected_config);\n-\n-\tfree(system_config);\n-\tfree(xdg_config);\n-\tfree(user_config);\n+\tconfig_with_options(config_set_callback, &protected_config,\n+\t\t\t NULL, &opts);\n }\n \n void git_protected_config(config_fn_t fn, void *data)\ndiff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\nindex aecb308cf66..dc3496897ab 100755\n--- a/t/t0033-safe-directory.sh\n+++ b/t/t0033-safe-directory.sh\n@@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '\n \texpect_rejected_dir\n '\n \n+test_expect_success 'safe.directory in included file' '\n+\tcat >gitconfig-include <<-EOF &&\n+\t[safe]\n+\t\tdirectory = \"$(pwd)\"\n+\tEOF\n+\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n+\tgit status\n+'\n+\n test_done\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex ecbdc8238db..11c15a48aab 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -51,4 +51,13 @@ test_expect_success 'safe.bareRepository on the command line' '\n \t\t-c safe.bareRepository=all\n '\n \n+test_expect_success 'safe.bareRepository in included file' '\n+\tcat >gitconfig-include <<-\\EOF &&\n+\t[safe]\n+\t\tbareRepository = explicit\n+\tEOF\n+\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n+\texpect_rejected -C outer-repo/bare-repo\n+'\n+\n test_done\n\nbase-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164\n-- \ngitgitgadget\n"},{"id":"464876","messageId":"kl6ly1tjzisx.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58615","inReplyTo":"pull.1360.v2.git.git.1665683027912.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] config: respect includes in protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-13T18:21:02Z","receivedAt":"2022-10-13T18:27:29Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> Cc: Derrick Stolee derrickstolee@github.com Cc: Taylor Blau\n> me@ttaylorr.com Cc: Johannes Schindelin Johannes.Schindelin@gmx.de Cc:\n> Junio C Hamano gitster@pobox.com Cc: Ævar Arnfjörð Bjarmason\n> avarab@gmail.com\n\nAck, cc-ing folks again. I was quite certain I got it correct this time\nthough...\n"},{"id":"464983","messageId":"Y0nDGFVxC4ImbLFQ@coredump.intra.peff.net","threadId":"58615","inReplyTo":"pull.1360.v2.git.git.1665683027912.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] config: respect includes in protected config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-14T20:14:16Z","receivedAt":"2022-10-14T20:14:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 13, 2022 at 05:43:47PM +0000, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n> \n> Protected config is implemented by reading a fixed set of paths,\n> which ignores config [include]-s. Replace this implementation with a\n> call to config_with_options(), which handles [include]-s and saves us\n> from duplicating the logic of 1) identifying which paths to read and 2)\n> reading command line config.\n\nFWIW, this left me scratching my head for a moment, as I thought it was\nadvocating reading includes from unsafe sources (which, since they can\nspecify arbitrary paths, can cause various bits of mischief).\n\nBut looking at the patch, you mean that when reading potentially-unsafe\nsources, we'll skip the unsafe ones (like .git/config) entirely, but we\nfail to respect includes in the safe ones (like ~/.gitconfig)? That\nmakes sense.\n\n-Peff\n"},{"id":"465206","messageId":"s3q2n244-r05n-07n8-6902-16q098331421@tzk.qr","threadId":"58615","inReplyTo":"pull.1360.v2.git.git.1665683027912.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] config: respect includes in protected config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-10-18T14:36:41Z","receivedAt":"2022-10-18T15:22:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Glen,\n\nOn Thu, 13 Oct 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> Protected config is implemented by reading a fixed set of paths,\n> which ignores config [include]-s. Replace this implementation with a\n> call to config_with_options(), which handles [include]-s and saves us\n> from duplicating the logic of 1) identifying which paths to read and 2)\n> reading command line config.\n\nAfter reading the patch, I agree that this is the right thing to do.\n\nThank you,\nDscho\n\n>\n> As a result, git_configset_add_parameters() is unused, so remove it. It\n> was introduced alongside protected config in 5b3c650777 (config: learn\n> `git_protected_config()`, 2022-07-14) as a way to handle command line\n> config.\n>\n> Signed-off-by: Glen Choo \n> ---\n> [2.38 regression] config: respect includes in protected config\n>\n> Thanks for the quick response, all :)\n>\n> Changes in v2:\n>\n> * Squash patches together (per Junio's comments)\n> * Use <<-\\EOF (per Ævar's comments)\n>\n> Cc: Derrick Stolee derrickstolee@github.com Cc: Taylor Blau\n> me@ttaylorr.com Cc: Johannes Schindelin Johannes.Schindelin@gmx.de Cc:\n> Junio C Hamano gitster@pobox.com Cc: Ævar Arnfjörð Bjarmason\n> avarab@gmail.com\n>\n> [1]\n> https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v2\n> Pull-Request: https://github.com/git/git/pull/1360\n>\n> Range-diff vs v1:\n>\n> 1: 8c0f40aed7e < -: ----------- t0033, t0035: test for included config\n> 2: 0ff5b5741a5 ! 1: 5c398a7f72a config: respect includes in protected config\n> @@ t/t0033-safe-directory.sh: test_expect_success 'safe.directory=*, but is reset'\n> \texpect_rejected_dir\n> '\n>\n> --test_expect_failure 'safe.directory in included file' '\n> +test_expect_success 'safe.directory in included file' '\n> - \tcat >gitconfig-include <<-EOF &&\n> - \t[safe]\n> - \t\tdirectory = \"$(pwd)\"\n> ++\tcat >gitconfig-include <<-EOF &&\n> ++\t[safe]\n> ++\t\tdirectory = \"$(pwd)\"\n> ++\tEOF\n> ++\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n> ++\tgit status\n> ++'\n> ++\n> + test_done\n>\n> ## t/t0035-safe-bare-repository.sh ##\n> @@ t/t0035-safe-bare-repository.sh: test_expect_success 'safe.bareRepository on the command line' '\n> \t\t-c safe.bareRepository=all\n> '\n>\n> --test_expect_failure 'safe.bareRepository in included file' '\n> +test_expect_success 'safe.bareRepository in included file' '\n> - \tcat >gitconfig-include <<-EOF &&\n> - \t[safe]\n> - \t\tbareRepository = explicit\n> ++\tcat >gitconfig-include <<-\\EOF &&\n> ++\t[safe]\n> ++\t\tbareRepository = explicit\n> ++\tEOF\n> ++\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n> ++\texpect_rejected -C outer-repo/bare-repo\n> ++'\n> ++\n> + test_done\n>\n>\n> config.c | 30 ++++++++----------------------\n> t/t0033-safe-directory.sh | 9 +++++++++\n> t/t0035-safe-bare-repository.sh | 9 +++++++++\n> 3 files changed, 26 insertions(+), 22 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index cbb5a3bab74..c157fb5ae3f 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2392,11 +2392,6 @@ int git_configset_add_file(struct config_set *cs, const char *filename)\n> \treturn git_config_from_file(config_set_callback, filename, cs);\n> }\n>\n> -int git_configset_add_parameters(struct config_set *cs)\n> -{\n> -\treturn git_config_from_parameters(config_set_callback, cs);\n> -}\n> -\n> int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n> {\n> \tconst struct string_list *values = NULL;\n> @@ -2641,24 +2636,15 @@ int repo_config_get_pathname(struct repository *repo,\n> /* Read values into protected_config. */\n> static void read_protected_config(void)\n> {\n> -\tchar *xdg_config = NULL, *user_config = NULL, *system_config = NULL;\n> -\n> +\tstruct config_options opts = {\n> +\t\t.respect_includes = 1,\n> +\t\t.ignore_repo = 1,\n> +\t\t.ignore_worktree = 1,\n> +\t\t.system_gently = 1,\n> +\t};\n> \tgit_configset_init(&protected_config);\n> -\n> -\tsystem_config = git_system_config();\n> -\tgit_global_config(&user_config, &xdg_config);\n> -\n> -\tif (system_config)\n> -\t\tgit_configset_add_file(&protected_config, system_config);\n> -\tif (xdg_config)\n> -\t\tgit_configset_add_file(&protected_config, xdg_config);\n> -\tif (user_config)\n> -\t\tgit_configset_add_file(&protected_config, user_config);\n> -\tgit_configset_add_parameters(&protected_config);\n> -\n> -\tfree(system_config);\n> -\tfree(xdg_config);\n> -\tfree(user_config);\n> +\tconfig_with_options(config_set_callback, &protected_config,\n> +\t\t\t NULL, &opts);\n> }\n>\n> void git_protected_config(config_fn_t fn, void *data)\n> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh\n> index aecb308cf66..dc3496897ab 100755\n> --- a/t/t0033-safe-directory.sh\n> +++ b/t/t0033-safe-directory.sh\n> @@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '\n> \texpect_rejected_dir\n> '\n>\n> +test_expect_success 'safe.directory in included file' '\n> +\tcat >gitconfig-include <<-EOF &&\n> +\t[safe]\n> +\t\tdirectory = \"$(pwd)\"\n> +\tEOF\n> +\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n> +\tgit status\n> +'\n> +\n> test_done\n> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> index ecbdc8238db..11c15a48aab 100755\n> --- a/t/t0035-safe-bare-repository.sh\n> +++ b/t/t0035-safe-bare-repository.sh\n> @@ -51,4 +51,13 @@ test_expect_success 'safe.bareRepository on the command line' '\n> \t\t-c safe.bareRepository=all\n> '\n>\n> +test_expect_success 'safe.bareRepository in included file' '\n> +\tcat >gitconfig-include <<-\\EOF &&\n> +\t[safe]\n> +\t\tbareRepository = explicit\n> +\tEOF\n> +\tgit config --global --add include.path \"$(pwd)/gitconfig-include\" &&\n> +\texpect_rejected -C outer-repo/bare-repo\n> +'\n> +\n> test_done\n>\n> base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164\n> --\n> gitgitgadget\n>\n"}]}