[PATCH] chore: use prefix from startup_info

[PATCH] chore: use prefix from startup_info

[Dmitry Torilov via GitGitGadget]

From: Dmitry Torilov <d.torilov@gmail.com>

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 <d.torilov@gmail.com>
---
    [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(-)

diff --git a/git.c b/git.c
index 9bc077a025cb..310cf54e08f6 100644
--- a/git.c
+++ b/git.c
@@ -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();
 
diff --git a/trace.c b/trace.c
index f726686fd92f..4c6414683414 100644
--- a/trace.c
+++ b/trace.c
@@ -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;
 
diff --git a/trace.h b/trace.h
index 0dbbad0e41cb..844b3ce47d2b 100644
--- a/trace.h
+++ b/trace.h
@@ -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
-- 
gitgitgadget

Re: [PATCH] chore: use prefix from startup_info

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

"Dmitry Torilov via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

> From: Dmitry Torilov <d.torilov@gmail.com>
>
> 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.

> diff --git a/git.c b/git.c
> index 9bc077a025cb..310cf54e08f6 100644
> --- a/git.c
> +++ b/git.c
> @@ -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();
>  	}

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

>  	commit_pager_choice();
>  
> diff --git a/trace.c b/trace.c
> index f726686fd92f..4c6414683414 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -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.

> +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(-)

diff --git c/git.c w/git.c
index 9bc077a025..4bd199cc84 100644
--- c/git.c
+++ w/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);
+		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 c/setup.c w/setup.c
index c04cd25a30..2f6a1f794a 100644
--- c/setup.c
+++ w/setup.c
@@ -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

Re: [PATCH] chore: use prefix from startup_info

[Torsten Bögershausen]

On Mon, Mar 29, 2021 at 04:53:54PM -0700, Junio C Hamano wrote:
> 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.

>
> "Dmitry Torilov via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> > From: Dmitry Torilov <d.torilov@gmail.com>
> >
> > 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.
>
> > diff --git a/git.c b/git.c
> > index 9bc077a025cb..310cf54e08f6 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -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();
> >  	}
>
> 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 ...
>
> >  	commit_pager_choice();
> >
> > diff --git a/trace.c b/trace.c
> > index f726686fd92f..4c6414683414 100644
> > --- a/trace.c
> > +++ b/trace.c
> > @@ -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.
>
> > +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.

Please see at the end.

>
>  git.c   | 2 +-
>  setup.c | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git c/git.c w/git.c
> index 9bc077a025..4bd199cc84 100644
> --- c/git.c
> +++ w/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);
> +		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 c/setup.c w/setup.c
> index c04cd25a30..2f6a1f794a 100644
> --- c/setup.c
> +++ w/setup.c
> @@ -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
>

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.

diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index ec560565a86..cce1d57a464 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -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);
 }


diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
index d70b84665c6..fea06cf28a5 100644
--- a/compat/precompose_utf8.h
+++ b/compat/precompose_utf8.h
@@ -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);
diff --git a/git-compat-util.h b/git-compat-util.h
index 9ddf9d7044b..a508dbe5a35 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -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

diff --git a/setup.c b/setup.c
index c04cd25a30d..5b80ccf86a6 100644
--- a/setup.c
+++ b/setup.c
@@ -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)

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

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

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 <tboegi@web.de>
---

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(-)

diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index ec560565a8..cce1d57a46 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -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);
 }


diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
index d70b84665c..fea06cf28a 100644
--- a/compat/precompose_utf8.h
+++ b/compat/precompose_utf8.h
@@ -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);
diff --git a/git-compat-util.h b/git-compat-util.h
index 9ddf9d7044..a508dbe5a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -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

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

[tboegi]

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(-)

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);
--
2.30.0.155.g66e871b664

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

[Junio C Hamano]

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.

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

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

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 <tboegi@web.de>
---
 compat/precompose_utf8.c | 9 ++++-----
 compat/precompose_utf8.h | 1 +
 git-compat-util.h        | 5 +++++
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index ec560565a8..cce1d57a46 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -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);
 }


diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
index d70b84665c..fea06cf28a 100644
--- a/compat/precompose_utf8.h
+++ b/compat/precompose_utf8.h
@@ -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);
diff --git a/git-compat-util.h b/git-compat-util.h
index 9ddf9d7044..a508dbe5a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -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

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

[tboegi]

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.

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

 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(-)

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..59e2facd9d 100644
--- a/setup.c
+++ b/setup.c
@@ -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