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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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