{"thread":{"id":"53804","subject":"[PATCH/RFC] config: default to protocol v2","startedAt":"2020-07-07T05:38:12Z","lastAt":"2020-07-09T23:00:40Z","messageCount":4,"participants":["Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401068","messageId":"20200707053805.GB784740@google.com","threadId":"53804","inReplyTo":null,"subject":"[PATCH/RFC] config: default to protocol v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-07T05:38:05Z","receivedAt":"2020-07-07T05:38:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Git 2.26 used protocol v2 as its default protocol, but soon after\nrelease, reports of edge-case regressions started rolling in.  So Git\n2.27 returned to protocol v0 as a default (but with the various fixes\nin place to make protocol v2 safe) and Git 2.28 will use protocol v0\nas default but enable protocol v2 for those adventurous users that\nenable experimental features by setting feature.experimental=true.\n\nThus if all goes well, by the time Git 2.29 is being released, we can\nbe confident in protocol v2 as a default again.  Make it the default.\n\nThis especially speeds up fetches from repositories with many refs,\nsuch as https://chromium.googlesource.com/chromium/src.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nMostly sending this to get the discussion started about what changes\nwe want before flipping the default.\n\nAre there tests we can run?  Should we make the negotiation code more\nsimilar?  Any other bits we'd want to change?\n\nThanks,\nJonathan\n\n Documentation/config/feature.txt  | 4 ----\n Documentation/config/protocol.txt | 3 +--\n protocol.c                        | 6 +-----\n 3 files changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 28c33602d52..4e3a5c0cebc 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -22,10 +22,6 @@ existing commit-graph file(s). Occasionally, these files will merge and the\n write may take longer. Having an updated commit-graph file helps performance\n of many Git commands, including `git merge-base`, `git push -f`, and\n `git log --graph`.\n-+\n-* `protocol.version=2` speeds up fetches from repositories with many refs by\n-allowing the client to specify which refs to list before the server lists\n-them.\n \n feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/Documentation/config/protocol.txt b/Documentation/config/protocol.txt\nindex c46e9b3d00a..756591d77b0 100644\n--- a/Documentation/config/protocol.txt\n+++ b/Documentation/config/protocol.txt\n@@ -48,8 +48,7 @@ protocol.version::\n \tIf set, clients will attempt to communicate with a server\n \tusing the specified protocol version.  If the server does\n \tnot support it, communication falls back to version 0.\n-\tIf unset, the default is `0`, unless `feature.experimental`\n-\tis enabled, in which case the default is `2`.\n+\tIf unset, the default is `2`.\n \tSupported versions:\n +\n --\ndiff --git a/protocol.c b/protocol.c\nindex d1dd3424bba..803bef5c87e 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -17,7 +17,6 @@ static enum protocol_version parse_protocol_version(const char *value)\n enum protocol_version get_protocol_version_config(void)\n {\n \tconst char *value;\n-\tint val;\n \tconst char *git_test_k = \"GIT_TEST_PROTOCOL_VERSION\";\n \tconst char *git_test_v;\n \n@@ -31,9 +30,6 @@ enum protocol_version get_protocol_version_config(void)\n \t\treturn version;\n \t}\n \n-\tif (!git_config_get_bool(\"feature.experimental\", &val) && val)\n-\t\treturn protocol_v2;\n-\n \tgit_test_v = getenv(git_test_k);\n \tif (git_test_v && *git_test_v) {\n \t\tenum protocol_version env = parse_protocol_version(git_test_v);\n@@ -43,7 +39,7 @@ enum protocol_version get_protocol_version_config(void)\n \t\treturn env;\n \t}\n \n-\treturn protocol_v0;\n+\treturn protocol_v2;\n }\n \n enum protocol_version determine_protocol_version_server(void)\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401177","messageId":"20200708045008.GC2303891@coredump.intra.peff.net","threadId":"53804","inReplyTo":"20200707053805.GB784740@google.com","subject":"Re: [PATCH/RFC] config: default to protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-08T04:50:08Z","receivedAt":"2020-07-08T04:50:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 06, 2020 at 10:38:05PM -0700, Jonathan Nieder wrote:\n\n> Git 2.26 used protocol v2 as its default protocol, but soon after\n> release, reports of edge-case regressions started rolling in.  So Git\n> 2.27 returned to protocol v0 as a default (but with the various fixes\n> in place to make protocol v2 safe) and Git 2.28 will use protocol v0\n> as default but enable protocol v2 for those adventurous users that\n> enable experimental features by setting feature.experimental=true.\n> \n> Thus if all goes well, by the time Git 2.29 is being released, we can\n> be confident in protocol v2 as a default again.  Make it the default.\n> \n> This especially speeds up fetches from repositories with many refs,\n> such as https://chromium.googlesource.com/chromium/src.\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Mostly sending this to get the discussion started about what changes\n> we want before flipping the default.\n\nI can't actually think of any changes we'd want to make. AFAIK aside\nfrom the negotiation problem, v2 is good to go. When we flipped it off\nby default for 2.27 out of caution, I had hoped we would flip it back on\nfor the 2.28 cycle to get more exposure.\n\nI guess it may be too late for that now if we wanted to get more testing\nand exposure during the development cycle. But I'm not entirely\nconvinced that buys us anything anyway. v2 was available via a config\nsetting for at least a year, and major hosting sites supported it, and\nstill nobody noticed the negotiation problem until it was turned on by\ndefault in 2.26.\n\nAnd that has been the only bug people have reported for 2.26. That\nimplies to me that:\n\n  - we won't get significantly more information by leaving v2-as-default\n    in \"next\" or even \"master\" before it actually hits a release\n\n  - there probably aren't other major problems lurking, given that\n    people clearly upgraded to 2.26, found the negotiation problem, but\n    never reported any other issues\n\n-Peff\n"},{"id":"401188","messageId":"xmqq7dve2etl.fsf@gitster.c.googlers.com","threadId":"53804","inReplyTo":"20200708045008.GC2303891@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] config: default to protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-08T15:42:46Z","receivedAt":"2020-07-08T15:42:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Mostly sending this to get the discussion started about what changes\n>> we want before flipping the default.\n>\n> I can't actually think of any changes we'd want to make. AFAIK aside\n> from the negotiation problem, v2 is good to go. When we flipped it off\n> by default for 2.27 out of caution, I had hoped we would flip it back on\n> for the 2.28 cycle to get more exposure.\n\nIf there were any changes we already can think of before going\npublic with v2 at this moment, it makes it definitely way too late\nto propose making it the default again.  I do not think of any\nchanges either, so I'd say it is a good sign ;-) \n\n> And that has been the only bug people have reported for 2.26. That\n> implies to me that:\n>\n>   - we won't get significantly more information by leaving v2-as-default\n>     in \"next\" or even \"master\" before it actually hits a release\n>\n>   - there probably aren't other major problems lurking, given that\n>     people clearly upgraded to 2.26, found the negotiation problem, but\n>     never reported any other issues\n\nI've already said elsewhere that it is way too late to propose this\nflipping back the default for this cycle but it was mainly out of\nprinciple.  I do agree with you and Jonathan that we won't see any\nfurther breakages in the v2 code until we expose more users to it by\nmaking it the default in a released version.\n\nI am afraid that \"there probably aren't other\" may be overly\noptimistic, as the bug in 2.26 crippled the negotiation logic and\nforced it to punt, which was so severe that it would have hidden any\nother bugs in the negotiation logic.  If there is another bug in v2\nnegotiation logic that makes the sender to omit objects that should\nbe sent, it would not have been observed in 2.26 because the effect\nof the more severe bug was to cripple the negotiation logic itself\nand to make it punt, sending more objects all the way down the\nhistory.  Now, with that larger bug fixed post 2.26, we can start to\nsee if there are other bugs hidden by it.\n\nIn any case, we've learned in 2.26 that it is unlikely that such\nbugs would be uncovered until v2 is made the default again in a\nreleased version to be used by more users.\n\nSo, let's flip the default in -rc0; we can revert if we see\nsomething funny in 2.28.1 in the worst case.\n\nThanks.\n"},{"id":"401274","messageId":"20200709230038.GB664420@coredump.intra.peff.net","threadId":"53804","inReplyTo":"xmqq7dve2etl.fsf@gitster.c.googlers.com","subject":"Re: [PATCH/RFC] config: default to protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-09T23:00:38Z","receivedAt":"2020-07-09T23:00:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2020 at 08:42:46AM -0700, Junio C Hamano wrote:\n\n> I am afraid that \"there probably aren't other\" may be overly\n> optimistic, as the bug in 2.26 crippled the negotiation logic and\n> forced it to punt, which was so severe that it would have hidden any\n> other bugs in the negotiation logic.  If there is another bug in v2\n> negotiation logic that makes the sender to omit objects that should\n> be sent, it would not have been observed in 2.26 because the effect\n> of the more severe bug was to cripple the negotiation logic itself\n> and to make it punt, sending more objects all the way down the\n> history.  Now, with that larger bug fixed post 2.26, we can start to\n> see if there are other bugs hidden by it.\n\nI half-agree with this. The negotiation logic wasn't completely broken,\nand usually did the right thing. It was only the max_in_vain counting\nthat was wrong. So definitely there could be another bug lurking that\nwas hidden by that failure, and/or our fix could be incomplete. But I\nthink we can have some confidence that there aren't other show-stopping\nbugs (in the negotiation code or elsewhere in v2) that showed up in\nother situations (and the real-world success reports we already got for\nthat particular bug are also encouraging).\n\nSo I'm not especially worried about having a repeat of the v2.26\nsituation (but I agree it's not impossible).\n\n> In any case, we've learned in 2.26 that it is unlikely that such\n> bugs would be uncovered until v2 is made the default again in a\n> released version to be used by more users.\n> \n> So, let's flip the default in -rc0; we can revert if we see\n> something funny in 2.28.1 in the worst case.\n\nAnd obviously I'm fine with this, given that my assessment of the risk\nis even less than yours. :)\n\n-Peff\n"}]}