chore: use prefix from startup_info

PATCH In review Dmitry Torilov via GitGitGadget 3 participants 8 messages Mar 29, 2021 – Apr 4, 2021

lore

Share & tools

lore

Dmitry Torilov via GitGitGadget

author

2021-03-29 22:35 UTC #0 lore raw

PATCH 1/1 chore: use prefix from startup_info

From: Dmitry Torilov <[email protected]>

trace.h: update trace_repo_setup signature
trace.c: update trace_repo_setup implementation
git.c: update trace_repo_setup usage

Signed-off-by Dmitry Torilov <[email protected]>

---
[PATCH] trace: use prefix from startup_info

trace.h: update trace_repo_setup signature trace.c: update trace_repo_setup implementation git.c: update trace_repo_setup usage

I'm new to git, I want to try to make a test contribution.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-922%2Ftorilov%2Ftrace_repo_setup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-922/torilov/trace_repo_setup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/922

 git.c   | 3 ++-
 trace.c | 4 ++--
 trace.h | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

git.c +2 -1

index 9bc077a025cb..310cf54e08f6 100644
@@ -424,6 +424,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 		prefix = precompose_argv_prefix(argc, argv, prefix);
+		startup_info->prefix = prefix;
 		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
 		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);

@@ -432,7 +433,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
 		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
-			trace_repo_setup(prefix);
+			trace_repo_setup();
 	}
 	commit_pager_choice();

trace.c +2 -2

