{"thread":{"id":"57327","subject":"[PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","startedAt":"2022-01-28T01:56:14Z","lastAt":"2022-02-02T17:50:15Z","messageCount":23,"participants":["Elijah Newren via GitGitGadget","Ævar Arnfjörð Bjarmason","Elijah Newren","Junio C Hamano","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"447166","messageId":"pull.1131.git.1643334969216.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":null,"subject":"[PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-28T01:56:09Z","receivedAt":"2022-01-28T01:56:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 3050b6dfc75d (repo-settings.c: simplify the setup,\n2021-09-21), the branch for handling fetch.negotiationAlgorithm=default\nwas deleted. Since this value is documented in\nDocumentation/config/fetch.txt, restore the check for this value.\n\nNote that this change caused an observable bug: if someone sets\nfeature.experimental=true in config, and then passes \"-c\nfetch.negotiationAlgorithm=default\" on the command line in an attempt to\noverride the config, then the override is ignored. Fix the bug by not\nignoring the value of \"default\".\n\nTechnically, before commit 3050b6dfc75d, repo-settings would treat any\nfetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\nrequest for \"default\", but I think it probably makes more sense to\nignore such broken requests and leave fetch.negotiationAlgorithm with\nthe default value rather than the value of \"default\". (If that sounds\nconfusing, note that \"default\" is usually the default value, but when\nfeature.experimental=true, \"skipping\" is the default value.)\n\nSigned-off-by: Elijah Newren \n---\n repo-settings: fix checking for fetch.negotiationAlgorithm=default\n \n This regression is not new in v2.35; it first appeared in v2.34. So, not\n urgent.\n \n A long sidenote about naming things \"default\":\n \n Many years ago, in the Gnome community, there was a huge fight that\n erupted, in part due to confusion over \"default\". There was a journalist\n who had been a designer in a past life, who had a little friction with\n the rest of the community, but intended well and generally improved\n things. At some point, they suggested some changes to improve the\n \"default\" theme (and they were a nice improvement), but not being a\n developer the changes weren't communicated in the form of a patch. And\n the changes accidentally got applied to the wrong theme: the default one\n (yes, there was a theme named \"default\" which was not the default\n theme). Now, basically no one used the default theme because it was so\n hideously ugly. I think we suffered from a case of not being able to\n change the default (again?) because no one could get an agreement on\n what the default should be. Who did actually use the default theme,\n though? The person writing the release notes (though they only used it\n for taking screenshots to include in the release notes, and otherwise\n used some other theme). So, with people under pressure for an imminent\n release, there were screenshots that looked like garbage, and\n investigation eventually uncovered that it was due to changes that were\n meant for the \"default\" theme having accidentally been applied to the\n default theme. It could have just been an amusing story if not for the\n other unfortunate factors happening around the same time and the heated\n and protracted flamewars that erupted.\n \n Don't name settings/themes/things \"default\" if it describes something\n specific, since someone may come along and decide that something else\n should be the default, and then you're stuck with a non-default\n \"default\". Sadly, the name was already picked and documented so for\n backward compatibility we need to support it...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1131\n\n repo-settings.c | 2 ++\n t/t5500-fetch-pack.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 00ca5571a1a..38c10f9977b 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -85,6 +85,8 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n+\t\telse if (!strcasecmp(strval, \"default\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n \t/*\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex f0dc4e69686..37958a376ca 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -928,6 +928,7 @@ test_expect_success 'fetching deepen' '\n '\n \n test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+\ttest_when_finished rm -rf clientv0 clientv2 &&\n \trm -rf server client &&\n \tgit init server &&\n \ttest_commit -C server both_have_1 &&\n@@ -960,6 +961,45 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n '\n \n+test_expect_success 'same as last but with config overrides' '\n+\ttest_when_finished rm -rf clientv0 clientv2 &&\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server both_have_1 &&\n+\tgit -C server tag -d both_have_1 &&\n+\ttest_commit -C server both_have_2 &&\n+\n+\tgit clone server client &&\n+\ttest_commit -C server server_has &&\n+\ttest_commit -C client client_has &&\n+\n+\t# In both protocol v0 and v2, ensure that the parent of both_have_2 is\n+\t# not sent as a \"have\" line. The client should know that the server has\n+\t# both_have_2, so it only needs to inform the server that it has\n+\t# both_have_2, and the server can infer the rest.\n+\n+\trm -f trace &&\n+\trm -rf clientv0 &&\n+\tcp -r client clientv0 &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n+\t\t-c feature.experimental=true \\\n+\t\t-c fetch.negotiationAlgorithm=default \\\n+\t\tfetch origin server_has both_have_2 &&\n+\tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n+\tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n+\t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n+\n+\trm -f trace &&\n+\tcp -r client clientv2 &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n+\t\t-c feature.experimental=true \\\n+\t\t-c fetch.negotiationAlgorithm=default \\\n+\t\tfetch origin server_has both_have_2 &&\n+\tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n+\tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n+\t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n+'\n+\n test_expect_success 'filtering by size' '\n \trm -rf server client &&\n \ttest_create_repo server &&\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\n-- \ngitgitgadget\n"},{"id":"447174","messageId":"220128.86ee4scn2s.gmgdl@evledraar.gmail.com","threadId":"57327","inReplyTo":"pull.1131.git.1643334969216.gitgitgadget@gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-28T07:25:10Z","receivedAt":"2022-01-28T07:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 28 2022, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> In commit 3050b6dfc75d (repo-settings.c: simplify the setup,\n> 2021-09-21), the branch for handling fetch.negotiationAlgorithm=default\n> was deleted. Since this value is documented in\n> Documentation/config/fetch.txt, restore the check for this value.\n>\n> Note that this change caused an observable bug: if someone sets\n> feature.experimental=true in config, and then passes \"-c\n> fetch.negotiationAlgorithm=default\" on the command line in an attempt to\n> override the config, then the override is ignored. Fix the bug by not\n> ignoring the value of \"default\".\n\nThis fix looks good, thanks for fixing my mess.\n\n> Technically, before commit 3050b6dfc75d, repo-settings would treat any\n> fetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\n> request for \"default\", but I think it probably makes more sense to\n> ignore such broken requests and leave fetch.negotiationAlgorithm with\n> the default value rather than the value of \"default\". (If that sounds\n> confusing, note that \"default\" is usually the default value, but when\n> feature.experimental=true, \"skipping\" is the default value.)\n>\n> [...]\n> A long sidenote about naming things \"default\":\n> \n> Many years ago, in the Gnome community, there was a huge fight that\n> erupted, in part due to confusion over \"default\". There was a journalist\n> who had been a designer in a past life, who had a little friction with\n> the rest of the community, but intended well and generally improved\n> things. At some point, they suggested some changes to improve the\n> \"default\" theme (and they were a nice improvement), but not being a\n> developer the changes weren't communicated in the form of a patch. And\n> the changes accidentally got applied to the wrong theme: the default one\n> (yes, there was a theme named \"default\" which was not the default\n> theme). Now, basically no one used the default theme because it was so\n> hideously ugly. I think we suffered from a case of not being able to\n> change the default (again?) because no one could get an agreement on\n> what the default should be. Who did actually use the default theme,\n> though? The person writing the release notes (though they only used it\n> for taking screenshots to include in the release notes, and otherwise\n> used some other theme). So, with people under pressure for an imminent\n> release, there were screenshots that looked like garbage, and\n> investigation eventually uncovered that it was due to changes that were\n> meant for the \"default\" theme having accidentally been applied to the\n> default theme. It could have just been an amusing story if not for the\n> other unfortunate factors happening around the same time and the heated\n> and protracted flamewars that erupted.\n> \n> Don't name settings/themes/things \"default\" if it describes something\n> specific, since someone may come along and decide that something else\n> should be the default, and then you're stuck with a non-default\n> \"default\". Sadly, the name was already picked and documented so for\n> backward compatibility we need to support it...\n\nFunny story, I think this is only going to bite us if we don't switch\nthe default over along with promoting this out of feature.experimental.\n\nI.e. =default should always be equivalent to not declaring that config\nat all anywhere, and not drift to being a reference to some name that\nhappens to be \"default\", as in the GNOME case.\n\nIn our case it's more of a story about the inconsistencies in our config\nspace, i.e. some values you can't reset at all, some take empty values\nto do so, others \"default\" etc.\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index f0dc4e69686..37958a376ca 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -928,6 +928,7 @@ test_expect_success 'fetching deepen' '\n> '\n> \n> test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n> +\ttest_when_finished rm -rf clientv0 clientv2 &&\n> \trm -rf server client &&\n> \tgit init server &&\n> \ttest_commit -C server both_have_1 &&\n> @@ -960,6 +961,45 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n> \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n> '\n> \n> +test_expect_success 'same as last but with config overrides' '\n\nSince it's the same as the preceding test, maybe we can squash this in\nto avoid the duplication? This works for me.\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 37958a376ca..3fb20eeec7e 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -927,7 +927,7 @@ test_expect_success 'fetching deepen' '\n \t)\n '\n \n-test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+test_negotiation_algorithm_default () {\n \ttest_when_finished rm -rf clientv0 clientv2 &&\n \trm -rf server client &&\n \tgit init server &&\n@@ -946,7 +946,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \n \trm -f trace &&\n \tcp -r client clientv0 &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 $@ \\\n \t\tfetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n@@ -954,50 +954,17 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \n \trm -f trace &&\n \tcp -r client clientv2 &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 $@ \\\n \t\tfetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n-'\n-\n-test_expect_success 'same as last but with config overrides' '\n-\ttest_when_finished rm -rf clientv0 clientv2 &&\n-\trm -rf server client &&\n-\tgit init server &&\n-\ttest_commit -C server both_have_1 &&\n-\tgit -C server tag -d both_have_1 &&\n-\ttest_commit -C server both_have_2 &&\n-\n-\tgit clone server client &&\n-\ttest_commit -C server server_has &&\n-\ttest_commit -C client client_has &&\n-\n-\t# In both protocol v0 and v2, ensure that the parent of both_have_2 is\n-\t# not sent as a \"have\" line. The client should know that the server has\n-\t# both_have_2, so it only needs to inform the server that it has\n-\t# both_have_2, and the server can infer the rest.\n-\n-\trm -f trace &&\n-\trm -rf clientv0 &&\n-\tcp -r client clientv0 &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n-\t\t-c feature.experimental=true \\\n-\t\t-c fetch.negotiationAlgorithm=default \\\n-\t\tfetch origin server_has both_have_2 &&\n-\tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n-\tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n-\t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n+}\n \n-\trm -f trace &&\n-\tcp -r client clientv2 &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n+test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+\ttest_negotiation_algorithm_default \\\n \t\t-c feature.experimental=true \\\n-\t\t-c fetch.negotiationAlgorithm=default \\\n-\t\tfetch origin server_has both_have_2 &&\n-\tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n-\tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n-\t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n+\t\t-c fetch.negotiationAlgorithm=default\n '\n \n test_expect_success 'filtering by size' '\n"},{"id":"447256","messageId":"CABPp-BEehHHbDi_muWHCaT1GvRmPbPiMmvB-2O9+0w2A+yEM0Q@mail.gmail.com","threadId":"57327","inReplyTo":"220128.86ee4scn2s.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-29T01:40:18Z","receivedAt":"2022-01-29T01:40:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ævar,\n\nOn Thu, Jan 27, 2022 at 11:54 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, Jan 28 2022, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > In commit 3050b6dfc75d (repo-settings.c: simplify the setup,\n> > 2021-09-21), the branch for handling fetch.negotiationAlgorithm=default\n> > was deleted. Since this value is documented in\n> > Documentation/config/fetch.txt, restore the check for this value.\n> >\n> > Note that this change caused an observable bug: if someone sets\n> > feature.experimental=true in config, and then passes \"-c\n> > fetch.negotiationAlgorithm=default\" on the command line in an attempt to\n> > override the config, then the override is ignored. Fix the bug by not\n> > ignoring the value of \"default\".\n>\n> This fix looks good, thanks for fixing my mess.\n\nNo worries, it was a pretty tiny mess. It was actually caught by\ntesting rather than by an end user. (I hadn't updated our internal\nGit distribution since before v2.34 due to various other priorities.\nSince that distribution includes some patches that turn on\nfeature.experimental, along with one testsuite change to explicitly\nrequest \"default\" handling in a test that was intended to check the\n\"default\" behavior, when I updated the distribution, that testcase\nfailed.)\n\nAnd you made the code in repo-settings.c *so* much nicer to read. I\nthink that's more important than this little issue.\n\n>> > Technically, before commit 3050b6dfc75d, repo-settings would treat any\n> > fetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\n> > request for \"default\", but I think it probably makes more sense to\n> > ignore such broken requests and leave fetch.negotiationAlgorithm with\n> > the default value rather than the value of \"default\". (If that sounds\n> > confusing, note that \"default\" is usually the default value, but when\n> > feature.experimental=true, \"skipping\" is the default value.)\n> >\n> > [...]\n> > A long sidenote about naming things \"default\":\n> >\n> > Many years ago, in the Gnome community, there was a huge fight that\n> > erupted, in part due to confusion over \"default\". There was a journalist\n> > who had been a designer in a past life, who had a little friction with\n> > the rest of the community, but intended well and generally improved\n> > things. At some point, they suggested some changes to improve the\n> > \"default\" theme (and they were a nice improvement), but not being a\n> > developer the changes weren't communicated in the form of a patch. And\n> > the changes accidentally got applied to the wrong theme: the default one\n> > (yes, there was a theme named \"default\" which was not the default\n> > theme). Now, basically no one used the default theme because it was so\n> > hideously ugly. I think we suffered from a case of not being able to\n> > change the default (again?) because no one could get an agreement on\n> > what the default should be. Who did actually use the default theme,\n> > though? The person writing the release notes (though they only used it\n> > for taking screenshots to include in the release notes, and otherwise\n> > used some other theme). So, with people under pressure for an imminent\n> > release, there were screenshots that looked like garbage, and\n> > investigation eventually uncovered that it was due to changes that were\n> > meant for the \"default\" theme having accidentally been applied to the\n> > default theme. It could have just been an amusing story if not for the\n> > other unfortunate factors happening around the same time and the heated\n> > and protracted flamewars that erupted.\n> >\n> > Don't name settings/themes/things \"default\" if it describes something\n> > specific, since someone may come along and decide that something else\n> > should be the default, and then you're stuck with a non-default\n> > \"default\". Sadly, the name was already picked and documented so for\n> > backward compatibility we need to support it...\n>\n> Funny story, I think this is only going to bite us if we don't switch\n> the default over along with promoting this out of feature.experimental.\n>\n> I.e. =default should always be equivalent to not declaring that config\n> at all anywhere, and not drift to being a reference to some name that\n> happens to be \"default\", as in the GNOME case.\n\nNo, we have the same problem as the Gnome case. See this part of the\ndocumentation for fetch.negotiationAlgorithm:\n\n\"\"\"\n The default is \"default\" which instructs Git to use the\n default algorithm that never skips commits (unless the server has\n acknowledged it or one of its descendants).\n\"\"\"\n\nfeatures.experimental turns on \"skipping\" as the default behavior, and\nthat text clearly rules out the possibility that \"default\" could be\nused to mean \"skipping\". So, if that experimental feature graduates,\nthen the default behavior of fetch.negotiationAlgorithm will NOT be\nthe \"default\" behavior of fetch.negotationAlgorithm.\n\n> In our case it's more of a story about the inconsistencies in our config\n> space, i.e. some values you can't reset at all, some take empty values\n> to do so, others \"default\" etc.\n>\n> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > index f0dc4e69686..37958a376ca 100755\n> > --- a/t/t5500-fetch-pack.sh\n> > +++ b/t/t5500-fetch-pack.sh\n> > @@ -928,6 +928,7 @@ test_expect_success 'fetching deepen' '\n> > '\n> >\n> > test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n> > + test_when_finished rm -rf clientv0 clientv2 &&\n> > rm -rf server client &&\n> > git init server &&\n> > test_commit -C server both_have_1 &&\n> > @@ -960,6 +961,45 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n> > ! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n> > '\n> >\n> > +test_expect_success 'same as last but with config overrides' '\n>\n> Since it's the same as the preceding test, maybe we can squash this in\n> to avoid the duplication? This works for me.\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 37958a376ca..3fb20eeec7e 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -927,7 +927,7 @@ test_expect_success 'fetching deepen' '\n> )\n> '\n>\n> -test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n> +test_negotiation_algorithm_default () {\n> test_when_finished rm -rf clientv0 clientv2 &&\n> rm -rf server client &&\n> git init server &&\n> @@ -946,7 +946,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n>\n> rm -f trace &&\n> cp -r client clientv0 &&\n> - GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 $@ \\\n> fetch origin server_has both_have_2 &&\n> grep \"have $(git -C client rev-parse client_has)\" trace &&\n> grep \"have $(git -C client rev-parse both_have_2)\" trace &&\n> @@ -954,50 +954,17 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n>\n> rm -f trace &&\n> cp -r client clientv2 &&\n> - GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 $@ \\\n> fetch origin server_has both_have_2 &&\n> grep \"have $(git -C client rev-parse client_has)\" trace &&\n> grep \"have $(git -C client rev-parse both_have_2)\" trace &&\n> ! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n> -'\n> -\n> -test_expect_success 'same as last but with config overrides' '\n> - test_when_finished rm -rf clientv0 clientv2 &&\n> - rm -rf server client &&\n> - git init server &&\n> - test_commit -C server both_have_1 &&\n> - git -C server tag -d both_have_1 &&\n> - test_commit -C server both_have_2 &&\n> -\n> - git clone server client &&\n> - test_commit -C server server_has &&\n> - test_commit -C client client_has &&\n> -\n> - # In both protocol v0 and v2, ensure that the parent of both_have_2 is\n> - # not sent as a \"have\" line. The client should know that the server has\n> - # both_have_2, so it only needs to inform the server that it has\n> - # both_have_2, and the server can infer the rest.\n> -\n> - rm -f trace &&\n> - rm -rf clientv0 &&\n> - cp -r client clientv0 &&\n> - GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n> - -c feature.experimental=true \\\n> - -c fetch.negotiationAlgorithm=default \\\n> - fetch origin server_has both_have_2 &&\n> - grep \"have $(git -C client rev-parse client_has)\" trace &&\n> - grep \"have $(git -C client rev-parse both_have_2)\" trace &&\n> - ! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n> +}\n>\n> - rm -f trace &&\n> - cp -r client clientv2 &&\n> - GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n> +test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n> + test_negotiation_algorithm_default \\\n> -c feature.experimental=true \\\n> - -c fetch.negotiationAlgorithm=default \\\n> - fetch origin server_has both_have_2 &&\n> - grep \"have $(git -C client rev-parse client_has)\" trace &&\n> - grep \"have $(git -C client rev-parse both_have_2)\" trace &&\n> - ! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n> + -c fetch.negotiationAlgorithm=default\n\nI think you accidentally dropped one of the two tests by turning it\ninto a function and then only calling it for the latter usage and not\nthe former, but I get your idea. It makes sense; I'll make the\nchange.\n"},{"id":"447268","messageId":"220129.86y22zaw3p.gmgdl@evledraar.gmail.com","threadId":"57327","inReplyTo":"CABPp-BEehHHbDi_muWHCaT1GvRmPbPiMmvB-2O9+0w2A+yEM0Q@mail.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-29T06:08:44Z","receivedAt":"2022-01-29T06:34:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 28 2022, Elijah Newren wrote:\n\n> Hi Ævar,\n>\n> On Thu, Jan 27, 2022 at 11:54 PM Ævar Arnfjörð Bjarmason\n> wrote:\n> [...]\n>>> > Technically, before commit 3050b6dfc75d, repo-settings would treat any\n>> > fetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\n>> > request for \"default\", but I think it probably makes more sense to\n>> > ignore such broken requests and leave fetch.negotiationAlgorithm with\n>> > the default value rather than the value of \"default\". (If that sounds\n>> > confusing, note that \"default\" is usually the default value, but when\n>> > feature.experimental=true, \"skipping\" is the default value.)\n>> >\n>> > [...]\n>> > A long sidenote about naming things \"default\":\n>> >\n>> > Many years ago, in the Gnome community, there was a huge fight that\n>> > erupted, in part due to confusion over \"default\". There was a journalist\n>> > who had been a designer in a past life, who had a little friction with\n>> > the rest of the community, but intended well and generally improved\n>> > things. At some point, they suggested some changes to improve the\n>> > \"default\" theme (and they were a nice improvement), but not being a\n>> > developer the changes weren't communicated in the form of a patch. And\n>> > the changes accidentally got applied to the wrong theme: the default one\n>> > (yes, there was a theme named \"default\" which was not the default\n>> > theme). Now, basically no one used the default theme because it was so\n>> > hideously ugly. I think we suffered from a case of not being able to\n>> > change the default (again?) because no one could get an agreement on\n>> > what the default should be. Who did actually use the default theme,\n>> > though? The person writing the release notes (though they only used it\n>> > for taking screenshots to include in the release notes, and otherwise\n>> > used some other theme). So, with people under pressure for an imminent\n>> > release, there were screenshots that looked like garbage, and\n>> > investigation eventually uncovered that it was due to changes that were\n>> > meant for the \"default\" theme having accidentally been applied to the\n>> > default theme. It could have just been an amusing story if not for the\n>> > other unfortunate factors happening around the same time and the heated\n>> > and protracted flamewars that erupted.\n>> >\n>> > Don't name settings/themes/things \"default\" if it describes something\n>> > specific, since someone may come along and decide that something else\n>> > should be the default, and then you're stuck with a non-default\n>> > \"default\". Sadly, the name was already picked and documented so for\n>> > backward compatibility we need to support it...\n>>\n>> Funny story, I think this is only going to bite us if we don't switch\n>> the default over along with promoting this out of feature.experimental.\n>>\n>> I.e. =default should always be equivalent to not declaring that config\n>> at all anywhere, and not drift to being a reference to some name that\n>> happens to be \"default\", as in the GNOME case.\n>\n> No, we have the same problem as the Gnome case. See this part of the\n> documentation for fetch.negotiationAlgorithm:\n>\n> \"\"\"\n> The default is \"default\" which instructs Git to use the\n> default algorithm that never skips commits (unless the server has\n> acknowledged it or one of its descendants).\n> \"\"\"\n>\n> features.experimental turns on \"skipping\" as the default behavior, and\n> that text clearly rules out the possibility that \"default\" could be\n> used to mean \"skipping\". So, if that experimental feature graduates,\n> then the default behavior of fetch.negotiationAlgorithm will NOT be\n> the \"default\" behavior of fetch.negotationAlgorithm.\n\nI see what you mean, and I'm aware that I'm debating this with a native\nspeaker :)\n\nFWIW I didn't read it that way, earlier it discusses \"skipping\", and\nhere it's describing what the default is. But especially since you'd\nhave no reason to set this except to \"reset to default\" I didn't take it\nto be a promise that the default wouldn't change.\n\nI.e. maybe we'll just make it \"skipping\" and drop the current \"default\"\ncode, or we'll give the current \"default\" a name at that point.\n\nBut I do see how us not having a name for the \"defult\" complicates that\nview of the world. For grep.patternType we've got the same thing, but\n\"default\" there is \"basic\", so that's a bit different.\n\nI do read log.date's \"default\" as being the sort of GNOME case you're\ndescribing however. But I don't think we'd ever change the default\nthere, a date format is too subjective, whereas an internal algorithm is\nliable to change.\n\nBut I think we should just change this to make it explicit (separate\nfrom this narrow bugfix). Maybe \"exhaustive\" would be a good permanent\nname for the default algorithm?\n\n>> In our case it's more of a story about the inconsistencies in our config\n>> space, i.e. some values you can't reset at all, some take empty values\n>> to do so, others \"default\" etc.\n>> [...]\n>>\n>> Since it's the same as the preceding test, maybe we can squash this in\n>> to avoid the duplication? This works for me.\n>> [...]\n>> - rm -f trace &&\n>> - cp -r client clientv2 &&\n>> - GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n>> +test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n>> + test_negotiation_algorithm_default \\\n>> -c feature.experimental=true \\\n>> - -c fetch.negotiationAlgorithm=default \\\n>> - fetch origin server_has both_have_2 &&\n>> - grep \"have $(git -C client rev-parse client_has)\" trace &&\n>> - grep \"have $(git -C client rev-parse both_have_2)\" trace &&\n>> - ! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n>> + -c fetch.negotiationAlgorithm=default\n>\n> I think you accidentally dropped one of the two tests by turning it\n> into a function and then only calling it for the latter usage and not\n> the former, but I get your idea. It makes sense; I'll make the\n> change.\n\nAh yes, oops. Yes we should clearly have the non-\"-c [...]\" case too\n(and it's what I was aiming for) :)\n"},{"id":"447285","messageId":"pull.1131.v2.git.1643478692337.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.git.1643334969216.gitgitgadget@gmail.com","subject":"[PATCH v2] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-29T17:51:32Z","receivedAt":"2022-01-29T17:51:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 3050b6dfc75d (repo-settings.c: simplify the setup,\n2021-09-21), the branch for handling fetch.negotiationAlgorithm=default\nwas deleted. Since this value is documented in\nDocumentation/config/fetch.txt, restore the check for this value.\n\nNote that this change caused an observable bug: if someone sets\nfeature.experimental=true in config, and then passes \"-c\nfetch.negotiationAlgorithm=default\" on the command line in an attempt to\noverride the config, then the override is ignored. Fix the bug by not\nignoring the value of \"default\".\n\nTechnically, before commit 3050b6dfc75d, repo-settings would treat any\nfetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\nrequest for \"default\", but I think it probably makes more sense to\nignore such broken requests and leave fetch.negotiationAlgorithm with\nthe default value rather than the value of \"default\". (If that sounds\nconfusing, note that \"default\" is usually the default value, but when\nfeature.experimental=true, \"skipping\" is the default value.)\n\nSigned-off-by: Elijah Newren \n---\n repo-settings: fix checking for fetch.negotiationAlgorithm=default\n \n This regression is not new in v2.35; it first appeared in v2.34. So, not\n urgent.\n \n Changes since v1:\n \n * Put the common code in two testcases into a function, and then just\n invoked it from each\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1131\n\nRange-diff vs v1:\n\n 1: 9c592baded1 ! 1: 633c873b513 repo-settings: fix checking for fetch.negotiationAlgorithm=default\n @@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n \n ## t/t5500-fetch-pack.sh ##\n @@ t/t5500-fetch-pack.sh: test_expect_success 'fetching deepen' '\n + \t)\n '\n \n - test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n +-test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n ++test_negotiation_algorithm_default () {\n +\ttest_when_finished rm -rf clientv0 clientv2 &&\n \trm -rf server client &&\n \tgit init server &&\n \ttest_commit -C server both_have_1 &&\n @@ t/t5500-fetch-pack.sh: test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n + \trm -f trace &&\n + \tcp -r client clientv0 &&\n + \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n +-\t\tfetch origin server_has both_have_2 &&\n ++\t\t\"$@\" fetch origin server_has both_have_2 &&\n + \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n + \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n + \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n +@@ t/t5500-fetch-pack.sh: test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n + \trm -f trace &&\n + \tcp -r client clientv2 &&\n + \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n +-\t\tfetch origin server_has both_have_2 &&\n ++\t\t\"$@\" fetch origin server_has both_have_2 &&\n + \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n + \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n - '\n - \n -+test_expect_success 'same as last but with config overrides' '\n -+\ttest_when_finished rm -rf clientv0 clientv2 &&\n -+\trm -rf server client &&\n -+\tgit init server &&\n -+\ttest_commit -C server both_have_1 &&\n -+\tgit -C server tag -d both_have_1 &&\n -+\ttest_commit -C server both_have_2 &&\n -+\n -+\tgit clone server client &&\n -+\ttest_commit -C server server_has &&\n -+\ttest_commit -C client client_has &&\n ++}\n +\n -+\t# In both protocol v0 and v2, ensure that the parent of both_have_2 is\n -+\t# not sent as a \"have\" line. The client should know that the server has\n -+\t# both_have_2, so it only needs to inform the server that it has\n -+\t# both_have_2, and the server can infer the rest.\n -+\n -+\trm -f trace &&\n -+\trm -rf clientv0 &&\n -+\tcp -r client clientv0 &&\n -+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n -+\t\t-c feature.experimental=true \\\n -+\t\t-c fetch.negotiationAlgorithm=default \\\n -+\t\tfetch origin server_has both_have_2 &&\n -+\tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n -+\tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n -+\t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n ++test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n ++\ttest_negotiation_algorithm_default\n ++'\n +\n -+\trm -f trace &&\n -+\tcp -r client clientv2 &&\n -+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n ++test_expect_success 'same as last but with config overrides' '\n ++\ttest_negotiation_algorithm_default \\\n +\t\t-c feature.experimental=true \\\n +\t\t-c fetch.negotiationAlgorithm=default \\\n -+\t\tfetch origin server_has both_have_2 &&\n -+\tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n -+\tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n -+\t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n -+'\n -+\n + '\n + \n test_expect_success 'filtering by size' '\n - \trm -rf server client &&\n - \ttest_create_repo server &&\n\n\n repo-settings.c | 2 ++\n t/t5500-fetch-pack.sh | 17 ++++++++++++++---\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 00ca5571a1a..38c10f9977b 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -85,6 +85,8 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n+\t\telse if (!strcasecmp(strval, \"default\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n \t/*\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex f0dc4e69686..0a737cf0a0a 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -927,7 +927,8 @@ test_expect_success 'fetching deepen' '\n \t)\n '\n \n-test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+test_negotiation_algorithm_default () {\n+\ttest_when_finished rm -rf clientv0 clientv2 &&\n \trm -rf server client &&\n \tgit init server &&\n \ttest_commit -C server both_have_1 &&\n@@ -946,7 +947,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \trm -f trace &&\n \tcp -r client clientv0 &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n-\t\tfetch origin server_has both_have_2 &&\n+\t\t\"$@\" fetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n@@ -954,10 +955,20 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \trm -f trace &&\n \tcp -r client clientv2 &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n-\t\tfetch origin server_has both_have_2 &&\n+\t\t\"$@\" fetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n+}\n+\n+test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+\ttest_negotiation_algorithm_default\n+'\n+\n+test_expect_success 'same as last but with config overrides' '\n+\ttest_negotiation_algorithm_default \\\n+\t\t-c feature.experimental=true \\\n+\t\t-c fetch.negotiationAlgorithm=default \\\n '\n \n test_expect_success 'filtering by size' '\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\n-- \ngitgitgadget\n"},{"id":"447343","messageId":"xmqqfsp3kfm0.fsf@gitster.g","threadId":"57327","inReplyTo":"CABPp-BEehHHbDi_muWHCaT1GvRmPbPiMmvB-2O9+0w2A+yEM0Q@mail.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T16:57:27Z","receivedAt":"2022-01-31T16:57:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> I.e. =default should always be equivalent to not declaring that config\n>> at all anywhere, and not drift to being a reference to some name that\n>> happens to be \"default\", as in the GNOME case.\n>\n> No, we have the same problem as the Gnome case. See this part of the\n> documentation for fetch.negotiationAlgorithm:\n>\n> \"\"\"\n> The default is \"default\" which instructs Git to use the\n> default algorithm that never skips commits (unless the server has\n> acknowledged it or one of its descendants).\n> \"\"\"\n\nThat looks more like one of the bugs introduced when skipping was\nturned on for the \"experimental\" folks. To fix this, without\nturning skipping into the default too hastily, there needs two and\nhalf things to happen:\n\n * Give a new name for the non-skipping algorithm, and describe the\n algorithm like the above.\n\n * Describe \"default\" is \"non-skipping\" but \"feature.experimental\"\n makes \"skipping\" the default.\n\n * Support \"non-skipping\" in the configuration parser, so that even\n when something else becomes the default, people can choose it.\n\nI would think.\n"},{"id":"447347","messageId":"CABPp-BHcS3nwupJSORUuVeVhGzrL3VqPm4YQSr7G-JuyLaqBpQ@mail.gmail.com","threadId":"57327","inReplyTo":"xmqqfsp3kfm0.fsf@gitster.g","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-31T17:33:14Z","receivedAt":"2022-01-31T17:33:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 31, 2022 at 8:57 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> >> I.e. =default should always be equivalent to not declaring that config\n> >> at all anywhere, and not drift to being a reference to some name that\n> >> happens to be \"default\", as in the GNOME case.\n> >\n> > No, we have the same problem as the Gnome case. See this part of the\n> > documentation for fetch.negotiationAlgorithm:\n> >\n> > \"\"\"\n> > The default is \"default\" which instructs Git to use the\n> > default algorithm that never skips commits (unless the server has\n> > acknowledged it or one of its descendants).\n> > \"\"\"\n>\n> That looks more like one of the bugs introduced when skipping was\n> turned on for the \"experimental\" folks. To fix this, without\n> turning skipping into the default too hastily, there needs two and\n> half things to happen:\n>\n> * Give a new name for the non-skipping algorithm, and describe the\n> algorithm like the above.\n>\n> * Describe \"default\" is \"non-skipping\" but \"feature.experimental\"\n> makes \"skipping\" the default.\n>\n> * Support \"non-skipping\" in the configuration parser, so that even\n> when something else becomes the default, people can choose it.\n>\n> I would think.\n\nSounds good to me. I'm not very creative, so I think I'd just use\n\"non-skipping\" as the new name.\n"},{"id":"447361","messageId":"220131.864k5jbo5h.gmgdl@evledraar.gmail.com","threadId":"57327","inReplyTo":"CABPp-BHcS3nwupJSORUuVeVhGzrL3VqPm4YQSr7G-JuyLaqBpQ@mail.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-31T21:03:18Z","receivedAt":"2022-01-31T21:17:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 31 2022, Elijah Newren wrote:\n\n> On Mon, Jan 31, 2022 at 8:57 AM Junio C Hamano wrote:\n>>\n>> Elijah Newren writes:\n>>\n>> >> I.e. =default should always be equivalent to not declaring that config\n>> >> at all anywhere, and not drift to being a reference to some name that\n>> >> happens to be \"default\", as in the GNOME case.\n>> >\n>> > No, we have the same problem as the Gnome case. See this part of the\n>> > documentation for fetch.negotiationAlgorithm:\n>> >\n>> > \"\"\"\n>> > The default is \"default\" which instructs Git to use the\n>> > default algorithm that never skips commits (unless the server has\n>> > acknowledged it or one of its descendants).\n>> > \"\"\"\n>>\n>> That looks more like one of the bugs introduced when skipping was\n>> turned on for the \"experimental\" folks. To fix this, without\n>> turning skipping into the default too hastily, there needs two and\n>> half things to happen:\n>>\n>> * Give a new name for the non-skipping algorithm, and describe the\n>> algorithm like the above.\n>>\n>> * Describe \"default\" is \"non-skipping\" but \"feature.experimental\"\n>> makes \"skipping\" the default.\n>>\n>> * Support \"non-skipping\" in the configuration parser, so that even\n>> when something else becomes the default, people can choose it.\n>>\n>> I would think.\n>\n> Sounds good to me. I'm not very creative, so I think I'd just use\n> \"non-skipping\" as the new name.\n\nI can't think of a better one either (aside from my already-suggested\n\"exhaustive\"), but that's naming it in terms of the only other\nnegotiator.\n\nIs it the case that the only thing anyone would want to tweak about the\ndefault one is its skipping behavior?\n\nE.g. if we were to make one called \"smart-topology\" or something (would\nskip sending some OIDs by assuming things about branch/tag topology,\ni.e. if you have X that probably includes Y) having negotiators \"A\",\n\"non-A\", and \"C\" would be odd :)\n\nWe're unlikely to change the \"default\" negotiatior behavior at this\npoint, so maybe some label that communicates \"the old one\" without\nimplying deprecation? Perhaps \"classic\"?\n\n"},{"id":"447365","messageId":"CABPp-BGRPOdhqV0uXGR2YfroB5Q3K2C6DiQqCdAbG4Xoi2-d9A@mail.gmail.com","threadId":"57327","inReplyTo":"220131.864k5jbo5h.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-31T21:47:26Z","receivedAt":"2022-01-31T21:48:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 31, 2022 at 1:17 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Mon, Jan 31 2022, Elijah Newren wrote:\n>\n> > On Mon, Jan 31, 2022 at 8:57 AM Junio C Hamano wrote:\n> >>\n> >> Elijah Newren writes:\n> >>\n> >> >> I.e. =default should always be equivalent to not declaring that config\n> >> >> at all anywhere, and not drift to being a reference to some name that\n> >> >> happens to be \"default\", as in the GNOME case.\n> >> >\n> >> > No, we have the same problem as the Gnome case. See this part of the\n> >> > documentation for fetch.negotiationAlgorithm:\n> >> >\n> >> > \"\"\"\n> >> > The default is \"default\" which instructs Git to use the\n> >> > default algorithm that never skips commits (unless the server has\n> >> > acknowledged it or one of its descendants).\n> >> > \"\"\"\n> >>\n> >> That looks more like one of the bugs introduced when skipping was\n> >> turned on for the \"experimental\" folks. To fix this, without\n> >> turning skipping into the default too hastily, there needs two and\n> >> half things to happen:\n> >>\n> >> * Give a new name for the non-skipping algorithm, and describe the\n> >> algorithm like the above.\n> >>\n> >> * Describe \"default\" is \"non-skipping\" but \"feature.experimental\"\n> >> makes \"skipping\" the default.\n> >>\n> >> * Support \"non-skipping\" in the configuration parser, so that even\n> >> when something else becomes the default, people can choose it.\n> >>\n> >> I would think.\n> >\n> > Sounds good to me. I'm not very creative, so I think I'd just use\n> > \"non-skipping\" as the new name.\n>\n> I can't think of a better one either (aside from my already-suggested\n> \"exhaustive\"), but that's naming it in terms of the only other\n> negotiator.\n>\n> Is it the case that the only thing anyone would want to tweak about the\n> default one is its skipping behavior?\n>\n> E.g. if we were to make one called \"smart-topology\" or something (would\n> skip sending some OIDs by assuming things about branch/tag topology,\n> i.e. if you have X that probably includes Y) having negotiators \"A\",\n> \"non-A\", and \"C\" would be odd :)\n>\n> We're unlikely to change the \"default\" negotiatior behavior at this\n> point, so maybe some label that communicates \"the old one\" without\n> implying deprecation? Perhaps \"classic\"?\n\nOh, sorry for forgetting your \"exhaustive\" suggestion; sometimes\nweekends do that to you. I actually like that suggestion of yours a\nbit better. classic could also work.\n\nMaybe Jonathan also has ideas/preferences, since he wrote the \"skipping\" code?\n"},{"id":"447369","messageId":"xmqq5ypzimr3.fsf@gitster.g","threadId":"57327","inReplyTo":"220131.864k5jbo5h.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T22:06:08Z","receivedAt":"2022-01-31T22:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Sounds good to me. I'm not very creative, so I think I'd just use\n>> \"non-skipping\" as the new name.\n>\n> I can't think of a better one either (aside from my already-suggested\n> \"exhaustive\"), but that's naming it in terms of the only other\n> negotiator.\n\nSkipping and the other one are both commit graph walkers. The\ntraditional one reports each and every commit without skipping, so\nif the negation in \"non-skipping\" turns out to be problematic in\nnaming, perhaps we can say \"consecutive\" vs \"skipping\" as the\ndifferentiator between the two?\n\n> E.g. if we were to make one called \"smart-topology\" or something (would\n> skip sending some OIDs by assuming things about branch/tag topology,\n> i.e. if you have X that probably includes Y) having negotiators \"A\",\n> \"non-A\", and \"C\" would be odd :)\n\nIt is good to anticipate that somebody cleverly invents negotiator\nthat is not based on \"commit walker\" concept ;-)\n\n"},{"id":"447428","messageId":"pull.1131.v3.git.1643734828.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v2.git.1643478692337.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T17:00:25Z","receivedAt":"2022-02-01T17:00:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This regression is not new in v2.35; it first appeared in v2.34. So, not\nurgent.\n\nChanges since v2:\n\n * Also fix the fact that fetch.negotationAlgorithm=$BOGUS_VALUE no longer\n errors out (yet another regression, this one dating back to v2.24.0), and\n add a test to make sure we don't regress it again.\n * Add 'consecutive' as a synonym for 'default', and remove 'default' from\n the documentation to guide people towards using 'consecutive' when they\n want the classic behavior.\n\nChanges since v1:\n\n * Put the common code in two testcases into a function, and then just\n invoked it from each\n\nElijah Newren (3):\n repo-settings: fix checking for fetch.negotiationAlgorithm=default\n repo-settings: fix error handling for unknown values\n repo-settings: name the default fetch.negotiationAlgorithm\n 'consecutive'\n\n Documentation/config/fetch.txt | 21 ++++++++++-----------\n fetch-negotiator.c | 2 +-\n repo-settings.c | 7 ++++++-\n repository.h | 2 +-\n t/t5500-fetch-pack.sh | 24 +++++++++++++++++++++---\n 5 files changed, 39 insertions(+), 17 deletions(-)\n\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1131\n\nRange-diff vs v2:\n\n 1: 633c873b513 ! 1: df0ec5ffe98 repo-settings: fix checking for fetch.negotiationAlgorithm=default\n @@ t/t5500-fetch-pack.sh: test_expect_success 'use ref advertisement to prune \"have\n +test_expect_success 'same as last but with config overrides' '\n +\ttest_negotiation_algorithm_default \\\n +\t\t-c feature.experimental=true \\\n -+\t\t-c fetch.negotiationAlgorithm=default \\\n ++\t\t-c fetch.negotiationAlgorithm=default\n '\n \n test_expect_success 'filtering by size' '\n -: ----------- > 2: 23f692b81be repo-settings: fix error handling for unknown values\n -: ----------- > 3: 7b28c527a90 repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'\n\n-- \ngitgitgadget\n"},{"id":"447429","messageId":"df0ec5ffe98a17e1ec7b572085e733d8748c0379.1643734828.git.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v3.git.1643734828.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T17:00:26Z","receivedAt":"2022-02-01T17:00:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 3050b6dfc75d (repo-settings.c: simplify the setup,\n2021-09-21), the branch for handling fetch.negotiationAlgorithm=default\nwas deleted. Since this value is documented in\nDocumentation/config/fetch.txt, restore the check for this value.\n\nNote that this change caused an observable bug: if someone sets\nfeature.experimental=true in config, and then passes \"-c\nfetch.negotiationAlgorithm=default\" on the command line in an attempt to\noverride the config, then the override is ignored. Fix the bug by not\nignoring the value of \"default\".\n\nTechnically, before commit 3050b6dfc75d, repo-settings would treat any\nfetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\nrequest for \"default\", but I think it probably makes more sense to\nignore such broken requests and leave fetch.negotiationAlgorithm with\nthe default value rather than the value of \"default\". (If that sounds\nconfusing, note that \"default\" is usually the default value, but when\nfeature.experimental=true, \"skipping\" is the default value.)\n\nSigned-off-by: Elijah Newren \n---\n repo-settings.c | 2 ++\n t/t5500-fetch-pack.sh | 17 ++++++++++++++---\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 00ca5571a1a..38c10f9977b 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -85,6 +85,8 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n+\t\telse if (!strcasecmp(strval, \"default\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n \t/*\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex f0dc4e69686..666502ed832 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -927,7 +927,8 @@ test_expect_success 'fetching deepen' '\n \t)\n '\n \n-test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+test_negotiation_algorithm_default () {\n+\ttest_when_finished rm -rf clientv0 clientv2 &&\n \trm -rf server client &&\n \tgit init server &&\n \ttest_commit -C server both_have_1 &&\n@@ -946,7 +947,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \trm -f trace &&\n \tcp -r client clientv0 &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n-\t\tfetch origin server_has both_have_2 &&\n+\t\t\"$@\" fetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n@@ -954,10 +955,20 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \trm -f trace &&\n \tcp -r client clientv2 &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n-\t\tfetch origin server_has both_have_2 &&\n+\t\t\"$@\" fetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n+}\n+\n+test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+\ttest_negotiation_algorithm_default\n+'\n+\n+test_expect_success 'same as last but with config overrides' '\n+\ttest_negotiation_algorithm_default \\\n+\t\t-c feature.experimental=true \\\n+\t\t-c fetch.negotiationAlgorithm=default\n '\n \n test_expect_success 'filtering by size' '\n-- \ngitgitgadget\n\n"},{"id":"447430","messageId":"23f692b81be26072f44609df8c78d1b8d81f01d3.1643734828.git.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v3.git.1643734828.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] repo-settings: fix error handling for unknown values","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T17:00:27Z","receivedAt":"2022-02-01T17:00:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit af3a67de01 (\"negotiator: unknown fetch.negotiationAlgorithm\nshould error out\", 2018-08-01), error handling for an unknown\nfetch.negotiationAlgorithm was added with the code die()ing. This was\nalso added to the documentation for the fetch.negotiationAlgorithm\noption, to make it explicit that the code would die on unknown values.\n\nThis behavior was lost with commit aaf633c2ad (\"repo-settings: create\nfeature.experimental setting\", 2019-08-13). Restore it so that the\nbehavior again matches the documentation.\n\nSigned-off-by: Elijah Newren \n---\n repo-settings.c | 2 ++\n t/t5500-fetch-pack.sh | 7 +++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 38c10f9977b..41e1c30845f 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -87,6 +87,8 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n \t\telse if (!strcasecmp(strval, \"default\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n+\t\telse\n+\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n \t}\n \n \t/*\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 666502ed832..41ea9f25de6 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -971,6 +971,13 @@ test_expect_success 'same as last but with config overrides' '\n \t\t-c fetch.negotiationAlgorithm=default\n '\n \n+test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n+\ttest_when_finished rm -rf clientv0 &&\n+\tcp -r client clientv0 &&\n+\ttest_must_fail git -C clientv0 --fetch.negotiationAlgorithm=bogus \\\n+\t\t fetch origin server_has both_have_2\n+'\n+\n test_expect_success 'filtering by size' '\n \trm -rf server client &&\n \ttest_create_repo server &&\n-- \ngitgitgadget\n\n"},{"id":"447431","messageId":"7b28c527a907c81c3d3d8a6a54d5c713cef4f2a0.1643734828.git.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v3.git.1643734828.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T17:00:28Z","receivedAt":"2022-02-01T17:00:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nGive the default fetch.negotiationAlgorithm the name 'consecutive' and\nupdate the documentation accordingly. Since there may be some users\nusing the name 'default' for this behavior, retain that name for now.\nWe do not want to use that name indefinitely, though, because if\n'skipping' becomes the default, then the \"default\" behavior will not be\nthe default behavior, which would be confusing.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/config/fetch.txt | 21 ++++++++++-----------\n fetch-negotiator.c | 2 +-\n repo-settings.c | 7 ++++---\n repository.h | 2 +-\n t/t5500-fetch-pack.sh | 2 +-\n 5 files changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex 63748c02b72..926bcfd8b48 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -56,17 +56,16 @@ fetch.output::\n \tOUTPUT in linkgit:git-fetch[1] for detail.\n \n fetch.negotiationAlgorithm::\n-\tControl how information about the commits in the local repository is\n-\tsent when negotiating the contents of the packfile to be sent by the\n-\tserver. Set to \"skipping\" to use an algorithm that skips commits in an\n-\teffort to converge faster, but may result in a larger-than-necessary\n-\tpackfile; or set to \"noop\" to not send any information at all, which\n-\twill almost certainly result in a larger-than-necessary packfile, but\n-\twill skip the negotiation step.\n-\tThe default is \"default\" which instructs Git to use the default algorithm\n-\tthat never skips commits (unless the server has acknowledged it or one\n-\tof its descendants). If `feature.experimental` is enabled, then this\n-\tsetting defaults to \"skipping\".\n+\tControl how information about the commits in the local repository\n+\tis sent when negotiating the contents of the packfile to be sent by\n+\tthe server. Set to \"consecutive\" to use an algorithm that walks\n+\tover consecutive commits checking each one. Set to \"skipping\" to\n+\tuse an algorithm that skips commits in an effort to converge\n+\tfaster, but may result in a larger-than-necessary packfile; or set\n+\tto \"noop\" to not send any information at all, which will almost\n+\tcertainly result in a larger-than-necessary packfile, but will skip\n+\tthe negotiation step. The default is normally \"consecutive\", but\n+\tif `feature.experimental` is true, then the default is \"skipping\".\n \tUnknown values will cause 'git fetch' to error out.\n +\n See also the `--negotiate-only` and `--negotiation-tip` options to\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 273390229fe..874797d767b 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -18,7 +18,7 @@ void fetch_negotiator_init(struct repository *r,\n \t\tnoop_negotiator_init(negotiator);\n \t\treturn;\n \n-\tcase FETCH_NEGOTIATION_DEFAULT:\n+\tcase FETCH_NEGOTIATION_CONSECUTIVE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n \t}\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 41e1c30845f..e984075df12 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)\n \t/* Defaults */\n \tr->settings.index_version = -1;\n \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n-\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n+\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n \n \t/* Booleans config or default, cascades to other settings */\n \trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n@@ -85,8 +85,9 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n-\t\telse if (!strcasecmp(strval, \"default\"))\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n+\t\telse if (!strcasecmp(strval, \"consecutive\") ||\n+\t\t\t !strcasecmp(strval, \"default\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n \t\telse\n \t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n \t}\ndiff --git a/repository.h b/repository.h\nindex 2b5cf97f31e..ca837cb9e91 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -20,7 +20,7 @@ enum untracked_cache_setting {\n };\n \n enum fetch_negotiation_setting {\n-\tFETCH_NEGOTIATION_DEFAULT,\n+\tFETCH_NEGOTIATION_CONSECUTIVE,\n \tFETCH_NEGOTIATION_SKIPPING,\n \tFETCH_NEGOTIATION_NOOP,\n };\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 41ea9f25de6..ee6d2dde9f3 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -968,7 +968,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n test_expect_success 'same as last but with config overrides' '\n \ttest_negotiation_algorithm_default \\\n \t\t-c feature.experimental=true \\\n-\t\t-c fetch.negotiationAlgorithm=default\n+\t\t-c fetch.negotiationAlgorithm=consecutive\n '\n \n test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n-- \ngitgitgadget\n"},{"id":"447434","messageId":"20220201173726.1577257-1-jonathantanmy@google.com","threadId":"57327","inReplyTo":"CABPp-BGRPOdhqV0uXGR2YfroB5Q3K2C6DiQqCdAbG4Xoi2-d9A@mail.gmail.com","subject":"Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-01T17:37:26Z","receivedAt":"2022-02-01T17:37:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Elijah Newren writes:\n> Maybe Jonathan also has ideas/preferences, since he wrote the \"skipping\" code?\n\nNo preferences - I think both \"consecutive\" and \"exhaustive\" make sense.\n"},{"id":"447439","messageId":"xmqq4k5ifnxg.fsf@gitster.g","threadId":"57327","inReplyTo":"df0ec5ffe98a17e1ec7b572085e733d8748c0379.1643734828.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T18:21:15Z","receivedAt":"2022-02-01T18:21:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> diff --git a/repo-settings.c b/repo-settings.c\n> index 00ca5571a1a..38c10f9977b 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -85,6 +85,8 @@ void prepare_repo_settings(struct repository *r)\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> \t\telse if (!strcasecmp(strval, \"noop\"))\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n> +\t\telse if (!strcasecmp(strval, \"default\"))\n> +\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n> \t}\n\nAfter this step, this function does:\n\n * fetch_negotiation_algorithm set to _DEFAULT\n * experimental bit flips it to _SKIPPING\n * config is read and skipping/noop/default overwrites it.\n\nwhich is better than ignoring \"default\", but it strikes me as\nunnatural. If it were done like this instead:\n\n * fetch_negotiation_algorithm is set to _DEFAULT\n * config is read and skipping/noop/default overwrites it.\n * experimental bit flips it to _SKIPPING only when it is still _DEFAULT\n\nthis bug wouldn't have happened, I suspect.\n\nMore importantly, those who want to say \"I want to keep up with the\n'possible future default', which is appropriately chosen by Git for\nopt-in experimenters\" by setting feature.experimental to true, would\nbe able to do so when the flow is reordered like so.\n\nSetting fetch.negotiationAlgorithm=default would be the way to do\nso.\n\nLet's read on and see later steps of this series achieves the same.\nPerhaps they may do the same reordering.\n\nThanks.\n"},{"id":"447440","messageId":"xmqqzgnae9c4.fsf@gitster.g","threadId":"57327","inReplyTo":"23f692b81be26072f44609df8c78d1b8d81f01d3.1643734828.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] repo-settings: fix error handling for unknown values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T18:21:47Z","receivedAt":"2022-02-01T18:21:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> In commit af3a67de01 (\"negotiator: unknown fetch.negotiationAlgorithm\n> should error out\", 2018-08-01), error handling for an unknown\n> fetch.negotiationAlgorithm was added with the code die()ing. This was\n> also added to the documentation for the fetch.negotiationAlgorithm\n> option, to make it explicit that the code would die on unknown values.\n>\n> This behavior was lost with commit aaf633c2ad (\"repo-settings: create\n> feature.experimental setting\", 2019-08-13). Restore it so that the\n> behavior again matches the documentation.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> repo-settings.c | 2 ++\n> t/t5500-fetch-pack.sh | 7 +++++++\n> 2 files changed, 9 insertions(+)\n>\n> diff --git a/repo-settings.c b/repo-settings.c\n> index 38c10f9977b..41e1c30845f 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -87,6 +87,8 @@ void prepare_repo_settings(struct repository *r)\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n> \t\telse if (!strcasecmp(strval, \"default\"))\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n> +\t\telse\n> +\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n> \t}\n\nMakes sense.\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 666502ed832..41ea9f25de6 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -971,6 +971,13 @@ test_expect_success 'same as last but with config overrides' '\n> \t\t-c fetch.negotiationAlgorithm=default\n> '\n> \n> +test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n> +\ttest_when_finished rm -rf clientv0 &&\n> +\tcp -r client clientv0 &&\n> +\ttest_must_fail git -C clientv0 --fetch.negotiationAlgorithm=bogus \\\n> +\t\t fetch origin server_has both_have_2\n> +'\n> +\n> test_expect_success 'filtering by size' '\n> \trm -rf server client &&\n> \ttest_create_repo server &&\n"},{"id":"447441","messageId":"xmqqleyue8pl.fsf@gitster.g","threadId":"57327","inReplyTo":"7b28c527a907c81c3d3d8a6a54d5c713cef4f2a0.1643734828.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T18:35:18Z","receivedAt":"2022-02-01T18:35:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Give the default fetch.negotiationAlgorithm the name 'consecutive' and\n> update the documentation accordingly. Since there may be some users\n> using the name 'default' for this behavior, retain that name for now.\n> We do not want to use that name indefinitely, though, because if\n> 'skipping' becomes the default, then the \"default\" behavior will not be\n> the default behavior, which would be confusing.\n\nThis is probably where we still disagree after the review exchange\nfor the previous round.\n\nI view the act of setting this variable to 'default' by the user as\nsaying \"let Git choose whatever is appropriate for me, which may\nchange over time as the Git project gains more insight\". In that\nworld view, 'default' that changes the behaviour with newer versions\nof Git, or when 'feature.experimental' is set/unset, is a desirable\noutcome.\n\n> fetch.negotiationAlgorithm::\n> +\tControl how information about the commits in the local repository\n> +\tis sent when negotiating the contents of the packfile to be sent by\n> +\tthe server. Set to \"consecutive\" to use an algorithm that walks\n> +\tover consecutive commits checking each one. Set to \"skipping\" to\n> +\tuse an algorithm that skips commits in an effort to converge\n> +\tfaster, but may result in a larger-than-necessary packfile; or set\n> +\tto \"noop\" to not send any information at all, which will almost\n> +\tcertainly result in a larger-than-necessary packfile, but will skip\n> +\tthe negotiation step. The default is normally \"consecutive\", but\n> +\tif `feature.experimental` is true, then the default is \"skipping\".\n\nSo, in my worldview, this statement is needed:\n\n Set to \"default\" to override settings made previously and use\n the default behaviour.\n\nbut of course if you do not want to allow a \"dear Git, please choose\nfor me appropriately\" setting, such a statement contradicts with it.\n\nAnd with the design in the patch, next person who notices the code\naccepts another value that is not documented will suggest an update\nto the documentation:\n\n The default is \"consecutive\" but \"skipping\" is used when\n feature.experimental is set. Setting it to \"default\" is the\n same as setting it to \"consecutive\".\n\nwhich looks quite confusing, no?\n\n> diff --git a/repo-settings.c b/repo-settings.c\n> index 41e1c30845f..e984075df12 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)\n> \t/* Defaults */\n> \tr->settings.index_version = -1;\n> \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n> -\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n> +\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n> \n> \t/* Booleans config or default, cascades to other settings */\n> \trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n> @@ -85,8 +85,9 @@ void prepare_repo_settings(struct repository *r)\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> \t\telse if (!strcasecmp(strval, \"noop\"))\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n> -\t\telse if (!strcasecmp(strval, \"default\"))\n> -\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n> +\t\telse if (!strcasecmp(strval, \"consecutive\") ||\n> +\t\t\t !strcasecmp(strval, \"default\"))\n> +\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n> \t\telse\n> \t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n> \t}\n\nSo, I am not sure this is a good idea.\n"},{"id":"447496","messageId":"pull.1131.v4.git.1643773361.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v3.git.1643734828.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-02T03:42:37Z","receivedAt":"2022-02-02T03:42:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This regression is not new in v2.35; it first appeared in v2.34. So, not\nurgent.\n\nChanges since v3:\n\n * 'consecutive' is used as the traditional default. 'default' means either\n 'consecutive' or 'skipping' depending on feature.experimental.\n\nChanges since v2:\n\n * Also fix the fact that fetch.negotationAlgorithm=$BOGUS_VALUE no longer\n errors out (yet another regression, this one dating back to v2.24.0), and\n add a test to make sure we don't regress it again.\n * Add 'consecutive' as a synonym for 'default', and remove 'default' from\n the documentation to guide people towards using 'consecutive' when they\n want the classic behavior.\n\nChanges since v1:\n\n * Put the common code in two testcases into a function, and then just\n invoked it from each\n\nElijah Newren (3):\n repo-settings: fix checking for fetch.negotiationAlgorithm=default\n repo-settings: fix error handling for unknown values\n repo-settings: rename the traditional default\n fetch.negotiationAlgorithm\n\n Documentation/config/fetch.txt | 25 +++++++++++++------------\n fetch-negotiator.c | 2 +-\n repo-settings.c | 9 ++++++++-\n repository.h | 2 +-\n t/t5500-fetch-pack.sh | 24 +++++++++++++++++++++---\n 5 files changed, 44 insertions(+), 18 deletions(-)\n\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1131\n\nRange-diff vs v3:\n\n 1: df0ec5ffe98 = 1: df0ec5ffe98 repo-settings: fix checking for fetch.negotiationAlgorithm=default\n 2: 23f692b81be = 2: 23f692b81be repo-settings: fix error handling for unknown values\n 3: 7b28c527a90 ! 3: 7500a4d2e44 repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'\n + repo-settings: rename the traditional default fetch.negotiationAlgorithm\n \n - Give the default fetch.negotiationAlgorithm the name 'consecutive' and\n - update the documentation accordingly. Since there may be some users\n - using the name 'default' for this behavior, retain that name for now.\n - We do not want to use that name indefinitely, though, because if\n - 'skipping' becomes the default, then the \"default\" behavior will not be\n - the default behavior, which would be confusing.\n + Give the traditional default fetch.negotiationAlgorithm the name\n + 'consecutive'. Also allow a choice of 'default' to have Git decide\n + between the choices (currently, picking 'skipping' if\n + feature.experimental is true and 'consecutive' otherwise). Update the\n + documentation accordingly.\n \n Signed-off-by: Elijah Newren \n \n @@ Documentation/config/fetch.txt: fetch.output::\n -\tthat never skips commits (unless the server has acknowledged it or one\n -\tof its descendants). If `feature.experimental` is enabled, then this\n -\tsetting defaults to \"skipping\".\n +-\tUnknown values will cause 'git fetch' to error out.\n +\tControl how information about the commits in the local repository\n +\tis sent when negotiating the contents of the packfile to be sent by\n +\tthe server. Set to \"consecutive\" to use an algorithm that walks\n @@ Documentation/config/fetch.txt: fetch.output::\n +\tfaster, but may result in a larger-than-necessary packfile; or set\n +\tto \"noop\" to not send any information at all, which will almost\n +\tcertainly result in a larger-than-necessary packfile, but will skip\n -+\tthe negotiation step. The default is normally \"consecutive\", but\n -+\tif `feature.experimental` is true, then the default is \"skipping\".\n - \tUnknown values will cause 'git fetch' to error out.\n ++\tthe negotiation step. Set to \"default\" to override settings made\n ++\tpreviously and use the default behaviour. The default is normally\n ++\t\"consecutive\", but if `feature.experimental` is true, then the\n ++\tdefault is \"skipping\". Unknown values will cause 'git fetch' to\n ++\terror out.\n +\n See also the `--negotiate-only` and `--negotiation-tip` options to\n + linkgit:git-fetch[1].\n \n ## fetch-negotiator.c ##\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n @@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n \t/* Booleans config or default, cascades to other settings */\n \trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n @@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n + \t}\n + \n + \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n ++\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n + \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n --\t\telse if (!strcasecmp(strval, \"default\"))\n --\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n -+\t\telse if (!strcasecmp(strval, \"consecutive\") ||\n -+\t\t\t !strcasecmp(strval, \"default\"))\n ++\t\telse if (!strcasecmp(strval, \"consecutive\"))\n +\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n + \t\telse if (!strcasecmp(strval, \"default\"))\n +-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n ++\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n \t\telse\n \t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n \t}\n\n-- \ngitgitgadget\n"},{"id":"447497","messageId":"df0ec5ffe98a17e1ec7b572085e733d8748c0379.1643773361.git.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v4.git.1643773361.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-02T03:42:38Z","receivedAt":"2022-02-02T03:42:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 3050b6dfc75d (repo-settings.c: simplify the setup,\n2021-09-21), the branch for handling fetch.negotiationAlgorithm=default\nwas deleted. Since this value is documented in\nDocumentation/config/fetch.txt, restore the check for this value.\n\nNote that this change caused an observable bug: if someone sets\nfeature.experimental=true in config, and then passes \"-c\nfetch.negotiationAlgorithm=default\" on the command line in an attempt to\noverride the config, then the override is ignored. Fix the bug by not\nignoring the value of \"default\".\n\nTechnically, before commit 3050b6dfc75d, repo-settings would treat any\nfetch.negotiationAlgorithm value other than \"skipping\" or \"noop\" as a\nrequest for \"default\", but I think it probably makes more sense to\nignore such broken requests and leave fetch.negotiationAlgorithm with\nthe default value rather than the value of \"default\". (If that sounds\nconfusing, note that \"default\" is usually the default value, but when\nfeature.experimental=true, \"skipping\" is the default value.)\n\nSigned-off-by: Elijah Newren \n---\n repo-settings.c | 2 ++\n t/t5500-fetch-pack.sh | 17 ++++++++++++++---\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 00ca5571a1a..38c10f9977b 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -85,6 +85,8 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n+\t\telse if (!strcasecmp(strval, \"default\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n \n \t/*\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex f0dc4e69686..666502ed832 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -927,7 +927,8 @@ test_expect_success 'fetching deepen' '\n \t)\n '\n \n-test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+test_negotiation_algorithm_default () {\n+\ttest_when_finished rm -rf clientv0 clientv2 &&\n \trm -rf server client &&\n \tgit init server &&\n \ttest_commit -C server both_have_1 &&\n@@ -946,7 +947,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \trm -f trace &&\n \tcp -r client clientv0 &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv0 \\\n-\t\tfetch origin server_has both_have_2 &&\n+\t\t\"$@\" fetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace &&\n@@ -954,10 +955,20 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n \trm -f trace &&\n \tcp -r client clientv2 &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C clientv2 -c protocol.version=2 \\\n-\t\tfetch origin server_has both_have_2 &&\n+\t\t\"$@\" fetch origin server_has both_have_2 &&\n \tgrep \"have $(git -C client rev-parse client_has)\" trace &&\n \tgrep \"have $(git -C client rev-parse both_have_2)\" trace &&\n \t! grep \"have $(git -C client rev-parse both_have_2^)\" trace\n+}\n+\n+test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n+\ttest_negotiation_algorithm_default\n+'\n+\n+test_expect_success 'same as last but with config overrides' '\n+\ttest_negotiation_algorithm_default \\\n+\t\t-c feature.experimental=true \\\n+\t\t-c fetch.negotiationAlgorithm=default\n '\n \n test_expect_success 'filtering by size' '\n-- \ngitgitgadget\n\n"},{"id":"447498","messageId":"23f692b81be26072f44609df8c78d1b8d81f01d3.1643773361.git.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v4.git.1643773361.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] repo-settings: fix error handling for unknown values","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-02T03:42:39Z","receivedAt":"2022-02-02T03:42:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit af3a67de01 (\"negotiator: unknown fetch.negotiationAlgorithm\nshould error out\", 2018-08-01), error handling for an unknown\nfetch.negotiationAlgorithm was added with the code die()ing. This was\nalso added to the documentation for the fetch.negotiationAlgorithm\noption, to make it explicit that the code would die on unknown values.\n\nThis behavior was lost with commit aaf633c2ad (\"repo-settings: create\nfeature.experimental setting\", 2019-08-13). Restore it so that the\nbehavior again matches the documentation.\n\nSigned-off-by: Elijah Newren \n---\n repo-settings.c | 2 ++\n t/t5500-fetch-pack.sh | 7 +++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 38c10f9977b..41e1c30845f 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -87,6 +87,8 @@ void prepare_repo_settings(struct repository *r)\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n \t\telse if (!strcasecmp(strval, \"default\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n+\t\telse\n+\t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n \t}\n \n \t/*\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 666502ed832..41ea9f25de6 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -971,6 +971,13 @@ test_expect_success 'same as last but with config overrides' '\n \t\t-c fetch.negotiationAlgorithm=default\n '\n \n+test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n+\ttest_when_finished rm -rf clientv0 &&\n+\tcp -r client clientv0 &&\n+\ttest_must_fail git -C clientv0 --fetch.negotiationAlgorithm=bogus \\\n+\t\t fetch origin server_has both_have_2\n+'\n+\n test_expect_success 'filtering by size' '\n \trm -rf server client &&\n \ttest_create_repo server &&\n-- \ngitgitgadget\n\n"},{"id":"447499","messageId":"7500a4d2e44008b1d8df9cc8b24b67ff973a98ae.1643773361.git.gitgitgadget@gmail.com","threadId":"57327","inReplyTo":"pull.1131.v4.git.1643773361.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] repo-settings: rename the traditional default fetch.negotiationAlgorithm","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-02T03:42:40Z","receivedAt":"2022-02-02T03:42:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nGive the traditional default fetch.negotiationAlgorithm the name\n'consecutive'. Also allow a choice of 'default' to have Git decide\nbetween the choices (currently, picking 'skipping' if\nfeature.experimental is true and 'consecutive' otherwise). Update the\ndocumentation accordingly.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/config/fetch.txt | 25 +++++++++++++------------\n fetch-negotiator.c | 2 +-\n repo-settings.c | 7 +++++--\n repository.h | 2 +-\n t/t5500-fetch-pack.sh | 2 +-\n 5 files changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex 63748c02b72..cd65d236b43 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -56,18 +56,19 @@ fetch.output::\n \tOUTPUT in linkgit:git-fetch[1] for detail.\n \n fetch.negotiationAlgorithm::\n-\tControl how information about the commits in the local repository is\n-\tsent when negotiating the contents of the packfile to be sent by the\n-\tserver. Set to \"skipping\" to use an algorithm that skips commits in an\n-\teffort to converge faster, but may result in a larger-than-necessary\n-\tpackfile; or set to \"noop\" to not send any information at all, which\n-\twill almost certainly result in a larger-than-necessary packfile, but\n-\twill skip the negotiation step.\n-\tThe default is \"default\" which instructs Git to use the default algorithm\n-\tthat never skips commits (unless the server has acknowledged it or one\n-\tof its descendants). If `feature.experimental` is enabled, then this\n-\tsetting defaults to \"skipping\".\n-\tUnknown values will cause 'git fetch' to error out.\n+\tControl how information about the commits in the local repository\n+\tis sent when negotiating the contents of the packfile to be sent by\n+\tthe server. Set to \"consecutive\" to use an algorithm that walks\n+\tover consecutive commits checking each one. Set to \"skipping\" to\n+\tuse an algorithm that skips commits in an effort to converge\n+\tfaster, but may result in a larger-than-necessary packfile; or set\n+\tto \"noop\" to not send any information at all, which will almost\n+\tcertainly result in a larger-than-necessary packfile, but will skip\n+\tthe negotiation step. Set to \"default\" to override settings made\n+\tpreviously and use the default behaviour. The default is normally\n+\t\"consecutive\", but if `feature.experimental` is true, then the\n+\tdefault is \"skipping\". Unknown values will cause 'git fetch' to\n+\terror out.\n +\n See also the `--negotiate-only` and `--negotiation-tip` options to\n linkgit:git-fetch[1].\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 273390229fe..874797d767b 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -18,7 +18,7 @@ void fetch_negotiator_init(struct repository *r,\n \t\tnoop_negotiator_init(negotiator);\n \t\treturn;\n \n-\tcase FETCH_NEGOTIATION_DEFAULT:\n+\tcase FETCH_NEGOTIATION_CONSECUTIVE:\n \t\tdefault_negotiator_init(negotiator);\n \t\treturn;\n \t}\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 41e1c30845f..b4fbd16cdcc 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)\n \t/* Defaults */\n \tr->settings.index_version = -1;\n \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n-\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n+\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n \n \t/* Booleans config or default, cascades to other settings */\n \trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n@@ -81,12 +81,15 @@ void prepare_repo_settings(struct repository *r)\n \t}\n \n \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n+\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\telse if (!strcasecmp(strval, \"noop\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n+\t\telse if (!strcasecmp(strval, \"consecutive\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n \t\telse if (!strcasecmp(strval, \"default\"))\n-\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n+\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n \t\telse\n \t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n \t}\ndiff --git a/repository.h b/repository.h\nindex 2b5cf97f31e..ca837cb9e91 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -20,7 +20,7 @@ enum untracked_cache_setting {\n };\n \n enum fetch_negotiation_setting {\n-\tFETCH_NEGOTIATION_DEFAULT,\n+\tFETCH_NEGOTIATION_CONSECUTIVE,\n \tFETCH_NEGOTIATION_SKIPPING,\n \tFETCH_NEGOTIATION_NOOP,\n };\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 41ea9f25de6..ee6d2dde9f3 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -968,7 +968,7 @@ test_expect_success 'use ref advertisement to prune \"have\" lines sent' '\n test_expect_success 'same as last but with config overrides' '\n \ttest_negotiation_algorithm_default \\\n \t\t-c feature.experimental=true \\\n-\t\t-c fetch.negotiationAlgorithm=default\n+\t\t-c fetch.negotiationAlgorithm=consecutive\n '\n \n test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n-- \ngitgitgadget\n"},{"id":"447547","messageId":"xmqqk0ed17l8.fsf@gitster.g","threadId":"57327","inReplyTo":"7500a4d2e44008b1d8df9cc8b24b67ff973a98ae.1643773361.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 3/3] repo-settings: rename the traditional default fetch.negotiationAlgorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-02T17:50:11Z","receivedAt":"2022-02-02T17:50:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> diff --git a/repo-settings.c b/repo-settings.c\n> index 41e1c30845f..b4fbd16cdcc 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)\n> \t/* Defaults */\n> \tr->settings.index_version = -1;\n> \tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n> -\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n> +\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n> \n> \t/* Booleans config or default, cascades to other settings */\n> \trepo_cfg_bool(r, \"feature.manyfiles\", &manyfiles, 0);\n> @@ -81,12 +81,15 @@ void prepare_repo_settings(struct repository *r)\n> \t}\n> \n> \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n> +\t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n> \t\tif (!strcasecmp(strval, \"skipping\"))\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n> \t\telse if (!strcasecmp(strval, \"noop\"))\n> \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n> +\t\telse if (!strcasecmp(strval, \"consecutive\"))\n> +\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;\n> \t\telse if (!strcasecmp(strval, \"default\"))\n> -\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n> +\t\t\tr->settings.fetch_negotiation_algorithm = fetch_default;\n> \t\telse\n> \t\t\tdie(\"unknown fetch negotiation algorithm '%s'\", strval);\n> \t}\n\nThis \n\n - set the default to whatever experimental says\n - parse the configuration and set it \n - to the specified value unless it is DEFAULT\n - to the value the experimental bit set as the default otherwise\n\ncertainly works, even though I find it a bit convoluted and\nbackwards. I have slight preference to \"if the user says 'default',\nhold onto it as a symbolic 'default' setting, and resolve it to a\nconcrete value at the very end\" pattern, which tends to handle the\n\"reverting to default\" case better.\n\nThere is the \"manyfiles\" precedent that sets index.version and\ncore.untrackedCache irreversibly nearby, and I am sympathetic to\nwhoever added the fetch_negotiation_algorithm support (it probably\nis not you, I am guessing) by mimicking it, so I am OK with the\nversion posted as-is.\n\nThanks.\n"}]}