| Message ID | pull.1131.git.1643334969216.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repo-settings: fix checking for fetch.negotiationAlgorithm=default | expand |
On Fri, Jan 28 2022, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > In commit 3050b6dfc75d (repo-settings.c: simplify the setup, > 2021-09-21), the branch for handling fetch.negotiationAlgorithm=default > was deleted. Since this value is documented in > Documentation/config/fetch.txt, restore the check for this value. > > Note that this change caused an observable bug: if someone sets > feature.experimental=true in config, and then passes "-c > fetch.negotiationAlgorithm=default" on the command line in an attempt to > override the config, then the override is ignored. Fix the bug by not > ignoring the value of "default". This fix looks good, thanks for fixing my mess. > Technically, before commit 3050b6dfc75d, repo-settings would treat any > fetch.negotiationAlgorithm value other than "skipping" or "noop" as a > request for "default", but I think it probably makes more sense to > ignore such broken requests and leave fetch.negotiationAlgorithm with > the default value rather than the value of "default". (If that sounds > confusing, note that "default" is usually the default value, but when > feature.experimental=true, "skipping" is the default value.) > > [...] > A long sidenote about naming things "default": > > Many years ago, in the Gnome community, there was a huge fight that > erupted, in part due to confusion over "default". There was a journalist > who had been a designer in a past life, who had a little friction with > the rest of the community, but intended well and generally improved > things. At some point, they suggested some changes to improve the > "default" theme (and they were a nice improvement), but not being a > developer the changes weren't communicated in the form of a patch. And > the changes accidentally got applied to the wrong theme: the default one > (yes, there was a theme named "default" which was not the default > theme). Now, basically no one used the default theme because it was so > hideously ugly. I think we suffered from a case of not being able to > change the default (again?) because no one could get an agreement on > what the default should be. Who did actually use the default theme, > though? The person writing the release notes (though they only used it > for taking screenshots to include in the release notes, and otherwise > used some other theme). So, with people under pressure for an imminent > release, there were screenshots that looked like garbage, and > investigation eventually uncovered that it was due to changes that were > meant for the "default" theme having accidentally been applied to the > default theme. It could have just been an amusing story if not for the > other unfortunate factors happening around the same time and the heated > and protracted flamewars that erupted. > > Don't name settings/themes/things "default" if it describes something > specific, since someone may come along and decide that something else > should be the default, and then you're stuck with a non-default > "default". Sadly, the name was already picked and documented so for > backward compatibility we need to support it... Funny story, I think this is only going to bite us if we don't switch the default over along with promoting this out of feature.experimental. I.e. =default should always be equivalent to not declaring that config at all anywhere, and not drift to being a reference to some name that happens to be "default", as in the GNOME case. In our case it's more of a story about the inconsistencies in our config space, i.e. some values you can't reset at all, some take empty values to do so, others "default" etc. > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index f0dc4e69686..37958a376ca 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -928,6 +928,7 @@ test_expect_success 'fetching deepen' ' > ' > > test_expect_success 'use ref advertisement to prune "have" lines sent' ' > + test_when_finished rm -rf clientv0 clientv2 && > rm -rf server client && > git init server && > test_commit -C server both_have_1 && > @@ -960,6 +961,45 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' > ! grep "have $(git -C client rev-parse both_have_2^)" trace > ' > > +test_expect_success 'same as last but with config overrides' ' Since it's the same as the preceding test, maybe we can squash this in to avoid the duplication? This works for me. diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 37958a376ca..3fb20eeec7e 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -927,7 +927,7 @@ test_expect_success 'fetching deepen' ' ) ' -test_expect_success 'use ref advertisement to prune "have" lines sent' ' +test_negotiation_algorithm_default () { test_when_finished rm -rf clientv0 clientv2 && rm -rf server client && git init server && @@ -946,7 +946,7 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' rm -f trace && cp -r client clientv0 && - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 $@ \ fetch origin server_has both_have_2 && grep "have $(git -C client rev-parse client_has)" trace && grep "have $(git -C client rev-parse both_have_2)" trace && @@ -954,50 +954,17 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' rm -f trace && cp -r client clientv2 && - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 $@ \ fetch origin server_has both_have_2 && grep "have $(git -C client rev-parse client_has)" trace && grep "have $(git -C client rev-parse both_have_2)" trace && ! grep "have $(git -C client rev-parse both_have_2^)" trace -' - -test_expect_success 'same as last but with config overrides' ' - test_when_finished rm -rf clientv0 clientv2 && - rm -rf server client && - git init server && - test_commit -C server both_have_1 && - git -C server tag -d both_have_1 && - test_commit -C server both_have_2 && - - git clone server client && - test_commit -C server server_has && - test_commit -C client client_has && - - # In both protocol v0 and v2, ensure that the parent of both_have_2 is - # not sent as a "have" line. The client should know that the server has - # both_have_2, so it only needs to inform the server that it has - # both_have_2, and the server can infer the rest. - - rm -f trace && - rm -rf clientv0 && - cp -r client clientv0 && - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ - -c feature.experimental=true \ - -c fetch.negotiationAlgorithm=default \ - fetch origin server_has both_have_2 && - grep "have $(git -C client rev-parse client_has)" trace && - grep "have $(git -C client rev-parse both_have_2)" trace && - ! grep "have $(git -C client rev-parse both_have_2^)" trace && +} - rm -f trace && - cp -r client clientv2 && - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ +test_expect_success 'use ref advertisement to prune "have" lines sent' ' + test_negotiation_algorithm_default \ -c feature.experimental=true \ - -c fetch.negotiationAlgorithm=default \ - fetch origin server_has both_have_2 && - grep "have $(git -C client rev-parse client_has)" trace && - grep "have $(git -C client rev-parse both_have_2)" trace && - ! grep "have $(git -C client rev-parse both_have_2^)" trace + -c fetch.negotiationAlgorithm=default ' test_expect_success 'filtering by size' '
Hi Ævar, On Thu, Jan 27, 2022 at 11:54 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Fri, Jan 28 2022, Elijah Newren via GitGitGadget wrote: > > > From: Elijah Newren <newren@gmail.com> > > > > In commit 3050b6dfc75d (repo-settings.c: simplify the setup, > > 2021-09-21), the branch for handling fetch.negotiationAlgorithm=default > > was deleted. Since this value is documented in > > Documentation/config/fetch.txt, restore the check for this value. > > > > Note that this change caused an observable bug: if someone sets > > feature.experimental=true in config, and then passes "-c > > fetch.negotiationAlgorithm=default" on the command line in an attempt to > > override the config, then the override is ignored. Fix the bug by not > > ignoring the value of "default". > > This fix looks good, thanks for fixing my mess. No worries, it was a pretty tiny mess. It was actually caught by testing rather than by an end user. (I hadn't updated our internal Git distribution since before v2.34 due to various other priorities. Since that distribution includes some patches that turn on feature.experimental, along with one testsuite change to explicitly request "default" handling in a test that was intended to check the "default" behavior, when I updated the distribution, that testcase failed.) And you made the code in repo-settings.c *so* much nicer to read. I think that's more important than this little issue. >> > Technically, before commit 3050b6dfc75d, repo-settings would treat any > > fetch.negotiationAlgorithm value other than "skipping" or "noop" as a > > request for "default", but I think it probably makes more sense to > > ignore such broken requests and leave fetch.negotiationAlgorithm with > > the default value rather than the value of "default". (If that sounds > > confusing, note that "default" is usually the default value, but when > > feature.experimental=true, "skipping" is the default value.) > > > > [...] > > A long sidenote about naming things "default": > > > > Many years ago, in the Gnome community, there was a huge fight that > > erupted, in part due to confusion over "default". There was a journalist > > who had been a designer in a past life, who had a little friction with > > the rest of the community, but intended well and generally improved > > things. At some point, they suggested some changes to improve the > > "default" theme (and they were a nice improvement), but not being a > > developer the changes weren't communicated in the form of a patch. And > > the changes accidentally got applied to the wrong theme: the default one > > (yes, there was a theme named "default" which was not the default > > theme). Now, basically no one used the default theme because it was so > > hideously ugly. I think we suffered from a case of not being able to > > change the default (again?) because no one could get an agreement on > > what the default should be. Who did actually use the default theme, > > though? The person writing the release notes (though they only used it > > for taking screenshots to include in the release notes, and otherwise > > used some other theme). So, with people under pressure for an imminent > > release, there were screenshots that looked like garbage, and > > investigation eventually uncovered that it was due to changes that were > > meant for the "default" theme having accidentally been applied to the > > default theme. It could have just been an amusing story if not for the > > other unfortunate factors happening around the same time and the heated > > and protracted flamewars that erupted. > > > > Don't name settings/themes/things "default" if it describes something > > specific, since someone may come along and decide that something else > > should be the default, and then you're stuck with a non-default > > "default". Sadly, the name was already picked and documented so for > > backward compatibility we need to support it... > > Funny story, I think this is only going to bite us if we don't switch > the default over along with promoting this out of feature.experimental. > > I.e. =default should always be equivalent to not declaring that config > at all anywhere, and not drift to being a reference to some name that > happens to be "default", as in the GNOME case. No, we have the same problem as the Gnome case. See this part of the documentation for fetch.negotiationAlgorithm: """ The default is "default" which instructs Git to use the default algorithm that never skips commits (unless the server has acknowledged it or one of its descendants). """ features.experimental turns on "skipping" as the default behavior, and that text clearly rules out the possibility that "default" could be used to mean "skipping". So, if that experimental feature graduates, then the default behavior of fetch.negotiationAlgorithm will NOT be the "default" behavior of fetch.negotationAlgorithm. > In our case it's more of a story about the inconsistencies in our config > space, i.e. some values you can't reset at all, some take empty values > to do so, others "default" etc. > > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > > index f0dc4e69686..37958a376ca 100755 > > --- a/t/t5500-fetch-pack.sh > > +++ b/t/t5500-fetch-pack.sh > > @@ -928,6 +928,7 @@ test_expect_success 'fetching deepen' ' > > ' > > > > test_expect_success 'use ref advertisement to prune "have" lines sent' ' > > + test_when_finished rm -rf clientv0 clientv2 && > > rm -rf server client && > > git init server && > > test_commit -C server both_have_1 && > > @@ -960,6 +961,45 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' > > ! grep "have $(git -C client rev-parse both_have_2^)" trace > > ' > > > > +test_expect_success 'same as last but with config overrides' ' > > Since it's the same as the preceding test, maybe we can squash this in > to avoid the duplication? This works for me. > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 37958a376ca..3fb20eeec7e 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -927,7 +927,7 @@ test_expect_success 'fetching deepen' ' > ) > ' > > -test_expect_success 'use ref advertisement to prune "have" lines sent' ' > +test_negotiation_algorithm_default () { > test_when_finished rm -rf clientv0 clientv2 && > rm -rf server client && > git init server && > @@ -946,7 +946,7 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' > > rm -f trace && > cp -r client clientv0 && > - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ > + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 $@ \ > fetch origin server_has both_have_2 && > grep "have $(git -C client rev-parse client_has)" trace && > grep "have $(git -C client rev-parse both_have_2)" trace && > @@ -954,50 +954,17 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' > > rm -f trace && > cp -r client clientv2 && > - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ > + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 $@ \ > fetch origin server_has both_have_2 && > grep "have $(git -C client rev-parse client_has)" trace && > grep "have $(git -C client rev-parse both_have_2)" trace && > ! grep "have $(git -C client rev-parse both_have_2^)" trace > -' > - > -test_expect_success 'same as last but with config overrides' ' > - test_when_finished rm -rf clientv0 clientv2 && > - rm -rf server client && > - git init server && > - test_commit -C server both_have_1 && > - git -C server tag -d both_have_1 && > - test_commit -C server both_have_2 && > - > - git clone server client && > - test_commit -C server server_has && > - test_commit -C client client_has && > - > - # In both protocol v0 and v2, ensure that the parent of both_have_2 is > - # not sent as a "have" line. The client should know that the server has > - # both_have_2, so it only needs to inform the server that it has > - # both_have_2, and the server can infer the rest. > - > - rm -f trace && > - rm -rf clientv0 && > - cp -r client clientv0 && > - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ > - -c feature.experimental=true \ > - -c fetch.negotiationAlgorithm=default \ > - fetch origin server_has both_have_2 && > - grep "have $(git -C client rev-parse client_has)" trace && > - grep "have $(git -C client rev-parse both_have_2)" trace && > - ! grep "have $(git -C client rev-parse both_have_2^)" trace && > +} > > - rm -f trace && > - cp -r client clientv2 && > - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ > +test_expect_success 'use ref advertisement to prune "have" lines sent' ' > + test_negotiation_algorithm_default \ > -c feature.experimental=true \ > - -c fetch.negotiationAlgorithm=default \ > - fetch origin server_has both_have_2 && > - grep "have $(git -C client rev-parse client_has)" trace && > - grep "have $(git -C client rev-parse both_have_2)" trace && > - ! grep "have $(git -C client rev-parse both_have_2^)" trace > + -c fetch.negotiationAlgorithm=default I think you accidentally dropped one of the two tests by turning it into a function and then only calling it for the latter usage and not the former, but I get your idea. It makes sense; I'll make the change.
On Fri, Jan 28 2022, Elijah Newren wrote: > Hi Ævar, > > On Thu, Jan 27, 2022 at 11:54 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > [...] >>> > Technically, before commit 3050b6dfc75d, repo-settings would treat any >> > fetch.negotiationAlgorithm value other than "skipping" or "noop" as a >> > request for "default", but I think it probably makes more sense to >> > ignore such broken requests and leave fetch.negotiationAlgorithm with >> > the default value rather than the value of "default". (If that sounds >> > confusing, note that "default" is usually the default value, but when >> > feature.experimental=true, "skipping" is the default value.) >> > >> > [...] >> > A long sidenote about naming things "default": >> > >> > Many years ago, in the Gnome community, there was a huge fight that >> > erupted, in part due to confusion over "default". There was a journalist >> > who had been a designer in a past life, who had a little friction with >> > the rest of the community, but intended well and generally improved >> > things. At some point, they suggested some changes to improve the >> > "default" theme (and they were a nice improvement), but not being a >> > developer the changes weren't communicated in the form of a patch. And >> > the changes accidentally got applied to the wrong theme: the default one >> > (yes, there was a theme named "default" which was not the default >> > theme). Now, basically no one used the default theme because it was so >> > hideously ugly. I think we suffered from a case of not being able to >> > change the default (again?) because no one could get an agreement on >> > what the default should be. Who did actually use the default theme, >> > though? The person writing the release notes (though they only used it >> > for taking screenshots to include in the release notes, and otherwise >> > used some other theme). So, with people under pressure for an imminent >> > release, there were screenshots that looked like garbage, and >> > investigation eventually uncovered that it was due to changes that were >> > meant for the "default" theme having accidentally been applied to the >> > default theme. It could have just been an amusing story if not for the >> > other unfortunate factors happening around the same time and the heated >> > and protracted flamewars that erupted. >> > >> > Don't name settings/themes/things "default" if it describes something >> > specific, since someone may come along and decide that something else >> > should be the default, and then you're stuck with a non-default >> > "default". Sadly, the name was already picked and documented so for >> > backward compatibility we need to support it... >> >> Funny story, I think this is only going to bite us if we don't switch >> the default over along with promoting this out of feature.experimental. >> >> I.e. =default should always be equivalent to not declaring that config >> at all anywhere, and not drift to being a reference to some name that >> happens to be "default", as in the GNOME case. > > No, we have the same problem as the Gnome case. See this part of the > documentation for fetch.negotiationAlgorithm: > > """ > The default is "default" which instructs Git to use the > default algorithm that never skips commits (unless the server has > acknowledged it or one of its descendants). > """ > > features.experimental turns on "skipping" as the default behavior, and > that text clearly rules out the possibility that "default" could be > used to mean "skipping". So, if that experimental feature graduates, > then the default behavior of fetch.negotiationAlgorithm will NOT be > the "default" behavior of fetch.negotationAlgorithm. I see what you mean, and I'm aware that I'm debating this with a native speaker :) FWIW I didn't read it that way, earlier it discusses "skipping", and here it's describing what the default is. But especially since you'd have no reason to set this except to "reset to default" I didn't take it to be a promise that the default wouldn't change. I.e. maybe we'll just make it "skipping" and drop the current "default" code, or we'll give the current "default" a name at that point. But I do see how us not having a name for the "defult" complicates that view of the world. For grep.patternType we've got the same thing, but "default" there is "basic", so that's a bit different. I do read log.date's "default" as being the sort of GNOME case you're describing however. But I don't think we'd ever change the default there, a date format is too subjective, whereas an internal algorithm is liable to change. But I think we should just change this to make it explicit (separate from this narrow bugfix). Maybe "exhaustive" would be a good permanent name for the default algorithm? >> In our case it's more of a story about the inconsistencies in our config >> space, i.e. some values you can't reset at all, some take empty values >> to do so, others "default" etc. >> [...] >> >> Since it's the same as the preceding test, maybe we can squash this in >> to avoid the duplication? This works for me. >> [...] >> - rm -f trace && >> - cp -r client clientv2 && >> - GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ >> +test_expect_success 'use ref advertisement to prune "have" lines sent' ' >> + test_negotiation_algorithm_default \ >> -c feature.experimental=true \ >> - -c fetch.negotiationAlgorithm=default \ >> - fetch origin server_has both_have_2 && >> - grep "have $(git -C client rev-parse client_has)" trace && >> - grep "have $(git -C client rev-parse both_have_2)" trace && >> - ! grep "have $(git -C client rev-parse both_have_2^)" trace >> + -c fetch.negotiationAlgorithm=default > > I think you accidentally dropped one of the two tests by turning it > into a function and then only calling it for the latter usage and not > the former, but I get your idea. It makes sense; I'll make the > change. Ah yes, oops. Yes we should clearly have the non-"-c [...]" case too (and it's what I was aiming for) :)
Elijah Newren <newren@gmail.com> writes: >> I.e. =default should always be equivalent to not declaring that config >> at all anywhere, and not drift to being a reference to some name that >> happens to be "default", as in the GNOME case. > > No, we have the same problem as the Gnome case. See this part of the > documentation for fetch.negotiationAlgorithm: > > """ > The default is "default" which instructs Git to use the > default algorithm that never skips commits (unless the server has > acknowledged it or one of its descendants). > """ That looks more like one of the bugs introduced when skipping was turned on for the "experimental" folks. To fix this, without turning skipping into the default too hastily, there needs two and half things to happen: * Give a new name for the non-skipping algorithm, and describe the algorithm like the above. * Describe "default" is "non-skipping" but "feature.experimental" makes "skipping" the default. * Support "non-skipping" in the configuration parser, so that even when something else becomes the default, people can choose it. I would think.
On Mon, Jan 31, 2022 at 8:57 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > >> I.e. =default should always be equivalent to not declaring that config > >> at all anywhere, and not drift to being a reference to some name that > >> happens to be "default", as in the GNOME case. > > > > No, we have the same problem as the Gnome case. See this part of the > > documentation for fetch.negotiationAlgorithm: > > > > """ > > The default is "default" which instructs Git to use the > > default algorithm that never skips commits (unless the server has > > acknowledged it or one of its descendants). > > """ > > That looks more like one of the bugs introduced when skipping was > turned on for the "experimental" folks. To fix this, without > turning skipping into the default too hastily, there needs two and > half things to happen: > > * Give a new name for the non-skipping algorithm, and describe the > algorithm like the above. > > * Describe "default" is "non-skipping" but "feature.experimental" > makes "skipping" the default. > > * Support "non-skipping" in the configuration parser, so that even > when something else becomes the default, people can choose it. > > I would think. Sounds good to me. I'm not very creative, so I think I'd just use "non-skipping" as the new name.
On Mon, Jan 31 2022, Elijah Newren wrote: > On Mon, Jan 31, 2022 at 8:57 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: >> >> >> I.e. =default should always be equivalent to not declaring that config >> >> at all anywhere, and not drift to being a reference to some name that >> >> happens to be "default", as in the GNOME case. >> > >> > No, we have the same problem as the Gnome case. See this part of the >> > documentation for fetch.negotiationAlgorithm: >> > >> > """ >> > The default is "default" which instructs Git to use the >> > default algorithm that never skips commits (unless the server has >> > acknowledged it or one of its descendants). >> > """ >> >> That looks more like one of the bugs introduced when skipping was >> turned on for the "experimental" folks. To fix this, without >> turning skipping into the default too hastily, there needs two and >> half things to happen: >> >> * Give a new name for the non-skipping algorithm, and describe the >> algorithm like the above. >> >> * Describe "default" is "non-skipping" but "feature.experimental" >> makes "skipping" the default. >> >> * Support "non-skipping" in the configuration parser, so that even >> when something else becomes the default, people can choose it. >> >> I would think. > > Sounds good to me. I'm not very creative, so I think I'd just use > "non-skipping" as the new name. I can't think of a better one either (aside from my already-suggested "exhaustive"), but that's naming it in terms of the only other negotiator. Is it the case that the only thing anyone would want to tweak about the default one is its skipping behavior? E.g. if we were to make one called "smart-topology" or something (would skip sending some OIDs by assuming things about branch/tag topology, i.e. if you have X that probably includes Y) having negotiators "A", "non-A", and "C" would be odd :) We're unlikely to change the "default" negotiatior behavior at this point, so maybe some label that communicates "the old one" without implying deprecation? Perhaps "classic"?
On Mon, Jan 31, 2022 at 1:17 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Mon, Jan 31 2022, Elijah Newren wrote: > > > On Mon, Jan 31, 2022 at 8:57 AM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Elijah Newren <newren@gmail.com> writes: > >> > >> >> I.e. =default should always be equivalent to not declaring that config > >> >> at all anywhere, and not drift to being a reference to some name that > >> >> happens to be "default", as in the GNOME case. > >> > > >> > No, we have the same problem as the Gnome case. See this part of the > >> > documentation for fetch.negotiationAlgorithm: > >> > > >> > """ > >> > The default is "default" which instructs Git to use the > >> > default algorithm that never skips commits (unless the server has > >> > acknowledged it or one of its descendants). > >> > """ > >> > >> That looks more like one of the bugs introduced when skipping was > >> turned on for the "experimental" folks. To fix this, without > >> turning skipping into the default too hastily, there needs two and > >> half things to happen: > >> > >> * Give a new name for the non-skipping algorithm, and describe the > >> algorithm like the above. > >> > >> * Describe "default" is "non-skipping" but "feature.experimental" > >> makes "skipping" the default. > >> > >> * Support "non-skipping" in the configuration parser, so that even > >> when something else becomes the default, people can choose it. > >> > >> I would think. > > > > Sounds good to me. I'm not very creative, so I think I'd just use > > "non-skipping" as the new name. > > I can't think of a better one either (aside from my already-suggested > "exhaustive"), but that's naming it in terms of the only other > negotiator. > > Is it the case that the only thing anyone would want to tweak about the > default one is its skipping behavior? > > E.g. if we were to make one called "smart-topology" or something (would > skip sending some OIDs by assuming things about branch/tag topology, > i.e. if you have X that probably includes Y) having negotiators "A", > "non-A", and "C" would be odd :) > > We're unlikely to change the "default" negotiatior behavior at this > point, so maybe some label that communicates "the old one" without > implying deprecation? Perhaps "classic"? Oh, sorry for forgetting your "exhaustive" suggestion; sometimes weekends do that to you. I actually like that suggestion of yours a bit better. classic could also work. Maybe Jonathan also has ideas/preferences, since he wrote the "skipping" code?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Sounds good to me. I'm not very creative, so I think I'd just use >> "non-skipping" as the new name. > > I can't think of a better one either (aside from my already-suggested > "exhaustive"), but that's naming it in terms of the only other > negotiator. Skipping and the other one are both commit graph walkers. The traditional one reports each and every commit without skipping, so if the negation in "non-skipping" turns out to be problematic in naming, perhaps we can say "consecutive" vs "skipping" as the differentiator between the two? > E.g. if we were to make one called "smart-topology" or something (would > skip sending some OIDs by assuming things about branch/tag topology, > i.e. if you have X that probably includes Y) having negotiators "A", > "non-A", and "C" would be odd :) It is good to anticipate that somebody cleverly invents negotiator that is not based on "commit walker" concept ;-)
Elijah Newren <newren@gmail.com> writes:
> Maybe Jonathan also has ideas/preferences, since he wrote the "skipping" code?
No preferences - I think both "consecutive" and "exhaustive" make sense.
diff --git a/repo-settings.c b/repo-settings.c index 00ca5571a1a..38c10f9977b 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -85,6 +85,8 @@ void prepare_repo_settings(struct repository *r) r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING; else if (!strcasecmp(strval, "noop")) r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP; + else if (!strcasecmp(strval, "default")) + r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT; } /* diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index f0dc4e69686..37958a376ca 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -928,6 +928,7 @@ test_expect_success 'fetching deepen' ' ' test_expect_success 'use ref advertisement to prune "have" lines sent' ' + test_when_finished rm -rf clientv0 clientv2 && rm -rf server client && git init server && test_commit -C server both_have_1 && @@ -960,6 +961,45 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' ' ! grep "have $(git -C client rev-parse both_have_2^)" trace ' +test_expect_success 'same as last but with config overrides' ' + test_when_finished rm -rf clientv0 clientv2 && + rm -rf server client && + git init server && + test_commit -C server both_have_1 && + git -C server tag -d both_have_1 && + test_commit -C server both_have_2 && + + git clone server client && + test_commit -C server server_has && + test_commit -C client client_has && + + # In both protocol v0 and v2, ensure that the parent of both_have_2 is + # not sent as a "have" line. The client should know that the server has + # both_have_2, so it only needs to inform the server that it has + # both_have_2, and the server can infer the rest. + + rm -f trace && + rm -rf clientv0 && + cp -r client clientv0 && + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ + -c feature.experimental=true \ + -c fetch.negotiationAlgorithm=default \ + fetch origin server_has both_have_2 && + grep "have $(git -C client rev-parse client_has)" trace && + grep "have $(git -C client rev-parse both_have_2)" trace && + ! grep "have $(git -C client rev-parse both_have_2^)" trace && + + rm -f trace && + cp -r client clientv2 && + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ + -c feature.experimental=true \ + -c fetch.negotiationAlgorithm=default \ + fetch origin server_has both_have_2 && + grep "have $(git -C client rev-parse client_has)" trace && + grep "have $(git -C client rev-parse both_have_2)" trace && + ! grep "have $(git -C client rev-parse both_have_2^)" trace +' + test_expect_success 'filtering by size' ' rm -rf server client && test_create_repo server &&