| Message ID | pull.915.v4.git.1617778489719.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] cache-tree.c: remove implicit dependency on the_repository | expand |
"Chinmoy via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Chinmoy Chakraborty <chinmoy12c@gmail.com> > > This kills the_repository dependency in cache_tree_update() > and prime_cache_tree(). > > Signed-off-by: Chinmoy Chakraborty <chinmoy12c@gmail.com> > --- > Replace the_repository with r Huh??? > diff --git a/cache-tree.c b/cache-tree.c > index add1f0771317..4928a9f0f13b 100644 > --- a/cache-tree.c > +++ b/cache-tree.c > @@ -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); > } 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.
On 4/8/21 4:33 AM, Junio C Hamano wrote: > "Chinmoy via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Chinmoy Chakraborty <chinmoy12c@gmail.com> >> >> This kills the_repository dependency in cache_tree_update() >> and prime_cache_tree(). >> >> Signed-off-by: Chinmoy Chakraborty <chinmoy12c@gmail.com> >> --- >> Replace the_repository with r > Huh??? Sorry I forgot to change the cover letter. > 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. 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?
Chinmoy Chakraborty <chinmoy12c@gmail.com> writes: > 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. > 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... > 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?
diff --git a/cache-tree.c b/cache-tree.c index add1f0771317..4928a9f0f13b 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -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); } /*