[PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren via GitGitGadget]

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".

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.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    repo-settings: fix checking for fetch.negotiationAlgorithm=default
    
    This regression is not new in v2.35; it first appeared in v2.34. So, not
    urgent.
    
    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...

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1131

 repo-settings.c       |  2 ++
 t/t5500-fetch-pack.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

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 &&

base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
-- 
gitgitgadget

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Ævar Arnfjörð Bjarmason]

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' '

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren]

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.

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Ævar Arnfjörð Bjarmason]

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) :)

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Junio C Hamano]

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.

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren]

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.

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Ævar Arnfjörð Bjarmason]

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"?

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren]

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?

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Jonathan Tan]

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.

Re: [PATCH] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Junio C Hamano]

Æ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 ;-)

[PATCH v2] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren via GitGitGadget]

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".

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.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    repo-settings: fix checking for fetch.negotiationAlgorithm=default
    
    This regression is not new in v2.35; it first appeared in v2.34. So, not
    urgent.
    
    Changes since v1:
    
     * Put the common code in two testcases into a function, and then just
       invoked it from each

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1131

Range-diff vs v1:

 1:  9c592baded1 ! 1:  633c873b513 repo-settings: fix checking for fetch.negotiationAlgorithm=default
     @@ repo-settings.c: void prepare_repo_settings(struct repository *r)
      
       ## t/t5500-fetch-pack.sh ##
      @@ t/t5500-fetch-pack.sh: test_expect_success 'fetching deepen' '
     + 	)
       '
       
     - test_expect_success 'use ref advertisement to prune "have" lines sent' '
     +-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 &&
       	test_commit -C server both_have_1 &&
      @@ t/t5500-fetch-pack.sh: 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 \
     +-		fetch origin server_has both_have_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 &&
     +@@ t/t5500-fetch-pack.sh: 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 \
     +-		fetch origin server_has both_have_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 &&
     ++test_expect_success 'use ref advertisement to prune "have" lines sent' '
     ++	test_negotiation_algorithm_default
     ++'
      +
     -+	rm -f trace &&
     -+	cp -r client clientv2 &&
     -+	GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \
     ++test_expect_success 'same as last but with config overrides' '
     ++	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
     -+'
     -+
     + '
     + 
       test_expect_success 'filtering by size' '
     - 	rm -rf server client &&
     - 	test_create_repo server &&


 repo-settings.c       |  2 ++
 t/t5500-fetch-pack.sh | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

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..0a737cf0a0a 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -927,7 +927,8 @@ 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 &&
 	test_commit -C server both_have_1 &&
@@ -946,7 +947,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 \
-		fetch origin server_has both_have_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 &&
@@ -954,10 +955,20 @@ 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 \
-		fetch origin server_has both_have_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 'use ref advertisement to prune "have" lines sent' '
+	test_negotiation_algorithm_default
+'
+
+test_expect_success 'same as last but with config overrides' '
+	test_negotiation_algorithm_default \
+		-c feature.experimental=true \
+		-c fetch.negotiationAlgorithm=default \
 '
 
 test_expect_success 'filtering by size' '

base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
-- 
gitgitgadget

[PATCH v3 0/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren via GitGitGadget]

This regression is not new in v2.35; it first appeared in v2.34. So, not
urgent.

Changes since v2:

 * Also fix the fact that fetch.negotationAlgorithm=$BOGUS_VALUE no longer
   errors out (yet another regression, this one dating back to v2.24.0), and
   add a test to make sure we don't regress it again.
 * Add 'consecutive' as a synonym for 'default', and remove 'default' from
   the documentation to guide people towards using 'consecutive' when they
   want the classic behavior.

Changes since v1:

 * Put the common code in two testcases into a function, and then just
   invoked it from each

Elijah Newren (3):
  repo-settings: fix checking for fetch.negotiationAlgorithm=default
  repo-settings: fix error handling for unknown values
  repo-settings: name the default fetch.negotiationAlgorithm
    'consecutive'

 Documentation/config/fetch.txt | 21 ++++++++++-----------
 fetch-negotiator.c             |  2 +-
 repo-settings.c                |  7 ++++++-
 repository.h                   |  2 +-
 t/t5500-fetch-pack.sh          | 24 +++++++++++++++++++++---
 5 files changed, 39 insertions(+), 17 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1131

