Re: [PATCH v2 15/22] pickaxe: assert that we must have a needle under -G or -S
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 30, 2021, 23:50 UTC
- Message-ID
- <xmqqmtuk9p74.fsf@gitster.g>
- In-Reply-To
- <20210216115801.4773-16-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 10 quoted lines
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c > index cff46f9f8f7..dd1b5c72332 100644 > --- a/diffcore-pickaxe.c > +++ b/diffcore-pickaxe.c > @@ -132,9 +132,6 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o, > oidset_contains(o->objfind, &p->two->oid)); > } > > - if (!o->pickaxe[0]) > - return 0;
So -S"" could pass o->pickaxe a non-NULL pointer, but the string pointed by that pointer could be 0-length. And that is not what we want to see happen.
Show 9 quoted lines
> if (o->flags.allow_textconv) {
> textconv_one = get_textconv(o->repo, p->one);
> textconv_two = get_textconv(o->repo, p->two);
> @@ -230,6 +227,8 @@ void diffcore_pickaxe(struct diff_options *o)
> kwset_t kws = NULL;
> pickaxe_fn fn;
>
> + if (opts & ~DIFF_PICKAXE_KIND_OBJFIND && !needle)
> + BUG("should have needle under -G or -S");Here, needle was picked up at the beginning of this function like so:
void diffcore_pickaxe(struct diff_options *o)
{
const char *needle = o->pickaxe;The two checks seem to be protecting from different things. Shouldn't the new BUG() condition be more like
if ((opts & ~DIFF_PICKAXE_KIND_OBJFIND) && (!needle || !*needle))
instead?
Show 19 quoted lines
> if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
> int cflags = REG_EXTENDED | REG_NEWLINE;
> if (o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE)
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index bcaca7e882c..4b65b89e7a5 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -56,6 +56,12 @@ test_expect_success setup '
> '
>
> test_expect_success 'usage' '
> + test_expect_code 129 git log -S 2>err &&
> + test_i18ngrep "switch.*requires a value" err &&
> +
> + test_expect_code 129 git log -G 2>err &&
> + test_i18ngrep "switch.*requires a value" err &&
> +
> test_expect_code 128 git log -Gregex -Sstring 2>err &&
> test_i18ngrep "mutually exclusive" err &&