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@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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)