Range-diff vs v2:

 1:  633c873b513 ! 1:  df0ec5ffe98 repo-settings: fix checking for fetch.negotiationAlgorithm=default
     @@ t/t5500-fetch-pack.sh: test_expect_success 'use ref advertisement to prune "have
      +test_expect_success 'same as last but with config overrides' '
      +	test_negotiation_algorithm_default \
      +		-c feature.experimental=true \
     -+		-c fetch.negotiationAlgorithm=default \
     ++		-c fetch.negotiationAlgorithm=default
       '
       
       test_expect_success 'filtering by size' '
 -:  ----------- > 2:  23f692b81be repo-settings: fix error handling for unknown values
 -:  ----------- > 3:  7b28c527a90 repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'

-- 
gitgitgadget

[PATCH v3 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren via GitGitGadget]

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".

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.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 repo-settings.c       |  2 ++
 t/t5500-fetch-pack.sh | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

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..666502ed832 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -927,7 +927,8 @@ 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 &&
 	test_commit -C server both_have_1 &&
@@ -946,7 +947,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 \
-		fetch origin server_has both_have_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 &&
@@ -954,10 +955,20 @@ 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 \
-		fetch origin server_has both_have_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 'use ref advertisement to prune "have" lines sent' '
+	test_negotiation_algorithm_default
+'
+
+test_expect_success 'same as last but with config overrides' '
+	test_negotiation_algorithm_default \
+		-c feature.experimental=true \
+		-c fetch.negotiationAlgorithm=default
 '
 
 test_expect_success 'filtering by size' '
-- 
gitgitgadget

Re: [PATCH v3 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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;
>  	}

After this step, this function does:

 * fetch_negotiation_algorithm set to _DEFAULT
 * experimental bit flips it to _SKIPPING
 * config is read and skipping/noop/default overwrites it.

which is better than ignoring "default", but it strikes me as
unnatural.  If it were done like this instead:

 * fetch_negotiation_algorithm is set to _DEFAULT
 * config is read and skipping/noop/default overwrites it.
 * experimental bit flips it to _SKIPPING only when it is still _DEFAULT

this bug wouldn't have happened, I suspect.

More importantly, those who want to say "I want to keep up with the
'possible future default', which is appropriately chosen by Git for
opt-in experimenters" by setting feature.experimental to true, would
be able to do so when the flow is reordered like so.

Setting fetch.negotiationAlgorithm=default would be the way to do
so.

Let's read on and see later steps of this series achieves the same.
Perhaps they may do the same reordering.

Thanks.

