Re: [PATCH] chore: use prefix from startup_info
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.
Show 5 quoted lines
> 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.
Show 19 quoted lines
> 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 ...
Show 12 quoted lines
> 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(-)
Show changes to diff +8 −1
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
