From a68c5b9eba0e62af19a84e1f8945fcdef06c96d1 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Feb 2022 03:42:38 +0000 Subject: [PATCH 1/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default 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 Signed-off-by: Junio C Hamano --- 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 b93e91a212..27f230681f 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -82,6 +82,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 8a5d3492c7..bc29f03ce4 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' ' From a9a136c23223bf6b211db0746f3c9f6769deb833 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Feb 2022 03:42:39 +0000 Subject: [PATCH 2/3] repo-settings: fix error handling for unknown values 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 Signed-off-by: Junio C Hamano --- 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 27f230681f..ab896fa84b 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -84,6 +84,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 bc29f03ce4..403d4e40a2 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 && From 714edc620c7ddca5d54ff148ac27da6b67217012 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Feb 2022 03:42:40 +0000 Subject: [PATCH 3/3] repo-settings: rename the traditional default fetch.negotiationAlgorithm 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 Signed-off-by: Junio C Hamano --- 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 63748c02b7..cd65d236b4 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 273390229f..874797d767 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 ab896fa84b..c9ca1fd427 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -23,7 +23,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); @@ -78,12 +78,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 a057653981..464e8ffe69 100644 --- a/repository.h +++ b/repository.h @@ -19,7 +19,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 403d4e40a2..fb94033a48 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' '