Re: [PATCH] chore: use prefix from startup_info

["Torsten Bögershausen" <tboegi@web.de>]

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)