Re: [PATCH v2 2/8] built-ins: trust the "prefix" from run_builtin()
From: Junio C Hamano <hidden>
Date: 2021-11-12 16:38:07
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted hunk
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c71..84bed6d5612 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c@@ -147,16 +147,15 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) OPT__ABBREV(&abbrev), OPT_END() }; - - git_config(git_default_config, NULL); ls_tree_prefix = prefix;
See below.
- if (prefix && *prefix) + if (prefix) chomp_prefix = strlen(prefix);
We now assume a non-NULL prefix means a non-NUL *prefix, so this change is understandable.
+ git_config(git_default_config, NULL);
This moving down of git_config() call is not. A necessary change, or an unrelated churn? If necessary, why? By not checking if prefix[0] is NUL, we now need to delay reading the configuration, because ...?
quoted hunk
argc = parse_options(argc, argv, prefix, ls_tree_options, ls_tree_usage, 0); if (full_tree) { - ls_tree_prefix = prefix = NULL; + ls_tree_prefix = NULL; chomp_prefix = 0; } /* -d -r should imply -t, but -d by itself should not have to. */@@ -178,7 +177,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL), PATHSPEC_PREFER_CWD, - prefix, argv + 1); + ls_tree_prefix, argv + 1);
The above two are unnecessary changes. It was not like the introduction of ls_tree_prefix was made in order to get rid of "prefix" altogether. We still have and use prefix, but we have ls_tree_prefix to expose the value of it to other functions as a way to "cheat", without having to pass it through as a parameter (and cheating is OK as its scope is limited to the file). Perhaps make a conscious effort to refrain from making such unnecessary changes, especially when working on a multi-patch series, which may avoid wearing down reviewers?
quoted hunk
diff --git a/git.c b/git.c index 5ff21be21f3..611bf2f63eb 100644 --- a/git.c +++ b/git.c@@ -420,9 +420,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) { int status, help; struct stat st; - const char *prefix; + const char *prefix = NULL; - prefix = NULL; help = argc == 2 && !strcmp(argv[1], "-h"); if (!help) { if (p->option & RUN_SETUP)
Likewise. Little cuts accumulate.
quoted hunk
@@ -431,6 +430,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) int nongit_ok; prefix = setup_git_directory_gently(&nongit_ok); } + assert(!prefix || *prefix);
Good.
quoted hunk
diff --git a/grep.h b/grep.h index 95cccb670f9..62deadb885f 100644 --- a/grep.h +++ b/grep.h@@ -134,8 +134,6 @@ struct grep_opt { */ struct repository *repo; - const char *prefix; - int prefix_length;
So, builtin/grep.c is the only user of the low-level grep machinery that needs to touch these two, and we can lose these members now that builtin/grep.c relies on the file-scope global instead. OK.
quoted hunk
diff --git a/revision.c b/revision.c index ab7c1358042..9f9b0d2429e 100644 --- a/revision.c +++ b/revision.c@@ -1833,7 +1833,7 @@ void repo_init_revisions(struct repository *r, revs->commit_format = CMIT_FMT_DEFAULT; revs->expand_tabs_in_log_default = 8; - grep_init(&revs->grep_filter, revs->repo, prefix); + grep_init(&revs->grep_filter, revs->repo); revs->grep_filter.status_only = 1; repo_diff_setup(revs->repo, &revs->diffopt);