Re: [PATCH v2 2/2] MacOs: Precompose startup_info->prefix

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 4, 2021, 07:58 UTC
Message-ID: <xmqqlf9y7a7a.fsf@gitster.g>
In-Reply-To: <20210404061754.19428-1-tboegi@web.de>

tboegi@web.de writes:

Show 8 quoted lines

> diff --git a/setup.c b/setup.c
> index c04cd25a30..dcc9c41a85 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1281,10 +1281,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  	} else {
>  		startup_info->have_repository = 1;
>  		startup_info->prefix = prefix;

Is this assignment sensible? As we'd defer precomposition (or not) after we run the repository discovery, would it break if we do not have this line here (i.e. leaving startup_info->prefix NULL), and ...

Show 13 quoted lines

> -		if (prefix)
> -			setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> -		else
> -			setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
>  	}
>
>  	/*
> @@ -1311,6 +1307,16 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		if (startup_info->have_repository)
>  			repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
>  	}
> +	/* Keep prefix, startup_info->prefix and GIT_PREFIX_ENVIRONMENT in sync */
> +	prefix = startup_info->prefix;

... not wipe prefix with this assignment, i.e. we learned prefix before the previous hunk, and we would tweak it here?

> +	if (prefix) {
> +		/* This calls git_config_get_bool() under the hood (MacOs only) */

It may be more friendly to ourselves in the future if we are a bit more explicit in what we want to convey with the comment, though. Here is my attempt.

		/*
		 * Since precompose_string_if_needed() needs to look at
		 * the core.precomposeunicode configuration, this
		 * has to happen after the above block that finds
		 * out where the repository is, i.e. a preparation
                 * for calling git_config_get_bool().
		 */

Show 11 quoted lines

> +		prefix = precompose_string_if_needed(prefix);
> +		startup_info->prefix = prefix;
> +		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> +	} else {
> +		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
> +	}
>
>  	strbuf_release(&dir);
>  	strbuf_release(&gitdir);
> --
> 2.30.0.155.g66e871b664

Other than that, both patches look sensible.

By the way, isn't the canonical way to spell the name of the particular operating system that needs this patch "macOS"?

cf. https://support.apple.com/macos

Thanks.