On Fri, Jan 28 2022, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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' '