[PATCH v3 2/3] repo-settings: fix error handling for unknown values

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In commit af3a67de01 ("negotiator: unknown fetch.negotiationAlgorithm
should error out", 2018-08-01), error handling for an unknown
fetch.negotiationAlgorithm was added with the code die()ing.  This was
also added to the documentation for the fetch.negotiationAlgorithm
option, to make it explicit that the code would die on unknown values.

This behavior was lost with commit aaf633c2ad ("repo-settings: create
feature.experimental setting", 2019-08-13).  Restore it so that the
behavior again matches the documentation.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 repo-settings.c       | 2 ++
 t/t5500-fetch-pack.sh | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/repo-settings.c b/repo-settings.c
index 38c10f9977b..41e1c30845f 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -87,6 +87,8 @@ void prepare_repo_settings(struct repository *r)
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
 		else if (!strcasecmp(strval, "default"))
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+		else
+			die("unknown fetch negotiation algorithm '%s'", strval);
 	}
 
 	/*
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 666502ed832..41ea9f25de6 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -971,6 +971,13 @@ test_expect_success 'same as last but with config overrides' '
 		-c fetch.negotiationAlgorithm=default
 '
 
+test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
+	test_when_finished rm -rf clientv0 &&
+	cp -r client clientv0 &&
+	test_must_fail git -C clientv0 --fetch.negotiationAlgorithm=bogus \
+		       fetch origin server_has both_have_2
+'
+
 test_expect_success 'filtering by size' '
 	rm -rf server client &&
 	test_create_repo server &&
-- 
gitgitgadget

Re: [PATCH v3 2/3] repo-settings: fix error handling for unknown values

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> In commit af3a67de01 ("negotiator: unknown fetch.negotiationAlgorithm
> should error out", 2018-08-01), error handling for an unknown
> fetch.negotiationAlgorithm was added with the code die()ing.  This was
> also added to the documentation for the fetch.negotiationAlgorithm
> option, to make it explicit that the code would die on unknown values.
>
> This behavior was lost with commit aaf633c2ad ("repo-settings: create
> feature.experimental setting", 2019-08-13).  Restore it so that the
> behavior again matches the documentation.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  repo-settings.c       | 2 ++
>  t/t5500-fetch-pack.sh | 7 +++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/repo-settings.c b/repo-settings.c
> index 38c10f9977b..41e1c30845f 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -87,6 +87,8 @@ void prepare_repo_settings(struct repository *r)
>  			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
>  		else if (!strcasecmp(strval, "default"))
>  			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
> +		else
> +			die("unknown fetch negotiation algorithm '%s'", strval);
>  	}

Makes sense.

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 666502ed832..41ea9f25de6 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -971,6 +971,13 @@ test_expect_success 'same as last but with config overrides' '
>  		-c fetch.negotiationAlgorithm=default
>  '
>  
> +test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
> +	test_when_finished rm -rf clientv0 &&
> +	cp -r client clientv0 &&
> +	test_must_fail git -C clientv0 --fetch.negotiationAlgorithm=bogus \
> +		       fetch origin server_has both_have_2
> +'
> +
>  test_expect_success 'filtering by size' '
>  	rm -rf server client &&
>  	test_create_repo server &&

[PATCH v3 3/3] repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Give the default fetch.negotiationAlgorithm the name 'consecutive' and
update the documentation accordingly.  Since there may be some users
using the name 'default' for this behavior, retain that name for now.
We do not want to use that name indefinitely, though, because if
'skipping' becomes the default, then the "default" behavior will not be
the default behavior, which would be confusing.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/config/fetch.txt | 21 ++++++++++-----------
 fetch-negotiator.c             |  2 +-
 repo-settings.c                |  7 ++++---
 repository.h                   |  2 +-
 t/t5500-fetch-pack.sh          |  2 +-
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 63748c02b72..926bcfd8b48 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -56,17 +56,16 @@ fetch.output::
 	OUTPUT in linkgit:git-fetch[1] for detail.
 
 fetch.negotiationAlgorithm::
-	Control how information about the commits in the local repository is
-	sent when negotiating the contents of the packfile to be sent by the
-	server. Set to "skipping" to use an algorithm that skips commits in an
-	effort to converge faster, but may result in a larger-than-necessary
-	packfile; or set to "noop" to not send any information at all, which
-	will almost certainly result in a larger-than-necessary packfile, but
-	will skip the negotiation step.
-	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). If `feature.experimental` is enabled, then this
-	setting defaults to "skipping".
+	Control how information about the commits in the local repository
+	is sent when negotiating the contents of the packfile to be sent by
+	the server.  Set to "consecutive" to use an algorithm that walks
+	over consecutive commits checking each one.  Set to "skipping" to
+	use an algorithm that skips commits in an effort to converge
+	faster, but may result in a larger-than-necessary packfile; or set
+	to "noop" to not send any information at all, which will almost
+	certainly result in a larger-than-necessary packfile, but will skip
+	the negotiation step.  The default is normally "consecutive", but
+	if `feature.experimental` is true, then the default is "skipping".
 	Unknown values will cause 'git fetch' to error out.
 +
 See also the `--negotiate-only` and `--negotiation-tip` options to
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 273390229fe..874797d767b 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -18,7 +18,7 @@ void fetch_negotiator_init(struct repository *r,
 		noop_negotiator_init(negotiator);
 		return;
 
-	case FETCH_NEGOTIATION_DEFAULT:
+	case FETCH_NEGOTIATION_CONSECUTIVE:
 		default_negotiator_init(negotiator);
 		return;
 	}
diff --git a/repo-settings.c b/repo-settings.c
index 41e1c30845f..e984075df12 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)
 	/* Defaults */
 	r->settings.index_version = -1;
 	r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
-	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
 
 	/* Booleans config or default, cascades to other settings */
 	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
@@ -85,8 +85,9 @@ 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;
+		else if (!strcasecmp(strval, "consecutive") ||
+			 !strcasecmp(strval, "default"))
+			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
 		else
 			die("unknown fetch negotiation algorithm '%s'", strval);
 	}
