| Message ID | patch-2.8-244715e3497-20211106T210711Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grep: simplify & delete code by changing obscure cfg variable behavior | expand |
On Sat, Nov 06, 2021 at 10:10:48PM +0100, Ævar Arnfjörð Bjarmason wrote: > @@ -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 && *prefix)); Small nit, but the check to `prefix` (in `prefix && *prefix`) is redundant with the left-hand side of the or. > precompose_argv_prefix(argc, argv, NULL); > if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) && > !(p->option & DELAY_PAGER_CONFIG)) > diff --git a/grep.c b/grep.c > index f6e113e9f0f..88ebc504630 100644 > --- a/grep.c > +++ b/grep.c > @@ -145,7 +145,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix > > opt->repo = repo; > opt->prefix = prefix; > - opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0; > + opt->prefix_length = prefix ? strlen(prefix) : 0; Looking around, ls-tree's initialization includes a conditional of the form: if (prefix && *prefix) chomp_prefix = strlen(prefix); So that could be cleaned up too. But honestly, the pre-image of this patch (and the spot in ls-tree) doesn't make a lot of sense to me to begin with. Even if prefix were the empty string, calling strlen() on it will just give us zero. So there is no difference between assigning `str && *str ? strlen(str) : 0` and `str ? strlen(str) : 0`. So I am confused why this needs hardening with an assertion when it seems like the checks before calling strlen() were overly restrictive to begin with. In other words: why not only include this hunk (either in this patch, or squashed into another patch later on)? Thanks, Taylor
diff --git a/git.c b/git.c index 5ff21be21f3..aa4f0d77c4b 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) @@ -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 && *prefix)); precompose_argv_prefix(argc, argv, NULL); if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) && !(p->option & DELAY_PAGER_CONFIG)) diff --git a/grep.c b/grep.c index f6e113e9f0f..88ebc504630 100644 --- a/grep.c +++ b/grep.c @@ -145,7 +145,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix opt->repo = repo; opt->prefix = prefix; - opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0; + opt->prefix_length = prefix ? strlen(prefix) : 0; opt->pattern_tail = &opt->pattern_list; opt->header_tail = &opt->header_list; }
The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "". So let's drop the "prefix && *prefix" check in grep.c added in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11). As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we have existing code that does away with this assertion. This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- git.c | 4 ++-- grep.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)