index f726686fd92f..4c6414683414 100644
@@ -367,9 +367,9 @@ static const char *quote_crnl(const char *path)
 	return new_path.buf;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup(void)
 {
+	const char *prefix = startup_info->prefix;
 	const char *git_work_tree;
 	char *cwd;

trace.h +1 -1

index 0dbbad0e41cb..844b3ce47d2b 100644
@@ -93,7 +93,7 @@ extern struct trace_key trace_default_key;
 extern struct trace_key trace_perf_key;
 extern struct trace_key trace_setup_key;
 
-void trace_repo_setup(const char *prefix);
+void trace_repo_setup(void);
 
 /**
  * Checks whether the trace key is enabled. Used to prevent expensive

base-commit: 84d06cdc06389ae7c462434cb7b1db0980f63860

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-03-29 23:54 UTC #1 lore raw

Dmitry, welcome to git development community.

Torsten, you are Cc'ed because I may have spotted a possible bug in your recent 5c327502 (MacOS: precompose_argv_prefix(), 2021-02-03).

Quoted 1 line · Dmitry Torilov via GitGitGadget

Subject: Re: [PATCH] chore: use prefix from startup_info

s/chore/git.c:/ perhaps. Otherwise the subject is perfect, which is rare for new contributors.

Quoted 5 lines

From: Dmitry Torilov <[email protected]>
trace.h: update trace_repo_setup signature
trace.c: update trace_repo_setup implementation
git.c: update trace_repo_setup usage

Unlike some GNU projects, we do not write summary of what the patch does to the code in the log message. What we do is to explain what problem the current codebase has, why it is a problem worth fixing, and justify why the approach the patch chose to solve the problem is a good one. See Documentation/SubmittingPatches[[describe-changes]] and "git log --no-merges origin/master" for recent examples.

Quoted 13 lines

git.c +1 -0

index 9bc077a025cb..310cf54e08f6 100644
@@ -424,6 +424,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 		prefix = precompose_argv_prefix(argc, argv, prefix);
+		startup_info->prefix = prefix;
 		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
 		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);

git.c static int run_builtin(struct cmd_struct *p, int argc, const char **argv) · line 437

Quoted 6 lines

git.c +1 -1

@@ -432,7 +433,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
 		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
-			trace_repo_setup(prefix);
+			trace_repo_setup();
 	}

This turns out to be the ONLY place that trace_repo_setup() is called, and the value of prefix here comes from the returned value from precompose_argv_prefix() we saw in the previous hunk. So as far as trace_repo_setup() is concerned, taken together with ...

trace.c static const char *quote_crnl(const char *path) · line 371

Quoted 12 lines

git.c +0 -0

 	commit_pager_choice();

trace.c +0 -2

index f726686fd92f..4c6414683414 100644
@@ -367,9 +367,9 @@ static const char *quote_crnl(const char *path)
 	return new_path.buf;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)

What is curious is that the caller in git.c this patch changed was the only caller back when this FIXME comment was written. It is unclear why the original commit a9ca8a85 (builtins: print setup info if repo is found, 2010-11-26) left it unfixed.

trace.c static const char *quote_crnl(const char *path) · line 372

Quoted 3 lines

trace.c +2 -0

+void trace_repo_setup(void)
 {
+	const char *prefix = startup_info->prefix;

... this change, the patch is correct.

What is not so clear is if the users of the startup_info->prefix may be affected by this change, and if so, does this change introduce a bug to them.

We assign to the .prefix member in either setup_git_directory() called by the other side of if/else before the precontext of the first hunk of this patch, or setup_git_directory_gently() we see in the precontext of that hunk.

But then we call precompose_argv_prefix() to munge the prefix value, and reassign it to the field. All the existing users of the startup_info->prefix member has been relying on the fact that it is the value before "precompose". With this patch, they see the value after "precompose". I would understand it better if you didn't add a new assignment to run_builtin().

Torsten, I _think_ this change actually fixes the bug (or sweeps it under the rug) in 5c327502 (MacOS: precompose_argv_prefix(), 2021-02-03), where we wanted to precompose not just the argv[] but the prefix on macOS. 5c327502 changed the prefix we pass around in the callchain as parameter correctly, but as we can see here, a copy w/o the precomposition is left in startup_info->prefix to confuse codepath that use it (instead of the third parameter given to cmd_foo(ac, av, prefix)).

Perhaps the "precompose" call should be moved from git.c to the place just before startup_info->prefix is assigned to in setup_git_directory_gently(), perhaps like the attached patch, to cover this codepath. I didn't looked at other calls to the precompopse added by 5c327502, but I suspect there might need similar adjustments.

Thanks.

 git.c   | 2 +-
 setup.c | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

git.c +1 -1

index 9bc077a025..4bd199cc84 100644
@@ -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);
+		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);

setup.c +7 -0

index c04cd25a30..2f6a1f794a 100644
@@ -1264,6 +1264,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		BUG("unhandled setup_git_directory_1() result");
 	}
 
+	/*
+	 * on macOS, prefix derived from the getcwd() may need to be
+	 * normalized into precomposed form.
+	 */
+	if (prefix)
+		prefix = precompose_string_if_needed(prefix);
+
 	/*
 	 * At this point, nongit_ok is stable. If it is non-NULL and points
 	 * to a non-zero value, then this means that we haven't found a

Torsten Bögershausen

2021-03-31 05:05 UTC #2 lore raw

Quoted 4 lines · Junio C Hamano

Dmitry, welcome to git development community.
Torsten, you are Cc'ed because I may have spotted a possible bug in your recent 5c327502 (MacOS: precompose_argv_prefix(), 2021-02-03).

Thanks Junio, I missed the patch in a moment of weakness.

Quoted 95 lines

Quoted 1 line · Dmitry Torilov via GitGitGadget
Subject: Re: [PATCH] chore: use prefix from startup_info
s/chore/git.c:/ perhaps. Otherwise the subject is perfect, which is rare for new contributors.
Quoted 5 lines
From: Dmitry Torilov <[email protected]>
trace.h: update trace_repo_setup signature
trace.c: update trace_repo_setup implementation
git.c: update trace_repo_setup usage
Unlike some GNU projects, we do not write summary of what the patch does to the code in the log message. What we do is to explain what problem the current codebase has, why it is a problem worth fixing, and justify why the approach the patch chose to solve the problem is a good one. See Documentation/SubmittingPatches[[describe-changes]] and "git log --no-merges origin/master" for recent examples.
Quoted 13 lines
git.c +1 -0
index 9bc077a025cb..310cf54e08f6 100644
@@ -424,6 +424,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 		prefix = precompose_argv_prefix(argc, argv, prefix);
+		startup_info->prefix = prefix;
 		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
 		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
git.c static int run_builtin(struct cmd_struct *p, int argc, const char **argv) · line 437
Quoted 6 lines
git.c +1 -1
@@ -432,7 +433,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
 		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
-			trace_repo_setup(prefix);
+			trace_repo_setup();
 	}
This turns out to be the ONLY place that trace_repo_setup() is called, and the value of prefix here comes from the returned value from precompose_argv_prefix() we saw in the previous hunk. So as far as trace_repo_setup() is concerned, taken together with ...
trace.c static const char *quote_crnl(const char *path) · line 371
Quoted 12 lines
git.c +0 -0
 	commit_pager_choice();
 
trace.c +0 -2
index f726686fd92f..4c6414683414 100644
@@ -367,9 +367,9 @@ static const char *quote_crnl(const char *path)
 	return new_path.buf;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
What is curious is that the caller in git.c this patch changed was the only caller back when this FIXME comment was written. It is unclear why the original commit a9ca8a85 (builtins: print setup info if repo is found, 2010-11-26) left it unfixed.
trace.c static const char *quote_crnl(const char *path) · line 372
Quoted 3 lines
trace.c +2 -0
+void trace_repo_setup(void)
 {
+	const char *prefix = startup_info->prefix;
... this change, the patch is correct.
What is not so clear is if the users of the startup_info->prefix may be affected by this change, and if so, does this change introduce a bug to them.
We assign to the .prefix member in either setup_git_directory() called by the other side of if/else before the precontext of the first hunk of this patch, or setup_git_directory_gently() we see in the precontext of that hunk.
But then we call precompose_argv_prefix() to munge the prefix value, and reassign it to the field. All the existing users of the startup_info->prefix member has been relying on the fact that it is the value before "precompose". With this patch, they see the value after "precompose". I would understand it better if you didn't add a new assignment to run_builtin().
Torsten, I _think_ this change actually fixes the bug (or sweeps it under the rug) in 5c327502 (MacOS: precompose_argv_prefix(), 2021-02-03), where we wanted to precompose not just the argv[] but the prefix on macOS. 5c327502 changed the prefix we pass around in the callchain as parameter correctly, but as we can see here, a copy w/o the precomposition is left in startup_info->prefix to confuse codepath that use it (instead of the third parameter given to cmd_foo(ac, av, prefix)).

Quoted 8 lines

Perhaps the "precompose" call should be moved from git.c to the place just before startup_info->prefix is assigned to in setup_git_directory_gently(), perhaps like the attached patch, to cover this codepath. I didn't looked at other calls to the precompopse added by 5c327502, but I suspect there might need similar adjustments.
Thanks.

Please see at the end.

Quoted 27 lines

 git.c   | 2 +-
 setup.c | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

git.c +1 -1

index 9bc077a025..4bd199cc84 100644
@@ -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);
+		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);

setup.c +0 -0

index c04cd25a30..2f6a1f794a 100644
@@ -1264,6 +1264,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		BUG("unhandled setup_git_directory_1() result");
 	}

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1276

Quoted 10 lines

setup.c +7 -0

+	/*
+	 * on macOS, prefix derived from the getcwd() may need to be
+	 * normalized into precomposed form.
+	 */
+	if (prefix)
+		prefix = precompose_string_if_needed(prefix);
+
 	/*
 	 * At this point, nongit_ok is stable. If it is non-NULL and points
 	 * to a non-zero value, then this means that we haven't found a

What I tried is to follow that suggestion.
On top of that, we need:
- precompose_string_if_needed() must be exposed public, and that is done in precompose_utf8.[ch] and git-compat-util.h below.
- setup.c learns to precompose and to set up the prefix, when needed.
- No change in git.c (yet), because the changes from below causes t3910 to fail in "git restore -p ."

If I remove the "prefix = precompose_string_if_needed(prefix);" in setup.c we see that t3910 passes again.

And here comes the complete patch - it looks nice, but doesn't work. Sorry guys for being shortish, thanks Dmitry and Junio for digging. And that's all I have for today.

compat/precompose_utf8.c +4 -5

index ec560565a86..cce1d57a464 100644
@@ -60,10 +60,12 @@ void probe_utf8_pathname_composition(void)
 	strbuf_release(&path);
 }
 
-static inline const char *precompose_string_if_needed(const char *in)
+const char *precompose_string_if_needed(const char *in)
 {
 	size_t inlen;
 	size_t outlen;
+	if (!in)
+		return NULL;
 	if (has_non_ascii(in, (size_t)-1, &inlen)) {
 		iconv_t ic_prec;
 		char *out;

@@ -96,10 +98,7 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref
 		argv[i] = precompose_string_if_needed(argv[i]);
 		i++;
 	}
-	if (prefix) {
-		prefix = precompose_string_if_needed(prefix);
-	}
-	return prefix;
+	return precompose_string_if_needed(prefix);
 }

compat/precompose_utf8.h +1 -0

index d70b84665c6..fea06cf28a5 100644
@@ -29,6 +29,7 @@ typedef struct {
 } PREC_DIR;
 
 const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix);
+const char *precompose_string_if_needed(const char *in);
 void probe_utf8_pathname_composition(void);
 
 PREC_DIR *precompose_utf8_opendir(const char *dirname);

git-compat-util.h +5 -0

index 9ddf9d7044b..a508dbe5a35 100644
@@ -256,6 +256,11 @@ static inline const char *precompose_argv_prefix(int argc, const char **argv, co
 {
 	return prefix;
 }
+static inline const char *precompose_string_if_needed(const char *in)
+{
+	return in;
+}
+
 #define probe_utf8_pathname_composition()
 #endif

setup.c +1 -0

index c04cd25a30d..5b80ccf86a6 100644
@@ -1279,6 +1279,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		startup_info->prefix = NULL;
 		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
 	} else {
+		prefix = precompose_string_if_needed(prefix);
 		startup_info->have_repository = 1;
 		startup_info->prefix = prefix;
 		if (prefix)

[email protected]

2021-04-04 06:18 UTC #3 lore raw

PATCH v2 1/2 precompose_utf8: Make precompose_string_if_needed() public

In reply to Junio C Hamano

From: Torsten Bögershausen <[email protected]>

commit 5c327502, MacOS: precompose_argv_prefix() uses the function precompose_string_if_needed() internally. It is only used from precompose_argv_prefix() and therefore static in compat/precompose_utf8.c

Expose this function, it will be used in the next commit.

While there, allow passing a NULL pointer, which will return NULL.

Signed-off-by Torsten Bögershausen <[email protected]>

---

This part 1/2 never made it to the list

 compat/precompose_utf8.c | 9 ++++-----
 compat/precompose_utf8.h | 1 +
 git-compat-util.h        | 5 +++++
 3 files changed, 10 insertions(+), 5 deletions(-)

compat/precompose_utf8.c +4 -5

index ec560565a8..cce1d57a46 100644
@@ -60,10 +60,12 @@ void probe_utf8_pathname_composition(void)
 	strbuf_release(&path);
 }
 
-static inline const char *precompose_string_if_needed(const char *in)
+const char *precompose_string_if_needed(const char *in)
 {
 	size_t inlen;
 	size_t outlen;
+	if (!in)
+		return NULL;
 	if (has_non_ascii(in, (size_t)-1, &inlen)) {
 		iconv_t ic_prec;
 		char *out;

@@ -96,10 +98,7 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref
 		argv[i] = precompose_string_if_needed(argv[i]);
 		i++;
 	}
-	if (prefix) {
-		prefix = precompose_string_if_needed(prefix);
-	}
-	return prefix;
+	return precompose_string_if_needed(prefix);
 }

compat/precompose_utf8.h +1 -0

index d70b84665c..fea06cf28a 100644
@@ -29,6 +29,7 @@ typedef struct {
 } PREC_DIR;
 
 const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix);
+const char *precompose_string_if_needed(const char *in);
 void probe_utf8_pathname_composition(void);
 
 PREC_DIR *precompose_utf8_opendir(const char *dirname);

git-compat-util.h +5 -0

index 9ddf9d7044..a508dbe5a3 100644
@@ -256,6 +256,11 @@ static inline const char *precompose_argv_prefix(int argc, const char **argv, co
 {
 	return prefix;
 }
+static inline const char *precompose_string_if_needed(const char *in)
+{
+	return in;
+}
+
 #define probe_utf8_pathname_composition()
 #endif

--
2.30.0.155.g66e871b664

[email protected]

2021-04-04 06:18 UTC #4 lore raw

PATCH v2 2/2 MacOs: Precompose startup_info->prefix

In reply to Junio C Hamano

From: Torsten Bögershausen <[email protected]>

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 <[email protected]>

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Torsten Bögershausen <[email protected]>

---

This part did never made it to the list - it should have gone only to [email protected]
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(-)

git.c +1 -1

index 9bc077a025..b53e665671 100644
@@ -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);

setup.c +10 -4

index c04cd25a30..dcc9c41a85 100644
@@ -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);

--
2.30.0.155.g66e871b664

Junio C Hamano

2021-04-04 07:58 UTC #5 lore raw

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

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1283

Quoted 8 lines · [email protected] writes:

setup.c +0 -0

index c04cd25a30..dcc9c41a85 100644
@@ -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 ...

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1311

Quoted 13 lines

setup.c +2 -4

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

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1313

Quoted 2 lines

setup.c +2 -0

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

Quoted 11 lines

setup.c +6 -1

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

[email protected]

2021-04-04 17:14 UTC #6 lore raw

PATCH v3 1/2 precompose_utf8: Make precompose_string_if_needed() public

In reply to Junio C Hamano

From: Torsten Bögershausen <[email protected]>

Expose this function, it will be used in the next commit.

While there, allow passing a NULL pointer, which will return NULL.

Signed-off-by Torsten Bögershausen <[email protected]>

---
 compat/precompose_utf8.c | 9 ++++-----
 compat/precompose_utf8.h | 1 +
 git-compat-util.h        | 5 +++++
 3 files changed, 10 insertions(+), 5 deletions(-)

compat/precompose_utf8.c +4 -5

index ec560565a8..cce1d57a46 100644
@@ -60,10 +60,12 @@ void probe_utf8_pathname_composition(void)
 	strbuf_release(&path);
 }
 
-static inline const char *precompose_string_if_needed(const char *in)
+const char *precompose_string_if_needed(const char *in)
 {
 	size_t inlen;
 	size_t outlen;
+	if (!in)
+		return NULL;
 	if (has_non_ascii(in, (size_t)-1, &inlen)) {
 		iconv_t ic_prec;
 		char *out;

@@ -96,10 +98,7 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref
 		argv[i] = precompose_string_if_needed(argv[i]);
 		i++;
 	}
-	if (prefix) {
-		prefix = precompose_string_if_needed(prefix);
-	}
-	return prefix;
+	return precompose_string_if_needed(prefix);
 }

compat/precompose_utf8.h +1 -0

index d70b84665c..fea06cf28a 100644
@@ -29,6 +29,7 @@ typedef struct {
 } PREC_DIR;
 
 const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix);
+const char *precompose_string_if_needed(const char *in);
 void probe_utf8_pathname_composition(void);
 
 PREC_DIR *precompose_utf8_opendir(const char *dirname);

git-compat-util.h +5 -0

index 9ddf9d7044..a508dbe5a3 100644
@@ -256,6 +256,11 @@ static inline const char *precompose_argv_prefix(int argc, const char **argv, co
 {
 	return prefix;
 }
+static inline const char *precompose_string_if_needed(const char *in)
+{
+	return in;
+}
+
 #define probe_utf8_pathname_composition()
 #endif

--
2.30.0.155.g66e871b664

[email protected]

2021-04-04 17:14 UTC #7 lore raw

PATCH v3 2/2 MacOs: Precompose startup_info->prefix

In reply to Junio C Hamano

From: Torsten Bögershausen <[email protected]>

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

However, this commit forgot to update "startup_info->prefix" after precomposing.

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 <[email protected]>

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Torsten Bögershausen <[email protected]>

---

Changes since V2:
- Re-arranged code in setup.c to be more consistent
- Added the longer comment block, as suggested by Junio, thanks for that.

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

git.c +1 -1

index 9bc077a025..b53e665671 100644
@@ -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);

setup.c +18 ~~-10~~

index c04cd25a30..59e2facd9d 100644
@@ -1274,18 +1274,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 * the GIT_PREFIX environment variable must always match. For details
 	 * see Documentation/config/alias.txt.
 	 */
-	if (nongit_ok && *nongit_ok) {
+	if (nongit_ok && *nongit_ok)
 		startup_info->have_repository = 0;
-		startup_info->prefix = NULL;
-		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
-	} else {
+	else
 		startup_info->have_repository = 1;
-		startup_info->prefix = prefix;
-		if (prefix)
-			setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
-		else
-			setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
-	}
 
 	/*
 	 * Not all paths through the setup code will call 'set_git_dir()' (which

@@ -1311,6 +1303,22 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		if (startup_info->have_repository)
 			repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
 	}
+	/*
+	 * 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().
+	 */
+	if (prefix) {
+		prefix = precompose_string_if_needed(prefix);
+		startup_info->prefix = prefix;
+		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
+	} else {
+		startup_info->prefix = NULL;
+		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
+	}
+
 
 	strbuf_release(&dir);
 	strbuf_release(&gitdir);

--
2.30.0.155.g66e871b664