From: Tian Yuchen <cat@malon.dev>
Date: Thu, 21 May 2026 16:37:29 GMT
Subject: Re: [PATCH v3 1/8] environment: move "trust_ctime" into `struct repo_config_values`
Message-ID: <08efcc49-0db8-49f6-8971-633aa55eb66c@malon.dev>
In-Reply-To: <20260423165432.143598-2-belkid98@gmail.com>

Hi Bello!

On 4/24/26 00:54, Olamide Caleb Bello wrote:

The code itself looks great to me, but I have some reservations about 
the description here (in terms of why trust_ctime is eagerly parsed):

 > `core.trustctime` is parsed eagerly
 > because it is used in low‑level stat‑matching functions
 > (`match_stat_data()`), where a lazy parse could cause unexpected
 > fatal errors and complicate libification efforts.

It's true that if we use repo_config_get_bool() to parse trust_ctime, 
following the call stack downwards, there is a die() call. The terminate 
condition is that the configuration does not exist or contains invalid 
characters.

But I think there is another factor: match_stat_data() is called on a 
hot path. The following code is implemented in read-cache.c, 
refresh_index() function:

	for (i = 0; i < istate->cache_nr; i++) {
		...
		new_entry = refresh_cache_ent(istate, ce, options,
					      &cache_errno, &changed,
					      &t2_did_lstat, &t2_did_scan);
		t2_sum_lstat += t2_did_lstat;
		t2_sum_scan += t2_did_scan;
		if (new_entry == ce)
		...

The call chain: refresh_index() -> refresh_cache_ent() -> 
ie_match_stat() -> ce_match_stat_basic() -> *match_stat_data()*

Therefore, if the variable is lazily parsed, this means there will be a 
performance regression whenever the index status needs to be checked, 
e.g. 'git status'.

So, I guess it would be better to extend a bit:

'...where a lazy parse could cause unexpected fatal, and result in a 
performance regression...'

Thanks, yuchen