git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Chinmoy via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org
Cc: Chinmoy <chinmoy12c@gmail.com>
Subject: Re: [PATCH] cache-tree.c: remove implicit dependency on the_repository
Date: Thu, 25 Mar 2021 16:31:23 -0400	[thread overview]
Message-ID: <f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com> (raw)
In-Reply-To: <pull.915.git.1616701733901.gitgitgadget@gmail.com>

n 3/25/2021 3:48 PM, Chinmoy via GitGitGadget wrote:
> From: Chinmoy Chakraborty <chinmoy12c@gmail.com>
> 
> This kills the_repository dependency in cache_tree_update(),
> but for unpack_trees(), they still assume the_repository
> (which also means the_index).

This is a worthwhile goal.
 
> Unfortunately the widespread use of unpack_trees() will make
> it hard to make the conversion now.

And this is true.

> The `update_main_cache_tree()` method uses `cache_tree_update(r, r->index, flags)`.
> `r->index` is easily deduced from `r` but the signature of `cache_tree_update()`
> is not changed to take `struct repository *` instead of `struct index_state *`
> because there can be temporary indexes. Therefore, one might want to update
> the cache tree for an index other than `r->index`.

Unfortunately, you're also hitting against a use of
cache_tree_update() that I'm introducing in my sparse-index
work [1], so that will cause a semantic, non-textual conflict.

[1] https://lore.kernel.org/git/pull.883.v4.git.1616507069.gitgitgadget@gmail.com/

There are a couple options here:

1. Wait a little while for that to settle and merge to master.
2. Rebase your work onto ds/sparse-index, so you also fix the
   new use in sparse-index.c.
3. Continue without it, and maybe Junio will update the merge
   with this diff:

diff --git a/sparse-index.c b/sparse-index.c
index e5712d98d8..585f269214 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -169,7 +169,7 @@ int convert_to_sparse(struct index_state *istate)
 		return -1;
 	}
 
-	if (cache_tree_update(istate, 0)) {
+	if (cache_tree_update(istate->repo, istate, 0)) {
 		warning(_("unable to update cache-tree, staying full"));
 		return -1;
 	}
@@ -183,7 +183,7 @@ int convert_to_sparse(struct index_state *istate)
 
 	/* Clear and recompute the cache-tree */
 	cache_tree_free(&istate->cache_tree);
-	cache_tree_update(istate, 0);
+	cache_tree_update(istate->repo, istate, 0);
 
 	istate->sparse_index = 1;
 	trace2_region_leave("index", "convert_to_sparse", istate->repo);

Finally, there is yet a fourth option: use istate->repo instead. In
1fd9ae51 (repository: add repo reference to index_state), I added a
'repo' member to struct index_state. This is intended for methods to
access a repository directly from the index.

However, this is slightly dangerous because there are some cases where
and index is loaded without even the_repository being populated (and
hence istate->repo can be NULL). They are rare, so perhaps
unpack_trees() is always used on an index_state with a populated repo.
If not, then a temporary fix can be done to set istate->repo to
the_repository at the start of the method.

This could help you avoid changing method prototypes, reducing the
conflicts in your patch.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2d6550bc3c86..3bc630ef64e7 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -684,6 +684,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  	int ret;
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct tree *new_tree;
> +	struct repository *r = the_repository;

I do appreciate you using this pattern so we could
possibly add 'r' as a method parameter later.

The rest of the patch looks pretty standard. I hope my comments
above are helpful in your direction here.

Thanks,
-Stolee

  reply	other threads:[~2021-03-25 20:32 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25 19:48 Chinmoy via GitGitGadget
2021-03-25 20:31 ` Derrick Stolee [this message]
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
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=f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com \
    --to=stolee@gmail.com \
    --cc=chinmoy12c@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --subject='Re: [PATCH] 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).