diff --git a/repository.h b/repository.h
index 2b5cf97f31e..ca837cb9e91 100644
--- a/repository.h
+++ b/repository.h
@@ -20,7 +20,7 @@ enum untracked_cache_setting {
 };
 
 enum fetch_negotiation_setting {
-	FETCH_NEGOTIATION_DEFAULT,
+	FETCH_NEGOTIATION_CONSECUTIVE,
 	FETCH_NEGOTIATION_SKIPPING,
 	FETCH_NEGOTIATION_NOOP,
 };
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 41ea9f25de6..ee6d2dde9f3 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -968,7 +968,7 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' '
 test_expect_success 'same as last but with config overrides' '
 	test_negotiation_algorithm_default \
 		-c feature.experimental=true \
-		-c fetch.negotiationAlgorithm=default
+		-c fetch.negotiationAlgorithm=consecutive
 '
 
 test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
-- 
gitgitgadget

Re: [PATCH v3 3/3] repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> Give the default fetch.negotiationAlgorithm the name 'consecutive' and
> update the documentation accordingly.  Since there may be some users
> using the name 'default' for this behavior, retain that name for now.
> We do not want to use that name indefinitely, though, because if
> 'skipping' becomes the default, then the "default" behavior will not be
> the default behavior, which would be confusing.

This is probably where we still disagree after the review exchange
for the previous round.

I view the act of setting this variable to 'default' by the user as
saying "let Git choose whatever is appropriate for me, which may
change over time as the Git project gains more insight".  In that
world view, 'default' that changes the behaviour with newer versions
of Git, or when 'feature.experimental' is set/unset, is a desirable
outcome.

>  fetch.negotiationAlgorithm::
> +	Control how information about the commits in the local repository
> +	is sent when negotiating the contents of the packfile to be sent by
> +	the server.  Set to "consecutive" to use an algorithm that walks
> +	over consecutive commits checking each one.  Set to "skipping" to
> +	use an algorithm that skips commits in an effort to converge
> +	faster, but may result in a larger-than-necessary packfile; or set
> +	to "noop" to not send any information at all, which will almost
> +	certainly result in a larger-than-necessary packfile, but will skip
> +	the negotiation step.  The default is normally "consecutive", but
> +	if `feature.experimental` is true, then the default is "skipping".

So, in my worldview, this statement is needed:

    Set to "default" to override settings made previously and use
    the default behaviour.

but of course if you do not want to allow a "dear Git, please choose
for me appropriately" setting, such a statement contradicts with it.

And with the design in the patch, next person who notices the code
accepts another value that is not documented will suggest an update
to the documentation:

    The default is "consecutive" but "skipping" is used when
    feature.experimental is set.  Setting it to "default" is the
    same as setting it to "consecutive".

which looks quite confusing, no?

> diff --git a/repo-settings.c b/repo-settings.c
> index 41e1c30845f..e984075df12 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)
>  	/* Defaults */
>  	r->settings.index_version = -1;
>  	r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
> -	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
> +	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
>  
>  	/* Booleans config or default, cascades to other settings */
>  	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
> @@ -85,8 +85,9 @@ 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;
> +		else if (!strcasecmp(strval, "consecutive") ||
> +			 !strcasecmp(strval, "default"))
> +			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
>  		else
>  			die("unknown fetch negotiation algorithm '%s'", strval);
>  	}

So, I am not sure this is a good idea.

[PATCH v4 0/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren via GitGitGadget]

This regression is not new in v2.35; it first appeared in v2.34. So, not
urgent.

Changes since v3:

 * 'consecutive' is used as the traditional default. 'default' means either
   'consecutive' or 'skipping' depending on feature.experimental.

Changes since v2:

 * Also fix the fact that fetch.negotationAlgorithm=$BOGUS_VALUE no longer
   errors out (yet another regression, this one dating back to v2.24.0), and
   add a test to make sure we don't regress it again.
 * Add 'consecutive' as a synonym for 'default', and remove 'default' from
   the documentation to guide people towards using 'consecutive' when they
   want the classic behavior.

