{"thread":{"id":"53028","subject":"[PATCH] config: set pack.useSparse=true by default","startedAt":"2020-03-19T01:58:43Z","lastAt":"2020-03-20T21:15:00Z","messageCount":9,"participants":["Derrick Stolee via GitGitGadget","Jonathan Nieder","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393432","messageId":"pull.585.git.1584583110914.gitgitgadget@gmail.com","threadId":"53028","inReplyTo":null,"subject":"[PATCH] config: set pack.useSparse=true by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-19T01:58:30Z","receivedAt":"2020-03-19T01:58:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe pack.useSparse config option was introduced by 3d036eb0\n(pack-objects: create pack.useSparse setting, 2019-01-19) and was\nfirst available in v2.21.0. When enabled, the pack-objects process\nduring 'git push' will use a sparse tree walk when deciding which\ntrees and blobs to send to the remote. The algorithm was introduced\nby d5d2e93 (revision: implement sparse algorithm, 2019-01-16) and\nhas been in production use by VFS for Git since around that time.\nThe features.experimental config option also enabled pack.useSparse,\nso hopefully that has also increased exposure.\n\nIt is worth noting that pack.useSparse has a possibility of\nsending more objects across a push, but requires a special\narrangement of exact _copies_ across directories. There is a test\nin t5322-pack-objects-sparse.sh that demonstrates this possibility.\n\nSince the downside is unlikely but the upside is significant, set\nthe default value of pack.useSparse to true. Remove it from the\nset of options implied by features.experimental.\n\nSigned-off-by: Derrick Stolee \n---\n config: set pack.useSparse=true by default\n \n Here is a small patch to convert pack.useSparse to true by default. It's\n been released for over a year, so the feature is quite stable. I'm\n submitting this now to allow it to cook for a while during the next\n release cycle.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-585%2Fderrickstolee%2Fpack-use-sparse-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-585/derrickstolee/pack-use-sparse-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/585\n\n Documentation/config/feature.txt | 3 ---\n Documentation/config/pack.txt | 4 ++--\n repo-settings.c | 3 ++-\n 3 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 875f8c8a66f..4e3a5c0cebc 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -12,9 +12,6 @@ feature.experimental::\n \tsetting if you are interested in providing feedback on experimental\n \tfeatures. The new default values are:\n +\n-* `pack.useSparse=true` uses a new algorithm when constructing a pack-file\n-which can improve `git push` performance in repos with many files.\n-+\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n +\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 0dac5805816..837f1b16792 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -119,8 +119,8 @@ pack.useSparse::\n \tobjects. This can have significant performance benefits when\n \tcomputing a pack to send a small change. However, it is possible\n \tthat extra objects are added to the pack-file if the included\n-\tcommits contain certain types of direct renames. Default is `false`\n-\tunless `feature.experimental` is enabled.\n+\tcommits contain certain types of direct renames. Default is\n+\t`true`.\n \n pack.writeBitmaps (deprecated)::\n \tThis is a deprecated synonym for `repack.writeBitmaps`.\ndiff --git a/repo-settings.c b/repo-settings.c\nindex a703e407a3f..dc6817daa95 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -45,6 +45,8 @@ void prepare_repo_settings(struct repository *r)\n \n \tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n \t\tr->settings.pack_use_sparse = value;\n+\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n+\n \tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n@@ -52,7 +54,6 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n \t\tr->settings.fetch_write_commit_graph = value;\n \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n \t}\n\nbase-commit: 6c85aac65fb455af85745130ce35ddae4678db84\n-- \ngitgitgadget\n"},{"id":"393494","messageId":"20200319231332.GB129493@google.com","threadId":"53028","inReplyTo":"pull.585.git.1584583110914.gitgitgadget@gmail.com","subject":"Re: [PATCH] config: set pack.useSparse=true by default","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-03-19T23:13:32Z","receivedAt":"2020-03-19T23:13:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n\n> The pack.useSparse config option was introduced by 3d036eb0\n> (pack-objects: create pack.useSparse setting, 2019-01-19) and was\n> first available in v2.21.0. When enabled, the pack-objects process\n> during 'git push' will use a sparse tree walk when deciding which\n> trees and blobs to send to the remote. The algorithm was introduced\n> by d5d2e93 (revision: implement sparse algorithm, 2019-01-16) and\n> has been in production use by VFS for Git since around that time.\n> The features.experimental config option also enabled pack.useSparse,\n> so hopefully that has also increased exposure.\n>\n> It is worth noting that pack.useSparse has a possibility of\n> sending more objects across a push, but requires a special\n> arrangement of exact _copies_ across directories. There is a test\n> in t5322-pack-objects-sparse.sh that demonstrates this possibility.\n>\n> Since the downside is unlikely but the upside is significant, set\n> the default value of pack.useSparse to true. Remove it from the\n> set of options implied by features.experimental.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> Documentation/config/feature.txt | 3 ---\n> Documentation/config/pack.txt | 4 ++--\n> repo-settings.c | 3 ++-\n> 3 files changed, 4 insertions(+), 6 deletions(-)\n\nMakes sense. Thanks for writing it.\n\nShould this have a test?\n\n[...]\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -45,6 +45,8 @@ void prepare_repo_settings(struct repository *r)\n> \n> \tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n> \t\tr->settings.pack_use_sparse = value;\n> +\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n> +\n> \tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n> \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n> \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n> @@ -52,7 +54,6 @@ void prepare_repo_settings(struct repository *r)\n> \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n> \t\tr->settings.fetch_write_commit_graph = value;\n> \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n> -\t\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n> \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n> \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n> \t}\n> \n\nReviewed-by: Jonathan Nieder \n"},{"id":"393501","messageId":"7986b9d0-34cb-5a60-484d-ba268dad146a@gmail.com","threadId":"53028","inReplyTo":"20200319231332.GB129493@google.com","subject":"Re: [PATCH] config: set pack.useSparse=true by default","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-20T00:34:02Z","receivedAt":"2020-03-20T00:34:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/19/2020 7:13 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> Derrick Stolee wrote:\n> \n>> The pack.useSparse config option was introduced by 3d036eb0\n>> (pack-objects: create pack.useSparse setting, 2019-01-19) and was\n>> first available in v2.21.0. When enabled, the pack-objects process\n>> during 'git push' will use a sparse tree walk when deciding which\n>> trees and blobs to send to the remote. The algorithm was introduced\n>> by d5d2e93 (revision: implement sparse algorithm, 2019-01-16) and\n>> has been in production use by VFS for Git since around that time.\n>> The features.experimental config option also enabled pack.useSparse,\n>> so hopefully that has also increased exposure.\n>>\n>> It is worth noting that pack.useSparse has a possibility of\n>> sending more objects across a push, but requires a special\n>> arrangement of exact _copies_ across directories. There is a test\n>> in t5322-pack-objects-sparse.sh that demonstrates this possibility.\n>>\n>> Since the downside is unlikely but the upside is significant, set\n>> the default value of pack.useSparse to true. Remove it from the\n>> set of options implied by features.experimental.\n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> Documentation/config/feature.txt | 3 ---\n>> Documentation/config/pack.txt | 4 ++--\n>> repo-settings.c | 3 ++-\n>> 3 files changed, 4 insertions(+), 6 deletions(-)\n> \n> Makes sense. Thanks for writing it.\n> \n> Should this have a test?\n\nI suppose the test that demonstrates the difference in algorithm\nin t5322-pack-objects-sparse.sh could be adjusted to drop the\nexplicit config setting, which would demonstrate that the config\noption is being set correctly.\n\nWhile looking at that test, I see that we use --[no-]sparse\nexplicitly everywhere to avoid conflicts with the GIT_TEST_*\nvariable that enables the algorithm. This leads to two things\nI will do in v2 that I did not do here:\n\n1. Update the docs for \"git pack-objects\" because it doesn't\n reference that --no-sparse is an option. Point out that the\n new default is --sparse.\n\n2. Remove GIT_TEST_PACK_SPARSE which was used to test this sparse\n algorithm throughout the test suite.\n\nThanks,\n-Stolee\n"},{"id":"393521","messageId":"pull.585.v2.git.1584707247753.gitgitgadget@gmail.com","threadId":"53028","inReplyTo":"pull.585.git.1584583110914.gitgitgadget@gmail.com","subject":"[PATCH v2] config: set pack.useSparse=true by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-20T12:27:27Z","receivedAt":"2020-03-20T12:27:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe pack.useSparse config option was introduced by 3d036eb0\n(pack-objects: create pack.useSparse setting, 2019-01-19) and was\nfirst available in v2.21.0. When enabled, the pack-objects process\nduring 'git push' will use a sparse tree walk when deciding which\ntrees and blobs to send to the remote. The algorithm was introduced\nby d5d2e93 (revision: implement sparse algorithm, 2019-01-16) and\nhas been in production use by VFS for Git since around that time.\nThe features.experimental config option also enabled pack.useSparse,\nso hopefully that has also increased exposure.\n\nIt is worth noting that pack.useSparse has a possibility of\nsending more objects across a push, but requires a special\narrangement of exact _copies_ across directories. There is a test\nin t5322-pack-objects-sparse.sh that demonstrates this possibility.\nThis test uses the --sparse option to \"git pack-objects\" but we\ncan make it implied by the config value to demonstrate that the\ndefault value has changed.\n\nWhile updating that test, I noticed that the documentation did not\ninclude an option for --no-sparse, which is now more important than\nit was before.\n\nSince the downside is unlikely but the upside is significant, set\nthe default value of pack.useSparse to true. Remove it from the\nset of options implied by features.experimental.\n\nSigned-off-by: Derrick Stolee \n---\n config: set pack.useSparse=true by default\n \n Here is a small patch to convert pack.useSparse to true by default. It's\n been released for over a year, so the feature is quite stable. I'm\n submitting this now to allow it to cook for a while during the next\n release cycle.\n \n Thanks to Jonathan Nieder pointing out the test implications, I've added\n a patch to swap the role of GIT_TEST_PACK_SPARSE to test the other mode.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-585%2Fderrickstolee%2Fpack-use-sparse-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-585/derrickstolee/pack-use-sparse-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/585\n\nRange-diff vs v1:\n\n 1: 02e9a813126 ! 1: 60b5cc6f337 config: set pack.useSparse=true by default\n @@ -16,6 +16,13 @@\n sending more objects across a push, but requires a special\n arrangement of exact _copies_ across directories. There is a test\n in t5322-pack-objects-sparse.sh that demonstrates this possibility.\n + This test uses the --sparse option to \"git pack-objects\" but we\n + can make it implied by the config value to demonstrate that the\n + default value has changed.\n +\n + While updating that test, I noticed that the documentation did not\n + include an option for --no-sparse, which is now more important than\n + it was before.\n \n Since the downside is unlikely but the upside is significant, set\n the default value of pack.useSparse to true. Remove it from the\n @@ -52,6 +59,39 @@\n pack.writeBitmaps (deprecated)::\n \tThis is a deprecated synonym for `repack.writeBitmaps`.\n \n + diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\n + --- a/Documentation/git-pack-objects.txt\n + +++ b/Documentation/git-pack-objects.txt\n +@@\n + \t[--local] [--incremental] [--window=] [--depth=]\n + \t[--revs [--unpacked | --all]] [--keep-pack=]\n + \t[--stdout [--filter=] | base-name]\n +-\t[--shallow] [--keep-true-parents] [--sparse] < object-list\n ++\t[--shallow] [--keep-true-parents] [--[no-]sparse] < object-list\n + \n + \n + DESCRIPTION\n +@@\n + \tAdd --no-reuse-object if you want to force a uniform compression\n + \tlevel on all data no matter the source.\n + \n +---sparse::\n +-\tUse the \"sparse\" algorithm to determine which objects to include in\n ++--[no-]sparse::\n ++\tToggle the \"sparse\" algorithm to determine which objects to include in\n + \tthe pack, when combined with the \"--revs\" option. This algorithm\n + \tonly walks trees that appear in paths that introduce new objects.\n + \tThis can have significant performance benefits when computing\n + \ta pack to send a small change. However, it is possible that extra\n + \tobjects are added to the pack-file if the included commits contain\n +-\tcertain types of direct renames.\n ++\tcertain types of direct renames. If this option is not included,\n ++\tit defaults to the value of `pack.useSparse`, which is true unless\n ++\totherwise specified.\n + \n + --thin::\n + \tCreate a \"thin\" pack by omitting the common objects between a\n +\n diff --git a/repo-settings.c b/repo-settings.c\n --- a/repo-settings.c\n +++ b/repo-settings.c\n @@ -72,3 +112,24 @@\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n \t}\n +\n + diff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh\n + --- a/t/t5322-pack-objects-sparse.sh\n + +++ b/t/t5322-pack-objects-sparse.sh\n +@@\n + \ttest_cmp required_objects.txt nonsparse_required_objects.txt\n + '\n + \n ++# --sparse is enabled by default by pack.useSparse\n + test_expect_success 'sparse pack-objects' '\n + \tgit rev-parse\t\t\t\\\n + \t\ttopic1\t\t\t\\\n +@@\n + \t\ttopic1:f3\t\t\\\n + \t\ttopic1:f3/f4\t\t\\\n + \t\ttopic1:f3/f4/data.txt | sort >expect_sparse_objects.txt &&\n +-\tgit pack-objects --stdout --revs --sparse sparse.pack &&\n ++\tgit pack-objects --stdout --revs sparse.pack &&\n + \tgit index-pack -o sparse.idx sparse.pack &&\n + \tgit show-index sparse_objects.txt &&\n + \ttest_cmp expect_sparse_objects.txt sparse_objects.txt\n\n\n Documentation/config/feature.txt | 3 ---\n Documentation/config/pack.txt | 4 ++--\n Documentation/git-pack-objects.txt | 10 ++++++----\n repo-settings.c | 3 ++-\n t/t5322-pack-objects-sparse.sh | 3 ++-\n 5 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 875f8c8a66f..4e3a5c0cebc 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -12,9 +12,6 @@ feature.experimental::\n \tsetting if you are interested in providing feedback on experimental\n \tfeatures. The new default values are:\n +\n-* `pack.useSparse=true` uses a new algorithm when constructing a pack-file\n-which can improve `git push` performance in repos with many files.\n-+\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n +\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 0dac5805816..837f1b16792 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -119,8 +119,8 @@ pack.useSparse::\n \tobjects. This can have significant performance benefits when\n \tcomputing a pack to send a small change. However, it is possible\n \tthat extra objects are added to the pack-file if the included\n-\tcommits contain certain types of direct renames. Default is `false`\n-\tunless `feature.experimental` is enabled.\n+\tcommits contain certain types of direct renames. Default is\n+\t`true`.\n \n pack.writeBitmaps (deprecated)::\n \tThis is a deprecated synonym for `repack.writeBitmaps`.\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex fecdf2600cc..eaa2f2a4041 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t[--local] [--incremental] [--window=] [--depth=]\n \t[--revs [--unpacked | --all]] [--keep-pack=]\n \t[--stdout [--filter=] | base-name]\n-\t[--shallow] [--keep-true-parents] [--sparse] < object-list\n+\t[--shallow] [--keep-true-parents] [--[no-]sparse] < object-list\n \n \n DESCRIPTION\n@@ -196,14 +196,16 @@ depth is 4095.\n \tAdd --no-reuse-object if you want to force a uniform compression\n \tlevel on all data no matter the source.\n \n---sparse::\n-\tUse the \"sparse\" algorithm to determine which objects to include in\n+--[no-]sparse::\n+\tToggle the \"sparse\" algorithm to determine which objects to include in\n \tthe pack, when combined with the \"--revs\" option. This algorithm\n \tonly walks trees that appear in paths that introduce new objects.\n \tThis can have significant performance benefits when computing\n \ta pack to send a small change. However, it is possible that extra\n \tobjects are added to the pack-file if the included commits contain\n-\tcertain types of direct renames.\n+\tcertain types of direct renames. If this option is not included,\n+\tit defaults to the value of `pack.useSparse`, which is true unless\n+\totherwise specified.\n \n --thin::\n \tCreate a \"thin\" pack by omitting the common objects between a\ndiff --git a/repo-settings.c b/repo-settings.c\nindex a703e407a3f..dc6817daa95 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -45,6 +45,8 @@ void prepare_repo_settings(struct repository *r)\n \n \tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n \t\tr->settings.pack_use_sparse = value;\n+\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n+\n \tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n@@ -52,7 +54,6 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n \t\tr->settings.fetch_write_commit_graph = value;\n \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n \t}\ndiff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh\nindex 7124b5581a0..6e5d6bdb0a7 100755\n--- a/t/t5322-pack-objects-sparse.sh\n+++ b/t/t5322-pack-objects-sparse.sh\n@@ -105,6 +105,7 @@ test_expect_success 'non-sparse pack-objects' '\n \ttest_cmp required_objects.txt nonsparse_required_objects.txt\n '\n \n+# --sparse is enabled by default by pack.useSparse\n test_expect_success 'sparse pack-objects' '\n \tgit rev-parse\t\t\t\\\n \t\ttopic1\t\t\t\\\n@@ -112,7 +113,7 @@ test_expect_success 'sparse pack-objects' '\n \t\ttopic1:f3\t\t\\\n \t\ttopic1:f3/f4\t\t\\\n \t\ttopic1:f3/f4/data.txt | sort >expect_sparse_objects.txt &&\n-\tgit pack-objects --stdout --revs --sparse sparse.pack &&\n+\tgit pack-objects --stdout --revs sparse.pack &&\n \tgit index-pack -o sparse.idx sparse.pack &&\n \tgit show-index sparse_objects.txt &&\n \ttest_cmp expect_sparse_objects.txt sparse_objects.txt\n\nbase-commit: 6c85aac65fb455af85745130ce35ddae4678db84\n-- \ngitgitgadget\n"},{"id":"393523","messageId":"pull.585.v3.git.1584707890.gitgitgadget@gmail.com","threadId":"53028","inReplyTo":"pull.585.v2.git.1584707247753.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] config: set pack.useSparse=true by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-20T12:38:08Z","receivedAt":"2020-03-20T12:38:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is a small patch to convert pack.useSparse to true by default. It's\nbeen released for over a year, so the feature is quite stable. I'm\nsubmitting this now to allow it to cook for a while during the next release\ncycle.\n\nUPDATE IN V3: (I'm sorry for the rapid v3, I forgot to push the commit that\nincluded this bit about GIT_TEST_PACK_SPARSE.)\n\nThanks to Jonathan Nieder pointing out the test implications, I've added a\npatch to swap the role of GIT_TEST_PACK_SPARSE to test the other mode.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n config: set pack.useSparse=true by default\n pack-objects: flip the use of GIT_TEST_PACK_SPARSE\n\n Documentation/config/feature.txt | 3 ---\n Documentation/config/pack.txt | 4 ++--\n Documentation/git-pack-objects.txt | 10 ++++++----\n builtin/pack-objects.c | 4 ++--\n repo-settings.c | 3 ++-\n t/README | 6 +++---\n t/t5322-pack-objects-sparse.sh | 4 +++-\n 7 files changed, 18 insertions(+), 16 deletions(-)\n\n\nbase-commit: 6c85aac65fb455af85745130ce35ddae4678db84\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-585%2Fderrickstolee%2Fpack-use-sparse-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-585/derrickstolee/pack-use-sparse-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/585\n\nRange-diff vs v2:\n\n 1: 60b5cc6f337 = 1: 60b5cc6f337 config: set pack.useSparse=true by default\n -: ----------- > 2: 908d5c77c96 pack-objects: flip the use of GIT_TEST_PACK_SPARSE\n\n-- \ngitgitgadget\n"},{"id":"393524","messageId":"60b5cc6f337011a7f2d5a229a83df7b82638d421.1584707890.git.gitgitgadget@gmail.com","threadId":"53028","inReplyTo":"pull.585.v3.git.1584707890.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] config: set pack.useSparse=true by default","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-20T12:38:09Z","receivedAt":"2020-03-20T12:38:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe pack.useSparse config option was introduced by 3d036eb0\n(pack-objects: create pack.useSparse setting, 2019-01-19) and was\nfirst available in v2.21.0. When enabled, the pack-objects process\nduring 'git push' will use a sparse tree walk when deciding which\ntrees and blobs to send to the remote. The algorithm was introduced\nby d5d2e93 (revision: implement sparse algorithm, 2019-01-16) and\nhas been in production use by VFS for Git since around that time.\nThe features.experimental config option also enabled pack.useSparse,\nso hopefully that has also increased exposure.\n\nIt is worth noting that pack.useSparse has a possibility of\nsending more objects across a push, but requires a special\narrangement of exact _copies_ across directories. There is a test\nin t5322-pack-objects-sparse.sh that demonstrates this possibility.\nThis test uses the --sparse option to \"git pack-objects\" but we\ncan make it implied by the config value to demonstrate that the\ndefault value has changed.\n\nWhile updating that test, I noticed that the documentation did not\ninclude an option for --no-sparse, which is now more important than\nit was before.\n\nSince the downside is unlikely but the upside is significant, set\nthe default value of pack.useSparse to true. Remove it from the\nset of options implied by features.experimental.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/feature.txt | 3 ---\n Documentation/config/pack.txt | 4 ++--\n Documentation/git-pack-objects.txt | 10 ++++++----\n repo-settings.c | 3 ++-\n t/t5322-pack-objects-sparse.sh | 3 ++-\n 5 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 875f8c8a66f..4e3a5c0cebc 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -12,9 +12,6 @@ feature.experimental::\n \tsetting if you are interested in providing feedback on experimental\n \tfeatures. The new default values are:\n +\n-* `pack.useSparse=true` uses a new algorithm when constructing a pack-file\n-which can improve `git push` performance in repos with many files.\n-+\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n +\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 0dac5805816..837f1b16792 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -119,8 +119,8 @@ pack.useSparse::\n \tobjects. This can have significant performance benefits when\n \tcomputing a pack to send a small change. However, it is possible\n \tthat extra objects are added to the pack-file if the included\n-\tcommits contain certain types of direct renames. Default is `false`\n-\tunless `feature.experimental` is enabled.\n+\tcommits contain certain types of direct renames. Default is\n+\t`true`.\n \n pack.writeBitmaps (deprecated)::\n \tThis is a deprecated synonym for `repack.writeBitmaps`.\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex fecdf2600cc..eaa2f2a4041 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t[--local] [--incremental] [--window=] [--depth=]\n \t[--revs [--unpacked | --all]] [--keep-pack=]\n \t[--stdout [--filter=] | base-name]\n-\t[--shallow] [--keep-true-parents] [--sparse] < object-list\n+\t[--shallow] [--keep-true-parents] [--[no-]sparse] < object-list\n \n \n DESCRIPTION\n@@ -196,14 +196,16 @@ depth is 4095.\n \tAdd --no-reuse-object if you want to force a uniform compression\n \tlevel on all data no matter the source.\n \n---sparse::\n-\tUse the \"sparse\" algorithm to determine which objects to include in\n+--[no-]sparse::\n+\tToggle the \"sparse\" algorithm to determine which objects to include in\n \tthe pack, when combined with the \"--revs\" option. This algorithm\n \tonly walks trees that appear in paths that introduce new objects.\n \tThis can have significant performance benefits when computing\n \ta pack to send a small change. However, it is possible that extra\n \tobjects are added to the pack-file if the included commits contain\n-\tcertain types of direct renames.\n+\tcertain types of direct renames. If this option is not included,\n+\tit defaults to the value of `pack.useSparse`, which is true unless\n+\totherwise specified.\n \n --thin::\n \tCreate a \"thin\" pack by omitting the common objects between a\ndiff --git a/repo-settings.c b/repo-settings.c\nindex a703e407a3f..dc6817daa95 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -45,6 +45,8 @@ void prepare_repo_settings(struct repository *r)\n \n \tif (!repo_config_get_bool(r, \"pack.usesparse\", &value))\n \t\tr->settings.pack_use_sparse = value;\n+\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n+\n \tif (!repo_config_get_bool(r, \"feature.manyfiles\", &value) && value) {\n \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n@@ -52,7 +54,6 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n \t\tr->settings.fetch_write_commit_graph = value;\n \tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n \t}\ndiff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh\nindex 7124b5581a0..6e5d6bdb0a7 100755\n--- a/t/t5322-pack-objects-sparse.sh\n+++ b/t/t5322-pack-objects-sparse.sh\n@@ -105,6 +105,7 @@ test_expect_success 'non-sparse pack-objects' '\n \ttest_cmp required_objects.txt nonsparse_required_objects.txt\n '\n \n+# --sparse is enabled by default by pack.useSparse\n test_expect_success 'sparse pack-objects' '\n \tgit rev-parse\t\t\t\\\n \t\ttopic1\t\t\t\\\n@@ -112,7 +113,7 @@ test_expect_success 'sparse pack-objects' '\n \t\ttopic1:f3\t\t\\\n \t\ttopic1:f3/f4\t\t\\\n \t\ttopic1:f3/f4/data.txt | sort >expect_sparse_objects.txt &&\n-\tgit pack-objects --stdout --revs --sparse sparse.pack &&\n+\tgit pack-objects --stdout --revs sparse.pack &&\n \tgit index-pack -o sparse.idx sparse.pack &&\n \tgit show-index sparse_objects.txt &&\n \ttest_cmp expect_sparse_objects.txt sparse_objects.txt\n-- \ngitgitgadget\n\n"},{"id":"393525","messageId":"908d5c77c96feeeda74144447586ccdc2be4665e.1584707890.git.gitgitgadget@gmail.com","threadId":"53028","inReplyTo":"pull.585.v3.git.1584707890.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] pack-objects: flip the use of GIT_TEST_PACK_SPARSE","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-20T12:38:10Z","receivedAt":"2020-03-20T12:38:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe environment variable GIT_TEST_PACK_SPARSE was previously used\nto allow testing the --sparse option for \"git pack-objects\" in\nthe test suite. This allowed interesting cases of \"git push\" to\nalso test this algorithm.\n\nSince pack.useSparse is now true by default, we do not need this\nvariable to _enable_ the --sparse option, but instead to _disable_\nit. This flips how we work with the variable a bit.\n\nWhen checking for the variable, default to a value of -1 for\n\"unset\". If unset, then take the default from the repo settings,\nwhich is currently 1. Then, the --[no-]sparse command-line option\nwill override either of these settings.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/pack-objects.c | 4 ++--\n t/README | 6 +++---\n t/t5322-pack-objects-sparse.sh | 1 +\n 3 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 02aa6ee4808..eff9542f09f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3469,9 +3469,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tread_replace_refs = 0;\n \n-\tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", 0);\n+\tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n \tprepare_repo_settings(the_repository);\n-\tif (!sparse && the_repository->settings.pack_use_sparse != -1)\n+\tif (sparse < 0)\n \t\tsparse = the_repository->settings.pack_use_sparse;\n \n \treset_pack_idx_option(&pack_idx_opts);\ndiff --git a/t/README b/t/README\nindex 9afd61e3ca0..99ebb18829f 100644\n--- a/t/README\n+++ b/t/README\n@@ -386,9 +386,9 @@ GIT_TEST_INDEX_VERSION= exercises the index read/write code path\n for the index version specified. Can be set to any valid version\n (currently 2, 3, or 4).\n \n-GIT_TEST_PACK_SPARSE= if enabled will default the pack-objects\n-builtin to use the sparse object walk. This can still be overridden by\n-the --no-sparse command-line argument.\n+GIT_TEST_PACK_SPARSE= if disabled will default the pack-objects\n+builtin to use the non-sparse object walk. This can still be overridden by\n+the --sparse command-line argument.\n \n GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path\n by overriding the minimum number of cache entries required per thread.\ndiff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh\nindex 6e5d6bdb0a7..a581eaf5293 100755\n--- a/t/t5322-pack-objects-sparse.sh\n+++ b/t/t5322-pack-objects-sparse.sh\n@@ -107,6 +107,7 @@ test_expect_success 'non-sparse pack-objects' '\n \n # --sparse is enabled by default by pack.useSparse\n test_expect_success 'sparse pack-objects' '\n+\tGIT_TEST_PACK_SPARSE=-1 &&\n \tgit rev-parse\t\t\t\\\n \t\ttopic1\t\t\t\\\n \t\ttopic1^{tree}\t\t\\\n-- \ngitgitgadget\n"},{"id":"393547","messageId":"xmqqmu8abvuf.fsf@gitster.c.googlers.com","threadId":"53028","inReplyTo":"pull.585.v2.git.1584707247753.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] config: set pack.useSparse=true by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-20T20:43:52Z","receivedAt":"2020-03-20T20:43:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Here is a small patch to convert pack.useSparse to true by default. It's\n> been released for over a year, so the feature is quite stable.\n\nI would not say anything more than \"its' been released for over a\nyear, so the feature is known not to cause problems when it is not\nenabled (in other words, we coded our if/else correctly)\", unless\nsome telemetry tells us that significant number of users with widely\ndiffering use patterns have enabled it and are not seeing much\nnegative effect. And we can tell if it is stable only if we flip\nthe default.\n\n> I'm submitting this now to allow it to cook for a while during the\n> next release cycle.\n\nI agree that it is about time to see if flipping of default would be\na good move for users whose usage patterns are unlike VFS for Git by\ncooking a change like this in 'next', definitely at least a cycle\nbut possible a bit more, and it is a good idea to have it at the\nbeginning of the next cycle. Very much appreciated.\n\nThanks.\n"},{"id":"393549","messageId":"e35c1bc5-7569-defc-2cee-e47039506521@gmail.com","threadId":"53028","inReplyTo":"xmqqmu8abvuf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] config: set pack.useSparse=true by default","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-20T21:14:56Z","receivedAt":"2020-03-20T21:15:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/20/2020 4:43 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> Here is a small patch to convert pack.useSparse to true by default. It's\n>> been released for over a year, so the feature is quite stable.\n> \n> I would not say anything more than \"its' been released for over a\n> year, so the feature is known not to cause problems when it is not\n> enabled (in other words, we coded our if/else correctly)\", unless\n> some telemetry tells us that significant number of users with widely\n> differing use patterns have enabled it and are not seeing much\n> negative effect. And we can tell if it is stable only if we flip\n> the default.\n\nTrue. I've done my best to advertise the feature but have heard very\nlittle from users about it outside of our \"captive audience\" in the\nWindows OS team. I've also been using it myself (as part of\nfeatures.experimental) on all of my machines, but that's hardly a\nvote for rigorous use in strange patterns.\n\n>> I'm submitting this now to allow it to cook for a while during the\n>> next release cycle.\n> \n> I agree that it is about time to see if flipping of default would be\n> a good move for users whose usage patterns are unlike VFS for Git by\n> cooking a change like this in 'next', definitely at least a cycle\n> but possible a bit more, and it is a good idea to have it at the\n> beginning of the next cycle. Very much appreciated.\n\nI'm happy to let you decide when this has cooked long enough. If\nyou feel that more than one cycle is needed, then absolutely let's\nbe cautious here.\n\nThanks,\n-Stolee\n"}]}