n 3/25/2021 3:48 PM, Chinmoy via GitGitGadget wrote: > From: Chinmoy Chakraborty <chinmoy12c@xxxxxxxxx> > > 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@xxxxxxxxx/ 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
