Re: [GSOC PATCH v4 1/3] environment: move access to "core.sparsecheckout" into repo_settings

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2025, 16:15 UTC
Message-ID: <xmqqzfe6uyyh.fsf@gitster.g>
In-Reply-To: <e221c68ab52e995adbb175dc4a09f6c3dfeaf7c8.1750157825.git.ayu.chandekar@gmail.com>

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

Show 17 quoted lines

> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index fa82ad2f6f..bf9e56bff3 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -1,6 +1,3 @@
> -/* We need this macro to access core_apply_sparse_checkout */
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "builtin.h"
>  #include "git-compat-util.h"
>  #include "config.h"
> @@ -137,9 +134,9 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>  			     0);
>  
>  	repo_config(repo, git_default_config, NULL);
> -
> +	prepare_repo_settings(repo);

At this point, because show_usage_with_options_if_asked() has already been called and returned, we know repo is not NULL, since the only time git.c:run_builtin() calls us with repo==NULL is when there is "-h" with nothing else on the command line and that causes show_usage_with_options_if_asked() to emit usage and exit.

OK.

>  	if (ctx.sparse < 0)
> -		ctx.sparse = core_apply_sparse_checkout;
> +		ctx.sparse = repo->settings.sparse_checkout;

This is safe for the same reason.

Show 10 quoted lines

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 91b9cd0d16..1bc9c1bada 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -621,7 +621,7 @@ static int git_sparse_checkout_init(const char *repo)
>  	 * We must apply the setting in the current process
>  	 * for the later checkout to use the sparse-checkout file.
>  	 */
> -	core_apply_sparse_checkout = 1;
> +	the_repository->settings.sparse_checkout = 1;

Have anybody called prepare_repo_settings() on the repository yet? What will prevent another call to the function from overwriting this value later?

Show 11 quoted lines

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 07548fe96a..1e9f4d3eba 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -572,7 +572,7 @@ int cmd_mv(int argc,
>  		rename_index_entry_at(the_repository->index, pos, dst);
>  
>  		if (ignore_sparse &&
> -		    core_apply_sparse_checkout &&
> +		    the_repository->settings.sparse_checkout &&
>  		    core_sparse_checkout_cone) {

Have anybody called prepare_repo_settings() on the repository yet before the control comes here?

The guarantee of the original code being correct relied on the fact that git_default_core_config() was called way before these places so the global variable has been already initialized correctly.

The .sparse_checkout member is read in prepare_repo_settings() in your new code; in order to give the correctness guarantee, there needs to be some way to make sure prepare_repo_settings() has already been called on the_repository before these places.

The same comment applies to all the code paths that access the_repository->settings.sparse_checkout member instead of the global. As the source of their correctness guarantee is quite different, a mechanical replacement from global to a struct member is not sufficient.