Re: [PATCH] Refactor 'trust_executable_bit' to repository-scoped setting

[Tian Yuchen <a3205153416@gmail.com>]

Hi drona,

On 3/2/26 02:59, drona wrote:

> This patch moves 'trust_executable_bit' into 'struct repo_settings', making
> it a repository-scoped configuration. All references in files have been updated to use
> 'the_repository->settings.trust_executable_bit'.

The original intent of this patch should be sound.

> Why this is a good candidate:
> - It's a self-contained global variable that only affects file mode logic.
> - Low risk: changes only impact mode calculations and related apply/update
>    operations.
> - Makes Git codebase more maintainable and prepares for future multi-repo
>    support.
> 
> - Manual sanity check with a test repo confirms executable bits behave correctly.
> 
> Signed-off-by: Dorna Raj Gyawali <dronarajgyawali@gmail.com>
> ---
>   apply.c                |  4 ++--
>   builtin/update-index.c |  2 +-
>   diff-lib.c             | 10 +++++-----
>   environment.c          |  3 +--
>   environment.h          |  1 -
>   read-cache.c           | 10 +++++-----
>   read-cache.h           | 11 +++++++----
>   repo-settings.h        |  6 +++++-
>   8 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/apply.c b/apply.c
> index d044c95d50..2bcb22a4bc 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3838,8 +3838,8 @@ static int check_preimage(struct apply_state *state,
>   		if (*ce && !(*ce)->ce_mode)
>   			BUG("ce_mode == 0 for path '%s'", old_name);
>   
> -		if (trust_executable_bit || !S_ISREG(st->st_mode))
> -			st_mode = ce_mode_from_stat(*ce, st->st_mode);
> +		if (the_repository->settings.trust_executable_bit  || !S_ISREG(st->st_mode))
> +			st_mode = ce_mode_from_stat(the_repository, *ce, st->st_mode);
>   		else if (*ce)
>   			st_mode = (*ce)->ce_mode;
>   		else

You can run git diff --check before committing. There's an extra space 
here.

> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 8a5907767b..7917bd286f 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -293,7 +293,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len
>   	ce->ce_flags = create_ce_flags(0);
>   	ce->ce_namelen = len;
>   	fill_stat_cache_info(the_repository->index, ce, st);
> -	ce->ce_mode = ce_mode_from_stat(old, st->st_mode);
> +	ce->ce_mode = ce_mode_from_stat(the_repository, old, st->st_mode);
>   
>   	if (index_path(the_repository->index, &ce->oid, path, st,
>   		       info_only ? 0 : INDEX_WRITE_OBJECT)) {
> diff --git a/diff-lib.c b/diff-lib.c
> index ae91027a02..894358c8b0 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -160,7 +160,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
>   
>   			changed = check_removed(ce, &st);
>   			if (!changed)
> -				wt_mode = ce_mode_from_stat(ce, st.st_mode);
> +				wt_mode = ce_mode_from_stat(the_repository, ce, st.st_mode);
>   			else {
>   				if (changed < 0) {
>   					perror(ce->name);
> @@ -193,7 +193,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
>   					num_compare_stages++;
>   					oidcpy(&dpath->parent[stage - 2].oid,
>   					       &nce->oid);
> -					dpath->parent[stage-2].mode = ce_mode_from_stat(nce, mode);
> +					dpath->parent[stage-2].mode = ce_mode_from_stat(the_repository,nce, mode);
>   					dpath->parent[stage-2].status =
>   						DIFF_STATUS_MODIFIED;
>   				}
> @@ -262,7 +262,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
>   				continue;
>   			} else if (revs->diffopt.ita_invisible_in_index &&
>   				   ce_intent_to_add(ce)) {
> -				newmode = ce_mode_from_stat(ce, st.st_mode);
> +				newmode = ce_mode_from_stat(the_repository, ce, st.st_mode);
>   				diff_addremove(&revs->diffopt, '+', newmode,
>   					       null_oid(the_hash_algo), 0, ce->name, 0);
>   				continue;
> @@ -270,7 +270,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
>   
>   			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
>   							    ce_option, &dirty_submodule);
> -			newmode = ce_mode_from_stat(ce, st.st_mode);
> +			newmode = ce_mode_from_stat(the_repository, ce, st.st_mode);
>   		}
>   
>   		if (!changed && !dirty_submodule) {
> @@ -338,7 +338,7 @@ static int get_stat_data(const struct cache_entry *ce,
>   		changed = match_stat_with_submodule(diffopt, ce, &st,
>   						    0, dirty_submodule);
>   		if (changed) {
> -			mode = ce_mode_from_stat(ce, st.st_mode);
> +			mode = ce_mode_from_stat(the_repository, ce, st.st_mode);
>   			oid = null_oid(the_hash_algo);
>   		}
>   	}
> diff --git a/environment.c b/environment.c
> index 0026eb2274..861ef084dc 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;

But the_repository itself is a global variable, isn't it?

If you understand what “removing global variables” means, you should 
grasp the direction we're heading:

	passing global variables -> passing context

For example, in

 > @@ -160,7 +160,7 @@ void run_diff_files(struct rev_info *revs, 
unsigned int option)
 >
 >   			changed = check_removed(ce, &st);
 >   			if (!changed)
 > -				wt_mode = ce_mode_from_stat(ce, st.st_mode);
 > +				wt_mode = ce_mode_from_stat(the_repository, ce, st.st_mode);
				perror(ce->name);

A more appropriate of passing value could be something like:

wt_mode = ce_mode_from_stat(revs->repo, ce, st.st_mode)

> @@ -306,7 +305,7 @@ int git_default_core_config(const char *var, const char *value,
>   {
>   	/* This needs a better name */
>   	if (!strcmp(var, "core.filemode")) {
> -		trust_executable_bit = git_config_bool(var, value);
> +		the_repository->settings.trust_executable_bit = git_config_bool(var, value);
>   		return 0;
>   	}
>   	if (!strcmp(var, "core.trustctime")) {

I didn't think it through, but my gut tells me there might be some weird 
variable overwriting issues here.

But more importantly, I feel that the changes here somewhat undermine 
lazy loading behavior. Variables in repo_settings are lazy-loaded, 
meaning you cannot read their values via actions like 
repo->settings.trust_executable_bit because they are not loaded until 
needed. The function prepare_repo_settings(repo) should be called to 
load the values.

However, the change here appears to manually assign a value during the 
global config parsing phase. I believe this is incorrect.

For more information you can look through 
https://lore.kernel.org/all/48821a3848bef25c13038be8377ad73e7c17a924.1771258573.git.belkid98@gmail.com/

Regards,

Yuchen