Re: [PATCH v7 3/4] environment: move trust_executable_bit into repo_config_values

On 7/18/26 00:01, Junio C Hamano wrote:
> Tian Yuchen <[email protected]> writes:
> 
>> diff --git a/environment.c b/environment.c
>> index fc3ed8bb1c..75069a884d 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -41,7 +41,6 @@
>>   static int pack_compression_seen;
>>   static int zlib_compression_seen;
>>   
>> -int trust_executable_bit = 1;
>>   int trust_ctime = 1;
>>   int check_stat = 1;
>>   int has_symlinks = 1;
>> @@ -142,6 +141,13 @@ int is_bare_repository(void)
>>   	return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
>>   }
>>   
>> +int repo_trust_executable_bit(struct repository *repo)
>> +{
>> +	return repo->gitdir?
>> +		repo_config_values(repo)->trust_executable_bit :
>> +		1;
>> +}
>> +
>>   int have_git_dir(void)
>>   {
>>   	return startup_info->have_repository
> 
> Two comments.
> 
>   * Missing SP before '?'.  It may be easier to read if it is written
>     like this:
> 
> 	return repo->gitdir
> 	       ? repo_config_values(repo)->trust_executable_bit
> 	       : 1;
> 
>     which more clearly highlights the ternary structure.  If you tilt
>     your head 90 degrees to the left, you can almost see the parse
>     tree of the expression.
>    

Okay.

>   * Does it make sense to protect against a NULL 'repo' case, as
>     repo_protect_ntfs() and repo_protect_hfs() helpers do?  Or is it
>     better to crash loudly with a segfault to let the developer know
>     they have a bug to fix?  I lean toward the latter myself, and if
>     we go that route, we should probably stop using 'repo &&
>     repo->gitdir' elsewhere, rather than sweeping the problem under
>     the rug with defensive checks.

Sounds sensible. I will drop the checks and adjust the calls themselves 
instead.


Note that repo_protect_ntfs() looks like this:

  int repo_protect_ntfs(struct repository *repo)
  {
	return (repo && repo->initialized) ?
  		repo_config_values(repo)->protect_ntfs :
  		PROTECT_NTFS_DEFAULT;
  }

Do we need another fixup commit for it?

Regards, yuchen