{"thread":{"id":"55410","subject":"[PATCH] chore: use prefix from startup_info","startedAt":"2021-03-29T22:35:20Z","lastAt":"2021-04-04T17:14:27Z","messageCount":8,"participants":["Dmitry Torilov via GitGitGadget","Junio C Hamano","Torsten Bögershausen","tboegi@web.de"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"420530","messageId":"pull.922.git.1617057233885.gitgitgadget@gmail.com","threadId":"55410","inReplyTo":null,"subject":"[PATCH] chore: use prefix from startup_info","fromName":"Dmitry Torilov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-29T22:33:53Z","receivedAt":"2021-03-29T22:35:20Z","isPatch":true,"sender":{"key":"name:Dmitry Torilov","avatar":null},"body":"From: Dmitry Torilov <d.torilov@gmail.com>\n\ntrace.h: update trace_repo_setup signature\ntrace.c: update trace_repo_setup implementation\ngit.c: update trace_repo_setup usage\n\nSigned-off-by: Dmitry Torilov <d.torilov@gmail.com>\n---\n    [PATCH] trace: use prefix from startup_info\n    \n    trace.h: update trace_repo_setup signature trace.c: update\n    trace_repo_setup implementation git.c: update trace_repo_setup usage\n    \n    I'm new to git, I want to try to make a test contribution.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-922%2Ftorilov%2Ftrace_repo_setup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-922/torilov/trace_repo_setup-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/922\n\n git.c   | 3 ++-\n trace.c | 4 ++--\n trace.h | 2 +-\n 3 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 9bc077a025cb..310cf54e08f6 100644\n--- a/git.c\n+++ b/git.c\n@@ -424,6 +424,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n \t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n+\t\tstartup_info->prefix = prefix;\n \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n \t\t    !(p->option & DELAY_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\n@@ -432,7 +433,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \n \t\tif ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&\n \t\t    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */\n-\t\t\ttrace_repo_setup(prefix);\n+\t\t\ttrace_repo_setup();\n \t}\n \tcommit_pager_choice();\n \ndiff --git a/trace.c b/trace.c\nindex f726686fd92f..4c6414683414 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -367,9 +367,9 @@ static const char *quote_crnl(const char *path)\n \treturn new_path.buf;\n }\n \n-/* FIXME: move prefix to startup_info struct and get rid of this arg */\n-void trace_repo_setup(const char *prefix)\n+void trace_repo_setup(void)\n {\n+\tconst char *prefix = startup_info->prefix;\n \tconst char *git_work_tree;\n \tchar *cwd;\n \ndiff --git a/trace.h b/trace.h\nindex 0dbbad0e41cb..844b3ce47d2b 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -93,7 +93,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n extern struct trace_key trace_setup_key;\n \n-void trace_repo_setup(const char *prefix);\n+void trace_repo_setup(void);\n \n /**\n  * Checks whether the trace key is enabled. Used to prevent expensive\n\nbase-commit: 84d06cdc06389ae7c462434cb7b1db0980f63860\n-- \ngitgitgadget\n"},{"id":"420541","messageId":"xmqqtuotfre5.fsf@gitster.g","threadId":"55410","inReplyTo":"pull.922.git.1617057233885.gitgitgadget@gmail.com","subject":"Re: [PATCH] chore: use prefix from startup_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-29T23:53:54Z","receivedAt":"2021-03-29T23:54:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry, welcome to git development community.\n\nTorsten, you are Cc'ed because I may have spotted a possible bug in\nyour recent 5c327502 (MacOS: precompose_argv_prefix(), 2021-02-03).\n\n\"Dmitry Torilov via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Subject: Re: [PATCH] chore: use prefix from startup_info\n\ns/chore/git.c:/ perhaps.  Otherwise the subject is perfect, which is\nrare for new contributors.\n\n> From: Dmitry Torilov <d.torilov@gmail.com>\n>\n> trace.h: update trace_repo_setup signature\n> trace.c: update trace_repo_setup implementation\n> git.c: update trace_repo_setup usage\n\nUnlike some GNU projects, we do not write summary of what the patch\ndoes to the code in the log message.  What we do is to explain what\nproblem the current codebase has, why it is a problem worth fixing,\nand justify why the approach the patch chose to solve the problem\nis a good one.  See Documentation/SubmittingPatches[[describe-changes]]\nand \"git log --no-merges origin/master\" for recent examples.\n\n> diff --git a/git.c b/git.c\n> index 9bc077a025cb..310cf54e08f6 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -424,6 +424,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>  \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n>  \t\t}\n>  \t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n> +\t\tstartup_info->prefix = prefix;\n>  \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n>  \t\t    !(p->option & DELAY_PAGER_CONFIG))\n>  \t\t\tuse_pager = check_pager_config(p->cmd);\n> @@ -432,7 +433,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>  \n>  \t\tif ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&\n>  \t\t    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */\n> -\t\t\ttrace_repo_setup(prefix);\n> +\t\t\ttrace_repo_setup();\n>  \t}\n\nThis turns out to be the ONLY place that trace_repo_setup() is\ncalled, and the value of prefix here comes from the returned value\nfrom precompose_argv_prefix() we saw in the previous hunk.  So as\nfar as trace_repo_setup() is concerned, taken together with ...\n\n>  \tcommit_pager_choice();\n>  \n> diff --git a/trace.c b/trace.c\n> index f726686fd92f..4c6414683414 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -367,9 +367,9 @@ static const char *quote_crnl(const char *path)\n>  \treturn new_path.buf;\n>  }\n>  \n> -/* FIXME: move prefix to startup_info struct and get rid of this arg */\n> -void trace_repo_setup(const char *prefix)\n\nWhat is curious is that the caller in git.c this patch changed was\nthe only caller back when this FIXME comment was written.  It is\nunclear why the original commit a9ca8a85 (builtins: print setup info\nif repo is found, 2010-11-26) left it unfixed.\n\n> +void trace_repo_setup(void)\n>  {\n> +\tconst char *prefix = startup_info->prefix;\n\n... this change, the patch is correct.\n\nWhat is not so clear is if the users of the startup_info->prefix may\nbe affected by this change, and if so, does this change introduce a\nbug to them.\n\nWe assign to the .prefix member in either setup_git_directory()\ncalled by the other side of if/else before the precontext of the\nfirst hunk of this patch, or setup_git_directory_gently() we see in\nthe precontext of that hunk.  \n\nBut then we call precompose_argv_prefix() to munge the prefix value,\nand reassign it to the field.  All the existing users of the\nstartup_info->prefix member has been relying on the fact that it is\nthe value before \"precompose\".  With this patch, they see the value\nafter \"precompose\".  I would understand it better if you didn't add\na new assignment to run_builtin().\n\nTorsten, I _think_ this change actually fixes the bug (or sweeps it\nunder the rug) in 5c327502 (MacOS: precompose_argv_prefix(),\n2021-02-03), where we wanted to precompose not just the argv[] but\nthe prefix on macOS.  5c327502 changed the prefix we pass around in\nthe callchain as parameter correctly, but as we can see here, a copy\nw/o the precomposition is left in startup_info->prefix to confuse\ncodepath that use it (instead of the third parameter given to\ncmd_foo(ac, av, prefix)).\n\nPerhaps the \"precompose\" call should be moved from git.c to the\nplace just before startup_info->prefix is assigned to in\nsetup_git_directory_gently(), perhaps like the attached patch,\nto cover this codepath.  I didn't looked at other calls to the\nprecompopse added by 5c327502, but I suspect there might need\nsimilar adjustments.\n\nThanks.\n\n\n git.c   | 2 +-\n setup.c | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git c/git.c w/git.c\nindex 9bc077a025..4bd199cc84 100644\n--- c/git.c\n+++ w/git.c\n@@ -423,7 +423,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tint nongit_ok;\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n-\t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n+\t\tprefix = precompose_argv_prefix(argc, argv, NULL);\n \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n \t\t    !(p->option & DELAY_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\ndiff --git c/setup.c w/setup.c\nindex c04cd25a30..2f6a1f794a 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -1264,6 +1264,13 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tBUG(\"unhandled setup_git_directory_1() result\");\n \t}\n \n+\t/*\n+\t * on macOS, prefix derived from the getcwd() may need to be\n+\t * normalized into precomposed form.\n+\t */\n+\tif (prefix)\n+\t\tprefix = precompose_string_if_needed(prefix);\n+\n \t/*\n \t * At this point, nongit_ok is stable. If it is non-NULL and points\n \t * to a non-zero value, then this means that we haven't found a\n\n"},{"id":"420686","messageId":"20210331050453.6wigph5flcmmyr3l@tb-raspi4","threadId":"55410","inReplyTo":"xmqqtuotfre5.fsf@gitster.g","subject":"Re: [PATCH] chore: use prefix from startup_info","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-03-31T05:04:53Z","receivedAt":"2021-03-31T05:05:57Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Mar 29, 2021 at 04:53:54PM -0700, Junio C Hamano wrote:\n> Dmitry, welcome to git development community.\n>\n> Torsten, you are Cc'ed because I may have spotted a possible bug in\n> your recent 5c327502 (MacOS: precompose_argv_prefix(), 2021-02-03).\n\nThanks Junio, I missed the patch in a moment of weakness.\n\n>\n> \"Dmitry Torilov via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > Subject: Re: [PATCH] chore: use prefix from startup_info\n>\n> s/chore/git.c:/ perhaps.  Otherwise the subject is perfect, which is\n> rare for new contributors.\n>\n> > From: Dmitry Torilov <d.torilov@gmail.com>\n> >\n> > trace.h: update trace_repo_setup signature\n> > trace.c: update trace_repo_setup implementation\n> > git.c: update trace_repo_setup usage\n>\n> Unlike some GNU projects, we do not write summary of what the patch\n> does to the code in the log message.  What we do is to explain what\n> problem the current codebase has, why it is a problem worth fixing,\n> and justify why the approach the patch chose to solve the problem\n> is a good one.  See Documentation/SubmittingPatches[[describe-changes]]\n> and \"git log --no-merges origin/master\" for recent examples.\n>\n> > diff --git a/git.c b/git.c\n> > index 9bc077a025cb..310cf54e08f6 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -424,6 +424,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n> >  \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n> >  \t\t}\n> >  \t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n> > +\t\tstartup_info->prefix = prefix;\n> >  \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n> >  \t\t    !(p->option & DELAY_PAGER_CONFIG))\n> >  \t\t\tuse_pager = check_pager_config(p->cmd);\n> > @@ -432,7 +433,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n> >\n> >  \t\tif ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&\n> >  \t\t    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */\n> > -\t\t\ttrace_repo_setup(prefix);\n> > +\t\t\ttrace_repo_setup();\n> >  \t}\n>\n> This turns out to be the ONLY place that trace_repo_setup() is\n> called, and the value of prefix here comes from the returned value\n> from precompose_argv_prefix() we saw in the previous hunk.  So as\n> far as trace_repo_setup() is concerned, taken together with ...\n>\n> >  \tcommit_pager_choice();\n> >\n> > diff --git a/trace.c b/trace.c\n> > index f726686fd92f..4c6414683414 100644\n> > --- a/trace.c\n> > +++ b/trace.c\n> > @@ -367,9 +367,9 @@ static const char *quote_crnl(const char *path)\n> >  \treturn new_path.buf;\n> >  }\n> >\n> > -/* FIXME: move prefix to startup_info struct and get rid of this arg */\n> > -void trace_repo_setup(const char *prefix)\n>\n> What is curious is that the caller in git.c this patch changed was\n> the only caller back when this FIXME comment was written.  It is\n> unclear why the original commit a9ca8a85 (builtins: print setup info\n> if repo is found, 2010-11-26) left it unfixed.\n>\n> > +void trace_repo_setup(void)\n> >  {\n> > +\tconst char *prefix = startup_info->prefix;\n>\n> ... this change, the patch is correct.\n>\n> What is not so clear is if the users of the startup_info->prefix may\n> be affected by this change, and if so, does this change introduce a\n> bug to them.\n>\n> We assign to the .prefix member in either setup_git_directory()\n> called by the other side of if/else before the precontext of the\n> first hunk of this patch, or setup_git_directory_gently() we see in\n> the precontext of that hunk.\n>\n> But then we call precompose_argv_prefix() to munge the prefix value,\n> and reassign it to the field.  All the existing users of the\n> startup_info->prefix member has been relying on the fact that it is\n> the value before \"precompose\".  With this patch, they see the value\n> after \"precompose\".  I would understand it better if you didn't add\n> a new assignment to run_builtin().\n>\n> Torsten, I _think_ this change actually fixes the bug (or sweeps it\n> under the rug) in 5c327502 (MacOS: precompose_argv_prefix(),\n> 2021-02-03), where we wanted to precompose not just the argv[] but\n> the prefix on macOS.  5c327502 changed the prefix we pass around in\n> the callchain as parameter correctly, but as we can see here, a copy\n> w/o the precomposition is left in startup_info->prefix to confuse\n> codepath that use it (instead of the third parameter given to\n> cmd_foo(ac, av, prefix)).\n>\n> Perhaps the \"precompose\" call should be moved from git.c to the\n> place just before startup_info->prefix is assigned to in\n> setup_git_directory_gently(), perhaps like the attached patch,\n> to cover this codepath.  I didn't looked at other calls to the\n> precompopse added by 5c327502, but I suspect there might need\n> similar adjustments.\n>\n> Thanks.\n\nPlease see at the end.\n\n>\n>  git.c   | 2 +-\n>  setup.c | 7 +++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git c/git.c w/git.c\n> index 9bc077a025..4bd199cc84 100644\n> --- c/git.c\n> +++ w/git.c\n> @@ -423,7 +423,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>  \t\t\tint nongit_ok;\n>  \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n>  \t\t}\n> -\t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n> +\t\tprefix = precompose_argv_prefix(argc, argv, NULL);\n>  \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n>  \t\t    !(p->option & DELAY_PAGER_CONFIG))\n>  \t\t\tuse_pager = check_pager_config(p->cmd);\n> diff --git c/setup.c w/setup.c\n> index c04cd25a30..2f6a1f794a 100644\n> --- c/setup.c\n> +++ w/setup.c\n> @@ -1264,6 +1264,13 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\tBUG(\"unhandled setup_git_directory_1() result\");\n>  \t}\n>\n> +\t/*\n> +\t * on macOS, prefix derived from the getcwd() may need to be\n> +\t * normalized into precomposed form.\n> +\t */\n> +\tif (prefix)\n> +\t\tprefix = precompose_string_if_needed(prefix);\n> +\n>  \t/*\n>  \t * At this point, nongit_ok is stable. If it is non-NULL and points\n>  \t * to a non-zero value, then this means that we haven't found a\n>\n\nWhat I tried is to follow that suggestion.\nOn top of that, we need:\n  - precompose_string_if_needed() must be exposed public, and that is done in\n    precompose_utf8.[ch] and git-compat-util.h below.\n  - setup.c learns to precompose and to set up the prefix, when needed.\n  - No change in git.c (yet), because the changes from below causes t3910 to fail in\n    \"git restore -p .\"\n\nIf I remove the \"prefix = precompose_string_if_needed(prefix);\" in setup.c we see\nthat t3910 passes again.\n\nAnd here comes the complete patch - it looks nice, but doesn't work.\nSorry guys for being shortish, thanks Dmitry  and Junio for digging.\nAnd that's all I have for today.\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex ec560565a86..cce1d57a464 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -60,10 +60,12 @@ void probe_utf8_pathname_composition(void)\n \tstrbuf_release(&path);\n }\n\n-static inline const char *precompose_string_if_needed(const char *in)\n+const char *precompose_string_if_needed(const char *in)\n {\n \tsize_t inlen;\n \tsize_t outlen;\n+\tif (!in)\n+\t\treturn NULL;\n \tif (has_non_ascii(in, (size_t)-1, &inlen)) {\n \t\ticonv_t ic_prec;\n \t\tchar *out;\n@@ -96,10 +98,7 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref\n \t\targv[i] = precompose_string_if_needed(argv[i]);\n \t\ti++;\n \t}\n-\tif (prefix) {\n-\t\tprefix = precompose_string_if_needed(prefix);\n-\t}\n-\treturn prefix;\n+\treturn precompose_string_if_needed(prefix);\n }\n\n\ndiff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h\nindex d70b84665c6..fea06cf28a5 100644\n--- a/compat/precompose_utf8.h\n+++ b/compat/precompose_utf8.h\n@@ -29,6 +29,7 @@ typedef struct {\n } PREC_DIR;\n\n const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix);\n+const char *precompose_string_if_needed(const char *in);\n void probe_utf8_pathname_composition(void);\n\n PREC_DIR *precompose_utf8_opendir(const char *dirname);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9ddf9d7044b..a508dbe5a35 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -256,6 +256,11 @@ static inline const char *precompose_argv_prefix(int argc, const char **argv, co\n {\n \treturn prefix;\n }\n+static inline const char *precompose_string_if_needed(const char *in)\n+{\n+\treturn in;\n+}\n+\n #define probe_utf8_pathname_composition()\n #endif\n\ndiff --git a/setup.c b/setup.c\nindex c04cd25a30d..5b80ccf86a6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1279,6 +1279,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tstartup_info->prefix = NULL;\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n \t} else {\n+\t\tprefix = precompose_string_if_needed(prefix);\n \t\tstartup_info->have_repository = 1;\n \t\tstartup_info->prefix = prefix;\n \t\tif (prefix)\n"},{"id":"420958","messageId":"20210404061745.19364-1-tboegi@web.de","threadId":"55410","inReplyTo":"xmqqtuotfre5.fsf@gitster.g","subject":"[PATCH v2 1/2] precompose_utf8: Make precompose_string_if_needed() public","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2021-04-04T06:17:45Z","receivedAt":"2021-04-04T06:18:03Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\ncommit 5c327502,  MacOS: precompose_argv_prefix()\nuses the function precompose_string_if_needed() internally.\nIt is only used from precompose_argv_prefix() and therefore\nstatic in compat/precompose_utf8.c\n\nExpose this function, it will be used in the next commit.\n\nWhile there, allow passing a NULL pointer, which will return NULL.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n\nThis part 1/2 never made it to the list\n\n compat/precompose_utf8.c | 9 ++++-----\n compat/precompose_utf8.h | 1 +\n git-compat-util.h        | 5 +++++\n 3 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex ec560565a8..cce1d57a46 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -60,10 +60,12 @@ void probe_utf8_pathname_composition(void)\n \tstrbuf_release(&path);\n }\n\n-static inline const char *precompose_string_if_needed(const char *in)\n+const char *precompose_string_if_needed(const char *in)\n {\n \tsize_t inlen;\n \tsize_t outlen;\n+\tif (!in)\n+\t\treturn NULL;\n \tif (has_non_ascii(in, (size_t)-1, &inlen)) {\n \t\ticonv_t ic_prec;\n \t\tchar *out;\n@@ -96,10 +98,7 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref\n \t\targv[i] = precompose_string_if_needed(argv[i]);\n \t\ti++;\n \t}\n-\tif (prefix) {\n-\t\tprefix = precompose_string_if_needed(prefix);\n-\t}\n-\treturn prefix;\n+\treturn precompose_string_if_needed(prefix);\n }\n\n\ndiff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h\nindex d70b84665c..fea06cf28a 100644\n--- a/compat/precompose_utf8.h\n+++ b/compat/precompose_utf8.h\n@@ -29,6 +29,7 @@ typedef struct {\n } PREC_DIR;\n\n const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix);\n+const char *precompose_string_if_needed(const char *in);\n void probe_utf8_pathname_composition(void);\n\n PREC_DIR *precompose_utf8_opendir(const char *dirname);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9ddf9d7044..a508dbe5a3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -256,6 +256,11 @@ static inline const char *precompose_argv_prefix(int argc, const char **argv, co\n {\n \treturn prefix;\n }\n+static inline const char *precompose_string_if_needed(const char *in)\n+{\n+\treturn in;\n+}\n+\n #define probe_utf8_pathname_composition()\n #endif\n\n--\n2.30.0.155.g66e871b664\n\n"},{"id":"420959","messageId":"20210404061754.19428-1-tboegi@web.de","threadId":"55410","inReplyTo":"xmqqtuotfre5.fsf@gitster.g","subject":"[PATCH v2 2/2] MacOs: Precompose startup_info->prefix","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2021-04-04T06:17:54Z","receivedAt":"2021-04-04T06:18:03Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\nThe \"prefix\" was precomposed for MacOs in commit 5c327502db,\nMacOS: precompose_argv_prefix()\n\nHowever, this commit forgot to update \"startup_info->prefix\" after\nprecomposing.\nRe-arrange the code in setup.c:\nMove the (possible) precomposition towards the end of\nsetup_git_directory_gently(), so that precompose_string_if_needed()\ncan use git_config_get_bool(\"core.precomposeunicode\") correctly.\n\nKeep prefix, startup_info->prefix and GIT_PREFIX_ENVIRONMENT all in sync.\n\nAnd as a result, the prefix no longer needs to be precomposed in git.c\n\nReported-by: Dmitry Torilov <d.torilov@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n\n This part did never made it to the list - it should have gone only\n to tboegi@web.de\n git send-email decided to cc to the \"Helped-by\" and \"Reported-by\" addresses,\n a feature that I was not aware of - and can be turned off with --suppress-cc=all\n In other words, I typically send these emails only to my self first, re-read\n with fresh eyes, and then send them out. End of of blabla.\n\nChanges since V1:\n Add a comment in setup.c, to make more clear that git_config_get_bool()\n is called, and the setup_XXX() must have prepared everything needed.\n\n git.c   |  2 +-\n setup.c | 14 ++++++++++----\n 2 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 9bc077a025..b53e665671 100644\n--- a/git.c\n+++ b/git.c\n@@ -423,7 +423,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tint nongit_ok;\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n-\t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n+\t\tprecompose_argv_prefix(argc, argv, NULL);\n \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n \t\t    !(p->option & DELAY_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\ndiff --git a/setup.c b/setup.c\nindex c04cd25a30..dcc9c41a85 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1281,10 +1281,6 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t} else {\n \t\tstartup_info->have_repository = 1;\n \t\tstartup_info->prefix = prefix;\n-\t\tif (prefix)\n-\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n-\t\telse\n-\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n \t}\n\n \t/*\n@@ -1311,6 +1307,16 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tif (startup_info->have_repository)\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n+\t/* Keep prefix, startup_info->prefix and GIT_PREFIX_ENVIRONMENT in sync */\n+\tprefix = startup_info->prefix;\n+\tif (prefix) {\n+\t\t/* This calls git_config_get_bool() under the hood (MacOs only) */\n+\t\tprefix = precompose_string_if_needed(prefix);\n+\t\tstartup_info->prefix = prefix;\n+\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n+\t} else {\n+\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n+\t}\n\n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n--\n2.30.0.155.g66e871b664\n\n"},{"id":"420962","messageId":"xmqqlf9y7a7a.fsf@gitster.g","threadId":"55410","inReplyTo":"20210404061754.19428-1-tboegi@web.de","subject":"Re: [PATCH v2 2/2] MacOs: Precompose startup_info->prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-04T07:58:17Z","receivedAt":"2021-04-04T07:58:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> diff --git a/setup.c b/setup.c\n> index c04cd25a30..dcc9c41a85 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1281,10 +1281,6 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t} else {\n>  \t\tstartup_info->have_repository = 1;\n>  \t\tstartup_info->prefix = prefix;\n\nIs this assignment sensible?  As we'd defer precomposition (or not)\nafter we run the repository discovery, would it break if we do not\nhave this line here (i.e. leaving startup_info->prefix NULL), and ...\n\n> -\t\tif (prefix)\n> -\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> -\t\telse\n> -\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n>  \t}\n>\n>  \t/*\n> @@ -1311,6 +1307,16 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>  \t\tif (startup_info->have_repository)\n>  \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n>  \t}\n> +\t/* Keep prefix, startup_info->prefix and GIT_PREFIX_ENVIRONMENT in sync */\n> +\tprefix = startup_info->prefix;\n\n... not wipe prefix with this assignment, i.e. we learned prefix\nbefore the previous hunk, and we would tweak it here?\n\n> +\tif (prefix) {\n> +\t\t/* This calls git_config_get_bool() under the hood (MacOs only) */\n\nIt may be more friendly to ourselves in the future if we are a bit\nmore explicit in what we want to convey with the comment, though.\nHere is my attempt.\n\n\t\t/*\n\t\t * Since precompose_string_if_needed() needs to look at\n\t\t * the core.precomposeunicode configuration, this\n\t\t * has to happen after the above block that finds\n\t\t * out where the repository is, i.e. a preparation\n                 * for calling git_config_get_bool().\n\t\t */\n\n> +\t\tprefix = precompose_string_if_needed(prefix);\n> +\t\tstartup_info->prefix = prefix;\n> +\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> +\t} else {\n> +\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n> +\t}\n>\n>  \tstrbuf_release(&dir);\n>  \tstrbuf_release(&gitdir);\n> --\n> 2.30.0.155.g66e871b664\n\nOther than that, both patches look sensible.\n\nBy the way, isn't the canonical way to spell the name of the\nparticular operating system that needs this patch \"macOS\"?\n\ncf. https://support.apple.com/macos\n\nThanks.\n"},{"id":"420971","messageId":"20210404171409.32265-1-tboegi@web.de","threadId":"55410","inReplyTo":"xmqqtuotfre5.fsf@gitster.g","subject":"[PATCH v3 1/2] precompose_utf8: Make precompose_string_if_needed() public","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2021-04-04T17:14:09Z","receivedAt":"2021-04-04T17:14:20Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\ncommit 5c327502,  MacOS: precompose_argv_prefix()\nuses the function precompose_string_if_needed() internally.\nIt is only used from precompose_argv_prefix() and therefore\nstatic in compat/precompose_utf8.c\n\nExpose this function, it will be used in the next commit.\n\nWhile there, allow passing a NULL pointer, which will return NULL.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n compat/precompose_utf8.c | 9 ++++-----\n compat/precompose_utf8.h | 1 +\n git-compat-util.h        | 5 +++++\n 3 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex ec560565a8..cce1d57a46 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -60,10 +60,12 @@ void probe_utf8_pathname_composition(void)\n \tstrbuf_release(&path);\n }\n\n-static inline const char *precompose_string_if_needed(const char *in)\n+const char *precompose_string_if_needed(const char *in)\n {\n \tsize_t inlen;\n \tsize_t outlen;\n+\tif (!in)\n+\t\treturn NULL;\n \tif (has_non_ascii(in, (size_t)-1, &inlen)) {\n \t\ticonv_t ic_prec;\n \t\tchar *out;\n@@ -96,10 +98,7 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref\n \t\targv[i] = precompose_string_if_needed(argv[i]);\n \t\ti++;\n \t}\n-\tif (prefix) {\n-\t\tprefix = precompose_string_if_needed(prefix);\n-\t}\n-\treturn prefix;\n+\treturn precompose_string_if_needed(prefix);\n }\n\n\ndiff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h\nindex d70b84665c..fea06cf28a 100644\n--- a/compat/precompose_utf8.h\n+++ b/compat/precompose_utf8.h\n@@ -29,6 +29,7 @@ typedef struct {\n } PREC_DIR;\n\n const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix);\n+const char *precompose_string_if_needed(const char *in);\n void probe_utf8_pathname_composition(void);\n\n PREC_DIR *precompose_utf8_opendir(const char *dirname);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9ddf9d7044..a508dbe5a3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -256,6 +256,11 @@ static inline const char *precompose_argv_prefix(int argc, const char **argv, co\n {\n \treturn prefix;\n }\n+static inline const char *precompose_string_if_needed(const char *in)\n+{\n+\treturn in;\n+}\n+\n #define probe_utf8_pathname_composition()\n #endif\n\n--\n2.30.0.155.g66e871b664\n\n"},{"id":"420972","messageId":"20210404171414.32322-1-tboegi@web.de","threadId":"55410","inReplyTo":"xmqqtuotfre5.fsf@gitster.g","subject":"[PATCH v3 2/2] MacOs: Precompose startup_info->prefix","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2021-04-04T17:14:14Z","receivedAt":"2021-04-04T17:14:27Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\nThe \"prefix\" was precomposed for MacOs in commit 5c327502db,\nMacOS: precompose_argv_prefix()\n\nHowever, this commit forgot to update \"startup_info->prefix\" after\nprecomposing.\n\nMove the (possible) precomposition towards the end of\nsetup_git_directory_gently(), so that precompose_string_if_needed()\ncan use git_config_get_bool(\"core.precomposeunicode\") correctly.\n\nKeep prefix, startup_info->prefix and GIT_PREFIX_ENVIRONMENT all in sync.\n\nAnd as a result, the prefix no longer needs to be precomposed in git.c\n\nReported-by: Dmitry Torilov <d.torilov@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n\n Changes since V2:\n - Re-arranged code in setup.c to be more consistent\n - Added the longer comment block, as suggested by Junio, thanks for that.\n\n git.c   |  2 +-\n setup.c | 28 ++++++++++++++++++----------\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 9bc077a025..b53e665671 100644\n--- a/git.c\n+++ b/git.c\n@@ -423,7 +423,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tint nongit_ok;\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n-\t\tprefix = precompose_argv_prefix(argc, argv, prefix);\n+\t\tprecompose_argv_prefix(argc, argv, NULL);\n \t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n \t\t    !(p->option & DELAY_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\ndiff --git a/setup.c b/setup.c\nindex c04cd25a30..59e2facd9d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1274,18 +1274,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t * the GIT_PREFIX environment variable must always match. For details\n \t * see Documentation/config/alias.txt.\n \t */\n-\tif (nongit_ok && *nongit_ok) {\n+\tif (nongit_ok && *nongit_ok)\n \t\tstartup_info->have_repository = 0;\n-\t\tstartup_info->prefix = NULL;\n-\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n-\t} else {\n+\telse\n \t\tstartup_info->have_repository = 1;\n-\t\tstartup_info->prefix = prefix;\n-\t\tif (prefix)\n-\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n-\t\telse\n-\t\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n-\t}\n\n \t/*\n \t * Not all paths through the setup code will call 'set_git_dir()' (which\n@@ -1311,6 +1303,22 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tif (startup_info->have_repository)\n \t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n+\t/*\n+\t * Since precompose_string_if_needed() needs to look at\n+\t * the core.precomposeunicode configuration, this\n+\t * has to happen after the above block that finds\n+\t * out where the repository is, i.e. a preparation\n+\t * for calling git_config_get_bool().\n+\t */\n+\tif (prefix) {\n+\t\tprefix = precompose_string_if_needed(prefix);\n+\t\tstartup_info->prefix = prefix;\n+\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n+\t} else {\n+\t\tstartup_info->prefix = NULL;\n+\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n+\t}\n+\n\n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n--\n2.30.0.155.g66e871b664\n\n"}]}