Changes since v1:

 * Put the common code in two testcases into a function, and then just
   invoked it from each

Elijah Newren (3):
  repo-settings: fix checking for fetch.negotiationAlgorithm=default
  repo-settings: fix error handling for unknown values
  repo-settings: rename the traditional default
    fetch.negotiationAlgorithm

 Documentation/config/fetch.txt | 25 +++++++++++++------------
 fetch-negotiator.c             |  2 +-
 repo-settings.c                |  9 ++++++++-
 repository.h                   |  2 +-
 t/t5500-fetch-pack.sh          | 24 +++++++++++++++++++++---
 5 files changed, 44 insertions(+), 18 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1131

Range-diff vs v3:

 1:  df0ec5ffe98 = 1:  df0ec5ffe98 repo-settings: fix checking for fetch.negotiationAlgorithm=default
 2:  23f692b81be = 2:  23f692b81be repo-settings: fix error handling for unknown values
 3:  7b28c527a90 ! 3:  7500a4d2e44 repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'
     +    repo-settings: rename the traditional default fetch.negotiationAlgorithm
      
     -    Give the default fetch.negotiationAlgorithm the name 'consecutive' and
     -    update the documentation accordingly.  Since there may be some users
     -    using the name 'default' for this behavior, retain that name for now.
     -    We do not want to use that name indefinitely, though, because if
     -    'skipping' becomes the default, then the "default" behavior will not be
     -    the default behavior, which would be confusing.
     +    Give the traditional default fetch.negotiationAlgorithm the name
     +    'consecutive'.  Also allow a choice of 'default' to have Git decide
     +    between the choices (currently, picking 'skipping' if
     +    feature.experimental is true and 'consecutive' otherwise).  Update the
     +    documentation accordingly.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ Documentation/config/fetch.txt: fetch.output::
      -	that never skips commits (unless the server has acknowledged it or one
      -	of its descendants). If `feature.experimental` is enabled, then this
      -	setting defaults to "skipping".
     +-	Unknown values will cause 'git fetch' to error out.
      +	Control how information about the commits in the local repository
      +	is sent when negotiating the contents of the packfile to be sent by
      +	the server.  Set to "consecutive" to use an algorithm that walks
     @@ Documentation/config/fetch.txt: fetch.output::
      +	faster, but may result in a larger-than-necessary packfile; or set
      +	to "noop" to not send any information at all, which will almost
      +	certainly result in a larger-than-necessary packfile, but will skip
     -+	the negotiation step.  The default is normally "consecutive", but
     -+	if `feature.experimental` is true, then the default is "skipping".
     - 	Unknown values will cause 'git fetch' to error out.
     ++	the negotiation step.  Set to "default" to override settings made
     ++	previously and use the default behaviour.  The default is normally
     ++	"consecutive", but if `feature.experimental` is true, then the
     ++	default is "skipping".  Unknown values will cause 'git fetch' to
     ++	error out.
       +
       See also the `--negotiate-only` and `--negotiation-tip` options to
     + linkgit:git-fetch[1].
      
       ## fetch-negotiator.c ##
      @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
     @@ repo-settings.c: void prepare_repo_settings(struct repository *r)
       	/* Booleans config or default, cascades to other settings */
       	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
      @@ repo-settings.c: void prepare_repo_settings(struct repository *r)
     + 	}
     + 
     + 	if (!repo_config_get_string(r, "fetch.negotiationalgorithm", &strval)) {
     ++		int fetch_default = r->settings.fetch_negotiation_algorithm;
     + 		if (!strcasecmp(strval, "skipping"))
       			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;
     -+		else if (!strcasecmp(strval, "consecutive") ||
     -+			 !strcasecmp(strval, "default"))
     ++		else if (!strcasecmp(strval, "consecutive"))
      +			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
     + 		else if (!strcasecmp(strval, "default"))
     +-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
     ++			r->settings.fetch_negotiation_algorithm = fetch_default;
       		else
       			die("unknown fetch negotiation algorithm '%s'", strval);
       	}

-- 
gitgitgadget

