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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> 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?

> 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.