Re: [PATCH] config: set pack.useSparse=true by default

From: Jonathan Nieder <jrnieder@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] config: set pack.useSparse=true by default
Date: Thu, 19 Mar 2020 16:13:32 -0700	[thread overview]
Message-ID: <20200319231332.GB129493@google.com> (raw)
In-Reply-To: <pull.585.git.1584583110914.gitgitgadget@gmail.com>

Hi,

Derrick Stolee wrote:

> The pack.useSparse config option was introduced by 3d036eb0
> (pack-objects: create pack.useSparse setting, 2019-01-19) and was
> first available in v2.21.0. When enabled, the pack-objects process
> during 'git push' will use a sparse tree walk when deciding which
> trees and blobs to send to the remote. The algorithm was introduced
> by d5d2e93 (revision: implement sparse algorithm, 2019-01-16) and
> has been in production use by VFS for Git since around that time.
> The features.experimental config option also enabled pack.useSparse,
> so hopefully that has also increased exposure.
>
> It is worth noting that pack.useSparse has a possibility of
> sending more objects across a push, but requires a special
> arrangement of exact _copies_ across directories. There is a test
> in t5322-pack-objects-sparse.sh that demonstrates this possibility.
>
> Since the downside is unlikely but the upside is significant, set
> the default value of pack.useSparse to true. Remove it from the
> set of options implied by features.experimental.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/config/feature.txt | 3 ---
>  Documentation/config/pack.txt    | 4 ++--
>  repo-settings.c                  | 3 ++-
>  3 files changed, 4 insertions(+), 6 deletions(-)

Makes sense.  Thanks for writing it.

Should this have a test?

[...]
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -45,6 +45,8 @@ void prepare_repo_settings(struct repository *r)
>  
>  	if (!repo_config_get_bool(r, "pack.usesparse", &value))
>  		r->settings.pack_use_sparse = value;
> +	UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);
> +
>  	if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) {
>  		UPDATE_DEFAULT_BOOL(r->settings.index_version, 4);
>  		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);
> @@ -52,7 +54,6 @@ void prepare_repo_settings(struct repository *r)
>  	if (!repo_config_get_bool(r, "fetch.writecommitgraph", &value))
>  		r->settings.fetch_write_commit_graph = value;
>  	if (!repo_config_get_bool(r, "feature.experimental", &value) && value) {
> -		UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);
>  		UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);
>  		UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);
>  	}
> 

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>