[v4,0/3] repo-settings: fix checking for fetch.negotiationAlgorithm=default

Message

Bruce Perry via GitGitGadget Feb. 2, 2022, 3:42 a.m. UTC

This regression is not new in v2.35; it first appeared in v2.34. So, not
urgent.

Changes since v3:

 * 'consecutive' is used as the traditional default. 'default' means either
   'consecutive' or 'skipping' depending on feature.experimental.

Changes since v2:

 * Also fix the fact that fetch.negotationAlgorithm=$BOGUS_VALUE no longer
   errors out (yet another regression, this one dating back to v2.24.0), and
   add a test to make sure we don't regress it again.
 * Add 'consecutive' as a synonym for 'default', and remove 'default' from
   the documentation to guide people towards using 'consecutive' when they
   want the classic behavior.

Changes since v1:

 * Put the common code in two testcases into a function, and then just
   invoked it from each

Elijah Newren (3):
  repo-settings: fix checking for fetch.negotiationAlgorithm=default
  repo-settings: fix error handling for unknown values
  repo-settings: rename the traditional default
    fetch.negotiationAlgorithm

 Documentation/config/fetch.txt | 25 +++++++++++++------------
 fetch-negotiator.c             |  2 +-
 repo-settings.c                |  9 ++++++++-
 repository.h                   |  2 +-
 t/t5500-fetch-pack.sh          | 24 +++++++++++++++++++++---
 5 files changed, 44 insertions(+), 18 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1131%2Fnewren%2Ffix-fetch-negotiation-algorithm-equals-default-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1131/newren/fix-fetch-negotiation-algorithm-equals-default-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1131

Range-diff vs v3:

 1:  df0ec5ffe98 = 1:  df0ec5ffe98 repo-settings: fix checking for fetch.negotiationAlgorithm=default
 2:  23f692b81be = 2:  23f692b81be repo-settings: fix error handling for unknown values
 3:  7b28c527a90 ! 3:  7500a4d2e44 repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'
     @@ Metadata
      Author: Elijah Newren <[e-mail]>
      
       ## Commit message ##
     -    repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'
     +    repo-settings: rename the traditional default fetch.negotiationAlgorithm
      
     -    Give the default fetch.negotiationAlgorithm the name 'consecutive' and
     -    update the documentation accordingly.  Since there may be some users
     -    using the name 'default' for this behavior, retain that name for now.
     -    We do not want to use that name indefinitely, though, because if
     -    'skipping' becomes the default, then the "default" behavior will not be
     -    the default behavior, which would be confusing.
     +    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 <[e-mail]>
      
     @@ Documentation/config/fetch.txt: fetch.output::
      -	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
     @@ Documentation/config/fetch.txt: fetch.output::
      +	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 normally "consecutive", but
     -+	if `feature.experimental` is true, then the default is "skipping".
     - 	Unknown values will cause 'git fetch' to error out.
     ++	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].
      
       ## fetch-negotiator.c ##
      @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
     @@ repo-settings.c: void prepare_repo_settings(struct repository *r)
       	/* Booleans config or default, cascades to other settings */
       	repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
      @@ repo-settings.c: 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, "default"))
     --			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
     -+		else if (!strcasecmp(strval, "consecutive") ||
     -+			 !strcasecmp(strval, "default"))
     ++		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);
       	}