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

["Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>]

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