Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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.
> 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?
> 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 &&
