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
next prev parent 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 [PATCH] cache-tree.c: remove implicit dependency on the_repository 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).