git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Chinmoy via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>,
	Chinmoy <chinmoy12c@gmail.com>
Subject: Re: [PATCH v4] cache-tree.c: remove implicit dependency on the_repository
Date: Wed, 07 Apr 2021 16:03:05 -0700	[thread overview]
Message-ID: <xmqqsg41wvdi.fsf@gitster.g> (raw)
In-Reply-To: <pull.915.v4.git.1617778489719.gitgitgadget@gmail.com>

"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.

  reply	other threads:[~2021-04-07 23:03 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25 19:48 [PATCH] " Chinmoy via GitGitGadget
2021-03-25 20:31 ` Derrick Stolee
2021-03-26  6:49   ` Chinmoy Chakraborty
2021-03-26  7:54   ` Chinmoy Chakraborty
2021-03-26 15:35 ` [PATCH v2] " Chinmoy via GitGitGadget
2021-04-03 15:57   ` [PATCH v3] " Chinmoy via GitGitGadget
2021-04-04  1:49     ` Junio C Hamano
2021-04-04  5:11       ` Chinmoy Chakraborty
2021-04-04  5:36         ` Junio C Hamano
2021-04-04  5:18       ` Chinmoy Chakraborty
2021-04-04  6:09     ` Junio C Hamano
2021-04-05 13:08       ` Derrick Stolee
2021-04-05 17:48         ` Junio C Hamano
2021-04-07  6:54     ` [PATCH v4] " Chinmoy via GitGitGadget
2021-04-07 23:03       ` Junio C Hamano [this message]
2021-04-08  3:56         ` Chinmoy Chakraborty
2021-04-08 13:23           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsg41wvdi.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chinmoy12c@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH v4] cache-tree.c: remove implicit dependency on the_repository' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).