Re: [PATCH v1 3/4] environment: move 'trust_executable_bit' into repo_config_values

[Junio C Hamano <gitster@pobox.com>]

Tian Yuchen <cat@malon.dev> writes:

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

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

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

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

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