{"thread":{"id":"59914","subject":"[PATCH] config: don't BUG when both kvi and source are set","startedAt":"2023-06-26T17:42:56Z","lastAt":"2023-06-26T23:06:18Z","messageCount":4,"participants":["Glen Choo via GitGitGadget","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478833","messageId":"pull.1535.git.git.1687801297404.gitgitgadget@gmail.com","threadId":"59914","inReplyTo":null,"subject":"[PATCH] config: don't BUG when both kvi and source are set","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-26T17:41:37Z","receivedAt":"2023-06-26T17:42:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nWhen iterating through config, we read config source metadata from\nglobal values - either a \"struct config_source + enum config_scope\"\nor a \"struct key_value_info\", using the current_config* functions. Prior\nto the series starting from 0c60285147 (config.c: create config_reader\nand the_reader, 2023-03-28), we weren't very picky about which values we\nshould read in which situation; we did note that both groups of values\ngenerally shouldn't be set together, but if both were set,\ncurrent_config* preferentially reads key_value_info. When that series\nadded more structure, we enforced that either the former (when parsing a\nconfig source) can be set, or the latter (when iterating a config set),\nbut *never* both at the same time. See 9828453ff0 (config.c: remove\ncurrent_config_kvi, 2023-03-28) and 5cdf18e7cd (config.c: remove\ncurrent_parsing_scope, 2023-03-28).\n\nThat was a good simplifying constraint that helped us reason about the\nglobal state, but it turns out that there is at least one situation\nwhere we need both to be set at the same time: in a blobless partial\nclone where .gitmodules is missing. \"git fetch\" in such a repo will\nstart a config parse over .gitmodules (setting the config_source), and\nGit will attempt to lazy-fetch it from the promisor remote. However,\nwhen we try to read the promisor configuration, we start iterating a\nconfig set (setting the key_value_info), and we BUG() out because that's\nnot allowed any more.\n\nTeaching config_reader to gracefully handle this is somewhat\ncomplicated, but fortunately, there are proposed changes to the config.c\nmachinery to get rid of this global state, and make the BUG() obsolete\n[1]. We should rely on that as the eventual solution, and avoid doing\nyet another refactor in the meantime.\n\nTherefore, fix the bug by removing the BUG() check. We're reverting to\nan older, less safe state, but that's generally okay since\nkey_value_info is always preferentially read, so we'd always read the\ncorrect values when we iterate a config set in the middle of a config\nparse (like we are here). The reverse would be wrong, but extremely\nunlikely to happen since very few callers parse config without going\nthrough a config set.\n\n[1] https://lore.kernel.org/git/pull.1497.v3.git.git.1687290231.gitgitgadget@gmail.com\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    config: don't BUG when both kvi and source are set\n    \n    Here's a quick fix for the bug reported at [1]. As noted in the commit\n    message and that thread, I think the real fix to take [2], which\n    simplifies the config.c state and makes this a non-issue, so this is\n    just a band-aid while we wait for that.\n    \n    [1]\n    https://lore.kernel.org/git/CAJSLrw6qhHj8Kxrqhp7xN=imTHgg79QB9Fxa9XpdZYFnBKhkvA@mail.gmail.com/\n    [2]\n    https://lore.kernel.org/git/pull.1497.v3.git.git.1687290231.gitgitgadget@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1535%2Fchooglen%2Fpush-ppuusrxwqpkt-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1535/chooglen/push-ppuusrxwqpkt-v1\nPull-Request: https://github.com/git/git/pull/1535\n\n config.c                 |  6 ------\n t/t5616-partial-clone.sh | 10 ++++++++--\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f5bdac0aeed..3edb9d72dd3 100644\n--- a/config.c\n+++ b/config.c\n@@ -106,8 +106,6 @@ static struct config_reader the_reader;\n static inline void config_reader_push_source(struct config_reader *reader,\n \t\t\t\t\t     struct config_source *top)\n {\n-\tif (reader->config_kvi)\n-\t\tBUG(\"source should not be set while iterating a config set\");\n \ttop->prev = reader->source;\n \treader->source = top;\n }\n@@ -125,16 +123,12 @@ static inline struct config_source *config_reader_pop_source(struct config_reade\n static inline void config_reader_set_kvi(struct config_reader *reader,\n \t\t\t\t\t struct key_value_info *kvi)\n {\n-\tif (kvi && (reader->source || reader->parsing_scope))\n-\t\tBUG(\"kvi should not be set while parsing a config source\");\n \treader->config_kvi = kvi;\n }\n \n static inline void config_reader_set_scope(struct config_reader *reader,\n \t\t\t\t\t   enum config_scope scope)\n {\n-\tif (scope && reader->config_kvi)\n-\t\tBUG(\"scope should only be set when iterating through a config source\");\n \treader->parsing_scope = scope;\n }\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex f519d2a87a7..8759fc28533 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -257,8 +257,8 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 works with submod\n \ttest_commit -C submodule mycommit &&\n \n \ttest_create_repo src_with_sub &&\n-\ttest_config -C src_with_sub uploadpack.allowfilter 1 &&\n-\ttest_config -C src_with_sub uploadpack.allowanysha1inwant 1 &&\n+\tgit -C src_with_sub config uploadpack.allowfilter 1 &&\n+\tgit -C src_with_sub config uploadpack.allowanysha1inwant 1 &&\n \n \ttest_config_global protocol.file.allow always &&\n \n@@ -270,6 +270,12 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 works with submod\n \ttest_when_finished rm -rf dst\n '\n \n+test_expect_success 'lazily fetched .gitmodules works' '\n+\tgit clone --filter=\"blob:none\" --no-checkout \"file://$(pwd)/src_with_sub\" dst &&\n+\tgit -C dst fetch &&\n+\ttest_when_finished rm -rf dst\n+'\n+\n test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack --fsck-objects' '\n \tgit init src &&\n \ttest_commit -C src x &&\n\nbase-commit: 6640c2d06d112675426cf436f0594f0e8c614848\n-- \ngitgitgadget\n"},{"id":"478855","messageId":"xmqq352e59h9.fsf@gitster.g","threadId":"59914","inReplyTo":"pull.1535.git.git.1687801297404.gitgitgadget@gmail.com","subject":"Re: [PATCH] config: don't BUG when both kvi and source are set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-26T19:06:42Z","receivedAt":"2023-06-26T19:06:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Therefore, fix the bug by removing the BUG() check. We're reverting to\n> an older, less safe state, but that's generally okay since\n> key_value_info is always preferentially read, so we'd always read the\n> correct values when we iterate a config set in the middle of a config\n> parse (like we are here).\n\nI wonder if the source being pushed and config_kvi value at this\npoint have some particular relationship (like \"if kvi exists, the\nsource must match kvi's source\" or something) that we can cheaply\nuse to avoid \"reverting to an older less safe state\"?\n\nI would agree that, as long as we know by the end of this summer a\nreal fix would come to rescue us ;-), it is sensible not to add too\nmuch code to work it around for the short-term.\n\n> The reverse would be wrong, but extremely\n> unlikely to happen since very few callers parse config without going\n> through a config set.\n\nSorry, but I do not quite get this comment.\n\n>     Here's a quick fix for the bug reported at [1]. As noted in the commit\n>     message and that thread, I think the real fix to take [2], which\n>     simplifies the config.c state and makes this a non-issue, so this is\n>     just a band-aid while we wait for that.\n\nThanks for a quick fix.  Will queue.\n\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index f519d2a87a7..8759fc28533 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -257,8 +257,8 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 works with submod\n>  \ttest_commit -C submodule mycommit &&\n>  \n>  \ttest_create_repo src_with_sub &&\n> -\ttest_config -C src_with_sub uploadpack.allowfilter 1 &&\n> -\ttest_config -C src_with_sub uploadpack.allowanysha1inwant 1 &&\n> +\tgit -C src_with_sub config uploadpack.allowfilter 1 &&\n> +\tgit -C src_with_sub config uploadpack.allowanysha1inwant 1 &&\n\nWe only tentatively configured uploadpack in src_with_sub in the\noriginal because this single test piece was the only place where\nsrc_with_sub repository was used, but now we use a more permanent\nconfiguration because ...\n\n> @@ -270,6 +270,12 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 works with submod\n>  \ttest_when_finished rm -rf dst\n>  '\n>  \n> +test_expect_success 'lazily fetched .gitmodules works' '\n> +\tgit clone --filter=\"blob:none\" --no-checkout \"file://$(pwd)/src_with_sub\" dst &&\n> +\tgit -C dst fetch &&\n> +\ttest_when_finished rm -rf dst\n> +'\n\n... we run another \"git clone\" from the repository now.\n\nOK.\n\nThanks.\n"},{"id":"478862","messageId":"kl6l7crpsuhs.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59914","inReplyTo":"xmqq352e59h9.fsf@gitster.g","subject":"Re: [PATCH] config: don't BUG when both kvi and source are set","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-26T22:56:31Z","receivedAt":"2023-06-26T22:56:44Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Therefore, fix the bug by removing the BUG() check. We're reverting to\n>> an older, less safe state, but that's generally okay since\n>> key_value_info is always preferentially read, so we'd always read the\n>> correct values when we iterate a config set in the middle of a config\n>> parse (like we are here).\n>\n> I wonder if the source being pushed and config_kvi value at this\n> point have some particular relationship (like \"if kvi exists, the\n> source must match kvi's source\" or something) that we can cheaply\n> use to avoid \"reverting to an older less safe state\"?\n\nNot at all. In this case, the source should reflect .gitmodules, but the\nconfig_kvi should reflect the promisor config (aka the full repo\nconfig). config_source implements stack semantics, so we could co-opt it\nby e.g. converting config_kvi into a fake config_source and pushing it\nonto the stack (at which point, we could just get rid of config_kvi\naltogether too), but that's really way too much work for something that\nwill _hopefully_ go away soon.\n\n>> The reverse would be wrong, but extremely\n>> unlikely to happen since very few callers parse config without going\n>> through a config set.\n>\n> Sorry, but I do not quite get this comment.\n\nAh, I meant that this bug occurred because most users of config use\ngit_config()/repo_config() (a wrapper around config sets), so it's very\neasy to accidentally read repo config, e.g. in the middle of parsing\nconfig (config file -> config set). I'd imagine it might also be quite\neasy to read repo config while reading repo config (config set -> config\nset), which would make current_config_* return the wrong thing, but at\nleast it doesn't BUG().\n\nThe \"reverse\" case (config set -> config file) is very _unlikely_\nbecause very few places need to know about config files, so it's\nunlikely that we'd have an explicit call to parse a config file,\nespecially in the middle of reading repo config.\n"},{"id":"478863","messageId":"xmqqh6qt4yeh.fsf@gitster.g","threadId":"59914","inReplyTo":"kl6l7crpsuhs.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] config: don't BUG when both kvi and source are set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-26T23:05:58Z","receivedAt":"2023-06-26T23:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> Ah, I meant that this bug occurred because most users of config use\n> git_config()/repo_config() (a wrapper around config sets), so it's very\n> easy to accidentally read repo config, e.g. in the middle of parsing\n> config (config file -> config set). I'd imagine it might also be quite\n> easy to read repo config while reading repo config (config set -> config\n> set), which would make current_config_* return the wrong thing, but at\n> least it doesn't BUG().\n\nI think BUG() is better than silently computing a wrong result, but\nit would probably be much rare than the problem at hand, and with\nthe getting rid of global dependencies, it won't be an issue anymore,\nhopefull?  So it is good.\n\n> The \"reverse\" case (config set -> config file) is very _unlikely_\n> because very few places need to know about config files, so it's\n> unlikely that we'd have an explicit call to parse a config file,\n> especially in the middle of reading repo config.\n\nAs long as existing codepaths do not do that, it would be OK ;-)\n\nThanks.\n"}]}