{"thread":{"id":"55621","subject":"[PATCH 1/2] sparse-index.c: remove set_index_sparse_config()","startedAt":"2021-05-05T12:14:55Z","lastAt":"2021-05-17T17:31:40Z","messageCount":8,"participants":["Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"423658","messageId":"patch-1.2-6872fd6a59b-20210505T121028Z-avarab@gmail.com","threadId":"55621","inReplyTo":"cover-0.2-00000000000-20210505T121028Z-avarab@gmail.com","subject":"[PATCH 1/2] sparse-index.c: remove set_index_sparse_config()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:11:58Z","receivedAt":"2021-05-05T12:14:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the set_index_sparse_config() function by folding it into\nset_sparse_index_config(), which was its only user.\n\nSince 122ba1f7b52 (sparse-checkout: toggle sparse index from builtin,\n2021-03-30) the flow of this code hasn't made much sense, we'd get\n\"enabled\" in set_sparse_index_config(), proceed to call\nset_index_sparse_config() with it.\n\nThere we'd call prepare_repo_settings() and set\n\"repo->settings.sparse_index = 1\", only to needlessly call\nprepare_repo_settings() again in set_sparse_index_config() (where it\nwould early abort), and finally setting \"repo->settings.sparse_index =\nenabled\".\n\nInstead we can just call prepare_repo_settings() once, and set the\nvariable to \"enabled\" in the first place.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n sparse-index.c | 11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 6f21397e2ee..b0d5dc5f081 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -102,7 +102,7 @@ static int convert_to_sparse_rec(struct index_state *istate,\n \treturn num_converted - start_converted;\n }\n \n-static int set_index_sparse_config(struct repository *repo, int enable)\n+int set_sparse_index_config(struct repository *repo, int enable)\n {\n \tint res;\n \tchar *config_path = repo_git_path(repo, \"config.worktree\");\n@@ -111,15 +111,6 @@ static int set_index_sparse_config(struct repository *repo, int enable)\n \t\t\t\t\t    enable ? \"true\" : NULL);\n \tfree(config_path);\n \n-\tprepare_repo_settings(repo);\n-\trepo->settings.sparse_index = 1;\n-\treturn res;\n-}\n-\n-int set_sparse_index_config(struct repository *repo, int enable)\n-{\n-\tint res = set_index_sparse_config(repo, enable);\n-\n \tprepare_repo_settings(repo);\n \trepo->settings.sparse_index = enable;\n \treturn res;\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423659","messageId":"cover-0.2-00000000000-20210505T121028Z-avarab@gmail.com","threadId":"55621","inReplyTo":null,"subject":"[PATCH 0/2] sparse-index.c: minor prepare_repo_settings() fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:11:57Z","receivedAt":"2021-05-05T12:14:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A couple of minor fixes for the already-landed recent sparse-index.c\nwork. The sparse setup code is (re-)setting variables it's already set\nfor no reason, and re-doing work it doesn't need to.\n\nÆvar Arnfjörð Bjarmason (2):\n  sparse-index.c: remove set_index_sparse_config()\n  sparse-index.c: don't call prepare_repo_settings() twice in a row\n\n sparse-index.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423660","messageId":"patch-2.2-8bca02efcee-20210505T121028Z-avarab@gmail.com","threadId":"55621","inReplyTo":"cover-0.2-00000000000-20210505T121028Z-avarab@gmail.com","subject":"[PATCH 2/2] sparse-index.c: don't call prepare_repo_settings() twice in a row","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:11:59Z","receivedAt":"2021-05-05T12:14:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code added in 58300f47432 (sparse-index: add index.sparse\nconfig option, 2021-03-30) to only call prepare_repo_settings()\nonce. We know that our own set_sparse_index_config() has just finished\ncalling it, so we don't need to call it if we're acting on \"test_env\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n sparse-index.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex b0d5dc5f081..5bad05de645 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -133,11 +133,12 @@ int convert_to_sparse(struct index_state *istate)\n \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n \tif (test_env >= 0)\n \t\tset_sparse_index_config(istate->repo, test_env);\n+\telse\n+\t\tprepare_repo_settings(istate->repo);\n \n \t/*\n \t * Only convert to sparse if index.sparse is set.\n \t */\n-\tprepare_repo_settings(istate->repo);\n \tif (!istate->repo->settings.sparse_index)\n \t\treturn 0;\n \n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423683","messageId":"7954338a-bb44-1345-61b6-787782063c3c@gmail.com","threadId":"55621","inReplyTo":"patch-2.2-8bca02efcee-20210505T121028Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] sparse-index.c: don't call prepare_repo_settings() twice in a row","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-05T14:00:55Z","receivedAt":"2021-05-05T14:01:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/5/2021 8:11 AM, Ævar Arnfjörð Bjarmason wrote:\n> Change code added in 58300f47432 (sparse-index: add index.sparse\n> config option, 2021-03-30) to only call prepare_repo_settings()\n> once. We know that our own set_sparse_index_config() has just finished\n> calling it, so we don't need to call it if we're acting on \"test_env\".\n\nI'm not sure about the value here. prepare_repo_settings() returns\nquickly if the settings have already been prepared, so a second call\nis negligible in cost.\n\n> @@ -133,11 +133,12 @@ int convert_to_sparse(struct index_state *istate)\n>  \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n>  \tif (test_env >= 0)\n>  \t\tset_sparse_index_config(istate->repo, test_env);\n> +\telse\n> +\t\tprepare_repo_settings(istate->repo);\n\nThe change presented here to either call set_sparse_index_config()\n_or_ prepare_repo_settings() seems like it knows too much about\nhow set_sparse_index_config() works.\n\nThanks,\n-Stolee\n\n"},{"id":"423684","messageId":"a9f4ec53-f9f0-3522-6ad2-510b46605a53@gmail.com","threadId":"55621","inReplyTo":"patch-1.2-6872fd6a59b-20210505T121028Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] sparse-index.c: remove set_index_sparse_config()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-05T14:01:41Z","receivedAt":"2021-05-05T14:02:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/5/2021 8:11 AM, Ævar Arnfjörð Bjarmason wrote:\n> Remove the set_index_sparse_config() function by folding it into\n> set_sparse_index_config(), which was its only user.\n> \n> Since 122ba1f7b52 (sparse-checkout: toggle sparse index from builtin,\n> 2021-03-30) the flow of this code hasn't made much sense, we'd get\n> \"enabled\" in set_sparse_index_config(), proceed to call\n> set_index_sparse_config() with it.\n> \n> There we'd call prepare_repo_settings() and set\n> \"repo->settings.sparse_index = 1\", only to needlessly call\n> prepare_repo_settings() again in set_sparse_index_config() (where it\n> would early abort), and finally setting \"repo->settings.sparse_index =\n> enabled\".\n> \n> Instead we can just call prepare_repo_settings() once, and set the\n> variable to \"enabled\" in the first place.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  sparse-index.c | 11 +----------\n>  1 file changed, 1 insertion(+), 10 deletions(-)\n> \n> diff --git a/sparse-index.c b/sparse-index.c\n> index 6f21397e2ee..b0d5dc5f081 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -102,7 +102,7 @@ static int convert_to_sparse_rec(struct index_state *istate,\n>  \treturn num_converted - start_converted;\n>  }\n>  \n> -static int set_index_sparse_config(struct repository *repo, int enable)\n> +int set_sparse_index_config(struct repository *repo, int enable)\n>  {\n>  \tint res;\n>  \tchar *config_path = repo_git_path(repo, \"config.worktree\");\n> @@ -111,15 +111,6 @@ static int set_index_sparse_config(struct repository *repo, int enable)\n>  \t\t\t\t\t    enable ? \"true\" : NULL);\n>  \tfree(config_path);\n>  \n> -\tprepare_repo_settings(repo);\n> -\trepo->settings.sparse_index = 1;\n> -\treturn res;\n> -}\n> -\n> -int set_sparse_index_config(struct repository *repo, int enable)\n> -{\n> -\tint res = set_index_sparse_config(repo, enable);\n> -\n>  \tprepare_repo_settings(repo);\n>  \trepo->settings.sparse_index = enable;\n>  \treturn res;\n> \n\nThis cleanup makes sense to me. The previous code was confusing and\nthis is an improvement.\n\nThanks,\n-Stolee\n"},{"id":"423736","messageId":"xmqqwnsczfbn.fsf@gitster.g","threadId":"55621","inReplyTo":"7954338a-bb44-1345-61b6-787782063c3c@gmail.com","subject":"Re: [PATCH 2/2] sparse-index.c: don't call prepare_repo_settings() twice in a row","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T03:55:24Z","receivedAt":"2021-05-06T03:55:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 5/5/2021 8:11 AM, Ævar Arnfjörð Bjarmason wrote:\n>> Change code added in 58300f47432 (sparse-index: add index.sparse\n>> config option, 2021-03-30) to only call prepare_repo_settings()\n>> once. We know that our own set_sparse_index_config() has just finished\n>> calling it, so we don't need to call it if we're acting on \"test_env\".\n>\n> I'm not sure about the value here. prepare_repo_settings() returns\n> quickly if the settings have already been prepared, so a second call\n> is negligible in cost.\n>\n>> @@ -133,11 +133,12 @@ int convert_to_sparse(struct index_state *istate)\n>>  \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n>>  \tif (test_env >= 0)\n>>  \t\tset_sparse_index_config(istate->repo, test_env);\n>> +\telse\n>> +\t\tprepare_repo_settings(istate->repo);\n>\n> The change presented here to either call set_sparse_index_config()\n> _or_ prepare_repo_settings() seems like it knows too much about\n> how set_sparse_index_config() works.\n\nI tend to agree on both counts.\n\nThanks.\n"},{"id":"423759","messageId":"87v97wuto9.fsf@evledraar.gmail.com","threadId":"55621","inReplyTo":"7954338a-bb44-1345-61b6-787782063c3c@gmail.com","subject":"Re: [PATCH 2/2] sparse-index.c: don't call prepare_repo_settings() twice in a row","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-06T08:49:59Z","receivedAt":"2021-05-06T08:56:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 05 2021, Derrick Stolee wrote:\n\n> On 5/5/2021 8:11 AM, Ævar Arnfjörð Bjarmason wrote:\n>> Change code added in 58300f47432 (sparse-index: add index.sparse\n>> config option, 2021-03-30) to only call prepare_repo_settings()\n>> once. We know that our own set_sparse_index_config() has just finished\n>> calling it, so we don't need to call it if we're acting on \"test_env\".\n>\n> I'm not sure about the value here. prepare_repo_settings() returns\n> quickly if the settings have already been prepared, so a second call\n> is negligible in cost.\n\nI changed that while I was at it to make it easier to read, it's not an\noptimization. I.e. one wonders what the side-effect is of calling\nprepare_repo_settings() twice, discovers there isn't one...\n\n>> @@ -133,11 +133,12 @@ int convert_to_sparse(struct index_state *istate)\n>>  \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n>>  \tif (test_env >= 0)\n>>  \t\tset_sparse_index_config(istate->repo, test_env);\n>> +\telse\n>> +\t\tprepare_repo_settings(istate->repo);\n>\n> The change presented here to either call set_sparse_index_config()\n> _or_ prepare_repo_settings() seems like it knows too much about\n> how set_sparse_index_config() works.\n\nIt seems reasonable to assume that a function to set config has\ninitialized (or died, if it couldn't) enough of our config state to do\nits job.\n\nBesides, it's a few lines above the changed code in the same file. \n\nBut looking at this again 2/3 callers of set_sparse_index_config()\naren't checking the return value. Wouldn't something like [1] on top be\na good idea here?\n\nAlso, is GIT_TEST_SPARSE_INDEX=true itself supposed to work? Running the\ntest suite with it fails 3 test files for me, all /sparse/, i.e. tests\nthat (presumably) assume it's not already turned on by this code.\n\n1. \n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex a4bdd7c4940..bea1e7dd00e 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -280,8 +280,9 @@ static int set_config(enum sparse_checkout_mode mode)\n \t\t\t\t      \"core.sparseCheckoutCone\",\n \t\t\t\t      mode == MODE_CONE_PATTERNS ? \"true\" : NULL);\n \n-\tif (mode == MODE_NO_PATTERNS)\n-\t\tset_sparse_index_config(the_repository, 0);\n+\tif (mode == MODE_NO_PATTERNS &&\n+\t    set_sparse_index_config(the_repository, 0) < 0)\n+\t\tdie(_(\"could not set index.sparse=0\"));\n \n \treturn 0;\n }\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 5bad05de645..3938bcec962 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -131,10 +131,13 @@ int convert_to_sparse(struct index_state *istate)\n \t * index.sparse config variable to be on.\n \t */\n \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n-\tif (test_env >= 0)\n-\t\tset_sparse_index_config(istate->repo, test_env);\n-\telse\n+\tif (test_env >= 0) {\n+\t\tif (set_sparse_index_config(istate->repo, test_env) < 0)\n+\t\t\tdie(_(\"could not set index.sparse based on GIT_TEST_SPARSE_INDEX=%d\"),\n+\t\t\t    test_env);\n+\t} else {\n \t\tprepare_repo_settings(istate->repo);\n+\t}\n \n \t/*\n \t * Only convert to sparse if index.sparse is set.\n"},{"id":"424771","messageId":"a7580f20-1b81-36fb-9cb1-b23690fa9599@gmail.com","threadId":"55621","inReplyTo":"87v97wuto9.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] sparse-index.c: don't call prepare_repo_settings() twice in a row","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-17T17:31:37Z","receivedAt":"2021-05-17T17:31:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/6/2021 4:49 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, May 05 2021, Derrick Stolee wrote:\n> \n>> On 5/5/2021 8:11 AM, Ævar Arnfjörð Bjarmason wrote:\n>>> Change code added in 58300f47432 (sparse-index: add index.sparse\n>>> config option, 2021-03-30) to only call prepare_repo_settings()\n>>> once. We know that our own set_sparse_index_config() has just finished\n>>> calling it, so we don't need to call it if we're acting on \"test_env\".\n>>\n>> I'm not sure about the value here. prepare_repo_settings() returns\n>> quickly if the settings have already been prepared, so a second call\n>> is negligible in cost.\n> \n> I changed that while I was at it to make it easier to read, it's not an\n> optimization. I.e. one wonders what the side-effect is of calling\n> prepare_repo_settings() twice, discovers there isn't one...\n\nThis is typical of the \"prepare_\" pattern, such as prepare_packed_git()\nor prepare_commit_graph(). It's saying \"be sure this is initialized\"\nand that initialization only needs to happen once.\n \n>>> @@ -133,11 +133,12 @@ int convert_to_sparse(struct index_state *istate)\n>>>  \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n>>>  \tif (test_env >= 0)\n>>>  \t\tset_sparse_index_config(istate->repo, test_env);\n>>> +\telse\n>>> +\t\tprepare_repo_settings(istate->repo);\n>>\n>> The change presented here to either call set_sparse_index_config()\n>> _or_ prepare_repo_settings() seems like it knows too much about\n>> how set_sparse_index_config() works.\n> \n> It seems reasonable to assume that a function to set config has\n> initialized (or died, if it couldn't) enough of our config state to do\n> its job.\n> \n> Besides, it's a few lines above the changed code in the same file. \n> \n> But looking at this again 2/3 callers of set_sparse_index_config()\n> aren't checking the return value. Wouldn't something like [1] on top be\n> a good idea here?\n> \n> Also, is GIT_TEST_SPARSE_INDEX=true itself supposed to work? Running the\n> test suite with it fails 3 test files for me, all /sparse/, i.e. tests\n> that (presumably) assume it's not already turned on by this code.\n\nI haven't ensured that this works in all cases. I was going to try and\nfind a way to make the tests be better about having a meaningful data\nshape that applies to the sparse-index, but I should go and verify that\nthe tests work with this enabled.\n\n> \n> 1. \n> \n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index a4bdd7c4940..bea1e7dd00e 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -280,8 +280,9 @@ static int set_config(enum sparse_checkout_mode mode)\n>  \t\t\t\t      \"core.sparseCheckoutCone\",\n>  \t\t\t\t      mode == MODE_CONE_PATTERNS ? \"true\" : NULL);\n>  \n> -\tif (mode == MODE_NO_PATTERNS)\n> -\t\tset_sparse_index_config(the_repository, 0);\n> +\tif (mode == MODE_NO_PATTERNS &&\n> +\t    set_sparse_index_config(the_repository, 0) < 0)\n> +\t\tdie(_(\"could not set index.sparse=0\"));\n\nThis seems reasonable, because we are in a builtin saying \"please disable\nthe sparse-index\". Is there not an instance here that _enables_ it?\n\nI would change the string to be \"could not disable index.sparse\" or\nsomething similar.\n\n>  \treturn 0;\n>  }\n> diff --git a/sparse-index.c b/sparse-index.c\n> index 5bad05de645..3938bcec962 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -131,10 +131,13 @@ int convert_to_sparse(struct index_state *istate)\n>  \t * index.sparse config variable to be on.\n>  \t */\n>  \ttest_env = git_env_bool(\"GIT_TEST_SPARSE_INDEX\", -1);\n> -\tif (test_env >= 0)\n> -\t\tset_sparse_index_config(istate->repo, test_env);\n> -\telse\n> +\tif (test_env >= 0) {\n> +\t\tif (set_sparse_index_config(istate->repo, test_env) < 0)\n> +\t\t\tdie(_(\"could not set index.sparse based on GIT_TEST_SPARSE_INDEX=%d\"),\n> +\t\t\t    test_env);\n> +\t} else {\n>  \t\tprepare_repo_settings(istate->repo);\n> +\t}\n\nThis one, I'm not so sure. There might be reasons why the GIT_TEST_*\nvariable won't work to set the config and we don't want that to be\na reason the test fails. There is no need for a translated string\nhere, either.\n\nThanks,\n-Stolee\n"}]}