Hi Junio,

Thanks for the feedback!

On 5/31/26 07:17, Junio C Hamano wrote:

Tian Yuchen [off-list ref] writes:

quoted

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.

quoted

@@ -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.

quoted

@@ -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.

quoted

@@ -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.

quoted

  	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