Re: [PATCH v7 2/4] read-cache: pass 'repo' to 'ce_mode_from_stat()'
- From
Tian Yuchen <cat@malon.dev>
- Date
- Jul 20, 2026, 09:13 UTC
- Message-ID
- <8b9528b8-198b-489f-8f0e-fbd0c7d07b64@malon.dev>
- In-Reply-To
- <alvNq8rXF/jofqUc@szeder.dev>
On 7/19/26 03:02, SZEDER Gábor wrote:
Show 71 quoted lines
> 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.
Show 20 quoted lines
> 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