Re: [PATCH v7 2/4] read-cache: pass 'repo' to 'ce_mode_from_stat()'

[Tian Yuchen <cat@malon.dev>]

On 7/19/26 03:02, SZEDER Gábor wrote:
> On Fri, Jul 17, 2026 at 02:35:57PM +0800, Tian Yuchen wrote:
>> The ce_mode_from_stat() function is a performance-critical static
>> inline helper in 'read-cache.h'. As we migrate configuration
>> variables into the repository struct, this helper needs access
>> to the repository context.
>>
>> Update the signature of ce_mode_from_stat() to take a 'struct
>> repository *' parameter, and update all callers to pass the
>> appropriate repository instance.
>>
>> To prepare for the overhead of replacing cheap global variable
>> accesses with getter functions, the boolean expressions are
>> reordered to evaluate 'S_ISREG(mode)' first.
>>
>> While at it, add a comment for ce_mode_from_stat().
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>> ---
> 
>> diff --git a/read-cache.h b/read-cache.h
>> index 043da1f1aa..94b8d3e547 100644
>> --- a/read-cache.h
>> +++ b/read-cache.h
>> @@ -4,15 +4,24 @@
>>   #include "read-cache-ll.h"
>>   #include "object.h"
>>   #include "pathspec.h"
>> +#include "environment.h"
>>   
>> -static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
>> +/*
>> + * Determine the appropriate index mode for a file based on its stat()
>> + * information and the existing cache entry (if any).
>> + *
>> + * This function handles degradation for filesystems that lack
>> + * symlink support or reliable executable bits.
>> + */
>> +static inline unsigned int ce_mode_from_stat(struct repository *repo,
> 
> This new parameter is not yet used in this function, which causes
> compilation errors in all source files which include "read-cache.h"
> when trying to build this commit using DEVELOPER=1, e.g.:
> 
>        CC pathspec.o
>    In file included from pathspec.c:11:
>    read-cache.h: In function ‘ce_mode_from_stat’:
>    read-cache.h:16:65: error: unused parameter ‘repo’ [-Werror=unused-parameter]
>       16 | static inline unsigned int ce_mode_from_stat(struct repository *repo,
>          |                                              ~~~~~~~~~~~~~~~~~~~^~~~
>    cc1: all warnings being treated as errors
>    make: *** [Makefile:2921: pathspec.o] Error 1
>        CC preload-index.o
>    In file included from preload-index.c:16:
>    read-cache.h: In function ‘ce_mode_from_stat’:
>    read-cache.h:16:65: error: unused parameter ‘repo’ [-Werror=unused-parameter]
>       16 | static inline unsigned int ce_mode_from_stat(struct repository *repo,
>          |                                              ~~~~~~~~~~~~~~~~~~~^~~~
>    cc1: all warnings being treated as errors
>    make: *** [Makefile:2921: preload-index.o] Error 1
>        CC read-cache.o
>    In file included from read-cache.c:34:
>    read-cache.h: In function ‘ce_mode_from_stat’:
>    read-cache.h:16:65: error: unused parameter ‘repo’ [-Werror=unused-parameter]
>       16 | static inline unsigned int ce_mode_from_stat(struct repository *repo,
>          |                                              ~~~~~~~~~~~~~~~~~~~^~~~
>    cc1: all warnings being treated as errors
>    make: *** [Makefile:2921: read-cache.o] Error 1
> 

Nice catch.

> I think the new parameter should be marked as UNUSED in this patch,
> and then the UNUSED should be dropped in the next, where you start
> using the parameter.
> 
>> +					     const struct cache_entry *ce,
>>   					     unsigned int mode)
>>   {
>>   	extern int trust_executable_bit, has_symlinks;
>> -	if (!has_symlinks && S_ISREG(mode) &&
>> +	if (S_ISREG(mode) && !has_symlinks &&
>>   	    ce && S_ISLNK(ce->ce_mode))
>>   		return ce->ce_mode;
>> -	if (!trust_executable_bit && S_ISREG(mode)) {
>> +	if (S_ISREG(mode) && !trust_executable_bit) {
>>   		if (ce && S_ISREG(ce->ce_mode))
>>   			return ce->ce_mode;
>>   		return create_ce_mode(0666);
>> -- 
>> 2.43.0
>>

But 'USUSED' cannot be used here since the corresponding header 
(git-compat-util.h, or more specifically compat/posix.h) is not included.

Can we write..

	(void)repo; /* TODO: use this parameter in the next patch */

..to keep it simple?


Regards, yuchen