Re: [PATCH v2 2/2] MacOs: Precompose startup_info->prefix
From: Junio C Hamano <hidden>
Date: 2021-04-04 07:58:28
tboegi@web.de writes:
quoted hunk
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 ...
quoted hunk
- 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.g66e871b664Other 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.