repo-settings: fix checking for fetch.negotiationAlgorithm=default #1131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1131.git.1643334969216.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@7ffe623. |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
newren
force-pushed
the
fix-fetch-negotiation-algorithm-equals-default
branch
from
9c592ba
to
633c873
Compare
|
That t3705 test failing on "CI / win+VS test (8)" has been discussed on the mailing list and is independent of this series. |
|
/submit |
|
Submitted as pull.1131.v2.git.1643478692337.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into seen via git@6f3aa4f. |
In commit 3050b6d (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 3050b6d, 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>
In commit af3a67d ("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 aaf633c ("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>
newren
force-pushed
the
fix-fetch-negotiation-algorithm-equals-default
branch
from
633c873
to
7b28c52
Compare
|
/submit |
|
Submitted as pull.1131.v3.git.1643734828.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jonathan Tan wrote (reply to this): |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
This patch series was integrated into seen via git@5699f4c. |
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>
newren
force-pushed
the
fix-fetch-negotiation-algorithm-equals-default
branch
from
7b28c52
to
7500a4d
Compare
|
/submit |
|
Submitted as pull.1131.v4.git.1643773361.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -56,18 +56,19 @@ fetch.output:: | |||
| OUTPUT in linkgit:git-fetch[1] for detail. | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
|
This patch series was integrated into seen via git@e2a55cc. |
|
This patch series was integrated into seen via git@66b4ab0. |
|
There was a status update in the "New Topics" section about the branch Fix interaction between fetch.negotiationAlgorithm and feature.experimental configuration variables. Will merge to 'next'. source: <pull.1131.v4.git.1643773361.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@dd5cdf6. |
|
This patch series was integrated into seen via git@8b3421c. |
|
This patch series was integrated into seen via git@a0685ea. |
|
This patch series was integrated into next via git@95a8a91. |
|
This patch series was integrated into seen via git@d35c718. |
|
There was a status update in the "Cooking" section about the branch Fix interaction between fetch.negotiationAlgorithm and feature.experimental configuration variables. Will merge to 'master'. source: <pull.1131.v4.git.1643773361.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@04c7851. |
|
This patch series was integrated into seen via git@a4dae70. |
|
This patch series was integrated into seen via git@6afdf51. |
|
There was a status update in the "Cooking" section about the branch Fix interaction between fetch.negotiationAlgorithm and feature.experimental configuration variables. Will merge to 'master'. source: <pull.1131.v4.git.1643773361.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@2cb54d5. |
|
This patch series was integrated into seen via git@70ff41f. |
|
This patch series was integrated into master via git@70ff41f. |
|
This patch series was integrated into next via git@70ff41f. |
|
Closed via 70ff41f. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This regression is not new in v2.35; it first appeared in v2.34. So, not urgent.
Changes since v3:
Changes since v2:
Changes since v1:
cc: Derrick Stolee dstolee@microsoft.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Elijah Newren newren@gmail.com
cc: Jonathan Tan jonathantanmy@google.com