git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Chinmoy via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Chinmoy <chinmoy12c@gmail.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository
Date: Sat, 03 Apr 2021 23:09:46 -0700	[thread overview]
Message-ID: <xmqqy2dyy40l.fsf@gitster.g> (raw)
In-Reply-To: <pull.915.v3.git.1617465421353.gitgitgadget@gmail.com> (Chinmoy via GitGitGadget's message of "Sat, 03 Apr 2021 15:57:00 +0000")

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

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

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


  parent reply	other threads:[~2021-04-04  6:09 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25 19:48 [PATCH] " Chinmoy via GitGitGadget
2021-03-25 20:31 ` Derrick Stolee
2021-03-26  6:49   ` Chinmoy Chakraborty
2021-03-26  7:54   ` Chinmoy Chakraborty
2021-03-26 15:35 ` [PATCH v2] " Chinmoy via GitGitGadget
2021-04-03 15:57   ` [PATCH v3] " Chinmoy via GitGitGadget
2021-04-04  1:49     ` Junio C Hamano
2021-04-04  5:11       ` Chinmoy Chakraborty
2021-04-04  5:36         ` Junio C Hamano
2021-04-04  5:18       ` Chinmoy Chakraborty
2021-04-04  6:09     ` Junio C Hamano [this message]
2021-04-05 13:08       ` Derrick Stolee
2021-04-05 17:48         ` Junio C Hamano
2021-04-07  6:54     ` [PATCH v4] " Chinmoy via GitGitGadget
2021-04-07 23:03       ` Junio C Hamano
2021-04-08  3:56         ` Chinmoy Chakraborty
2021-04-08 13:23           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy2dyy40l.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chinmoy12c@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --subject='Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).