Re: [PATCH v3 3/3] repo-settings: name the default fetch.negotiationAlgorithm 'consecutive'

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2022, 18:35 UTC
Message-ID: <xmqqleyue8pl.fsf@gitster.g>
In-Reply-To: <7b28c527a907c81c3d3d8a6a54d5c713cef4f2a0.1643734828.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> 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.

This is probably where we still disagree after the review exchange for the previous round.

I view the act of setting this variable to 'default' by the user as saying "let Git choose whatever is appropriate for me, which may change over time as the Git project gains more insight". In that world view, 'default' that changes the behaviour with newer versions of Git, or when 'feature.experimental' is set/unset, is a desirable outcome.

Show 11 quoted lines

>  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 "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.  The default is normally "consecutive", but
> +	if `feature.experimental` is true, then the default is "skipping".

So, in my worldview, this statement is needed:

    Set to "default" to override settings made previously and use
    the default behaviour.

but of course if you do not want to allow a "dear Git, please choose for me appropriately" setting, such a statement contradicts with it.

And with the design in the patch, next person who notices the code accepts another value that is not documented will suggest an update to the documentation:

    The default is "consecutive" but "skipping" is used when
    feature.experimental is set.  Setting it to "default" is the
    same as setting it to "consecutive".

which looks quite confusing, no?

Show 25 quoted lines

> diff --git a/repo-settings.c b/repo-settings.c
> index 41e1c30845f..e984075df12 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);
> @@ -85,8 +85,9 @@ 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;
> +		else if (!strcasecmp(strval, "consecutive") ||
> +			 !strcasecmp(strval, "default"))
> +			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE;
>  		else
>  			die("unknown fetch negotiation algorithm '%s'", strval);
>  	}

So, I am not sure this is a good idea.