{"thread":{"id":"55388","subject":"[PATCH] cache-tree.c: remove implicit dependency on the_repository","startedAt":"2021-03-25T19:49:54Z","lastAt":"2021-04-08T13:23:14Z","messageCount":17,"participants":["Chinmoy via GitGitGadget","Derrick Stolee","Chinmoy Chakraborty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"420202","messageId":"pull.915.git.1616701733901.gitgitgadget@gmail.com","threadId":"55388","inReplyTo":null,"subject":"[PATCH] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-25T19:48:53Z","receivedAt":"2021-03-25T19:49:54Z","isPatch":true,"sender":{"key":"name:Chinmoy","avatar":null},"body":"From: Chinmoy Chakraborty \n\nThis kills the_repository dependency in cache_tree_update(),\nbut for unpack_trees(), they still assume the_repository\n(which also means the_index).\n\nUnfortunately the widespread use of unpack_trees() will make\nit hard to make the conversion now.\n\nThe `update_main_cache_tree()` method uses `cache_tree_update(r, r->index, flags)`.\n`r->index` is easily deduced from `r` but the signature of `cache_tree_update()`\nis not changed to take `struct repository *` instead of `struct index_state *`\nbecause there can be temporary indexes. Therefore, one might want to update\nthe cache tree for an index other than `r->index`.\n\nSigned-off-by: Chinmoy Chakraborty \n---\n Replace the_repository with r\n \n There are multiple files that try to reference the repository and\n the_index directly. To follow a more object-oriented convention these\n references should be replaced with r and index and passed through\n functions.\n \n Signed-off-by: Chinmoy Chakraborty chinmoy12c@gmail.com\n \n \n Related issue\n =============\n \n #379\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/915\n\n builtin/checkout.c | 3 ++-\n cache-tree.c | 12 ++++++------\n cache-tree.h | 6 ++++--\n sequencer.c | 6 +++---\n t/helper/test-dump-cache-tree.c | 2 +-\n unpack-trees.c | 14 ++++++++------\n 6 files changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2d6550bc3c86..3bc630ef64e7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -684,6 +684,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tint ret;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct tree *new_tree;\n+\tstruct repository *r = the_repository;\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n@@ -822,7 +823,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t}\n \n \tif (!cache_tree_fully_valid(active_cache_tree))\n-\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t\tcache_tree_update(r, r->index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\ndiff --git a/cache-tree.c b/cache-tree.c\nindex add1f0771317..7ce33dc87cf7 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -433,7 +433,7 @@ static int update_one(struct cache_tree *it,\n \treturn i;\n }\n \n-int cache_tree_update(struct index_state *istate, int flags)\n+int cache_tree_update(struct repository *r, struct index_state *istate, int flags)\n {\n \tint skip, i;\n \n@@ -446,10 +446,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \t\tistate->cache_tree = cache_tree();\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_enter(\"cache_tree\", \"update\", r);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_leave(\"cache_tree\", \"update\", r);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n \t\treturn i;\n@@ -638,7 +638,7 @@ static int write_index_as_tree_internal(struct object_id *oid,\n \t\tcache_tree_valid = 0;\n \t}\n \n-\tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n+\tif (!cache_tree_valid && cache_tree_update(the_repository, index_state, flags) < 0)\n \t\treturn WRITE_TREE_UNMERGED_INDEX;\n \n \tif (prefix) {\n@@ -746,13 +746,13 @@ void prime_cache_tree(struct repository *r,\n \t\t struct index_state *istate,\n \t\t struct tree *tree)\n {\n-\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", the_repository);\n+\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", r);\n \tcache_tree_free(&istate->cache_tree);\n \tistate->cache_tree = cache_tree();\n \n \tprime_cache_tree_rec(r, istate->cache_tree, tree);\n \tistate->cache_changed |= CACHE_TREE_CHANGED;\n-\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", the_repository);\n+\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", r);\n }\n \n /*\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 8efeccebfc9f..80cc38f176c2 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n int cache_tree_fully_valid(struct cache_tree *);\n-int cache_tree_update(struct index_state *, int);\n+int cache_tree_update(struct repository *, struct index_state *, int);\n void cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n@@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const ch\n \n static inline int update_main_cache_tree(int flags)\n {\n+\tstruct repository *r = the_repository;\n+\n \tif (!the_index.cache_tree)\n \t\tthe_index.cache_tree = cache_tree();\n-\treturn cache_tree_update(&the_index, flags);\n+\treturn cache_tree_update(r, r->index, flags);\n }\n #endif\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 848204d3dc3f..dee2d2aac5d6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,\n \treturn !clean;\n }\n \n-static struct object_id *get_cache_tree_oid(struct index_state *istate)\n+static struct object_id *get_cache_tree_oid(struct repository *r, struct index_state *istate)\n {\n \tif (!cache_tree_fully_valid(istate->cache_tree))\n-\t\tif (cache_tree_update(istate, 0)) {\n+\t\tif (cache_tree_update(r, istate, 0)) {\n \t\t\terror(_(\"unable to update cache tree\"));\n \t\t\treturn NULL;\n \t\t}\n@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)\n \tif (parse_commit(head_commit))\n \t\treturn -1;\n \n-\tif (!(cache_tree_oid = get_cache_tree_oid(istate)))\n+\tif (!(cache_tree_oid = get_cache_tree_oid(r, istate)))\n \t\treturn -1;\n \n \treturn oideq(cache_tree_oid, get_commit_tree_oid(head_commit));\ndiff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c\nindex 6a3f88f5f5d4..e6d57f9900f6 100644\n--- a/t/helper/test-dump-cache-tree.c\n+++ b/t/helper/test-dump-cache-tree.c\n@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)\n \t\tdie(\"unable to read index file\");\n \tistate = the_index;\n \tistate.cache_tree = another;\n-\tcache_tree_update(&istate, WRITE_TREE_DRY_RUN);\n+\tcache_tree_update(the_repository, &istate, WRITE_TREE_DRY_RUN);\n \treturn dump_cache_tree(active_cache_tree, another, \"\");\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 9af8e796b338..54bf9a7300b8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1574,12 +1574,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tstatic struct cache_entry *dfc;\n \tstruct pattern_list pl;\n \tint free_pattern_list = 0;\n+\tstruct repository *r = the_repository;\n \n \tif (len > MAX_UNPACK_TREES)\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", the_repository);\n+\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", r);\n \n \tif (!core_apply_sparse_checkout || !o->update)\n \t\to->skip_sparse_checkout = 1;\n@@ -1654,9 +1655,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t}\n \n \t\ttrace_performance_enter();\n-\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", the_repository);\n+\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", r);\n \t\tret = traverse_trees(o->src_index, len, t, &info);\n-\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", the_repository);\n+\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", r);\n \t\ttrace_performance_leave(\"traverse_trees\");\n \t\tif (ret < 0)\n \t\t\tgoto return_failed;\n@@ -1724,9 +1725,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tmove_index_extensions(&o->result, o->src_index);\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\t\t\tcache_tree_verify(the_repository, &o->result);\n+\t\t\t\tcache_tree_verify(r, &o->result);\n \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n-\t\t\t\tcache_tree_update(&o->result,\n+\t\t\t\tcache_tree_update(r,\n+\t\t\t\t\t\t &o->result,\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\n \t\t}\n@@ -1742,7 +1744,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n done:\n \tif (free_pattern_list)\n \t\tclear_pattern_list(&pl);\n-\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", the_repository);\n+\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", r);\n \ttrace_performance_leave(\"unpack_trees\");\n \treturn ret;\n \n\nbase-commit: 142430338477d9d1bb25be66267225fb58498d92\n-- \ngitgitgadget\n"},{"id":"420206","messageId":"f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com","threadId":"55388","inReplyTo":"pull.915.git.1616701733901.gitgitgadget@gmail.com","subject":"Re: [PATCH] cache-tree.c: remove implicit dependency on the_repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-25T20:31:23Z","receivedAt":"2021-03-25T20:32:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"n 3/25/2021 3:48 PM, Chinmoy via GitGitGadget wrote:\n> From: Chinmoy Chakraborty \n> \n> This kills the_repository dependency in cache_tree_update(),\n> but for unpack_trees(), they still assume the_repository\n> (which also means the_index).\n\nThis is a worthwhile goal.\n \n> Unfortunately the widespread use of unpack_trees() will make\n> it hard to make the conversion now.\n\nAnd this is true.\n\n> The `update_main_cache_tree()` method uses `cache_tree_update(r, r->index, flags)`.\n> `r->index` is easily deduced from `r` but the signature of `cache_tree_update()`\n> is not changed to take `struct repository *` instead of `struct index_state *`\n> because there can be temporary indexes. Therefore, one might want to update\n> the cache tree for an index other than `r->index`.\n\nUnfortunately, you're also hitting against a use of\ncache_tree_update() that I'm introducing in my sparse-index\nwork [1], so that will cause a semantic, non-textual conflict.\n\n[1] https://lore.kernel.org/git/pull.883.v4.git.1616507069.gitgitgadget@gmail.com/\n\nThere are a couple options here:\n\n1. Wait a little while for that to settle and merge to master.\n2. Rebase your work onto ds/sparse-index, so you also fix the\n new use in sparse-index.c.\n3. Continue without it, and maybe Junio will update the merge\n with this diff:\n\ndiff --git a/sparse-index.c b/sparse-index.c\nindex e5712d98d8..585f269214 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -169,7 +169,7 @@ int convert_to_sparse(struct index_state *istate)\n \t\treturn -1;\n \t}\n \n-\tif (cache_tree_update(istate, 0)) {\n+\tif (cache_tree_update(istate->repo, istate, 0)) {\n \t\twarning(_(\"unable to update cache-tree, staying full\"));\n \t\treturn -1;\n \t}\n@@ -183,7 +183,7 @@ int convert_to_sparse(struct index_state *istate)\n \n \t/* Clear and recompute the cache-tree */\n \tcache_tree_free(&istate->cache_tree);\n-\tcache_tree_update(istate, 0);\n+\tcache_tree_update(istate->repo, istate, 0);\n \n \tistate->sparse_index = 1;\n \ttrace2_region_leave(\"index\", \"convert_to_sparse\", istate->repo);\n\nFinally, there is yet a fourth option: use istate->repo instead. In\n1fd9ae51 (repository: add repo reference to index_state), I added a\n'repo' member to struct index_state. This is intended for methods to\naccess a repository directly from the index.\n\nHowever, this is slightly dangerous because there are some cases where\nand index is loaded without even the_repository being populated (and\nhence istate->repo can be NULL). They are rare, so perhaps\nunpack_trees() is always used on an index_state with a populated repo.\nIf not, then a temporary fix can be done to set istate->repo to\nthe_repository at the start of the method.\n\nThis could help you avoid changing method prototypes, reducing the\nconflicts in your patch.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 2d6550bc3c86..3bc630ef64e7 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -684,6 +684,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \tint ret;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \tstruct tree *new_tree;\n> +\tstruct repository *r = the_repository;\n\nI do appreciate you using this pattern so we could\npossibly add 'r' as a method parameter later.\n\nThe rest of the patch looks pretty standard. I hope my comments\nabove are helpful in your direction here.\n\nThanks,\n-Stolee\n"},{"id":"420235","messageId":"123f4d24-0975-d4f0-286b-d8c053e079ef@gmail.com","threadId":"55388","inReplyTo":"f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com","subject":"Re: [PATCH] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy Chakraborty","fromEmail":"chinmoy12c@gmail.com","sentAt":"2021-03-26T06:49:34Z","receivedAt":"2021-03-26T06:49:27Z","isPatch":true,"sender":{"key":"chinmoy12c@gmail.com","avatar":"https://avatars.githubusercontent.com/u/57670338?v=4"},"body":"\nOn 3/26/21 2:01 AM, Derrick Stolee wrote:\n> n 3/25/2021 3:48 PM, Chinmoy via GitGitGadget wrote:\n> 2. Rebase your work onto ds/sparse-index, so you also fix the\n> new use in sparse-index.c.\n\n     I'll try to rebase my work on top of ds/sparse-index so that it's \nfixed too.\n\n     Thanks, for the reply :)\n\n"},{"id":"420240","messageId":"b8f81261-635f-ba5c-5f10-9f96349a9583@gmail.com","threadId":"55388","inReplyTo":"f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com","subject":"Re: [PATCH] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy Chakraborty","fromEmail":"chinmoy12c@gmail.com","sentAt":"2021-03-26T07:54:03Z","receivedAt":"2021-03-26T07:53:38Z","isPatch":true,"sender":{"key":"chinmoy12c@gmail.com","avatar":"https://avatars.githubusercontent.com/u/57670338?v=4"},"body":"\nOn 3/26/21 2:01 AM, Derrick Stolee wrote\n> 2. Rebase your work onto ds/sparse-index, so you also fix the\n> new use in sparse-index.c.\nShould I open a separate branch (and PR) on my repo, with the the \nupdated ds/sparse-index and then submit it?\n"},{"id":"420266","messageId":"pull.915.v2.git.1616772930098.gitgitgadget@gmail.com","threadId":"55388","inReplyTo":"pull.915.git.1616701733901.gitgitgadget@gmail.com","subject":"[PATCH v2] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-26T15:35:29Z","receivedAt":"2021-03-26T15:36:26Z","isPatch":true,"sender":{"key":"name:Chinmoy","avatar":null},"body":"From: Chinmoy Chakraborty \n\nThis kills the_repository dependency in cache_tree_update(),\nbut for unpack_trees(), they still assume the_repository\n(which also means the_index).\n\nUnfortunately the widespread use of unpack_trees() will make\nit hard to make the conversion now.\n\nThe `update_main_cache_tree()` method uses `cache_tree_update(r, r->index, flags)`.\n`r->index` is easily deduced from `r` but the signature of `cache_tree_update()`\nis not changed to take `struct repository *` instead of `struct index_state *`\nbecause there can be temporary indexes. Therefore, one might want to update\nthe cache tree for an index other than `r->index`.\n\nThis commit also fixes the `sparse-index.c` file in which\nthe `convert_to_sparse()` and `ensure_full_index()`\nmethod use `cache_tree_update()`.\n\nSigned-off-by: Chinmoy Chakraborty \n---\n Replace the_repository with r\n \n There are multiple files that try to reference the repository and\n the_index directly. To follow a more object-oriented convention these\n references should be replaced with r and index and passed through\n functions.\n \n Signed-off-by: Chinmoy Chakraborty chinmoy12c@gmail.com\n \n \n Related issue\n =============\n \n #379\n \n cc: Derrick Stolee stolee@gmail.com\n \n \n Changes since v1\n ================\n \n * Fixed the sparse-index.c file in which the convert_to_sparse() and\n ensure_full_index() method use cache_tree_update().\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/915\n\nRange-diff vs v1:\n\n 1: 7df6968b576f ! 1: c9c2fbdc2bd1 cache-tree.c: remove implicit dependency on the_repository\n @@ Commit message\n because there can be temporary indexes. Therefore, one might want to update\n the cache tree for an index other than `r->index`.\n \n + This commit also fixes the `sparse-index.c` file in which\n + the `convert_to_sparse()` and `ensure_full_index()`\n + method use `cache_tree_update()`.\n +\n Signed-off-by: Chinmoy Chakraborty \n \n ## builtin/checkout.c ##\n @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n +@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,\n + \t\t\t\treturn 1;\n + \t\t\told_tree = get_commit_tree(old_branch_info->commit);\n + \n +-\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n ++\t\t\tif (repo_index_has_changes(r, old_tree, &sb))\n + \t\t\t\tdie(_(\"cannot continue with staged changes in \"\n + \t\t\t\t \"the following files:\\n%s\"), sb.buf);\n + \t\t\tstrbuf_release(&sb);\n +@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,\n + \t\t\t */\n + \n + \t\t\tadd_files_to_cache(NULL, NULL, 0);\n +-\t\t\tinit_merge_options(&o, the_repository);\n ++\t\t\tinit_merge_options(&o, r);\n + \t\t\to.verbosity = 0;\n +-\t\t\twork = write_in_core_index_as_tree(the_repository);\n ++\t\t\twork = write_in_core_index_as_tree(r);\n + \n + \t\t\tret = reset_tree(new_tree,\n + \t\t\t\t\t opts, 1,\n @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,\n \t}\n \n @@ cache-tree.c: int cache_tree_update(struct index_state *istate, int flags)\n \tif (i < 0)\n \t\treturn i;\n @@ cache-tree.c: static int write_index_as_tree_internal(struct object_id *oid,\n + \t\t\t\t\tint flags,\n + \t\t\t\t\tconst char *prefix)\n + {\n ++\tstruct repository *r = the_repository;\n ++\n + \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE) {\n + \t\tcache_tree_free(&index_state->cache_tree);\n \t\tcache_tree_valid = 0;\n \t}\n \n -\tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n -+\tif (!cache_tree_valid && cache_tree_update(the_repository, index_state, flags) < 0)\n ++\tif (!cache_tree_valid && cache_tree_update(r, index_state, flags) < 0)\n \t\treturn WRITE_TREE_UNMERGED_INDEX;\n \n \tif (prefix) {\n -@@ cache-tree.c: void prime_cache_tree(struct repository *r,\n - \t\t struct index_state *istate,\n - \t\t struct tree *tree)\n - {\n --\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", the_repository);\n -+\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", r);\n - \tcache_tree_free(&istate->cache_tree);\n - \tistate->cache_tree = cache_tree();\n - \n - \tprime_cache_tree_rec(r, istate->cache_tree, tree);\n - \tistate->cache_changed |= CACHE_TREE_CHANGED;\n --\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", the_repository);\n -+\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", r);\n - }\n - \n - /*\n \n ## cache-tree.h ##\n @@ cache-tree.h: void cache_tree_write(struct strbuf *, struct cache_tree *root);\n @@ cache-tree.h: void cache_tree_write(struct strbuf *, struct cache_tree *root);\n \n int cache_tree_fully_valid(struct cache_tree *);\n -int cache_tree_update(struct index_state *, int);\n -+int cache_tree_update(struct repository *, struct index_state *, int);\n ++int cache_tree_update(struct repository *r, struct index_state *, int);\n void cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n @@ sequencer.c: static int is_index_unchanged(struct repository *r)\n \n \treturn oideq(cache_tree_oid, get_commit_tree_oid(head_commit));\n \n + ## sparse-index.c ##\n +@@ sparse-index.c: int set_sparse_index_config(struct repository *repo, int enable)\n + int convert_to_sparse(struct index_state *istate)\n + {\n + \tint test_env;\n ++\tstruct repository *r= the_repository;\n ++\n + \tif (istate->split_index || istate->sparse_index ||\n + \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n + \t\treturn 0;\n +@@ sparse-index.c: int convert_to_sparse(struct index_state *istate)\n + \t\treturn -1;\n + \t}\n + \n +-\tif (cache_tree_update(istate, 0)) {\n ++\tif (cache_tree_update(r, istate, 0)) {\n + \t\twarning(_(\"unable to update cache-tree, staying full\"));\n + \t\treturn -1;\n + \t}\n +@@ sparse-index.c: int convert_to_sparse(struct index_state *istate)\n + \n + \t/* Clear and recompute the cache-tree */\n + \tcache_tree_free(&istate->cache_tree);\n +-\tcache_tree_update(istate, 0);\n ++\tcache_tree_update(r, istate, 0);\n + \n + \tistate->sparse_index = 1;\n + \ttrace2_region_leave(\"index\", \"convert_to_sparse\", istate->repo);\n +@@ sparse-index.c: void ensure_full_index(struct index_state *istate)\n + \tint i;\n + \tstruct index_state *full;\n + \tstruct strbuf base = STRBUF_INIT;\n ++\tstruct repository *r = the_repository;\n + \n + \tif (!istate || !istate->sparse_index)\n + \t\treturn;\n +@@ sparse-index.c: void ensure_full_index(struct index_state *istate)\n + \n + \t/* Clear and recompute the cache-tree */\n + \tcache_tree_free(&istate->cache_tree);\n +-\tcache_tree_update(istate, 0);\n ++\tcache_tree_update(r, istate, 0);\n + \n + \ttrace2_region_leave(\"index\", \"ensure_full_index\", istate->repo);\n + }\n +\n ## t/helper/test-dump-cache-tree.c ##\n @@ t/helper/test-dump-cache-tree.c: int cmd__dump_cache_tree(int ac, const char **av)\n \t\tdie(\"unable to read index file\");\n @@ t/helper/test-dump-cache-tree.c: int cmd__dump_cache_tree(int ac, const char **a\n }\n \n ## unpack-trees.c ##\n -@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n +@@ unpack-trees.c: static int verify_absent(const struct cache_entry *,\n + */\n + int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)\n + {\n +-\tstruct repository *repo = the_repository;\n ++\tstruct repository *r = the_repository;\n + \tint i, ret;\n \tstatic struct cache_entry *dfc;\n \tstruct pattern_list pl;\n - \tint free_pattern_list = 0;\n -+\tstruct repository *r = the_repository;\n - \n - \tif (len > MAX_UNPACK_TREES)\n +@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n \n \ttrace_performance_enter();\n -\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", the_repository);\n +\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", r);\n \n - \tif (!core_apply_sparse_checkout || !o->update)\n - \t\to->skip_sparse_checkout = 1;\n +-\tprepare_repo_settings(repo);\n +-\tif (repo->settings.command_requires_full_index) {\n ++\tprepare_repo_settings(r);\n ++\tif (r->settings.command_requires_full_index) {\n + \t\tensure_full_index(o->src_index);\n + \t\tensure_full_index(o->dst_index);\n + \t}\n @@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t}\n \n\n\n builtin/checkout.c | 9 +++++----\n cache-tree.c | 10 ++++++----\n cache-tree.h | 6 ++++--\n sequencer.c | 6 +++---\n sparse-index.c | 9 ++++++---\n t/helper/test-dump-cache-tree.c | 2 +-\n unpack-trees.c | 19 ++++++++++---------\n 7 files changed, 35 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0e6639052001..b2ec561d52a2 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -684,6 +684,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tint ret;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct tree *new_tree;\n+\tstruct repository *r = the_repository;\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n@@ -768,7 +769,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\treturn 1;\n \t\t\told_tree = get_commit_tree(old_branch_info->commit);\n \n-\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n+\t\t\tif (repo_index_has_changes(r, old_tree, &sb))\n \t\t\t\tdie(_(\"cannot continue with staged changes in \"\n \t\t\t\t \"the following files:\\n%s\"), sb.buf);\n \t\t\tstrbuf_release(&sb);\n@@ -787,9 +788,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t */\n \n \t\t\tadd_files_to_cache(NULL, NULL, 0);\n-\t\t\tinit_merge_options(&o, the_repository);\n+\t\t\tinit_merge_options(&o, r);\n \t\t\to.verbosity = 0;\n-\t\t\twork = write_in_core_index_as_tree(the_repository);\n+\t\t\twork = write_in_core_index_as_tree(r);\n \n \t\t\tret = reset_tree(new_tree,\n \t\t\t\t\t opts, 1,\n@@ -822,7 +823,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t}\n \n \tif (!cache_tree_fully_valid(active_cache_tree))\n-\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t\tcache_tree_update(r, r->index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 11bf1fcae6e1..011cfd01c565 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -452,7 +452,7 @@ static int update_one(struct cache_tree *it,\n \treturn i;\n }\n \n-int cache_tree_update(struct index_state *istate, int flags)\n+int cache_tree_update(struct repository *r, struct index_state *istate, int flags)\n {\n \tint skip, i;\n \n@@ -467,10 +467,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \t\tistate->cache_tree = cache_tree();\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_enter(\"cache_tree\", \"update\", r);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_leave(\"cache_tree\", \"update\", r);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n \t\treturn i;\n@@ -654,12 +654,14 @@ static int write_index_as_tree_internal(struct object_id *oid,\n \t\t\t\t\tint flags,\n \t\t\t\t\tconst char *prefix)\n {\n+\tstruct repository *r = the_repository;\n+\n \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE) {\n \t\tcache_tree_free(&index_state->cache_tree);\n \t\tcache_tree_valid = 0;\n \t}\n \n-\tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n+\tif (!cache_tree_valid && cache_tree_update(r, index_state, flags) < 0)\n \t\treturn WRITE_TREE_UNMERGED_INDEX;\n \n \tif (prefix) {\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 8efeccebfc9f..b905a738b71a 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n int cache_tree_fully_valid(struct cache_tree *);\n-int cache_tree_update(struct index_state *, int);\n+int cache_tree_update(struct repository *r, struct index_state *, int);\n void cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n@@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const ch\n \n static inline int update_main_cache_tree(int flags)\n {\n+\tstruct repository *r = the_repository;\n+\n \tif (!the_index.cache_tree)\n \t\tthe_index.cache_tree = cache_tree();\n-\treturn cache_tree_update(&the_index, flags);\n+\treturn cache_tree_update(r, r->index, flags);\n }\n #endif\n \ndiff --git a/sequencer.c b/sequencer.c\nindex d2332d3e1787..b0fad54ae5a4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,\n \treturn !clean;\n }\n \n-static struct object_id *get_cache_tree_oid(struct index_state *istate)\n+static struct object_id *get_cache_tree_oid(struct repository *r, struct index_state *istate)\n {\n \tif (!cache_tree_fully_valid(istate->cache_tree))\n-\t\tif (cache_tree_update(istate, 0)) {\n+\t\tif (cache_tree_update(r, istate, 0)) {\n \t\t\terror(_(\"unable to update cache tree\"));\n \t\t\treturn NULL;\n \t\t}\n@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)\n \tif (parse_commit(head_commit))\n \t\treturn -1;\n \n-\tif (!(cache_tree_oid = get_cache_tree_oid(istate)))\n+\tif (!(cache_tree_oid = get_cache_tree_oid(r, istate)))\n \t\treturn -1;\n \n \treturn oideq(cache_tree_oid, get_commit_tree_oid(head_commit));\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 56313e805d9d..0638b66204e6 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -135,6 +135,8 @@ int set_sparse_index_config(struct repository *repo, int enable)\n int convert_to_sparse(struct index_state *istate)\n {\n \tint test_env;\n+\tstruct repository *r= the_repository;\n+\n \tif (istate->split_index || istate->sparse_index ||\n \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n \t\treturn 0;\n@@ -169,7 +171,7 @@ int convert_to_sparse(struct index_state *istate)\n \t\treturn -1;\n \t}\n \n-\tif (cache_tree_update(istate, 0)) {\n+\tif (cache_tree_update(r, istate, 0)) {\n \t\twarning(_(\"unable to update cache-tree, staying full\"));\n \t\treturn -1;\n \t}\n@@ -183,7 +185,7 @@ int convert_to_sparse(struct index_state *istate)\n \n \t/* Clear and recompute the cache-tree */\n \tcache_tree_free(&istate->cache_tree);\n-\tcache_tree_update(istate, 0);\n+\tcache_tree_update(r, istate, 0);\n \n \tistate->sparse_index = 1;\n \ttrace2_region_leave(\"index\", \"convert_to_sparse\", istate->repo);\n@@ -224,6 +226,7 @@ void ensure_full_index(struct index_state *istate)\n \tint i;\n \tstruct index_state *full;\n \tstruct strbuf base = STRBUF_INIT;\n+\tstruct repository *r = the_repository;\n \n \tif (!istate || !istate->sparse_index)\n \t\treturn;\n@@ -287,7 +290,7 @@ void ensure_full_index(struct index_state *istate)\n \n \t/* Clear and recompute the cache-tree */\n \tcache_tree_free(&istate->cache_tree);\n-\tcache_tree_update(istate, 0);\n+\tcache_tree_update(r, istate, 0);\n \n \ttrace2_region_leave(\"index\", \"ensure_full_index\", istate->repo);\n }\ndiff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c\nindex 6a3f88f5f5d4..e6d57f9900f6 100644\n--- a/t/helper/test-dump-cache-tree.c\n+++ b/t/helper/test-dump-cache-tree.c\n@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)\n \t\tdie(\"unable to read index file\");\n \tistate = the_index;\n \tistate.cache_tree = another;\n-\tcache_tree_update(&istate, WRITE_TREE_DRY_RUN);\n+\tcache_tree_update(the_repository, &istate, WRITE_TREE_DRY_RUN);\n \treturn dump_cache_tree(active_cache_tree, another, \"\");\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 0b888dab2246..e6bf576e174a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1571,7 +1571,7 @@ static int verify_absent(const struct cache_entry *,\n */\n int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)\n {\n-\tstruct repository *repo = the_repository;\n+\tstruct repository *r = the_repository;\n \tint i, ret;\n \tstatic struct cache_entry *dfc;\n \tstruct pattern_list pl;\n@@ -1581,10 +1581,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", the_repository);\n+\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", r);\n \n-\tprepare_repo_settings(repo);\n-\tif (repo->settings.command_requires_full_index) {\n+\tprepare_repo_settings(r);\n+\tif (r->settings.command_requires_full_index) {\n \t\tensure_full_index(o->src_index);\n \t\tensure_full_index(o->dst_index);\n \t}\n@@ -1662,9 +1662,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t}\n \n \t\ttrace_performance_enter();\n-\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", the_repository);\n+\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", r);\n \t\tret = traverse_trees(o->src_index, len, t, &info);\n-\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", the_repository);\n+\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", r);\n \t\ttrace_performance_leave(\"traverse_trees\");\n \t\tif (ret < 0)\n \t\t\tgoto return_failed;\n@@ -1732,9 +1732,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tmove_index_extensions(&o->result, o->src_index);\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\t\t\tcache_tree_verify(the_repository, &o->result);\n+\t\t\t\tcache_tree_verify(r, &o->result);\n \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n-\t\t\t\tcache_tree_update(&o->result,\n+\t\t\t\tcache_tree_update(r,\n+\t\t\t\t\t\t &o->result,\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\n \t\t}\n@@ -1750,7 +1751,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n done:\n \tif (free_pattern_list)\n \t\tclear_pattern_list(&pl);\n-\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", the_repository);\n+\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", r);\n \ttrace_performance_leave(\"unpack_trees\");\n \treturn ret;\n \n\nbase-commit: 2e85e4c16a11f3ffa9bf5a433a72618331e1fb70\n-- \ngitgitgadget\n"},{"id":"420939","messageId":"pull.915.v3.git.1617465421353.gitgitgadget@gmail.com","threadId":"55388","inReplyTo":"pull.915.v2.git.1616772930098.gitgitgadget@gmail.com","subject":"[PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-03T15:57:00Z","receivedAt":"2021-04-03T15:57:06Z","isPatch":true,"sender":{"key":"name:Chinmoy","avatar":null},"body":"From: Chinmoy Chakraborty \n\nThis kills the_repository dependency in cache_tree_update(),\nbut for unpack_trees(), they still assume the_repository\n(which also means the_index).\n\nUnfortunately the widespread use of unpack_trees() will make\nit hard to make the conversion now.\n\nThe `update_main_cache_tree()` method uses `cache_tree_update(r, r->index, flags)`.\n`r->index` is easily deduced from `r` but the signature of `cache_tree_update()`\nis not changed to take `struct repository *` instead of `struct index_state *`\nbecause there can be temporary indexes. Therefore, one might want to update\nthe cache tree for an index other than `r->index`.\n\nThis commit also fixes the `sparse-index.c` file in which\nthe `convert_to_sparse()` and `ensure_full_index()`\nmethod use `cache_tree_update()`.\n\nSigned-off-by: Chinmoy Chakraborty \n---\n Replace the_repository with r\n \n There are multiple files that try to reference the repository and\n the_index directly. To follow a more object-oriented convention these\n references should be replaced with r and index and passed through\n functions.\n \n Signed-off-by: Chinmoy Chakraborty chinmoy12c@gmail.com\n \n \n Related issue\n =============\n \n #379\n \n cc: Derrick Stolee stolee@gmail.com\n \n \n Changes since v1\n ================\n \n * Fixed the sparse-index.c file in which the convert_to_sparse() and\n ensure_full_index() method use cache_tree_update().\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/915\n\nRange-diff vs v2:\n\n 1: c9c2fbdc2bd1 ! 1: 2a4fad2781e3 cache-tree.c: remove implicit dependency on the_repository\n @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op\n -\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n +\t\tcache_tree_update(r, r->index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n - \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n +-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n ++\tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n + \n + \tif (!opts->discard_changes && !opts->quiet && new_branch_info->commit)\n \n ## cache-tree.c ##\n @@ cache-tree.c: static int update_one(struct cache_tree *it,\n @@ cache-tree.h: void cache_tree_write(struct strbuf *, struct cache_tree *root);\n \n int cache_tree_fully_valid(struct cache_tree *);\n -int cache_tree_update(struct index_state *, int);\n -+int cache_tree_update(struct repository *r, struct index_state *, int);\n ++int cache_tree_update(struct repository *, struct index_state *, int);\n void cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n @@ sparse-index.c: int set_sparse_index_config(struct repository *repo, int enable)\n int convert_to_sparse(struct index_state *istate)\n {\n \tint test_env;\n -+\tstruct repository *r= the_repository;\n ++\tstruct repository *r = the_repository;\n +\n \tif (istate->split_index || istate->sparse_index ||\n \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n \t\treturn 0;\n + \n + \tif (!istate->repo)\n +-\t\tistate->repo = the_repository;\n ++\t\tistate->repo = r;\n + \n + \t/*\n + \t * The GIT_TEST_SPARSE_INDEX environment variable triggers the\n @@ sparse-index.c: int convert_to_sparse(struct index_state *istate)\n \t\treturn -1;\n \t}\n @@ sparse-index.c: void ensure_full_index(struct index_state *istate)\n \n \tif (!istate || !istate->sparse_index)\n \t\treturn;\n + \n + \tif (!istate->repo)\n +-\t\tistate->repo = the_repository;\n ++\t\tistate->repo = r;\n + \n + \ttrace2_region_enter(\"index\", \"ensure_full_index\", istate->repo);\n + \n @@ sparse-index.c: void ensure_full_index(struct index_state *istate)\n \n \t/* Clear and recompute the cache-tree */\n\n\n builtin/checkout.c | 11 ++++++-----\n cache-tree.c | 10 ++++++----\n cache-tree.h | 6 ++++--\n sequencer.c | 6 +++---\n sparse-index.c | 13 ++++++++-----\n t/helper/test-dump-cache-tree.c | 2 +-\n unpack-trees.c | 19 ++++++++++---------\n 7 files changed, 38 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0e6639052001..33d762a7caf9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -684,6 +684,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tint ret;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct tree *new_tree;\n+\tstruct repository *r = the_repository;\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n@@ -768,7 +769,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\treturn 1;\n \t\t\told_tree = get_commit_tree(old_branch_info->commit);\n \n-\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n+\t\t\tif (repo_index_has_changes(r, old_tree, &sb))\n \t\t\t\tdie(_(\"cannot continue with staged changes in \"\n \t\t\t\t \"the following files:\\n%s\"), sb.buf);\n \t\t\tstrbuf_release(&sb);\n@@ -787,9 +788,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t */\n \n \t\t\tadd_files_to_cache(NULL, NULL, 0);\n-\t\t\tinit_merge_options(&o, the_repository);\n+\t\t\tinit_merge_options(&o, r);\n \t\t\to.verbosity = 0;\n-\t\t\twork = write_in_core_index_as_tree(the_repository);\n+\t\t\twork = write_in_core_index_as_tree(r);\n \n \t\t\tret = reset_tree(new_tree,\n \t\t\t\t\t opts, 1,\n@@ -822,9 +823,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t}\n \n \tif (!cache_tree_fully_valid(active_cache_tree))\n-\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t\tcache_tree_update(r, r->index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tif (!opts->discard_changes && !opts->quiet && new_branch_info->commit)\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 11bf1fcae6e1..011cfd01c565 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -452,7 +452,7 @@ static int update_one(struct cache_tree *it,\n \treturn i;\n }\n \n-int cache_tree_update(struct index_state *istate, int flags)\n+int cache_tree_update(struct repository *r, struct index_state *istate, int flags)\n {\n \tint skip, i;\n \n@@ -467,10 +467,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \t\tistate->cache_tree = cache_tree();\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_enter(\"cache_tree\", \"update\", r);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_leave(\"cache_tree\", \"update\", r);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n \t\treturn i;\n@@ -654,12 +654,14 @@ static int write_index_as_tree_internal(struct object_id *oid,\n \t\t\t\t\tint flags,\n \t\t\t\t\tconst char *prefix)\n {\n+\tstruct repository *r = the_repository;\n+\n \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE) {\n \t\tcache_tree_free(&index_state->cache_tree);\n \t\tcache_tree_valid = 0;\n \t}\n \n-\tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n+\tif (!cache_tree_valid && cache_tree_update(r, index_state, flags) < 0)\n \t\treturn WRITE_TREE_UNMERGED_INDEX;\n \n \tif (prefix) {\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 8efeccebfc9f..80cc38f176c2 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n int cache_tree_fully_valid(struct cache_tree *);\n-int cache_tree_update(struct index_state *, int);\n+int cache_tree_update(struct repository *, struct index_state *, int);\n void cache_tree_verify(struct repository *, struct index_state *);\n \n /* bitmasks to write_index_as_tree flags */\n@@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const ch\n \n static inline int update_main_cache_tree(int flags)\n {\n+\tstruct repository *r = the_repository;\n+\n \tif (!the_index.cache_tree)\n \t\tthe_index.cache_tree = cache_tree();\n-\treturn cache_tree_update(&the_index, flags);\n+\treturn cache_tree_update(r, r->index, flags);\n }\n #endif\n \ndiff --git a/sequencer.c b/sequencer.c\nindex d2332d3e1787..b0fad54ae5a4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,\n \treturn !clean;\n }\n \n-static struct object_id *get_cache_tree_oid(struct index_state *istate)\n+static struct object_id *get_cache_tree_oid(struct repository *r, struct index_state *istate)\n {\n \tif (!cache_tree_fully_valid(istate->cache_tree))\n-\t\tif (cache_tree_update(istate, 0)) {\n+\t\tif (cache_tree_update(r, istate, 0)) {\n \t\t\terror(_(\"unable to update cache tree\"));\n \t\t\treturn NULL;\n \t\t}\n@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)\n \tif (parse_commit(head_commit))\n \t\treturn -1;\n \n-\tif (!(cache_tree_oid = get_cache_tree_oid(istate)))\n+\tif (!(cache_tree_oid = get_cache_tree_oid(r, istate)))\n \t\treturn -1;\n \n \treturn oideq(cache_tree_oid, get_commit_tree_oid(head_commit));\ndiff --git a/sparse-index.c b/sparse-index.c\nindex 95ea17174da3..e4323ffd81db 100644\n--- a/sparse-index.c\n+++ b/sparse-index.c\n@@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)\n int convert_to_sparse(struct index_state *istate)\n {\n \tint test_env;\n+\tstruct repository *r = the_repository;\n+\n \tif (istate->split_index || istate->sparse_index ||\n \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n \t\treturn 0;\n \n \tif (!istate->repo)\n-\t\tistate->repo = the_repository;\n+\t\tistate->repo = r;\n \n \t/*\n \t * The GIT_TEST_SPARSE_INDEX environment variable triggers the\n@@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)\n \t\treturn -1;\n \t}\n \n-\tif (cache_tree_update(istate, 0)) {\n+\tif (cache_tree_update(r, istate, 0)) {\n \t\twarning(_(\"unable to update cache-tree, staying full\"));\n \t\treturn -1;\n \t}\n@@ -175,7 +177,7 @@ int convert_to_sparse(struct index_state *istate)\n \n \t/* Clear and recompute the cache-tree */\n \tcache_tree_free(&istate->cache_tree);\n-\tcache_tree_update(istate, 0);\n+\tcache_tree_update(r, istate, 0);\n \n \tistate->sparse_index = 1;\n \ttrace2_region_leave(\"index\", \"convert_to_sparse\", istate->repo);\n@@ -216,12 +218,13 @@ void ensure_full_index(struct index_state *istate)\n \tint i;\n \tstruct index_state *full;\n \tstruct strbuf base = STRBUF_INIT;\n+\tstruct repository *r = the_repository;\n \n \tif (!istate || !istate->sparse_index)\n \t\treturn;\n \n \tif (!istate->repo)\n-\t\tistate->repo = the_repository;\n+\t\tistate->repo = r;\n \n \ttrace2_region_enter(\"index\", \"ensure_full_index\", istate->repo);\n \n@@ -279,7 +282,7 @@ void ensure_full_index(struct index_state *istate)\n \n \t/* Clear and recompute the cache-tree */\n \tcache_tree_free(&istate->cache_tree);\n-\tcache_tree_update(istate, 0);\n+\tcache_tree_update(r, istate, 0);\n \n \ttrace2_region_leave(\"index\", \"ensure_full_index\", istate->repo);\n }\ndiff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c\nindex 6a3f88f5f5d4..e6d57f9900f6 100644\n--- a/t/helper/test-dump-cache-tree.c\n+++ b/t/helper/test-dump-cache-tree.c\n@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)\n \t\tdie(\"unable to read index file\");\n \tistate = the_index;\n \tistate.cache_tree = another;\n-\tcache_tree_update(&istate, WRITE_TREE_DRY_RUN);\n+\tcache_tree_update(the_repository, &istate, WRITE_TREE_DRY_RUN);\n \treturn dump_cache_tree(active_cache_tree, another, \"\");\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 0b888dab2246..e6bf576e174a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1571,7 +1571,7 @@ static int verify_absent(const struct cache_entry *,\n */\n int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)\n {\n-\tstruct repository *repo = the_repository;\n+\tstruct repository *r = the_repository;\n \tint i, ret;\n \tstatic struct cache_entry *dfc;\n \tstruct pattern_list pl;\n@@ -1581,10 +1581,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", the_repository);\n+\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", r);\n \n-\tprepare_repo_settings(repo);\n-\tif (repo->settings.command_requires_full_index) {\n+\tprepare_repo_settings(r);\n+\tif (r->settings.command_requires_full_index) {\n \t\tensure_full_index(o->src_index);\n \t\tensure_full_index(o->dst_index);\n \t}\n@@ -1662,9 +1662,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t}\n \n \t\ttrace_performance_enter();\n-\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", the_repository);\n+\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", r);\n \t\tret = traverse_trees(o->src_index, len, t, &info);\n-\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", the_repository);\n+\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", r);\n \t\ttrace_performance_leave(\"traverse_trees\");\n \t\tif (ret < 0)\n \t\t\tgoto return_failed;\n@@ -1732,9 +1732,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tmove_index_extensions(&o->result, o->src_index);\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n-\t\t\t\tcache_tree_verify(the_repository, &o->result);\n+\t\t\t\tcache_tree_verify(r, &o->result);\n \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n-\t\t\t\tcache_tree_update(&o->result,\n+\t\t\t\tcache_tree_update(r,\n+\t\t\t\t\t\t &o->result,\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\n \t\t}\n@@ -1750,7 +1751,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n done:\n \tif (free_pattern_list)\n \t\tclear_pattern_list(&pl);\n-\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", the_repository);\n+\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", r);\n \ttrace_performance_leave(\"unpack_trees\");\n \treturn ret;\n \n\nbase-commit: c9e40ae8ec41c5566e5849a87c969fa81ef49fcd\n-- \ngitgitgadget\n"},{"id":"420946","messageId":"xmqqpmzazund.fsf@gitster.g","threadId":"55388","inReplyTo":"pull.915.v3.git.1617465421353.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-04T01:49:10Z","receivedAt":"2021-04-04T02:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chinmoy via GitGitGadget\" writes:\n\n> From: Chinmoy Chakraborty \n>\n> This kills the_repository dependency in cache_tree_update(),\n\nThat part of the sentence is good. But the next four lines are not.\n\n> but for unpack_trees(), they still assume the_repository\n> (which also means the_index).\n>\n> Unfortunately the widespread use of unpack_trees() will make\n> it hard to make the conversion now.\n\nAs long as all the users of unpack_trees() want to work with the\nprimary in-core index instance, that is not a problem. There is no\nneed to lament with a phrase like \"unfortunately\". If you are not\ntouching unpack_trees(), but you needed to adjust its use of\ncache_tree_update(), then none of the above needs to be said. If\nyou change one API function (i.e. cache_tree_update()), all callers\nneed to be adjusted for that change, but if one caller only wants to\nwork in the primary repository and with its primary index, that is\nfine. You change it so that it passes the repository and the index\nit wants the updated unpack_trees() to work on, and there is nothing\nnotable in it.\n\n> The `update_main_cache_tree()` method uses `cache_tree_update(r, r->index, flags)`.\n> `r->index` is easily deduced from `r` but the signature of `cache_tree_update()`\n> is not changed to take `struct repository *` instead of `struct index_state *`\n> because there can be temporary indexes. Therefore, one might want to update\n> the cache tree for an index other than `r->index`.\n\nThe flow of explanation of the thought in this paragraph is probably\nbackwards for most people to understand what you wanted to say.\n\n The cache_tree_update() was originally designed to take an istate so\n that it can work on an instance of in-core index that is not the\n primary one for the repository, and we want to keep that property\n while teaching the subsystem how to deal with a repository other\n than the_repository. Hence it can take a pointer to the repository\n and a pointer to istate separately.\n\nNothing else needs to be said. The update-main-cache-tree is a thin\nwrapper to work on cache tree for THE MAIN in-core index instance,\nso it is natural if it needs to pass r and r->index, exactly because\nthe latter is THE MAIN in-core index instance for the repository.\nThat is too obvious that it is best left unsaid, for the same reason\nwhy you shouldn't even mention what you did in updating the use of\nupdate_cache_tree() inside unpack_trees(). There is nothing notable\nin there.\n\ncf. Documentation/SubmittingPatches\n[[describe-changes]]\n[[summary-section]]\n[[meaningful-message]]\n[[imperative-mood]]\n\n\n> This commit also fixes the `sparse-index.c` file in which\n> the `convert_to_sparse()` and `ensure_full_index()`\n> method use `cache_tree_update()`.\n\nWhat branch did you base this patch on? There is no update_cache_tree()\ncall in sparse-index.c on 'master' (well, there is no sparse-index.c\nin 'master' to begin with). \n\nWhile it is not prohibited to build on a commit that is not on\n'master', you'd need to keep your dependencies to the minimum so\nthat the topic won't be taken hostage by other topics that do not\nmove quickly enough to your liking. Building on 'next' or 'seen'\nas a whole is a guarantee that the topic will never hit 'master',\nas no tip of 'next' will ever be merged directly to 'master'.\n\nAnd if you are building somewhere other than the tip of 'master', it\nis necessary to say where, in order to save confusion and wasted\neffort for your readers. E.g. \"This patch was made on top of\nmerging topic 'X' and 'Y' into 'master'\". Then you'd only have to\nwait for 'X' and 'Y' to graduate to 'master' before the topic can\nproceed.\n\nThanks.\n\n\n> builtin/checkout.c | 11 ++++++-----\n> cache-tree.c | 10 ++++++----\n> cache-tree.h | 6 ++++--\n> sequencer.c | 6 +++---\n> sparse-index.c | 13 ++++++++-----\n> t/helper/test-dump-cache-tree.c | 2 +-\n> unpack-trees.c | 19 ++++++++++---------\n> 7 files changed, 38 insertions(+), 29 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 0e6639052001..33d762a7caf9 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -684,6 +684,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \tint ret;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \tstruct tree *new_tree;\n> +\tstruct repository *r = the_repository;\n> \n> \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n> \tif (read_cache_preload(NULL) < 0)\n> @@ -768,7 +769,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\t\t\treturn 1;\n> \t\t\told_tree = get_commit_tree(old_branch_info->commit);\n> \n> -\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n> +\t\t\tif (repo_index_has_changes(r, old_tree, &sb))\n> \t\t\t\tdie(_(\"cannot continue with staged changes in \"\n> \t\t\t\t \"the following files:\\n%s\"), sb.buf);\n> \t\t\tstrbuf_release(&sb);\n> @@ -787,9 +788,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\t\t */\n> \n> \t\t\tadd_files_to_cache(NULL, NULL, 0);\n> -\t\t\tinit_merge_options(&o, the_repository);\n> +\t\t\tinit_merge_options(&o, r);\n> \t\t\to.verbosity = 0;\n> -\t\t\twork = write_in_core_index_as_tree(the_repository);\n> +\t\t\twork = write_in_core_index_as_tree(r);\n> \n> \t\t\tret = reset_tree(new_tree,\n> \t\t\t\t\t opts, 1,\n> @@ -822,9 +823,9 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t}\n> \n> \tif (!cache_tree_fully_valid(active_cache_tree))\n> -\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> +\t\tcache_tree_update(r, r->index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> \n> -\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> +\tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n> \t\tdie(_(\"unable to write new index file\"));\n> \n> \tif (!opts->discard_changes && !opts->quiet && new_branch_info->commit)\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 11bf1fcae6e1..011cfd01c565 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -452,7 +452,7 @@ static int update_one(struct cache_tree *it,\n> \treturn i;\n> }\n> \n> -int cache_tree_update(struct index_state *istate, int flags)\n> +int cache_tree_update(struct repository *r, struct index_state *istate, int flags)\n> {\n> \tint skip, i;\n> \n> @@ -467,10 +467,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n> \t\tistate->cache_tree = cache_tree();\n> \n> \ttrace_performance_enter();\n> -\ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n> +\ttrace2_region_enter(\"cache_tree\", \"update\", r);\n> \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n> \t\t \"\", 0, &skip, flags);\n> -\ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n> +\ttrace2_region_leave(\"cache_tree\", \"update\", r);\n> \ttrace_performance_leave(\"cache_tree_update\");\n> \tif (i < 0)\n> \t\treturn i;\n> @@ -654,12 +654,14 @@ static int write_index_as_tree_internal(struct object_id *oid,\n> \t\t\t\t\tint flags,\n> \t\t\t\t\tconst char *prefix)\n> {\n> +\tstruct repository *r = the_repository;\n> +\n> \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE) {\n> \t\tcache_tree_free(&index_state->cache_tree);\n> \t\tcache_tree_valid = 0;\n> \t}\n> \n> -\tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n> +\tif (!cache_tree_valid && cache_tree_update(r, index_state, flags) < 0)\n> \t\treturn WRITE_TREE_UNMERGED_INDEX;\n> \n> \tif (prefix) {\n> diff --git a/cache-tree.h b/cache-tree.h\n> index 8efeccebfc9f..80cc38f176c2 100644\n> --- a/cache-tree.h\n> +++ b/cache-tree.h\n> @@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);\n> struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n> \n> int cache_tree_fully_valid(struct cache_tree *);\n> -int cache_tree_update(struct index_state *, int);\n> +int cache_tree_update(struct repository *, struct index_state *, int);\n> void cache_tree_verify(struct repository *, struct index_state *);\n> \n> /* bitmasks to write_index_as_tree flags */\n> @@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const ch\n> \n> static inline int update_main_cache_tree(int flags)\n> {\n> +\tstruct repository *r = the_repository;\n> +\n> \tif (!the_index.cache_tree)\n> \t\tthe_index.cache_tree = cache_tree();\n> -\treturn cache_tree_update(&the_index, flags);\n> +\treturn cache_tree_update(r, r->index, flags);\n> }\n> #endif\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index d2332d3e1787..b0fad54ae5a4 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,\n> \treturn !clean;\n> }\n> \n> -static struct object_id *get_cache_tree_oid(struct index_state *istate)\n> +static struct object_id *get_cache_tree_oid(struct repository *r, struct index_state *istate)\n> {\n> \tif (!cache_tree_fully_valid(istate->cache_tree))\n> -\t\tif (cache_tree_update(istate, 0)) {\n> +\t\tif (cache_tree_update(r, istate, 0)) {\n> \t\t\terror(_(\"unable to update cache tree\"));\n> \t\t\treturn NULL;\n> \t\t}\n> @@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)\n> \tif (parse_commit(head_commit))\n> \t\treturn -1;\n> \n> -\tif (!(cache_tree_oid = get_cache_tree_oid(istate)))\n> +\tif (!(cache_tree_oid = get_cache_tree_oid(r, istate)))\n> \t\treturn -1;\n> \n> \treturn oideq(cache_tree_oid, get_commit_tree_oid(head_commit));\n> diff --git a/sparse-index.c b/sparse-index.c\n> index 95ea17174da3..e4323ffd81db 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)\n> int convert_to_sparse(struct index_state *istate)\n> {\n> \tint test_env;\n> +\tstruct repository *r = the_repository;\n> +\n> \tif (istate->split_index || istate->sparse_index ||\n> \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n> \t\treturn 0;\n> \n> \tif (!istate->repo)\n> -\t\tistate->repo = the_repository;\n> +\t\tistate->repo = r;\n> \n> \t/*\n> \t * The GIT_TEST_SPARSE_INDEX environment variable triggers the\n> @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)\n> \t\treturn -1;\n> \t}\n> \n> -\tif (cache_tree_update(istate, 0)) {\n> +\tif (cache_tree_update(r, istate, 0)) {\n> \t\twarning(_(\"unable to update cache-tree, staying full\"));\n> \t\treturn -1;\n> \t}\n> @@ -175,7 +177,7 @@ int convert_to_sparse(struct index_state *istate)\n> \n> \t/* Clear and recompute the cache-tree */\n> \tcache_tree_free(&istate->cache_tree);\n> -\tcache_tree_update(istate, 0);\n> +\tcache_tree_update(r, istate, 0);\n> \n> \tistate->sparse_index = 1;\n> \ttrace2_region_leave(\"index\", \"convert_to_sparse\", istate->repo);\n> @@ -216,12 +218,13 @@ void ensure_full_index(struct index_state *istate)\n> \tint i;\n> \tstruct index_state *full;\n> \tstruct strbuf base = STRBUF_INIT;\n> +\tstruct repository *r = the_repository;\n> \n> \tif (!istate || !istate->sparse_index)\n> \t\treturn;\n> \n> \tif (!istate->repo)\n> -\t\tistate->repo = the_repository;\n> +\t\tistate->repo = r;\n> \n> \ttrace2_region_enter(\"index\", \"ensure_full_index\", istate->repo);\n> \n> @@ -279,7 +282,7 @@ void ensure_full_index(struct index_state *istate)\n> \n> \t/* Clear and recompute the cache-tree */\n> \tcache_tree_free(&istate->cache_tree);\n> -\tcache_tree_update(istate, 0);\n> +\tcache_tree_update(r, istate, 0);\n> \n> \ttrace2_region_leave(\"index\", \"ensure_full_index\", istate->repo);\n> }\n> diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c\n> index 6a3f88f5f5d4..e6d57f9900f6 100644\n> --- a/t/helper/test-dump-cache-tree.c\n> +++ b/t/helper/test-dump-cache-tree.c\n> @@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)\n> \t\tdie(\"unable to read index file\");\n> \tistate = the_index;\n> \tistate.cache_tree = another;\n> -\tcache_tree_update(&istate, WRITE_TREE_DRY_RUN);\n> +\tcache_tree_update(the_repository, &istate, WRITE_TREE_DRY_RUN);\n> \treturn dump_cache_tree(active_cache_tree, another, \"\");\n> }\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 0b888dab2246..e6bf576e174a 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1571,7 +1571,7 @@ static int verify_absent(const struct cache_entry *,\n> */\n> int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)\n> {\n> -\tstruct repository *repo = the_repository;\n> +\tstruct repository *r = the_repository;\n> \tint i, ret;\n> \tstatic struct cache_entry *dfc;\n> \tstruct pattern_list pl;\n> @@ -1581,10 +1581,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n> \n> \ttrace_performance_enter();\n> -\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", the_repository);\n> +\ttrace2_region_enter(\"unpack_trees\", \"unpack_trees\", r);\n> \n> -\tprepare_repo_settings(repo);\n> -\tif (repo->settings.command_requires_full_index) {\n> +\tprepare_repo_settings(r);\n> +\tif (r->settings.command_requires_full_index) {\n> \t\tensure_full_index(o->src_index);\n> \t\tensure_full_index(o->dst_index);\n> \t}\n> @@ -1662,9 +1662,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \t\t}\n> \n> \t\ttrace_performance_enter();\n> -\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", the_repository);\n> +\t\ttrace2_region_enter(\"unpack_trees\", \"traverse_trees\", r);\n> \t\tret = traverse_trees(o->src_index, len, t, &info);\n> -\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", the_repository);\n> +\t\ttrace2_region_leave(\"unpack_trees\", \"traverse_trees\", r);\n> \t\ttrace_performance_leave(\"traverse_trees\");\n> \t\tif (ret < 0)\n> \t\t\tgoto return_failed;\n> @@ -1732,9 +1732,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \t\tmove_index_extensions(&o->result, o->src_index);\n> \t\tif (!ret) {\n> \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n> -\t\t\t\tcache_tree_verify(the_repository, &o->result);\n> +\t\t\t\tcache_tree_verify(r, &o->result);\n> \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n> -\t\t\t\tcache_tree_update(&o->result,\n> +\t\t\t\tcache_tree_update(r,\n> +\t\t\t\t\t\t &o->result,\n> \t\t\t\t\t\t WRITE_TREE_SILENT |\n> \t\t\t\t\t\t WRITE_TREE_REPAIR);\n> \t\t}\n> @@ -1750,7 +1751,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> done:\n> \tif (free_pattern_list)\n> \t\tclear_pattern_list(&pl);\n> -\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", the_repository);\n> +\ttrace2_region_leave(\"unpack_trees\", \"unpack_trees\", r);\n> \ttrace_performance_leave(\"unpack_trees\");\n> \treturn ret;\n> \n>\n> base-commit: c9e40ae8ec41c5566e5849a87c969fa81ef49fcd\n"},{"id":"420952","messageId":"16da2d91-9eda-ddf4-dd87-e07fb2bfbbcb@gmail.com","threadId":"55388","inReplyTo":"xmqqpmzazund.fsf@gitster.g","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy Chakraborty","fromEmail":"chinmoy12c@gmail.com","sentAt":"2021-04-04T05:11:35Z","receivedAt":"2021-04-04T05:17:11Z","isPatch":true,"sender":{"key":"chinmoy12c@gmail.com","avatar":"https://avatars.githubusercontent.com/u/57670338?v=4"},"body":"\nOn 4/4/21 7:19 AM, Junio C Hamano wrote:\n> Nothing else needs to be said. The update-main-cache-tree is a thin\n> wrapper to work on cache tree for THE MAIN in-core index instance,\n> so it is natural if it needs to pass r and r->index, exactly because\n> the latter is THE MAIN in-core index instance for the repository.\n> That is too obvious that it is best left unsaid, for the same reason\n> why you shouldn't even mention what you did in updating the use of\n> update_cache_tree() inside unpack_trees(). There is nothing notable\n> in there.\n>\n> cf. Documentation/SubmittingPatches\n> [[describe-changes]]\n> [[summary-section]]\n> [[meaningful-message]]\n> [[imperative-mood]]\n\nOkay, I'll update the message. Thanks for the review.\n\n\n>> This commit also fixes the `sparse-index.c` file in which\n>> the `convert_to_sparse()` and `ensure_full_index()`\n>> method use `cache_tree_update()`.\n> What branch did you base this patch on? There is no update_cache_tree()\n> call in sparse-index.c on 'master' (well, there is no sparse-index.c\n> in 'master' to begin with).\n\nActually the patch hit against a use of `cache_tree_update()` in\n\nthe branch 'ds/sparse-index', so I was suggested by Derrick Stolee\n\nto rebase my work on to of his branch.\n\nYou may view the discussion here:\n\nhttps://lore.kernel.org/git/f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com/\n\n\n> And if you are building somewhere other than the tip of 'master', it\n> is necessary to say where, in order to save confusion and wasted\n> effort for your readers. E.g. \"This patch was made on top of\n> merging topic 'X' and 'Y' into 'master'\". Then you'd only have to\n> wait for 'X' and 'Y' to graduate to 'master' before the topic can\n> proceed.\n>\n> Thanks.\n\nShould this information also be added to the commit message\n\nor just the cover letter of the patch?\n\n"},{"id":"420953","messageId":"92e1255b-f058-04ef-1901-90cc7d7fb979@gmail.com","threadId":"55388","inReplyTo":"xmqqpmzazund.fsf@gitster.g","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy Chakraborty","fromEmail":"chinmoy12c@gmail.com","sentAt":"2021-04-04T05:18:33Z","receivedAt":"2021-04-04T05:17:11Z","isPatch":true,"sender":{"key":"chinmoy12c@gmail.com","avatar":"https://avatars.githubusercontent.com/u/57670338?v=4"},"body":"\nOn 4/4/21 7:19 AM, Junio C Hamano wrote:\n> Nothing else needs to be said. The update-main-cache-tree is a thin\n> wrapper to work on cache tree for THE MAIN in-core index instance,\n> so it is natural if it needs to pass r and r->index, exactly because\n> the latter is THE MAIN in-core index instance for the repository.\n> That is too obvious that it is best left unsaid, for the same reason\n> why you shouldn't even mention what you did in updating the use of\n> update_cache_tree() inside unpack_trees(). There is nothing notable\n> in there.\n>\n> cf. Documentation/SubmittingPatches\n> [[describe-changes]]\n> [[summary-section]]\n> [[meaningful-message]]\n> [[imperative-mood]]\n\nOkay, I'll update the message. Thanks for the review.\n\n\n>> This commit also fixes the `sparse-index.c` file in which\n>> the `convert_to_sparse()` and `ensure_full_index()`\n>> method use `cache_tree_update()`.\n> What branch did you base this patch on? There is no update_cache_tree()\n> call in sparse-index.c on 'master' (well, there is no sparse-index.c\n> in 'master' to begin with).\n\nActually the patch hit against a use of `cache_tree_update()` in\n\nthe branch 'ds/sparse-index', so I was suggested by Derrick Stolee\n\nto rebase my work on to of his branch.\n\nYou may view the discussion here:\n\nhttps://lore.kernel.org/git/f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com/\n\n\n> And if you are building somewhere other than the tip of 'master', it\n> is necessary to say where, in order to save confusion and wasted\n> effort for your readers. E.g. \"This patch was made on top of\n> merging topic 'X' and 'Y' into 'master'\". Then you'd only have to\n> wait for 'X' and 'Y' to graduate to 'master' before the topic can\n> proceed.\n>\n> Thanks.\n\nShould this information be added to the commit message\n\nor just the cover letter of the patch?\n\n"},{"id":"420955","messageId":"xmqq5z12zk3q.fsf@gitster.g","threadId":"55388","inReplyTo":"16da2d91-9eda-ddf4-dd87-e07fb2bfbbcb@gmail.com","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-04T05:36:57Z","receivedAt":"2021-04-04T05:37:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chinmoy Chakraborty writes:\n\n>> And if you are building somewhere other than the tip of 'master', it\n>> is necessary to say where, in order to save confusion and wasted\n>> effort for your readers. E.g. \"This patch was made on top of\n>> merging topic 'X' and 'Y' into 'master'\". Then you'd only have to\n>> wait for 'X' and 'Y' to graduate to 'master' before the topic can\n>> proceed.\n>>\n>> Thanks.\n>\n> Should this information also be added to the commit message\n>\n> or just the cover letter of the patch?\n\nWe often see people write it in the cover letter for multi-patch\nseries. For a single patch topic, it is written under the\nthree-dash line.\n\nThanks.\n"},{"id":"420957","messageId":"xmqqy2dyy40l.fsf@gitster.g","threadId":"55388","inReplyTo":"pull.915.v3.git.1617465421353.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-04T06:09:46Z","receivedAt":"2021-04-04T06:09:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chinmoy via GitGitGadget\" writes:\n\n> diff --git a/sparse-index.c b/sparse-index.c\n> index 95ea17174da3..e4323ffd81db 100644\n> --- a/sparse-index.c\n> +++ b/sparse-index.c\n> @@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)\n> int convert_to_sparse(struct index_state *istate)\n> {\n> \tint test_env;\n> +\tstruct repository *r = the_repository;\n> +\n> \tif (istate->split_index || istate->sparse_index ||\n> \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n> \t\treturn 0;\n> \n> \tif (!istate->repo)\n> -\t\tistate->repo = the_repository;\n> +\t\tistate->repo = r;\n> \n\nI am not quite sure if this is a reasonable conversion. Surely it\nwould not make the compiler barf, but are we sure that the caller of\nconvert_to_sparse() wants us to work on the_repository instance and\nno other repository? As an istate has a .repo member, it seems to\nme that using istate->repo would be a lot saner approach than\nassigning the_repository upfront to r. It might be even needed, if\ncache_tree_update() wants to take a repository instance, to ensure\nthat all callers to this helper sets istate->repo before they call\nit so that the above \"if not set yet, use the_repository\" code does\nnot have to kick in.\n\n> \t/*\n> \t * The GIT_TEST_SPARSE_INDEX environment variable triggers the\n> @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)\n> \t\treturn -1;\n> \t}\n> \n> -\tif (cache_tree_update(istate, 0)) {\n> +\tif (cache_tree_update(r, istate, 0)) {\n\nAnd this looks like a bad conversion. It may happen to do the same\nthing, but the flow of the logic up to this point in the function\nwas to make sure istate->repo is not empty by filling it if it is\nnot yet set, and update the cache tree of that istate. So, it seems\nmore logical if this call were like so, no?\n\n\tif (cache_tree_update(istate->repo, istate, 0)) {\n\nIn fact, in the world after 1fd9ae51 (repository: add repo reference\nto index_state, 2021-01-23), it is dubious that this topic to teach\ncache_tree_update() to take a repository pointer is sensible. While\nworking on a single repository, we may create multiple in-core index\ninstances that represent temporary indices, but each of these in-core\nindex instances (i.e. istate) belong to a single repository.\n\nAnd in a call to cache_tree_update(R, I, F), if I->repo is *NOT* R,\nthat must mean a bug. Here is what 1fd9ae51 says on this point.\n\n repository: add repo reference to index_state\n\n It will be helpful to add behavior to index operations that might\n trigger an object lookup. Since each index belongs to a specific\n repository, add a 'repo' pointer to struct index_state that allows\n access to this repository.\n\n Add a BUG() statement if the repo already has an index, and the index\n already has a repo, but somehow the index points to a different repo.\n\n This will prevent future changes from needing to pass an additional\n 'struct repository *repo' parameter and instead rely only on the 'struct\n index_state *istate' parameter.\n\nDerrick, what's you thought on this? The patch under discussion\nlooks to me a prime example of \"future change(s)\" needing \"to pass\nan additional 'struct repository *repo' parameter\".\n\nThanks.\n\n"},{"id":"420986","messageId":"be9793f0-f437-8e42-d463-21a48d2ee948@gmail.com","threadId":"55388","inReplyTo":"xmqqy2dyy40l.fsf@gitster.g","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-05T13:08:44Z","receivedAt":"2021-04-05T13:08:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/4/2021 2:09 AM, Junio C Hamano wrote:\n> \"Chinmoy via GitGitGadget\" writes:\n> \n>> diff --git a/sparse-index.c b/sparse-index.c\n>> index 95ea17174da3..e4323ffd81db 100644\n>> --- a/sparse-index.c\n>> +++ b/sparse-index.c\n>> @@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)\n>> int convert_to_sparse(struct index_state *istate)\n>> {\n>> \tint test_env;\n>> +\tstruct repository *r = the_repository;\n>> +\n>> \tif (istate->split_index || istate->sparse_index ||\n>> \t !core_apply_sparse_checkout || !core_sparse_checkout_cone)\n>> \t\treturn 0;\n>> \n>> \tif (!istate->repo)\n>> -\t\tistate->repo = the_repository;\n>> +\t\tistate->repo = r;\n>> \n> \n> I am not quite sure if this is a reasonable conversion. Surely it\n> would not make the compiler barf, but are we sure that the caller of\n> convert_to_sparse() wants us to work on the_repository instance and\n> no other repository? As an istate has a .repo member, it seems to\n> me that using istate->repo would be a lot saner approach than\n> assigning the_repository upfront to r. It might be even needed, if\n> cache_tree_update() wants to take a repository instance, to ensure\n> that all callers to this helper sets istate->repo before they call\n> it so that the above \"if not set yet, use the_repository\" code does\n> not have to kick in.\n> \n>> \t/*\n>> \t * The GIT_TEST_SPARSE_INDEX environment variable triggers the\n>> @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)\n>> \t\treturn -1;\n>> \t}\n>> \n>> -\tif (cache_tree_update(istate, 0)) {\n>> +\tif (cache_tree_update(r, istate, 0)) {\n> \n> And this looks like a bad conversion. It may happen to do the same\n> thing, but the flow of the logic up to this point in the function\n> was to make sure istate->repo is not empty by filling it if it is\n> not yet set, and update the cache tree of that istate. So, it seems\n> more logical if this call were like so, no?\n> \n> \tif (cache_tree_update(istate->repo, istate, 0)) {\n> \n> In fact, in the world after 1fd9ae51 (repository: add repo reference\n> to index_state, 2021-01-23), it is dubious that this topic to teach\n> cache_tree_update() to take a repository pointer is sensible. While\n> working on a single repository, we may create multiple in-core index\n> instances that represent temporary indices, but each of these in-core\n> index instances (i.e. istate) belong to a single repository.\n> \n> And in a call to cache_tree_update(R, I, F), if I->repo is *NOT* R,\n> that must mean a bug. Here is what 1fd9ae51 says on this point.\n> \n> repository: add repo reference to index_state\n> \n> It will be helpful to add behavior to index operations that might\n> trigger an object lookup. Since each index belongs to a specific\n> repository, add a 'repo' pointer to struct index_state that allows\n> access to this repository.\n> \n> Add a BUG() statement if the repo already has an index, and the index\n> already has a repo, but somehow the index points to a different repo.\n> \n> This will prevent future changes from needing to pass an additional\n> 'struct repository *repo' parameter and instead rely only on the 'struct\n> index_state *istate' parameter.\n> \n> Derrick, what's you thought on this? The patch under discussion\n> looks to me a prime example of \"future change(s)\" needing \"to pass\n> an additional 'struct repository *repo' parameter\".\n\nWith your additional comments, I think it is clear that the \"fourth\noption\" I mentioned earlier [1] is the way to go:\n\n Finally, there is yet a fourth option: use istate->repo instead. In\n 1fd9ae51 (repository: add repo reference to index_state), I added a\n 'repo' member to struct index_state. This is intended for methods to\n access a repository directly from the index.\n\n[1] https://lore.kernel.org/git/f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com/\n\nSo in this sense, we should always use istate->repo, but we might\nstill need the following guard in some places:\n\n\tif (!istate->repo)\n\t\tistate->repo = the_repository;\n\nin case there are situations where the index is loaded before\nthe_repository is loaded. I have hit this in testing, but don't fully\nunderstand the cases where this can happen.\n\nThe way it would change this patch is to drop the 'struct repository *r'\npointers and changes to the method signatures. Instead, keep the\nmethods only taking a 'struct index_state *istate' and use istate->repo\neverywhere.\n\nThanks,\n-Stolee\n"},{"id":"420999","messageId":"xmqqv99062sr.fsf@gitster.g","threadId":"55388","inReplyTo":"be9793f0-f437-8e42-d463-21a48d2ee948@gmail.com","subject":"Re: [PATCH v3] cache-tree.c: remove implicit dependency on the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-05T17:48:04Z","receivedAt":"2021-04-05T17:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> With your additional comments, I think it is clear that the \"fourth\n> option\" I mentioned earlier [1] is the way to go:\n>\n> Finally, there is yet a fourth option: use istate->repo instead. In\n> 1fd9ae51 (repository: add repo reference to index_state), I added a\n> 'repo' member to struct index_state. This is intended for methods to\n> access a repository directly from the index.\n>\n> [1] https://lore.kernel.org/git/f187df01-8e59-ac74-01e1-586a7a63fd4e@gmail.com/\n\nThanks. I wasn't following the earlier discussion closely, as the\ntopic seemed to be in the hands of good reviewers ;-)\n\n> So in this sense, we should always use istate->repo, but we might\n> still need the following guard in some places:\n>\n> \tif (!istate->repo)\n> \t\tistate->repo = the_repository;\n>\n> in case there are situations where the index is loaded before\n> the_repository is loaded. I have hit this in testing, but don't fully\n> understand the cases where this can happen.\n\nAs a longer term goal, it may not be a bad idea to make sure that\nanybody who passes an istate should not have to pass a repo. I do\nnot think of a reason why, other than historical inertia, to do so\nin post 1fd9ae51 world.\n\n> The way it would change this patch is to drop the 'struct repository *r'\n> pointers and changes to the method signatures. Instead, keep the\n> methods only taking a 'struct index_state *istate' and use istate->repo\n> everywhere.\n\nYes, and that would result in a patch that touches very limited\nsmall parts of cache-tree.c without needing to touch any caller, I\nwould presume.\n\nThanks.\n"},{"id":"421109","messageId":"pull.915.v4.git.1617778489719.gitgitgadget@gmail.com","threadId":"55388","inReplyTo":"pull.915.v3.git.1617465421353.gitgitgadget@gmail.com","subject":"[PATCH v4] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-07T06:54:49Z","receivedAt":"2021-04-07T06:54:54Z","isPatch":true,"sender":{"key":"name:Chinmoy","avatar":null},"body":"From: Chinmoy Chakraborty \n\nThis kills the_repository dependency in cache_tree_update()\nand prime_cache_tree().\n\nSigned-off-by: Chinmoy Chakraborty \n---\n Replace the_repository with r\n \n There are multiple files that try to reference the repository and\n the_index directly. To follow a more object-oriented convention these\n references should be replaced with r and index and passed through\n functions.\n \n Signed-off-by: Chinmoy Chakraborty chinmoy12c@gmail.com\n \n \n Related issue\n =============\n \n #379\n \n cc: Derrick Stolee stolee@gmail.com\n \n \n Changes since v3\n ================\n \n * Used istate->repo instead of the_repository to prevent making changes\n in callers of the function.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/915\n\nRange-diff vs v3:\n\n 1: 2a4fad2781e3 < -: ------------ cache-tree.c: remove implicit dependency on the_repository\n -: ------------ > 1: 25f09954b9df cache-tree.c: remove implicit dependency on the_repository\n\n\n cache-tree.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex add1f0771317..4928a9f0f13b 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -446,10 +446,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n \t\tistate->cache_tree = cache_tree();\n \n \ttrace_performance_enter();\n-\ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_enter(\"cache_tree\", \"update\", istate->repo);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n \t\t \"\", 0, &skip, flags);\n-\ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n+\ttrace2_region_leave(\"cache_tree\", \"update\", istate->repo);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n \t\treturn i;\n@@ -746,13 +746,13 @@ void prime_cache_tree(struct repository *r,\n \t\t struct index_state *istate,\n \t\t struct tree *tree)\n {\n-\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", the_repository);\n+\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", r);\n \tcache_tree_free(&istate->cache_tree);\n \tistate->cache_tree = cache_tree();\n \n \tprime_cache_tree_rec(r, istate->cache_tree, tree);\n \tistate->cache_changed |= CACHE_TREE_CHANGED;\n-\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", the_repository);\n+\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", r);\n }\n \n /*\n\nbase-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48\n-- \ngitgitgadget\n"},{"id":"421180","messageId":"xmqqsg41wvdi.fsf@gitster.g","threadId":"55388","inReplyTo":"pull.915.v4.git.1617778489719.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] cache-tree.c: remove implicit dependency on the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T23:03:05Z","receivedAt":"2021-04-07T23:03:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chinmoy via GitGitGadget\" writes:\n\n> From: Chinmoy Chakraborty \n>\n> This kills the_repository dependency in cache_tree_update()\n> and prime_cache_tree().\n>\n> Signed-off-by: Chinmoy Chakraborty \n> ---\n> Replace the_repository with r\n\nHuh???\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index add1f0771317..4928a9f0f13b 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -446,10 +446,10 @@ int cache_tree_update(struct index_state *istate, int flags)\n> \t\tistate->cache_tree = cache_tree();\n> \n> \ttrace_performance_enter();\n> -\ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n> +\ttrace2_region_enter(\"cache_tree\", \"update\", istate->repo);\n> \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n> \t\t \"\", 0, &skip, flags);\n> -\ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n> +\ttrace2_region_leave(\"cache_tree\", \"update\", istate->repo);\n> \ttrace_performance_leave(\"cache_tree_update\");\n> \tif (i < 0)\n> \t\treturn i;\n> @@ -746,13 +746,13 @@ void prime_cache_tree(struct repository *r,\n> \t\t struct index_state *istate,\n> \t\t struct tree *tree)\n> {\n> -\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", the_repository);\n> +\ttrace2_region_enter(\"cache-tree\", \"prime_cache_tree\", r);\n> \tcache_tree_free(&istate->cache_tree);\n> \tistate->cache_tree = cache_tree();\n> \n> \tprime_cache_tree_rec(r, istate->cache_tree, tree);\n> \tistate->cache_changed |= CACHE_TREE_CHANGED;\n> -\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", the_repository);\n> +\ttrace2_region_leave(\"cache-tree\", \"prime_cache_tree\", r);\n> }\n\nThe patch assumes that istate->repo will always set, but it does not\neven try to justify why that assumption is safe to make (e.g. \"the\nentire codebase that leads to this function has been audited and\nmade sure istate at this point will always have its .repo member is\nset\" in the log message, if such an audit has actually been done,\nmay have been convincing), which I find quite troubling.\n"},{"id":"421193","messageId":"0df415b7-73b7-6e72-1885-3ca606acf8fd@gmail.com","threadId":"55388","inReplyTo":"xmqqsg41wvdi.fsf@gitster.g","subject":"Re: [PATCH v4] cache-tree.c: remove implicit dependency on the_repository","fromName":"Chinmoy Chakraborty","fromEmail":"chinmoy12c@gmail.com","sentAt":"2021-04-08T03:56:12Z","receivedAt":"2021-04-08T03:54:50Z","isPatch":true,"sender":{"key":"chinmoy12c@gmail.com","avatar":"https://avatars.githubusercontent.com/u/57670338?v=4"},"body":"\nOn 4/8/21 4:33 AM, Junio C Hamano wrote:\n> \"Chinmoy via GitGitGadget\" writes:\n>\n>> From: Chinmoy Chakraborty \n>>\n>> This kills the_repository dependency in cache_tree_update()\n>> and prime_cache_tree().\n>>\n>> Signed-off-by: Chinmoy Chakraborty \n>> ---\n>> Replace the_repository with r\n> Huh???\n\n\nSorry I forgot to change the cover letter.\n\n\n> The patch assumes that istate->repo will always set, but it does not\n> even try to justify why that assumption is safe to make (e.g. \"the\n> entire codebase that leads to this function has been audited and\n> made sure istate at this point will always have its .repo member is\n> set\" in the log message, if such an audit has actually been done,\n> may have been convincing), which I find quite troubling.\n\n\nIs it safe to make this assumption? I mean to be completely\n\nsure of this, one would have to track back to all the callers.\n\nShould a check like:\n\n     if(!istate->repo)\n\n         istate->repo = the_repository;\n\n\nbe required?\n\n"},{"id":"421207","messageId":"xmqqmtu8vrk6.fsf@gitster.g","threadId":"55388","inReplyTo":"0df415b7-73b7-6e72-1885-3ca606acf8fd@gmail.com","subject":"Re: [PATCH v4] cache-tree.c: remove implicit dependency on the_repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T13:23:05Z","receivedAt":"2021-04-08T13:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chinmoy Chakraborty writes:\n\n> Is it safe to make this assumption?\n\nIt was the question I asked, and I didn't see a reason to believe it\nis safe.\n\n> I mean to be completely sure of this, one would have to track back\n> to all the callers.\n\nYes. If we audit the callers to make sure istate->repo always\npoints at the right repository, add missing assignment to\nistate->repo as necessary, and add\n\n\tif (!istate->repo)\n\t\tBUG(\"caller of cache_tree_udpate() did not fill istate->repo\");\n\nthen that would be an improvement. But...\n\n> Should a check like:\n>\n> if(!istate->repo)\n>\n> istate->repo = the_repository;\n>\n> be required?\n\n... if we add such an \"if the caller did not set istate->repo,\nassume the_repository\" code, then the resulting code explicitly\nassumes that the istate the caller passed to us without setting\nistate->repo belongs to the default repository.\n\nI do not quite see the point of such a change---it is not all that\nbetter than \"implicit dependency on the_repository\" the patch tries\nto address, is it?\n\n"}]}