[PATCH v4 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default

[Elijah Newren via GitGitGadget]

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".

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.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 repo-settings.c       |  2 ++
 t/t5500-fetch-pack.sh | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

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..666502ed832 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -927,7 +927,8 @@ 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 &&
 	test_commit -C server both_have_1 &&
@@ -946,7 +947,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 \
-		fetch origin server_has both_have_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 &&
@@ -954,10 +955,20 @@ 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 \
-		fetch origin server_has both_have_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 'use ref advertisement to prune "have" lines sent' '
+	test_negotiation_algorithm_default
+'
+
+test_expect_success 'same as last but with config overrides' '
+	test_negotiation_algorithm_default \
+		-c feature.experimental=true \
+		-c fetch.negotiationAlgorithm=default
 '
 
 test_expect_success 'filtering by size' '
-- 
gitgitgadget

[PATCH v4 2/3] repo-settings: fix error handling for unknown values

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In commit af3a67de01 ("negotiator: unknown fetch.negotiationAlgorithm
should error out", 2018-08-01), error handling for an unknown
fetch.negotiationAlgorithm was added with the code die()ing.  This was
also added to the documentation for the fetch.negotiationAlgorithm
option, to make it explicit that the code would die on unknown values.

This behavior was lost with commit aaf633c2ad ("repo-settings: create
feature.experimental setting", 2019-08-13).  Restore it so that the
behavior again matches the documentation.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 repo-settings.c       | 2 ++
 t/t5500-fetch-pack.sh | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/repo-settings.c b/repo-settings.c
index 38c10f9977b..41e1c30845f 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -87,6 +87,8 @@ void prepare_repo_settings(struct repository *r)
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
 		else if (!strcasecmp(strval, "default"))
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+		else
+			die("unknown fetch negotiation algorithm '%s'", strval);
 	}
 
 	/*
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 666502ed832..41ea9f25de6 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -971,6 +971,13 @@ test_expect_success 'same as last but with config overrides' '
 		-c fetch.negotiationAlgorithm=default
 '
 
+test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
+	test_when_finished rm -rf clientv0 &&
+	cp -r client clientv0 &&
+	test_must_fail git -C clientv0 --fetch.negotiationAlgorithm=bogus \
+		       fetch origin server_has both_have_2
+'
+
 test_expect_success 'filtering by size' '
 	rm -rf server client &&
 	test_create_repo server &&
-- 
gitgitgadget

[PATCH v4 3/3] repo-settings: rename the traditional default fetch.negotiationAlgorithm

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Give the traditional default fetch.negotiationAlgorithm the name
'consecutive'.  Also allow a choice of 'default' to have Git decide
between the choices (currently, picking 'skipping' if
feature.experimental is true and 'consecutive' otherwise).  Update the
documentation accordingly.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/config/fetch.txt | 25 +++++++++++++------------
 fetch-negotiator.c             |  2 +-
 repo-settings.c                |  7 +++++--
 repository.h                   |  2 +-
 t/t5500-fetch-pack.sh          |  2 +-
 5 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 63748c02b72..cd65d236b43 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -56,18 +56,19 @@ fetch.output::
 	OUTPUT in linkgit:git-fetch[1] for detail.
 
 fetch.negotiationAlgorithm::
-	Control how information about the commits in the local repository is
-	sent when negotiating the contents of the packfile to be sent by the
-	server. Set to "skipping" to use an algorithm that skips commits in an
-	effort to converge faster, but may result in a larger-than-necessary
-	packfile; or set to "noop" to not send any information at all, which
-	will almost certainly result in a larger-than-necessary packfile, but
-	will skip the negotiation step.
-	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). If `feature.experimental` is enabled, then this
-	setting defaults to "skipping".
-	Unknown values will cause 'git fetch' to error out.
+	Control how information about the commits in the local repository
+	is sent when negotiating the contents of the packfile to be sent by
+	the server.  Set to "consecutive" to use an algorithm that walks
+	over consecutive commits checking each one.  Set to "skipping" to
+	use an algorithm that skips commits in an effort to converge
+	faster, but may result in a larger-than-necessary packfile; or set
+	to "noop" to not send any information at all, which will almost
+	certainly result in a larger-than-necessary packfile, but will skip
+	the negotiation step.  Set to "default" to override settings made
+	previously and use the default behaviour.  The default is normally
+	"consecutive", but if `feature.experimental` is true, then the
+	default is "skipping".  Unknown values will cause 'git fetch' to
+	error out.
 +
 See also the `--negotiate-only` and `--negotiation-tip` options to
 linkgit:git-fetch[1].
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 273390229fe..874797d767b 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -18,7 +18,7 @@ void fetch_negotiator_init(struct repository *r,
 		noop_negotiator_init(negotiator);
 		return;
 
-	case FETCH_NEGOTIATION_DEFAULT:
+	case FETCH_NEGOTIATION_CONSECUTIVE:
 		default_negotiator_init(negotiator);
 		return;
 	}
diff --git a/repo-settings.c b/repo-settings.c
index 41e1c30845f..b4fbd16cdcc 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)
 	/* Defaults */
 	r->settings.index_version = -1;
 	r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
-	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
 
 	/* Booleans config or default, cascades to other settings */
 	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
@@ -81,12 +81,15 @@ void prepare_repo_settings(struct repository *r)
 	}
 
 	if (!repo_config_get_string(r, "fetch.negotiationalgorithm", &strval)) {
+		int fetch_default = r->settings.fetch_negotiation_algorithm;
 		if (!strcasecmp(strval, "skipping"))
 			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, "consecutive"))
