diff mbox series

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

Message ID 20210404061754.19428-1-tboegi@web.de (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] precompose_utf8: Make precompose_string_if_needed() public | expand

Commit Message

Torsten Bögershausen April 4, 2021, 6:17 a.m. UTC 
From: Torsten Bögershausen <tboegi@web.de>

The "prefix" was precomposed for MacOs in commit 5c327502db,
MacOS: precompose_argv_prefix()

However, this commit forgot to update "startup_info->prefix" after
precomposing.
Re-arrange the code in setup.c:
Move the (possible) precomposition towards the end of
setup_git_directory_gently(), so that precompose_string_if_needed()
can use git_config_get_bool("core.precomposeunicode") correctly.

Keep prefix, startup_info->prefix and GIT_PREFIX_ENVIRONMENT all in sync.

And as a result, the prefix no longer needs to be precomposed in git.c

Reported-by: Dmitry Torilov <d.torilov@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---

 This part did never made it to the list - it should have gone only
 to tboegi@web.de
 git send-email decided to cc to the "Helped-by" and "Reported-by" addresses,
 a feature that I was not aware of - and can be turned off with --suppress-cc=all
 In other words, I typically send these emails only to my self first, re-read
 with fresh eyes, and then send them out. End of of blabla.

Changes since V1:
 Add a comment in setup.c, to make more clear that git_config_get_bool()
 is called, and the setup_XXX() must have prepared everything needed.

 git.c   |  2 +-
 setup.c | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

--
2.30.0.155.g66e871b664

Comments

Junio C Hamano April 4, 2021, 7:58 a.m. UTC | #1 
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.
diff mbox series

Patch

diff --git a/git.c b/git.c
index 9bc077a025..b53e665671 100644
--- a/git.c
+++ b/git.c
@@ -423,7 +423,7 @@  static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			int nongit_ok;
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
-		prefix = precompose_argv_prefix(argc, argv, prefix);
+		precompose_argv_prefix(argc, argv, NULL);
 		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
 		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
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;
-		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;
+	if (prefix) {
+		/* This calls git_config_get_bool() under the hood (MacOs only) */
+		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);