Re: [PATCH v2 15/22] pickaxe: assert that we must have a needle under -G or -S

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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 &&