From: Tian Yuchen Date: Fri, 06 Mar 2026 18:02:18 GMT Subject: Re: [PATCH] Refactor 'trust_executable_bit' to repository-scoped setting Message-ID: <24f40e5a-a5fd-49ec-86e7-921b44e4abd9@gmail.com> In-Reply-To: <20260301190017.53539-1-dronarajgyawali@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 > --- > 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