+			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
 		else if (!strcasecmp(strval, "default"))
-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+			r->settings.fetch_negotiation_algorithm = fetch_default;
 		else
 			die("unknown fetch negotiation algorithm '%s'", strval);
 	}
diff --git a/repository.h b/repository.h
index 2b5cf97f31e..ca837cb9e91 100644
--- a/repository.h
+++ b/repository.h
@@ -20,7 +20,7 @@ enum untracked_cache_setting {
 };
 
 enum fetch_negotiation_setting {
-	FETCH_NEGOTIATION_DEFAULT,
+	FETCH_NEGOTIATION_CONSECUTIVE,
 	FETCH_NEGOTIATION_SKIPPING,
 	FETCH_NEGOTIATION_NOOP,
 };
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 41ea9f25de6..ee6d2dde9f3 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -968,7 +968,7 @@ test_expect_success 'use ref advertisement to prune "have" lines sent' '
 test_expect_success 'same as last but with config overrides' '
 	test_negotiation_algorithm_default \
 		-c feature.experimental=true \
-		-c fetch.negotiationAlgorithm=default
+		-c fetch.negotiationAlgorithm=consecutive
 '
 
 test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
-- 
gitgitgadget

Re: [PATCH v4 3/3] repo-settings: rename the traditional default fetch.negotiationAlgorithm

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/repo-settings.c b/repo-settings.c
> index 41e1c30845f..b4fbd16cdcc 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -26,7 +26,7 @@ void prepare_repo_settings(struct repository *r)
>  	/* Defaults */
>  	r->settings.index_version = -1;
>  	r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
> -	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
> +	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
>  
>  	/* Booleans config or default, cascades to other settings */
>  	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
> @@ -81,12 +81,15 @@ void prepare_repo_settings(struct repository *r)
>  	}
>  
>  	if (!repo_config_get_string(r, "fetch.negotiationalgorithm", &strval)) {
> +		int fetch_default = r->settings.fetch_negotiation_algorithm;
>  		if (!strcasecmp(strval, "skipping"))
>  			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, "consecutive"))
> +			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
>  		else if (!strcasecmp(strval, "default"))
> -			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
> +			r->settings.fetch_negotiation_algorithm = fetch_default;
>  		else
>  			die("unknown fetch negotiation algorithm '%s'", strval);
>  	}

This 

    - set the default to whatever experimental says
    - parse the configuration and set it 
      - to the specified value unless it is DEFAULT
      - to the value the experimental bit set as the default otherwise

certainly works, even though I find it a bit convoluted and
backwards.  I have slight preference to "if the user says 'default',
hold onto it as a symbolic 'default' setting, and resolve it to a
concrete value at the very end" pattern, which tends to handle the
"reverting to default" case better.

There is the "manyfiles" precedent that sets index.version and
core.untrackedCache irreversibly nearby, and I am sympathetic to
whoever added the fetch_negotiation_algorithm support (it probably
is not you, I am guessing) by mimicking it, so I am OK with the
version posted as-is.

Thanks.