cache-tree.c: remove implicit dependency on the_repository
PATCH 1/1 cache-tree.c: remove implicit dependency on the_repository
From: Chinmoy Chakraborty <[email protected]>
This kills the_repository dependency in cache_tree_update(), but for unpack_trees(), they still assume the_repository (which also means the_index).
Unfortunately the widespread use of unpack_trees() will make it hard to make the conversion now.
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.
---
Replace the_repository with r
There are multiple files that try to reference the repository and the_index directly. To follow a more object-oriented convention these references should be replaced with r and index and passed through functions.
Related issue
=============
#379
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/915
builtin/checkout.c | 3 ++- cache-tree.c | 12 ++++++------ cache-tree.h | 6 ++++-- sequencer.c | 6 +++--- t/helper/test-dump-cache-tree.c | 2 +- unpack-trees.c | 14 ++++++++------ 6 files changed, 24 insertions(+), 19 deletions(-)
builtin/checkout.c +2 -1
@@ -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;hold_locked_index (&lock_file,LOCK_DIE_ON_ERROR );if (read_cache_preload (NULL ) <0 )
@@ -822,7 +823,7 @@ static int merge_working_tree(const struct checkout_opts *opts, }if (!cache_tree_fully_valid (active_cache_tree)) -cache_tree_update (&the_index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); +cache_tree_update (r, r->index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR );if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK ))die (_ ("unable to write new index file" ));
cache-tree.c +6 -6
@@ -433,7 +433,7 @@ static int update_one(struct cache_tree *it,return i; } -int cache_tree_update (struct index_state *istate,int flags) +int cache_tree_update (struct repository *r,struct index_state *istate,int flags) {int skip, i;
@@ -446,10 +446,10 @@ int cache_tree_update(struct index_state *istate, int flags) istate->cache_tree =cache_tree ();trace_performance_enter (); -trace2_region_enter ("cache_tree" ,"update" , the_repository); +trace2_region_enter ("cache_tree" ,"update" , r); i =update_one (istate->cache_tree, istate->cache, istate->cache_nr,"" ,0 , &skip, flags); -trace2_region_leave ("cache_tree" ,"update" , the_repository); +trace2_region_leave ("cache_tree" ,"update" , r);trace_performance_leave ("cache_tree_update" );if (i <0 )return i;
@@ -638,7 +638,7 @@ static int write_index_as_tree_internal(struct object_id *oid, cache_tree_valid =0 ; } -if (!cache_tree_valid &&cache_tree_update (index_state, flags) <0 ) +if (!cache_tree_valid &&cache_tree_update (the_repository, index_state, flags) <0 )return WRITE_TREE_UNMERGED_INDEX ;if (prefix) {
@@ -746,13 +746,13 @@ void prime_cache_tree(struct repository *r, struct index_state *istate, struct tree *tree) { - trace2_region_enter("cache-tree", "prime_cache_tree", the_repository); + trace2_region_enter("cache-tree", "prime_cache_tree", r);cache_tree_free (&istate->cache_tree); istate->cache_tree =cache_tree ();prime_cache_tree_rec (r, istate->cache_tree, tree); istate->cache_changed |=CACHE_TREE_CHANGED ; -trace2_region_leave ("cache-tree" ,"prime_cache_tree" , the_repository); +trace2_region_leave ("cache-tree" ,"prime_cache_tree" , r); }/*
cache-tree.h +4 -2
@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);struct cache_tree *cache_tree_read (const char *buffer,unsigned long size);int cache_tree_fully_valid (struct cache_tree *); -int cache_tree_update (struct index_state *,int ); +int cache_tree_update (struct repository *,struct index_state *,int );void cache_tree_verify (struct repository *,struct index_state *);/* bitmasks to write_index_as_tree flags */
@@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const chstatic inline int update_main_cache_tree (int flags) { +struct repository *r = the_repository; +if (!the_index.cache_tree) the_index.cache_tree =cache_tree (); -return cache_tree_update (&the_index, flags); +return cache_tree_update (r, r->index, flags); }#endif
sequencer.c +3 -3
@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,return !clean; } -static struct object_id *get_cache_tree_oid (struct index_state *istate) +static struct object_id *get_cache_tree_oid (struct repository *r,struct index_state *istate) {if (!cache_tree_fully_valid (istate->cache_tree)) -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {error (_ ("unable to update cache tree" ));return NULL ; }
@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)if (parse_commit (head_commit))return -1 ; -if (!(cache_tree_oid =get_cache_tree_oid (istate))) +if (!(cache_tree_oid =get_cache_tree_oid (r, istate)))return -1 ;return oideq (cache_tree_oid,get_commit_tree_oid (head_commit));
t/helper/test-dump-cache-tree.c +1 -1
@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)die ("unable to read index file" ); istate = the_index; istate.cache_tree = another; -cache_tree_update (&istate,WRITE_TREE_DRY_RUN ); +cache_tree_update (the_repository, &istate,WRITE_TREE_DRY_RUN );return dump_cache_tree (active_cache_tree, another,"" ); }
unpack-trees.c +8 -6
@@ -1574,12 +1574,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsstatic struct cache_entry *dfc;struct pattern_list pl;int free_pattern_list =0 ; +struct repository *r = the_repository;if (len >MAX_UNPACK_TREES )die ("unpack_trees takes at most %d trees" ,MAX_UNPACK_TREES );trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"unpack_trees" , r);if (!core_apply_sparse_checkout || !o->update) o->skip_sparse_checkout =1 ;
@@ -1654,9 +1655,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options }trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"traverse_trees" , r); ret =traverse_trees (o->src_index, len, t, &info); -trace2_region_leave ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"traverse_trees" , r);trace_performance_leave ("traverse_trees" );if (ret <0 ) goto return_failed;
@@ -1724,9 +1725,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsmove_index_extensions (&o->result, o->src_index);if (!ret) {if (git_env_bool ("GIT_TEST_CHECK_CACHE_TREE" ,0 )) -cache_tree_verify (the_repository, &o->result); +cache_tree_verify (r, &o->result);if (!cache_tree_fully_valid (o->result.cache_tree)) -cache_tree_update (&o->result, +cache_tree_update (r, + &o->result,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); }
@@ -1742,7 +1744,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done:if (free_pattern_list)clear_pattern_list (&pl); -trace2_region_leave ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"unpack_trees" , r);trace_performance_leave ("unpack_trees" );return ret;
base-commit: 142430338477d9d1bb25be66267225fb58498d92
Signature · 1 line
gitgitgadget
Quoted 5 lines · Chinmoy via GitGitGadget
From: Chinmoy Chakraborty <[email protected]>
This kills
the_repositorydependency incache_tree_update(), but forunpack_trees(), they still assumethe_repository(which also meansthe_index).
This is a worthwhile goal.
Quoted 2 lines
Unfortunately the widespread use of
unpack_trees()will make it hard to make the conversion now.
And this is true.
Quoted 5 lines
The
update_main_cache_tree()method usescache_tree_update(r, r->index, flags).r->indexis easily deduced fromrbut the signature ofcache_tree_update()is not changed to takestruct repository *instead ofstruct index_state *because there can be temporary indexes. Therefore, one might want to update the cache tree for an index other thanr->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/[email protected]/
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:
sparse-index.c +2 -2
@@ -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.
builtin/checkout.c static int merge_working_tree(const struct checkout_opts *opts, · line 687
Quoted 9 lines
builtin/checkout.c +1
-0@@ -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
Quoted 3 lines · Derrick Stolee
n 3/25/2021 3:48 PM, Chinmoy via GitGitGadget wrote:
2. Rebase your work onto ds/sparse-index, so you also fix the new use insparse-index.c.
I'll try to rebase my work on top of ds/sparse-index so that it's fixed too.
Thanks, for the reply :)
On 3/26/21 2:01 AM, Derrick Stolee wrote
Quoted 2 lines
2. Rebase your work onto ds/sparse-index, so you also fix the new use in
sparse-index.c.
Should I open a separate branch (and PR) on my repo, with the the updated ds/sparse-index and then submit it?
PATCH v2 1/1 cache-tree.c: remove implicit dependency on the_repository
In reply to Chinmoy via GitGitGadgetFrom: Chinmoy Chakraborty <[email protected]>
This kills the_repository dependency in cache_tree_update(), but for unpack_trees(), they still assume the_repository (which also means the_index).
Unfortunately the widespread use of unpack_trees() will make it hard to make the conversion now.
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.
This commit also fixes the sparse-index.c file in which the convert_to_sparse() and ensure_full_index() method use cache_tree_update().
---
Replace the_repository with r
There are multiple files that try to reference the repository and the_index directly. To follow a more object-oriented convention these references should be replaced with r and index and passed through functions.
Related issue
=============
#379
Changes since v1
================
* Fixed the sparse-index.c file in which the convert_to_sparse() and ensure_full_index() method use cache_tree_update().
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/915
Range-diff vs v1:
1: 7df6968b576f ! 1: c9c2fbdc2bd1 cache-tree.c: remove implicit dependency on the_repository
@@ Commit message
because there can be temporary indexes. Therefore, one might want to update the cache tree for an index other than r->index.
+ This commit also fixes the sparse-index.c file in which
+ the convert_to_sparse() and ensure_full_index()
+ method use cache_tree_update().
+
## builtin/checkout.c ##
@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op
hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
if (read_cache_preload(NULL) < 0)
+@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
+ return 1;
+ old_tree = get_commit_tree(old_branch_info->commit);
+
+- if (repo_index_has_changes(the_repository, old_tree, &sb))
++ if (repo_index_has_changes(r, old_tree, &sb))
+ die(_("cannot continue with staged changes in "
+ "the following files:\n%s"), sb.buf);
+ strbuf_release(&sb);
+@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
+ */
+
+ add_files_to_cache(NULL, NULL, 0);
+- init_merge_options(&o, the_repository);
++ init_merge_options(&o, r);
+ o.verbosity = 0;
+- work = write_in_core_index_as_tree(the_repository);
++ work = write_in_core_index_as_tree(r);
+
+ ret = reset_tree(new_tree,
+ opts, 1,
@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,
}
@@ cache-tree.c: int cache_tree_update(struct index_state *istate, int flags)
if (i < 0)
return i;
@@ cache-tree.c: static int write_index_as_tree_internal(struct object_id *oid,
+ int flags,
+ const char *prefix)
+ {
++ struct repository *r = the_repository;
++
+ if (flags & WRITE_TREE_IGNORE_CACHE_TREE) {
+ cache_tree_free(&index_state->cache_tree);
cache_tree_valid = 0;
}
- if (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)
-+ if (!cache_tree_valid && cache_tree_update(the_repository, index_state, flags) < 0)
++ if (!cache_tree_valid && cache_tree_update(r, index_state, flags) < 0)
return WRITE_TREE_UNMERGED_INDEX;
if (prefix) {
-@@ cache-tree.c: void prime_cache_tree(struct repository *r,
- struct index_state *istate,
- struct tree *tree)
- {
-- trace2_region_enter("cache-tree", "prime_cache_tree", the_repository);
-+ trace2_region_enter("cache-tree", "prime_cache_tree", r);
- cache_tree_free(&istate->cache_tree);
- istate->cache_tree = cache_tree();
-
- prime_cache_tree_rec(r, istate->cache_tree, tree);
- istate->cache_changed |= CACHE_TREE_CHANGED;
-- trace2_region_leave("cache-tree", "prime_cache_tree", the_repository);
-+ trace2_region_leave("cache-tree", "prime_cache_tree", r);
- }
-
- /*
## cache-tree.h ##
@@ cache-tree.h: void cache_tree_write(struct strbuf *, struct cache_tree *root);
@@ cache-tree.h: void cache_tree_write(struct strbuf *, struct cache_tree *root);
int cache_tree_fully_valid(struct cache_tree *);
-int cache_tree_update(struct index_state *, int);
-+int cache_tree_update(struct repository *, struct index_state *, int); ++int cache_tree_update(struct repository *r, struct index_state *, int);
void cache_tree_verify(struct repository *, struct index_state *);
/* bitmasks to write_index_as_tree flags */
@@ sequencer.c: static int is_index_unchanged(struct repository *r)
return oideq(cache_tree_oid, get_commit_tree_oid(head_commit));
+ ## sparse-index.c ##
+@@ sparse-index.c: int set_sparse_index_config(struct repository *repo, int enable)
+ int convert_to_sparse(struct index_state *istate)
+ {
+ int test_env;
++ struct repository *r= the_repository;
++
+ if (istate->split_index || istate->sparse_index ||
+ !core_apply_sparse_checkout || !core_sparse_checkout_cone)
+ return 0;
+@@ sparse-index.c: int convert_to_sparse(struct index_state *istate)
+ return -1;
+ }
+
+- if (cache_tree_update(istate, 0)) {
++ if (cache_tree_update(r, istate, 0)) {
+ warning(_("unable to update cache-tree, staying full"));
+ return -1;
+ }
+@@ sparse-index.c: 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(r, istate, 0);
+
+ istate->sparse_index = 1;
+ trace2_region_leave("index", "convert_to_sparse", istate->repo);
+@@ sparse-index.c: void ensure_full_index(struct index_state *istate)
+ int i;
+ struct index_state *full;
+ struct strbuf base = STRBUF_INIT;
++ struct repository *r = the_repository;
+
+ if (!istate || !istate->sparse_index)
+ return;
+@@ sparse-index.c: void ensure_full_index(struct index_state *istate) +
+ /* Clear and recompute the cache-tree */
+ cache_tree_free(&istate->cache_tree);
+- cache_tree_update(istate, 0);
++ cache_tree_update(r, istate, 0);
+
+ trace2_region_leave("index", "ensure_full_index", istate->repo);
+ }
+
## t/helper/test-dump-cache-tree.c ##
@@ t/helper/test-dump-cache-tree.c: int cmd__dump_cache_tree(int ac, const char **av)
die("unable to read index file");
@@ t/helper/test-dump-cache-tree.c: int cmd__dump_cache_tree(int ac, const char **a
}
## unpack-trees.c ##
-@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options +@@ unpack-trees.c: static int verify_absent(const struct cache_entry *,
+ */
+ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
+ {
+- struct repository *repo = the_repository;
++ struct repository *r = the_repository;
+ int i, ret;
static struct cache_entry *dfc;
struct pattern_list pl;
- int free_pattern_list = 0;
-+ struct repository *r = the_repository;
-
- if (len > MAX_UNPACK_TREES)
+@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
trace_performance_enter();
- trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
+ trace2_region_enter("unpack_trees", "unpack_trees", r);
- if (!core_apply_sparse_checkout || !o->update)
- o->skip_sparse_checkout = 1;
+- prepare_repo_settings(repo);
+- if (repo->settings.command_requires_full_index) {
++ prepare_repo_settings(r);
++ if (r->settings.command_requires_full_index) {
+ ensure_full_index(o->src_index);
+ ensure_full_index(o->dst_index);
+ }
@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
builtin/checkout.c | 9 +++++---- cache-tree.c | 10 ++++++---- cache-tree.h | 6 ++++-- sequencer.c | 6 +++--- sparse-index.c | 9 ++++++--- t/helper/test-dump-cache-tree.c | 2 +- unpack-trees.c | 19 ++++++++++--------- 7 files changed, 35 insertions(+), 26 deletions(-)
builtin/checkout.c +5 -4
@@ -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;hold_locked_index (&lock_file,LOCK_DIE_ON_ERROR );if (read_cache_preload (NULL ) <0 )
@@ -768,7 +769,7 @@ static int merge_working_tree(const struct checkout_opts *opts,return 1 ; old_tree =get_commit_tree (old_branch_info->commit); -if (repo_index_has_changes (the_repository, old_tree, &sb)) +if (repo_index_has_changes (r, old_tree, &sb))die (_ ("cannot continue with staged changes in " "the following files:\n%s" ), sb.buf);strbuf_release (&sb);
@@ -787,9 +788,9 @@ static int merge_working_tree(const struct checkout_opts *opts,*/ add_files_to_cache (NULL ,NULL ,0 ); -init_merge_options (&o, the_repository); +init_merge_options (&o, r); o.verbosity =0 ; - work =write_in_core_index_as_tree (the_repository); + work =write_in_core_index_as_tree (r); ret = reset_tree(new_tree, opts, 1,
@@ -822,7 +823,7 @@ static int merge_working_tree(const struct checkout_opts *opts, }if (!cache_tree_fully_valid (active_cache_tree)) -cache_tree_update (&the_index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); +cache_tree_update (r, r->index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR );if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK ))die (_ ("unable to write new index file" ));
cache-tree.c +6 -4
@@ -452,7 +452,7 @@ static int update_one(struct cache_tree *it,return i; } -int cache_tree_update (struct index_state *istate,int flags) +int cache_tree_update (struct repository *r,struct index_state *istate,int flags) {int skip, i;
@@ -467,10 +467,10 @@ int cache_tree_update(struct index_state *istate, int flags) istate->cache_tree =cache_tree ();trace_performance_enter (); -trace2_region_enter ("cache_tree" ,"update" , the_repository); +trace2_region_enter ("cache_tree" ,"update" , r); i =update_one (istate->cache_tree, istate->cache, istate->cache_nr,"" ,0 , &skip, flags); -trace2_region_leave ("cache_tree" ,"update" , the_repository); +trace2_region_leave ("cache_tree" ,"update" , r);trace_performance_leave ("cache_tree_update" );if (i <0 )return i;
@@ -654,12 +654,14 @@ static int write_index_as_tree_internal(struct object_id *oid,int flags, const char *prefix) { + struct repository *r = the_repository; +if (flags &WRITE_TREE_IGNORE_CACHE_TREE ) {cache_tree_free (&index_state->cache_tree); cache_tree_valid =0 ; } -if (!cache_tree_valid &&cache_tree_update (index_state, flags) <0 ) +if (!cache_tree_valid &&cache_tree_update (r, index_state, flags) <0 )return WRITE_TREE_UNMERGED_INDEX ;if (prefix) {
cache-tree.h +4 -2
@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);struct cache_tree *cache_tree_read (const char *buffer,unsigned long size);int cache_tree_fully_valid (struct cache_tree *); -int cache_tree_update (struct index_state *,int ); +int cache_tree_update (struct repository *r,struct index_state *,int );void cache_tree_verify (struct repository *,struct index_state *);/* bitmasks to write_index_as_tree flags */
@@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const chstatic inline int update_main_cache_tree (int flags) { +struct repository *r = the_repository; +if (!the_index.cache_tree) the_index.cache_tree =cache_tree (); -return cache_tree_update (&the_index, flags); +return cache_tree_update (r, r->index, flags); }#endif
sequencer.c +3 -3
@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,return !clean; } -static struct object_id *get_cache_tree_oid (struct index_state *istate) +static struct object_id *get_cache_tree_oid (struct repository *r,struct index_state *istate) {if (!cache_tree_fully_valid (istate->cache_tree)) -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {error (_ ("unable to update cache tree" ));return NULL ; }
@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)if (parse_commit (head_commit))return -1 ; -if (!(cache_tree_oid =get_cache_tree_oid (istate))) +if (!(cache_tree_oid =get_cache_tree_oid (r, istate)))return -1 ;return oideq (cache_tree_oid,get_commit_tree_oid (head_commit));
sparse-index.c +6 -3
@@ -135,6 +135,8 @@ int set_sparse_index_config(struct repository *repo, int enable)int convert_to_sparse (struct index_state *istate) {int test_env; +struct repository *r= the_repository; +if (istate->split_index || istate->sparse_index || !core_apply_sparse_checkout || !core_sparse_checkout_cone)return 0 ;
@@ -169,7 +171,7 @@ int convert_to_sparse(struct index_state *istate)return -1 ; } -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {warning (_ ("unable to update cache-tree, staying full" ));return -1 ; }
@@ -183,7 +185,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 (r, istate,0 ); istate->sparse_index =1 ;trace2_region_leave ("index" ,"convert_to_sparse" , istate->repo);
@@ -224,6 +226,7 @@ void ensure_full_index(struct index_state *istate)int i;struct index_state *full;struct strbuf base =STRBUF_INIT ; +struct repository *r = the_repository;if (!istate || !istate->sparse_index)return ;
@@ -287,7 +290,7 @@ void ensure_full_index(struct index_state *istate)/* Clear and recompute the cache-tree */ cache_tree_free (&istate->cache_tree); -cache_tree_update (istate,0 ); +cache_tree_update (r, istate,0 );trace2_region_leave ("index" ,"ensure_full_index" , istate->repo); }
t/helper/test-dump-cache-tree.c +1 -1
@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)die ("unable to read index file" ); istate = the_index; istate.cache_tree = another; -cache_tree_update (&istate,WRITE_TREE_DRY_RUN ); +cache_tree_update (the_repository, &istate,WRITE_TREE_DRY_RUN );return dump_cache_tree (active_cache_tree, another,"" ); }
unpack-trees.c +10 -9
@@ -1571,7 +1571,7 @@ static int verify_absent(const struct cache_entry *,*/ int unpack_trees (unsigned len,struct tree_desc *t,struct unpack_trees_options *o) { -struct repository *repo = the_repository; +struct repository *r = the_repository;int i, ret;static struct cache_entry *dfc;struct pattern_list pl;
@@ -1581,10 +1581,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsdie ("unpack_trees takes at most %d trees" ,MAX_UNPACK_TREES );trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"unpack_trees" , r); -prepare_repo_settings (repo); -if (repo->settings.command_requires_full_index) { +prepare_repo_settings (r); +if (r->settings.command_requires_full_index) {ensure_full_index (o->src_index);ensure_full_index (o->dst_index); }
@@ -1662,9 +1662,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options }trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"traverse_trees" , r); ret =traverse_trees (o->src_index, len, t, &info); -trace2_region_leave ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"traverse_trees" , r);trace_performance_leave ("traverse_trees" );if (ret <0 ) goto return_failed;
@@ -1732,9 +1732,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsmove_index_extensions (&o->result, o->src_index);if (!ret) {if (git_env_bool ("GIT_TEST_CHECK_CACHE_TREE" ,0 )) -cache_tree_verify (the_repository, &o->result); +cache_tree_verify (r, &o->result);if (!cache_tree_fully_valid (o->result.cache_tree)) -cache_tree_update (&o->result, +cache_tree_update (r, + &o->result,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); }
@@ -1750,7 +1751,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done:if (free_pattern_list)clear_pattern_list (&pl); -trace2_region_leave ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"unpack_trees" , r);trace_performance_leave ("unpack_trees" );return ret;
base-commit: 2e85e4c16a11f3ffa9bf5a433a72618331e1fb70
Signature · 1 line
gitgitgadget
PATCH v3 1/1 cache-tree.c: remove implicit dependency on the_repository
From: Chinmoy Chakraborty <[email protected]>
This kills the_repository dependency in cache_tree_update(), but for unpack_trees(), they still assume the_repository (which also means the_index).
Unfortunately the widespread use of unpack_trees() will make it hard to make the conversion now.
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.
This commit also fixes the sparse-index.c file in which the convert_to_sparse() and ensure_full_index() method use cache_tree_update().
---
Replace the_repository with r
There are multiple files that try to reference the repository and the_index directly. To follow a more object-oriented convention these references should be replaced with r and index and passed through functions.
Related issue
=============
#379
Changes since v1
================
* Fixed the sparse-index.c file in which the convert_to_sparse() and ensure_full_index() method use cache_tree_update().
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/915
Range-diff vs v2:
1: c9c2fbdc2bd1 ! 1: 2a4fad2781e3 cache-tree.c: remove implicit dependency on the_repository
@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op
- cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);
+ cache_tree_update(r, r->index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);
- if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+- if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
++ if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
die(_("unable to write new index file"));
+
+ if (!opts->discard_changes && !opts->quiet && new_branch_info->commit)
## cache-tree.c ##
@@ cache-tree.c: static int update_one(struct cache_tree *it,
@@ cache-tree.h: void cache_tree_write(struct strbuf *, struct cache_tree *root);
int cache_tree_fully_valid(struct cache_tree *);
-int cache_tree_update(struct index_state *, int);
-+int cache_tree_update(struct repository *r, struct index_state *, int);
++int cache_tree_update(struct repository *, struct index_state *, int);
void cache_tree_verify(struct repository *, struct index_state *);
/* bitmasks to write_index_as_tree flags */
@@ sparse-index.c: int set_sparse_index_config(struct repository *repo, int enable)
int convert_to_sparse(struct index_state *istate)
{
int test_env;
-+ struct repository *r= the_repository;
++ struct repository *r = the_repository;
+
if (istate->split_index || istate->sparse_index ||
!core_apply_sparse_checkout || !core_sparse_checkout_cone)
return 0;
+
+ if (!istate->repo)
+- istate->repo = the_repository;
++ istate->repo = r;
+
+ /*
+ * The GIT_TEST_SPARSE_INDEX environment variable triggers the
@@ sparse-index.c: int convert_to_sparse(struct index_state *istate)
return -1;
}
@@ sparse-index.c: void ensure_full_index(struct index_state *istate)
if (!istate || !istate->sparse_index)
return;
+
+ if (!istate->repo)
+- istate->repo = the_repository;
++ istate->repo = r;
+
+ trace2_region_enter("index", "ensure_full_index", istate->repo);
+
@@ sparse-index.c: void ensure_full_index(struct index_state *istate)
/* Clear and recompute the cache-tree */builtin/checkout.c | 11 ++++++----- cache-tree.c | 10 ++++++---- cache-tree.h | 6 ++++-- sequencer.c | 6 +++--- sparse-index.c | 13 ++++++++----- t/helper/test-dump-cache-tree.c | 2 +- unpack-trees.c | 19 ++++++++++--------- 7 files changed, 38 insertions(+), 29 deletions(-)
builtin/checkout.c +6 -5
@@ -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;hold_locked_index (&lock_file,LOCK_DIE_ON_ERROR );if (read_cache_preload (NULL ) <0 )
@@ -768,7 +769,7 @@ static int merge_working_tree(const struct checkout_opts *opts,return 1 ; old_tree =get_commit_tree (old_branch_info->commit); -if (repo_index_has_changes (the_repository, old_tree, &sb)) +if (repo_index_has_changes (r, old_tree, &sb))die (_ ("cannot continue with staged changes in " "the following files:\n%s" ), sb.buf);strbuf_release (&sb);
@@ -787,9 +788,9 @@ static int merge_working_tree(const struct checkout_opts *opts,*/ add_files_to_cache (NULL ,NULL ,0 ); -init_merge_options (&o, the_repository); +init_merge_options (&o, r); o.verbosity =0 ; - work =write_in_core_index_as_tree (the_repository); + work =write_in_core_index_as_tree (r); ret = reset_tree(new_tree, opts, 1,
@@ -822,9 +823,9 @@ static int merge_working_tree(const struct checkout_opts *opts, }if (!cache_tree_fully_valid (active_cache_tree)) -cache_tree_update (&the_index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); +cache_tree_update (r, r->index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); -if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) +if (write_locked_index (r->index, &lock_file,COMMIT_LOCK ))die (_ ("unable to write new index file" ));if (!opts->discard_changes && !opts->quiet && new_branch_info->commit)
cache-tree.c +6 -4
@@ -452,7 +452,7 @@ static int update_one(struct cache_tree *it,return i; } -int cache_tree_update (struct index_state *istate,int flags) +int cache_tree_update (struct repository *r,struct index_state *istate,int flags) {int skip, i;
@@ -467,10 +467,10 @@ int cache_tree_update(struct index_state *istate, int flags) istate->cache_tree =cache_tree ();trace_performance_enter (); -trace2_region_enter ("cache_tree" ,"update" , the_repository); +trace2_region_enter ("cache_tree" ,"update" , r); i =update_one (istate->cache_tree, istate->cache, istate->cache_nr,"" ,0 , &skip, flags); -trace2_region_leave ("cache_tree" ,"update" , the_repository); +trace2_region_leave ("cache_tree" ,"update" , r);trace_performance_leave ("cache_tree_update" );if (i <0 )return i;
@@ -654,12 +654,14 @@ static int write_index_as_tree_internal(struct object_id *oid,int flags, const char *prefix) { + struct repository *r = the_repository; +if (flags &WRITE_TREE_IGNORE_CACHE_TREE ) {cache_tree_free (&index_state->cache_tree); cache_tree_valid =0 ; } -if (!cache_tree_valid &&cache_tree_update (index_state, flags) <0 ) +if (!cache_tree_valid &&cache_tree_update (r, index_state, flags) <0 )return WRITE_TREE_UNMERGED_INDEX ;if (prefix) {
cache-tree.h +4 -2
@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);struct cache_tree *cache_tree_read (const char *buffer,unsigned long size);int cache_tree_fully_valid (struct cache_tree *); -int cache_tree_update (struct index_state *,int ); +int cache_tree_update (struct repository *,struct index_state *,int );void cache_tree_verify (struct repository *,struct index_state *);/* bitmasks to write_index_as_tree flags */
@@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const chstatic inline int update_main_cache_tree (int flags) { +struct repository *r = the_repository; +if (!the_index.cache_tree) the_index.cache_tree =cache_tree (); -return cache_tree_update (&the_index, flags); +return cache_tree_update (r, r->index, flags); }#endif
sequencer.c +3 -3
@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,return !clean; } -static struct object_id *get_cache_tree_oid (struct index_state *istate) +static struct object_id *get_cache_tree_oid (struct repository *r,struct index_state *istate) {if (!cache_tree_fully_valid (istate->cache_tree)) -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {error (_ ("unable to update cache tree" ));return NULL ; }
@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)if (parse_commit (head_commit))return -1 ; -if (!(cache_tree_oid =get_cache_tree_oid (istate))) +if (!(cache_tree_oid =get_cache_tree_oid (r, istate)))return -1 ;return oideq (cache_tree_oid,get_commit_tree_oid (head_commit));
sparse-index.c +8 -5
@@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)int convert_to_sparse (struct index_state *istate) {int test_env; +struct repository *r = the_repository; +if (istate->split_index || istate->sparse_index || !core_apply_sparse_checkout || !core_sparse_checkout_cone)return 0 ;if (!istate->repo) - istate->repo = the_repository; + istate->repo = r;/* * The GIT_TEST_SPARSE_INDEX environment variable triggers the
@@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)return -1 ; } -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {warning (_ ("unable to update cache-tree, staying full" ));return -1 ; }
@@ -175,7 +177,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 (r, istate,0 ); istate->sparse_index =1 ;trace2_region_leave ("index" ,"convert_to_sparse" , istate->repo);
@@ -216,12 +218,13 @@ void ensure_full_index(struct index_state *istate)int i;struct index_state *full;struct strbuf base =STRBUF_INIT ; +struct repository *r = the_repository;if (!istate || !istate->sparse_index)return ;if (!istate->repo) - istate->repo = the_repository; + istate->repo = r;trace2_region_enter ("index" ,"ensure_full_index" , istate->repo);
@@ -279,7 +282,7 @@ void ensure_full_index(struct index_state *istate)/* Clear and recompute the cache-tree */ cache_tree_free (&istate->cache_tree); -cache_tree_update (istate,0 ); +cache_tree_update (r, istate,0 );trace2_region_leave ("index" ,"ensure_full_index" , istate->repo); }
t/helper/test-dump-cache-tree.c +1 -1
@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)die ("unable to read index file" ); istate = the_index; istate.cache_tree = another; -cache_tree_update (&istate,WRITE_TREE_DRY_RUN ); +cache_tree_update (the_repository, &istate,WRITE_TREE_DRY_RUN );return dump_cache_tree (active_cache_tree, another,"" ); }
unpack-trees.c +10 -9
@@ -1571,7 +1571,7 @@ static int verify_absent(const struct cache_entry *,*/ int unpack_trees (unsigned len,struct tree_desc *t,struct unpack_trees_options *o) { -struct repository *repo = the_repository; +struct repository *r = the_repository;int i, ret;static struct cache_entry *dfc;struct pattern_list pl;
@@ -1581,10 +1581,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsdie ("unpack_trees takes at most %d trees" ,MAX_UNPACK_TREES );trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"unpack_trees" , r); -prepare_repo_settings (repo); -if (repo->settings.command_requires_full_index) { +prepare_repo_settings (r); +if (r->settings.command_requires_full_index) {ensure_full_index (o->src_index);ensure_full_index (o->dst_index); }
@@ -1662,9 +1662,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options }trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"traverse_trees" , r); ret =traverse_trees (o->src_index, len, t, &info); -trace2_region_leave ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"traverse_trees" , r);trace_performance_leave ("traverse_trees" );if (ret <0 ) goto return_failed;
@@ -1732,9 +1732,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsmove_index_extensions (&o->result, o->src_index);if (!ret) {if (git_env_bool ("GIT_TEST_CHECK_CACHE_TREE" ,0 )) -cache_tree_verify (the_repository, &o->result); +cache_tree_verify (r, &o->result);if (!cache_tree_fully_valid (o->result.cache_tree)) -cache_tree_update (&o->result, +cache_tree_update (r, + &o->result,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); }
@@ -1750,7 +1751,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done:if (free_pattern_list)clear_pattern_list (&pl); -trace2_region_leave ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"unpack_trees" , r);trace_performance_leave ("unpack_trees" );return ret;
base-commit: c9e40ae8ec41c5566e5849a87c969fa81ef49fcd
Signature · 1 line
gitgitgadget
Quoted 3 lines · Chinmoy via GitGitGadget
From: Chinmoy Chakraborty <[email protected]>
This kills
the_repositorydependency incache_tree_update(),
That part of the sentence is good. But the next four lines are not.
Quoted 5 lines
but for
unpack_trees(), they still assumethe_repository(which also meansthe_index).Unfortunately the widespread use of
unpack_trees()will make it hard to make the conversion now.
As long as all the users of unpack_trees() want to work with the primary in-core index instance, that is not a problem. There is no need to lament with a phrase like "unfortunately". If you are not touching unpack_trees(), but you needed to adjust its use of cache_tree_update(), then none of the above needs to be said. If you change one API function (i.e. cache_tree_update()), all callers need to be adjusted for that change, but if one caller only wants to work in the primary repository and with its primary index, that is fine. You change it so that it passes the repository and the index it wants the updated unpack_trees() to work on, and there is nothing notable in it.
Quoted 5 lines
The
update_main_cache_tree()method usescache_tree_update(r, r->index, flags).r->indexis easily deduced fromrbut the signature ofcache_tree_update()is not changed to takestruct repository *instead ofstruct index_state *because there can be temporary indexes. Therefore, one might want to update the cache tree for an index other thanr->index.
The flow of explanation of the thought in this paragraph is probably backwards for most people to understand what you wanted to say.
The cache_tree_update() was originally designed to take an istate so that it can work on an instance of in-core index that is not the primary one for the repository, and we want to keep that property while teaching the subsystem how to deal with a repository other than the_repository. Hence it can take a pointer to the repository and a pointer to istate separately.
Nothing else needs to be said. The update-main-cache-tree is a thin wrapper to work on cache tree for THE MAIN in-core index instance, so it is natural if it needs to pass r and r->index, exactly because the latter is THE MAIN in-core index instance for the repository. That is too obvious that it is best left unsaid, for the same reason why you shouldn't even mention what you did in updating the use of update_cache_tree() inside unpack_trees(). There is nothing notable in there.
cf. Documentation/SubmittingPatches
[[describe-changes]]
[[summary-section]]
[[meaningful-message]]
[[imperative-mood]]
Quoted 3 lines
This commit also fixes the
sparse-index.cfile in which theconvert_to_sparse()andensure_full_index()method usecache_tree_update().
What branch did you base this patch on? There is no update_cache_tree() call in sparse-index.c on 'master' (well, there is no sparse-index.c in 'master' to begin with).
While it is not prohibited to build on a commit that is not on 'master', you'd need to keep your dependencies to the minimum so that the topic won't be taken hostage by other topics that do not move quickly enough to your liking. Building on 'next' or 'seen' as a whole is a guarantee that the topic will never hit 'master', as no tip of 'next' will ever be merged directly to 'master'.
And if you are building somewhere other than the tip of 'master', it is necessary to say where, in order to save confusion and wasted effort for your readers. E.g. "This patch was made on top of merging topic 'X' and 'Y' into 'master'". Then you'd only have to wait for 'X' and 'Y' to graduate to 'master' before the topic can proceed.
Thanks.
Quoted 285 lines
builtin/checkout.c | 11 ++++++----- cache-tree.c | 10 ++++++---- cache-tree.h | 6 ++++-- sequencer.c | 6 +++--- sparse-index.c | 13 ++++++++----- t/helper/test-dump-cache-tree.c | 2 +- unpack-trees.c | 19 ++++++++++--------- 7 files changed, 38 insertions(+), 29 deletions(-)builtin/checkout.c +6
-5@@ -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;hold_locked_index (&lock_file,LOCK_DIE_ON_ERROR );if (read_cache_preload (NULL ) <0 )@@ -768,7 +769,7 @@ static int merge_working_tree(const struct checkout_opts *opts,return 1 ; old_tree =get_commit_tree (old_branch_info->commit); -if (repo_index_has_changes (the_repository, old_tree, &sb)) +if (repo_index_has_changes (r, old_tree, &sb))die (_ ("cannot continue with staged changes in " "the following files:\n%s" ), sb.buf);strbuf_release (&sb);@@ -787,9 +788,9 @@ static int merge_working_tree(const struct checkout_opts *opts,*/ add_files_to_cache (NULL ,NULL ,0 ); -init_merge_options (&o, the_repository); +init_merge_options (&o, r); o.verbosity =0 ; - work =write_in_core_index_as_tree (the_repository); + work =write_in_core_index_as_tree (r); ret = reset_tree(new_tree, opts, 1,@@ -822,9 +823,9 @@ static int merge_working_tree(const struct checkout_opts *opts, }if (!cache_tree_fully_valid (active_cache_tree)) -cache_tree_update (&the_index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); +cache_tree_update (r, r->index,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); -if (write_locked_index (&the_index, &lock_file,COMMIT_LOCK )) +if (write_locked_index (r->index, &lock_file,COMMIT_LOCK ))die (_ ("unable to write new index file" ));if (!opts->discard_changes && !opts->quiet && new_branch_info->commit)cache-tree.c +6
-4@@ -452,7 +452,7 @@ static int update_one(struct cache_tree *it,return i; } -int cache_tree_update (struct index_state *istate,int flags) +int cache_tree_update (struct repository *r,struct index_state *istate,int flags) {int skip, i;@@ -467,10 +467,10 @@ int cache_tree_update(struct index_state *istate, int flags) istate->cache_tree =cache_tree ();trace_performance_enter (); -trace2_region_enter ("cache_tree" ,"update" , the_repository); +trace2_region_enter ("cache_tree" ,"update" , r); i =update_one (istate->cache_tree, istate->cache, istate->cache_nr,"" ,0 , &skip, flags); -trace2_region_leave ("cache_tree" ,"update" , the_repository); +trace2_region_leave ("cache_tree" ,"update" , r);trace_performance_leave ("cache_tree_update" );if (i <0 )return i;@@ -654,12 +654,14 @@ static int write_index_as_tree_internal(struct object_id *oid,int flags, const char *prefix) { + struct repository *r = the_repository; +if (flags &WRITE_TREE_IGNORE_CACHE_TREE ) {cache_tree_free (&index_state->cache_tree); cache_tree_valid =0 ; } -if (!cache_tree_valid &&cache_tree_update (index_state, flags) <0 ) +if (!cache_tree_valid &&cache_tree_update (r, index_state, flags) <0 )return WRITE_TREE_UNMERGED_INDEX ;if (prefix) {cache-tree.h +4
-2@@ -33,7 +33,7 @@ void cache_tree_write(struct strbuf *, struct cache_tree *root);struct cache_tree *cache_tree_read (const char *buffer,unsigned long size);int cache_tree_fully_valid (struct cache_tree *); -int cache_tree_update (struct index_state *,int ); +int cache_tree_update (struct repository *,struct index_state *,int );void cache_tree_verify (struct repository *,struct index_state *);/* bitmasks to write_index_as_tree flags */ @@ -62,9 +62,11 @@ static inline int write_cache_as_tree(struct object_id *oid, int flags, const chstatic inline int update_main_cache_tree (int flags) { +struct repository *r = the_repository; +if (!the_index.cache_tree) the_index.cache_tree =cache_tree (); -return cache_tree_update (&the_index, flags); +return cache_tree_update (r, r->index, flags); }#endif sequencer.c +3
-3@@ -677,10 +677,10 @@ static int do_recursive_merge(struct repository *r,return !clean; } -static struct object_id *get_cache_tree_oid (struct index_state *istate) +static struct object_id *get_cache_tree_oid (struct repository *r,struct index_state *istate) {if (!cache_tree_fully_valid (istate->cache_tree)) -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {error (_ ("unable to update cache tree" ));return NULL ; }@@ -710,7 +710,7 @@ static int is_index_unchanged(struct repository *r)if (parse_commit (head_commit))return -1 ; -if (!(cache_tree_oid =get_cache_tree_oid (istate))) +if (!(cache_tree_oid =get_cache_tree_oid (r, istate)))return -1 ;return oideq (cache_tree_oid,get_commit_tree_oid (head_commit));sparse-index.c +8
-5@@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)int convert_to_sparse (struct index_state *istate) {int test_env; +struct repository *r = the_repository; +if (istate->split_index || istate->sparse_index || !core_apply_sparse_checkout || !core_sparse_checkout_cone)return 0 ;if (!istate->repo) - istate->repo = the_repository; + istate->repo = r;/* * The GIT_TEST_SPARSE_INDEX environment variable triggers the @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)return -1 ; } -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {warning (_ ("unable to update cache-tree, staying full" ));return -1 ; }@@ -175,7 +177,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 (r, istate,0 ); istate->sparse_index =1 ;trace2_region_leave ("index" ,"convert_to_sparse" , istate->repo);@@ -216,12 +218,13 @@ void ensure_full_index(struct index_state *istate)int i;struct index_state *full;struct strbuf base =STRBUF_INIT ; +struct repository *r = the_repository;if (!istate || !istate->sparse_index)return ;if (!istate->repo) - istate->repo = the_repository; + istate->repo = r;trace2_region_enter ("index" ,"ensure_full_index" , istate->repo);@@ -279,7 +282,7 @@ void ensure_full_index(struct index_state *istate)/* Clear and recompute the cache-tree */ cache_tree_free (&istate->cache_tree); -cache_tree_update (istate,0 ); +cache_tree_update (r, istate,0 );trace2_region_leave ("index" ,"ensure_full_index" , istate->repo); }t/helper/test-dump-cache-tree.c +1
-1@@ -64,6 +64,6 @@ int cmd__dump_cache_tree(int ac, const char **av)die ("unable to read index file" ); istate = the_index; istate.cache_tree = another; -cache_tree_update (&istate,WRITE_TREE_DRY_RUN ); +cache_tree_update (the_repository, &istate,WRITE_TREE_DRY_RUN );return dump_cache_tree (active_cache_tree, another,"" ); }unpack-trees.c +10
-9@@ -1571,7 +1571,7 @@ static int verify_absent(const struct cache_entry *,*/ int unpack_trees (unsigned len,struct tree_desc *t,struct unpack_trees_options *o) { -struct repository *repo = the_repository; +struct repository *r = the_repository;int i, ret;static struct cache_entry *dfc;struct pattern_list pl;@@ -1581,10 +1581,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsdie ("unpack_trees takes at most %d trees" ,MAX_UNPACK_TREES );trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"unpack_trees" , r); -prepare_repo_settings (repo); -if (repo->settings.command_requires_full_index) { +prepare_repo_settings (r); +if (r->settings.command_requires_full_index) {ensure_full_index (o->src_index);ensure_full_index (o->dst_index); }@@ -1662,9 +1662,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options }trace_performance_enter (); -trace2_region_enter ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_enter ("unpack_trees" ,"traverse_trees" , r); ret =traverse_trees (o->src_index, len, t, &info); -trace2_region_leave ("unpack_trees" ,"traverse_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"traverse_trees" , r);trace_performance_leave ("traverse_trees" );if (ret <0 ) goto return_failed;@@ -1732,9 +1732,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_optionsmove_index_extensions (&o->result, o->src_index);if (!ret) {if (git_env_bool ("GIT_TEST_CHECK_CACHE_TREE" ,0 )) -cache_tree_verify (the_repository, &o->result); +cache_tree_verify (r, &o->result);if (!cache_tree_fully_valid (o->result.cache_tree)) -cache_tree_update (&o->result, +cache_tree_update (r, + &o->result,WRITE_TREE_SILENT |WRITE_TREE_REPAIR ); }@@ -1750,7 +1751,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done:if (free_pattern_list)clear_pattern_list (&pl); -trace2_region_leave ("unpack_trees" ,"unpack_trees" , the_repository); +trace2_region_leave ("unpack_trees" ,"unpack_trees" , r);trace_performance_leave ("unpack_trees" );return ret;base-commit:
c9e40ae8ec41c5566e5849a87c969fa81ef49fcd
Quoted 9 lines · Junio C Hamano
Nothing else needs to be said. The update-main-cache-tree is a thin wrapper to work on cache tree for THE MAIN in-core index instance, so it is natural if it needs to pass r and
r->index, exactly because the latter is THE MAIN in-core index instance for the repository. That is too obvious that it is best left unsaid, for the same reason why you shouldn't even mention what you did in updating the use ofupdate_cache_tree()insideunpack_trees(). There is nothing notable in there.
Quoted 5 lines
cf.
Documentation/SubmittingPatches
[[describe-changes]]
[[summary-section]]
[[meaningful-message]]
[[imperative-mood]]
Okay, I'll update the message. Thanks for the review.
Quoted 6 lines
Quoted 3 lines
This commit also fixes the
sparse-index.cfile in which theconvert_to_sparse()andensure_full_index()method usecache_tree_update().What branch did you base this patch on? There is no
update_cache_tree()call insparse-index.con 'master' (well, there is nosparse-index.cin 'master' to begin with).
Actually the patch hit against a use of cache_tree_update() in
the branch 'ds/sparse-index', so I was suggested by Derrick Stolee
to rebase my work on to of his branch.
You may view the discussion here:
https://lore.kernel.org/git/[email protected]/
Quoted 8 lines
And if you are building somewhere other than the tip of 'master', it is necessary to say where, in order to save confusion and wasted effort for your readers. E.g. "This patch was made on top of merging topic 'X' and 'Y' into 'master'". Then you'd only have to wait for 'X' and 'Y' to graduate to 'master' before the topic can proceed.
Thanks.
Should this information also be added to the commit message
or just the cover letter of the patch?
Quoted 9 lines · Junio C Hamano
Nothing else needs to be said. The update-main-cache-tree is a thin wrapper to work on cache tree for THE MAIN in-core index instance, so it is natural if it needs to pass r and
r->index, exactly because the latter is THE MAIN in-core index instance for the repository. That is too obvious that it is best left unsaid, for the same reason why you shouldn't even mention what you did in updating the use ofupdate_cache_tree()insideunpack_trees(). There is nothing notable in there.
Quoted 5 lines
cf.
Documentation/SubmittingPatches
[[describe-changes]]
[[summary-section]]
[[meaningful-message]]
[[imperative-mood]]
Okay, I'll update the message. Thanks for the review.
Quoted 6 lines
Quoted 3 lines
This commit also fixes the
sparse-index.cfile in which theconvert_to_sparse()andensure_full_index()method usecache_tree_update().What branch did you base this patch on? There is no
update_cache_tree()call insparse-index.con 'master' (well, there is nosparse-index.cin 'master' to begin with).
Actually the patch hit against a use of cache_tree_update() in
the branch 'ds/sparse-index', so I was suggested by Derrick Stolee
to rebase my work on to of his branch.
You may view the discussion here:
https://lore.kernel.org/git/[email protected]/
Quoted 8 lines
And if you are building somewhere other than the tip of 'master', it is necessary to say where, in order to save confusion and wasted effort for your readers. E.g. "This patch was made on top of merging topic 'X' and 'Y' into 'master'". Then you'd only have to wait for 'X' and 'Y' to graduate to 'master' before the topic can proceed.
Thanks.
Should this information be added to the commit message
or just the cover letter of the patch?
Quoted 7 lines · Chinmoy Chakraborty
Quoted 7 lines
And if you are building somewhere other than the tip of 'master', it is necessary to say where, in order to save confusion and wasted effort for your readers. E.g. "This patch was made on top of merging topic 'X' and 'Y' into 'master'". Then you'd only have to wait for 'X' and 'Y' to graduate to 'master' before the topic can proceed.
Quoted 5 lines
Quoted 1 line
Thanks.
Should this information also be added to the commit message
or just the cover letter of the patch?
We often see people write it in the cover letter for multi-patch series. For a single patch topic, it is written under the three-dash line.
Thanks.
Quoted 8 lines · Chinmoy via GitGitGadget
sparse-index.c +0
-0@@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)int convert_to_sparse (struct index_state *istate) {int test_env;
sparse-index.c int set_sparse_index_config(struct repository *repo, int enable) · line 139
Quoted 10 lines
sparse-index.c +3
-1+struct repository *r = the_repository; +if (istate->split_index || istate->sparse_index || !core_apply_sparse_checkout || !core_sparse_checkout_cone)return 0 ;if (!istate->repo) - istate->repo = the_repository; + istate->repo = r;
I am not quite sure if this is a reasonable conversion. Surely it would not make the compiler barf, but are we sure that the caller of convert_to_sparse() wants us to work on the_repository instance and no other repository? As an istate has a .repo member, it seems to me that using istate->repo would be a lot saner approach than assigning the_repository upfront to r. It might be even needed, if cache_tree_update() wants to take a repository instance, to ensure that all callers to this helper sets istate->repo before they call it so that the above "if not set yet, use the_repository" code does not have to kick in.
sparse-index.c int convert_to_sparse(struct index_state *istate) · line 166
Quoted 8 lines
sparse-index.c +1
-1/* * The GIT_TEST_SPARSE_INDEX environment variable triggers the @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)return -1 ; } -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {
And this looks like a bad conversion. It may happen to do the same thing, but the flow of the logic up to this point in the function was to make sure istate->repo is not empty by filling it if it is not yet set, and update the cache tree of that istate. So, it seems more logical if this call were like so, no?
if (cache_tree_update(istate->repo, istate, 0)) {In fact, in the world after 1fd9ae51 (repository: add repo reference to index_state, 2021-01-23), it is dubious that this topic to teach cache_tree_update() to take a repository pointer is sensible. While working on a single repository, we may create multiple in-core index instances that represent temporary indices, but each of these in-core index instances (i.e. istate) belong to a single repository.
And in a call to cache_tree_update(R, I, F), if I->repo is *NOT* R, that must mean a bug. Here is what 1fd9ae51 says on this point.
repository: add repo reference to index_state
It will be helpful to add behavior to index operations that might trigger an object lookup. Since each index belongs to a specific repository, add a 'repo' pointer to struct index_state that allows access to this repository.
Add a BUG() statement if the repo already has an index, and the index already has a repo, but somehow the index points to a different repo.
This will prevent future changes from needing to pass an additional 'struct repository *repo' parameter and instead rely only on the 'struct index_state *istate' parameter.
Derrick, what's you thought on this? The patch under discussion looks to me a prime example of "future change(s)" needing "to pass an additional 'struct repository *repo' parameter".
Thanks.
Quoted 69 lines · Junio C Hamano
Quoted 8 lines · Chinmoy via GitGitGadget
sparse-index.c +0
-0@@ -128,12 +128,14 @@ int set_sparse_index_config(struct repository *repo, int enable)int convert_to_sparse (struct index_state *istate) {int test_env;sparse-index.c int set_sparse_index_config(struct repository *repo, int enable) · line 139
Quoted 10 lines
sparse-index.c +3
-1+struct repository *r = the_repository; +if (istate->split_index || istate->sparse_index || !core_apply_sparse_checkout || !core_sparse_checkout_cone)return 0 ;if (!istate->repo) - istate->repo = the_repository; + istate->repo = r;I am not quite sure if this is a reasonable conversion. Surely it would not make the compiler barf, but are we sure that the caller of
convert_to_sparse()wants us to work onthe_repositoryinstance and no other repository? As an istate has a .repo member, it seems to me that usingistate->repowould be a lot saner approach than assigningthe_repositoryupfront to r. It might be even needed, ifcache_tree_update()wants to take a repository instance, to ensure that all callers to this helper setsistate->repobefore they call it so that the above "if not set yet, usethe_repository" code does not have to kick in.sparse-index.c int convert_to_sparse(struct index_state *istate) · line 166
Quoted 8 lines
sparse-index.c +1
-1/* * The GIT_TEST_SPARSE_INDEX environment variable triggers the @@ -161,7 +163,7 @@ int convert_to_sparse(struct index_state *istate)return -1 ; } -if (cache_tree_update (istate,0 )) { +if (cache_tree_update (r, istate,0 )) {And this looks like a bad conversion. It may happen to do the same thing, but the flow of the logic up to this point in the function was to make sure
istate->repois not empty by filling it if it is not yet set, and update the cache tree of that istate. So, it seems more logical if this call were like so, no?if (cache_tree_update(istate->repo, istate, 0)) {In fact, in the world after
1fd9ae51(repository: add repo reference toindex_state, 2021-01-23), it is dubious that this topic to teachcache_tree_update()to take a repository pointer is sensible. While working on a single repository, we may create multiple in-core index instances that represent temporary indices, but each of these in-core index instances (i.e. istate) belong to a single repository.And in a call to
cache_tree_update(R, I, F), ifI->repois *NOT* R, that must mean a bug. Here is what1fd9ae51says on this point.repository: add repo reference to
index_stateIt will be helpful to add behavior to index operations that might trigger an object lookup. Since each index belongs to a specific repository, add a 'repo' pointer to struct
index_statethat allows access to this repository.Add a
BUG()statement if the repo already has an index, and the index already has a repo, but somehow the index points to a different repo.
Quoted 7 lines
This will prevent future changes from needing to pass an additional 'struct repository *repo' parameter and instead rely only on the 'struct
index_state*istate' parameter.Derrick, what's you thought on this? The patch under discussion looks to me a prime example of "future change(s)" needing "to pass an additional 'struct repository *repo' parameter".
With your additional comments, I think it is clear that the "fourth option" I mentioned earlier [1] is the way to go:
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.
[1] https://lore.kernel.org/git/[email protected]/
So in this sense, we should always use istate->repo, but we might still need the following guard in some places:
if (!istate->repo) istate->repo = the_repository;
in case there are situations where the index is loaded before the_repository is loaded. I have hit this in testing, but don't fully understand the cases where this can happen.
The way it would change this patch is to drop the 'struct repository *r' pointers and changes to the method signatures. Instead, keep the methods only taking a 'struct index_state *istate' and use istate->repo everywhere.
Thanks,
-Stolee
Quoted 9 lines · Derrick Stolee
With your additional comments, I think it is clear that the "fourth option" I mentioned earlier [1] is the way to go:
Finally, there is yet a fourth option: use
istate->repoinstead. In1fd9ae51(repository: add repo reference toindex_state), I added a 'repo' member to structindex_state. This is intended for methods to access a repository directly from the index.
Thanks. I wasn't following the earlier discussion closely, as the topic seemed to be in the hands of good reviewers ;-)
Quoted 9 lines
So in this sense, we should always use
istate->repo, but we might still need the following guard in some places:if (!istate->repo) istate->repo = the_repository;in case there are situations where the index is loaded before
the_repositoryis loaded. I have hit this in testing, but don't fully understand the cases where this can happen.
As a longer term goal, it may not be a bad idea to make sure that anybody who passes an istate should not have to pass a repo. I do not think of a reason why, other than historical inertia, to do so in post 1fd9ae51 world.
Quoted 4 lines
The way it would change this patch is to drop the 'struct repository *r' pointers and changes to the method signatures. Instead, keep the methods only taking a 'struct
index_state*istate' and useistate->repoeverywhere.
Yes, and that would result in a patch that touches very limited small parts of cache-tree.c without needing to touch any caller, I would presume.
Thanks.
PATCH v4 1/1 cache-tree.c: remove implicit dependency on the_repository
In reply to Chinmoy via GitGitGadgetFrom: Chinmoy Chakraborty <[email protected]>
This kills the_repository dependency in cache_tree_update() and prime_cache_tree().
---
Replace the_repository with r
There are multiple files that try to reference the repository and the_index directly. To follow a more object-oriented convention these references should be replaced with r and index and passed through functions.
Related issue
=============
#379
Changes since v3
================
* Used istate->repo instead of the_repository to prevent making changes in callers of the function.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-915%2Fchinmoy12c%2Fissue_379-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-915/chinmoy12c/issue_379-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/915
Range-diff vs v3:
1: 2a4fad2781e3 < -: ------------ cache-tree.c: remove implicit dependency on the_repository -: ------------ > 1: 25f09954b9df cache-tree.c: remove implicit dependency on the_repository
cache-tree.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
cache-tree.c +4 -4
@@ -446,10 +446,10 @@ int cache_tree_update(struct index_state *istate, int flags) istate->cache_tree =cache_tree ();trace_performance_enter (); -trace2_region_enter ("cache_tree" ,"update" , the_repository); +trace2_region_enter ("cache_tree" ,"update" , istate->repo); i =update_one (istate->cache_tree, istate->cache, istate->cache_nr,"" ,0 , &skip, flags); -trace2_region_leave ("cache_tree" ,"update" , the_repository); +trace2_region_leave ("cache_tree" ,"update" , istate->repo);trace_performance_leave ("cache_tree_update" );if (i <0 )return i;
@@ -746,13 +746,13 @@ void prime_cache_tree(struct repository *r, struct index_state *istate, struct tree *tree) { - trace2_region_enter("cache-tree", "prime_cache_tree", the_repository); + trace2_region_enter("cache-tree", "prime_cache_tree", r);cache_tree_free (&istate->cache_tree); istate->cache_tree =cache_tree ();prime_cache_tree_rec (r, istate->cache_tree, tree); istate->cache_changed |=CACHE_TREE_CHANGED ; -trace2_region_leave ("cache-tree" ,"prime_cache_tree" , the_repository); +trace2_region_leave ("cache-tree" ,"prime_cache_tree" , r); }/*
base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48
Signature · 1 line
gitgitgadget
Quoted 8 lines · Chinmoy via GitGitGadget
From: Chinmoy Chakraborty <[email protected]>
This kills
the_repositorydependency incache_tree_update()andprime_cache_tree().Signed-off-by Chinmoy Chakraborty <[email protected]>---
Replacethe_repositorywith r
Huh???
Quoted 21 lines
cache-tree.c +2
-2@@ -446,10 +446,10 @@ int cache_tree_update(struct index_state *istate, int flags) istate->cache_tree =cache_tree ();trace_performance_enter (); -trace2_region_enter ("cache_tree" ,"update" , the_repository); +trace2_region_enter ("cache_tree" ,"update" , istate->repo); i =update_one (istate->cache_tree, istate->cache, istate->cache_nr,"" ,0 , &skip, flags); -trace2_region_leave ("cache_tree" ,"update" , the_repository); +trace2_region_leave ("cache_tree" ,"update" , istate->repo);trace_performance_leave ("cache_tree_update" );if (i <0 )return i;@@ -746,13 +746,13 @@ void prime_cache_tree(struct repository *r,struct index_state *istate, struct tree *tree) {
cache-tree.c void prime_cache_tree(struct repository *r, · line 756
Quoted 10 lines
cache-tree.c +2
-2-trace2_region_enter ("cache-tree" ,"prime_cache_tree" , the_repository); +trace2_region_enter ("cache-tree" ,"prime_cache_tree" , r);cache_tree_free (&istate->cache_tree); istate->cache_tree =cache_tree ();prime_cache_tree_rec (r, istate->cache_tree, tree); istate->cache_changed |=CACHE_TREE_CHANGED ; -trace2_region_leave ("cache-tree" ,"prime_cache_tree" , the_repository); +trace2_region_leave ("cache-tree" ,"prime_cache_tree" , r); }
The patch assumes that istate->repo will always set, but it does not even try to justify why that assumption is safe to make (e.g. "the entire codebase that leads to this function has been audited and made sure istate at this point will always have its .repo member is set" in the log message, if such an audit has actually been done, may have been convincing), which I find quite troubling.
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Chinmoy via GitGitGadget
From: Chinmoy Chakraborty <[email protected]>
This kills
the_repositorydependency incache_tree_update()andprime_cache_tree().Signed-off-by Chinmoy Chakraborty <[email protected]>---
Replacethe_repositorywith rHuh???
Sorry I forgot to change the cover letter.
Quoted 6 lines
The patch assumes that
istate->repowill always set, but it does not even try to justify why that assumption is safe to make (e.g. "the entire codebase that leads to this function has been audited and made sure istate at this point will always have its .repo member is set" in the log message, if such an audit has actually been done, may have been convincing), which I find quite troubling.
Is it safe to make this assumption? I mean to be completely
sure of this, one would have to track back to all the callers.
Should a check like:
if(!istate->repo)
istate->repo = the_repository;
be required?
Quoted 1 line · Chinmoy Chakraborty
Is it safe to make this assumption?
It was the question I asked, and I didn't see a reason to believe it is safe.
Quoted 2 lines
I mean to be completely sure of this, one would have to track back to all the callers.
Yes. If we audit the callers to make sure istate->repo always points at the right repository, add missing assignment to istate->repo as necessary, and add
if (!istate->repo)
BUG("caller of cache_tree_udpate() did not fill istate->repo");then that would be an improvement. But...
Quoted 7 lines
Should a check like:
if(!istate->repo) istate->repo = the_repository;be required?
... if we add such an "if the caller did not set istate->repo, assume the_repository" code, then the resulting code explicitly assumes that the istate the caller passed to us without setting istate->repo belongs to the default repository.
I do not quite see the point of such a change---it is not all that better than "implicit dependency on the_repository" the patch tries to address, is it?