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
