From: Tian Yuchen <cat@malon.dev>
Date: Mon, 01 Jun 2026 10:10:08 GMT
Subject: Re: [PATCH v1 3/4] environment: move 'trust_executable_bit' into repo_config_values
Message-ID: <e0d5b1af-b040-49e2-90f9-d8325682826b@malon.dev>
In-Reply-To: <xmqq7bokebct.fsf@gitster.g>

Hi Junio,

Thanks for the feedback!

On 5/31/26 07:17, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> diff --git a/apply.c b/apply.c
>> index 249248d4f2..73ca9907f8 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -3890,10 +3890,12 @@ static int check_preimage(struct apply_state *state,
>>   	}
>>   
>>   	if (!state->cached && !previous) {
>> +		struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>>   		if (*ce && !(*ce)->ce_mode)
>>   			BUG("ce_mode == 0 for path '%s'", old_name);
>>   
>> -		if (trust_executable_bit || !S_ISREG(st->st_mode))
>> +		if (cfg->trust_executable_bit || !S_ISREG(st->st_mode))
>>   			st_mode = ce_mode_from_stat(*ce, st->st_mode);
>>   		else if (*ce)
>>   			st_mode = (*ce)->ce_mode;
>> diff --git a/read-cache.c b/read-cache.c
>> index 54150fe756..18af533649 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -204,10 +204,12 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
>>   
>>   unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
>>   {
>> +	struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>>   	if (!has_symlinks && S_ISREG(mode) &&
>>   	    ce && S_ISLNK(ce->ce_mode))
>>   		return ce->ce_mode;
>> -	if (!trust_executable_bit && S_ISREG(mode)) {
>> +	if (!cfg->trust_executable_bit && S_ISREG(mode)) {
>>   		if (ce && S_ISREG(ce->ce_mode))
>>   			return ce->ce_mode;
>>   		return create_ce_mode(0666);
> 
> How hot are the code paths that call into this helper function?  In
> the original under some condition, it was possible to return without
> even consulting the trust_executable_bit variable, but in the
> updated code, the helper unconditionally makes a call to the
> repo_config_values() helper function even before it knows it needs
> to know the value of trust_executable_bit.

That sounds reasonable to me. I’ll adjust the conditional logic in some 
of the statements so that they short-circuit appropriately to avoid 
performance overhead.

> 
>> @@ -217,11 +219,13 @@ unsigned int ce_mode_from_stat(const struct cache_entry *ce, unsigned int mode)
>>   
>>   static unsigned int st_mode_from_ce(const struct cache_entry *ce)
>>   {
>> +	struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
>>   	switch (ce->ce_mode & S_IFMT) {
>>   	case S_IFLNK:
>>   		return has_symlinks ? S_IFLNK : (S_IFREG | 0644);
>>   	case S_IFREG:
>> -		return (ce->ce_mode & (trust_executable_bit ? 0755 : 0644)) | S_IFREG;
>> +		return (ce->ce_mode & (cfg->trust_executable_bit ? 0755 : 0644)) | S_IFREG;
>>   	case S_IFGITLINK:
>>   		return S_IFDIR | 0755;
>>   	case S_IFDIR:
> 
> Ditto.
> 
>> @@ -321,6 +325,7 @@ static int ce_modified_check_fs(struct index_state *istate,
>>   static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
>>   {
>>   	unsigned int changed = 0;
>> +	struct repo_config_values *cfg = repo_config_values(the_repository);
>>   
>>   	if (ce->ce_flags & CE_REMOVE)
>>   		return MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;
>> @@ -331,7 +336,7 @@ static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
>>   		/* We consider only the owner x bit to be relevant for
>>   		 * "mode changes"
>>   		 */
>> -		if (trust_executable_bit &&
>> +		if (cfg->trust_executable_bit &&
>>   		    (0100 & (ce->ce_mode ^ st->st_mode)))
>>   			changed |= MODE_CHANGED;
>>   		break;
> 
> Ditto.
> 
>> @@ -732,6 +737,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>>   			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
>>   	unsigned hash_flags = pretend ? 0 : INDEX_WRITE_OBJECT;
>>   
>> +	struct repo_config_values *cfg = repo_config_values(the_repository);
>> +
> 
> Lose the excess blank line before the new declaration.
> 
>>   	if (flags & ADD_CACHE_RENORMALIZE)
>>   		hash_flags |= INDEX_RENORMALIZE;
>>   
>> @@ -752,7 +759,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>>   		ce->ce_flags |= CE_INTENT_TO_ADD;
>>   
>>   
>> -	if (trust_executable_bit && has_symlinks) {
>> +	if (cfg->trust_executable_bit && has_symlinks) {
>>   		ce->ce_mode = create_ce_mode(st_mode);
>>   	} else {
>>   		/* If there is an existing entry, pick the mode bits and type
> 
> Almost all of these places that care about trust_executable_bit also
> cares about has_symlinks.  I wonder if they should be converted to
> repo-local settings in the same series.

That’s true: I had actually planned to start migrating has_symlinks as 
soon as this series was approved. Since you think it would be better to 
merge them into a single series, I’ll go ahead and do that ;)

Thanks, yuchen