| Message ID | 8009fdb38b0b4c3880588119b99ac5387d398540.1728099043.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove the_repository global for am, annotate, apply, archive builtins | expand |
On Sat, Oct 05, 2024 at 03:30:41AM +0000, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > The current code in run_builtin() passes in a repository to the builtin > based on whether cmd_struct's option flag has RUN_SETUP. > > This is incorrect, however, since some builtins that only have > RUN_SETUP_GENTLY can potentially take a repository. > setup_git_directory_gently() tells us whether or not a command is being > run inside of a repository. > > Use the output of setup_git_directory_gently() to help determine whether > or not there is a repository to pass to the builtin. If not, then we > just pass NULL. > > As part of this patch, we need to modify add to check for a NULL repo > before calling repo_git_config(), since add -h can be run outside of a > repository. > > Signed-off-by: John Cai <johncai86@gmail.com> > --- > builtin/add.c | 3 ++- > git.c | 7 ++++--- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index 773b7224a49..7d353077921 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -385,7 +385,8 @@ int cmd_add(int argc, > char *ps_matched = NULL; > struct lock_file lock_file = LOCK_INIT; > > - repo_config(repo, add_config, NULL); > + if (repo) > + repo_config(repo, add_config, NULL); The reason why we need to check whether the `repo` is NULL is that when using "git add -h", the RUN_SETUP flag would be converted to RUN_SETUP_GENTLY. I think this change is OK. But I wonder whether we should encapsulate the logic into the "repo_config" function. It's a little cumbersome to check whether the "repo" exists. I also feel it's a bad idea to check in the "repo_config" function because in the most time, we run commands inside the repository. So, in my view, at current, this is enough. Thanks, Jialuo
diff --git a/builtin/add.c b/builtin/add.c index 773b7224a49..7d353077921 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -385,7 +385,8 @@ int cmd_add(int argc, char *ps_matched = NULL; struct lock_file lock_file = LOCK_INIT; - repo_config(repo, add_config, NULL); + if (repo) + repo_config(repo, add_config, NULL); argc = parse_options(argc, argv, prefix, builtin_add_options, builtin_add_usage, PARSE_OPT_KEEP_ARGV0); diff --git a/git.c b/git.c index 2fbea24ec92..47741be3e4c 100644 --- a/git.c +++ b/git.c @@ -444,6 +444,7 @@ static int handle_alias(int *argcp, const char ***argv) static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo) { int status, help; + int no_repo = 1; struct stat st; const char *prefix; int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY)); @@ -455,9 +456,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct if (run_setup & RUN_SETUP) { prefix = setup_git_directory(); + no_repo = 0; } else if (run_setup & RUN_SETUP_GENTLY) { - int nongit_ok; - prefix = setup_git_directory_gently(&nongit_ok); + prefix = setup_git_directory_gently(&no_repo); } else { prefix = NULL; } @@ -480,7 +481,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct trace2_cmd_name(p->cmd); validate_cache_entries(repo->index); - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); + status = p->fn(argc, argv, prefix, no_repo ? NULL : repo); validate_cache_entries(repo->index); if (status)