Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 4, 2021, 06:09 UTC
Message-ID: <xmqqy2dyy40l.fsf@gitster.g>
In-Reply-To: <pull.915.v3.git.1617465421353.gitgitgadget@gmail.com>

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

Show 18 quoted lines

> diff --git a/sparse-index.c b/sparse-index.c
> index 95ea17174da3..e4323ffd81db 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)
>  int convert_to_sparse(struct index_state *istate)
>  {
>  	int test_env;
> +	struct repository *r = the_repository;
> +
>  	if (istate->split_index || istate->sparse_index ||
>  	    !core_apply_sparse_checkout || !core_sparse_checkout_cone)
>  		return 0;
>  
>  	if (!istate->repo)
> -		istate->repo = the_repository;
> +		istate->repo = r;
>

I am not quite sure if this is a reasonable conversion. Surely it would not make the compiler barf, but are we sure that the caller of convert_to_sparse() wants us to work on the_repository instance and no other repository? As an istate has a .repo member, it seems to me that using istate->repo would be a lot saner approach than assigning the_repository upfront to r. It might be even needed, if cache_tree_update() wants to take a repository instance, to ensure that all callers to this helper sets istate->repo before they call it so that the above "if not set yet, use the_repository" code does not have to kick in.

Show 8 quoted lines

>  	/*
>  	 * The GIT_TEST_SPARSE_INDEX environment variable triggers the
> @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)
>  		return -1;
>  	}
>  
> -	if (cache_tree_update(istate, 0)) {
> +	if (cache_tree_update(r, istate, 0)) {

And this looks like a bad conversion. It may happen to do the same thing, but the flow of the logic up to this point in the function was to make sure istate->repo is not empty by filling it if it is not yet set, and update the cache tree of that istate. So, it seems more logical if this call were like so, no?

	if (cache_tree_update(istate->repo, istate, 0)) {

In fact, in the world after 1fd9ae51 (repository: add repo reference to index_state, 2021-01-23), it is dubious that this topic to teach cache_tree_update() to take a repository pointer is sensible. While working on a single repository, we may create multiple in-core index instances that represent temporary indices, but each of these in-core index instances (i.e. istate) belong to a single repository.

And in a call to cache_tree_update(R, I, F), if I->repo is *NOT* R, that must mean a bug. Here is what 1fd9ae51 says on this point.

    repository: add repo reference to index_state

    It will be helpful to add behavior to index operations that might
    trigger an object lookup. Since each index belongs to a specific
    repository, add a 'repo' pointer to struct index_state that allows
    access to this repository.

    Add a BUG() statement if the repo already has an index, and the index
    already has a repo, but somehow the index points to a different repo.

    This will prevent future changes from needing to pass an additional
    'struct repository *repo' parameter and instead rely only on the 'struct
    index_state *istate' parameter.

Derrick, what's you thought on this? The patch under discussion looks to me a prime example of "future change(s)" needing "to pass an additional 'struct repository *repo' parameter".

Thanks.