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

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

tboegi@web.de writes:

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

> -		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().
		 */

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