Re: [PATCH v2 14/22] pickaxe: refactor function selection in diffcore-pickaxe()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2021, 23:45 UTC
Message-ID: <xmqqr1jw9pf8.fsf@gitster.g>
In-Reply-To: <20210216115801.4773-15-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 10 quoted lines

> +	pickaxe_fn fn;
>  
>  	if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
>  		int cflags = REG_EXTENDED | REG_NEWLINE;
> @@ -235,6 +236,14 @@ void diffcore_pickaxe(struct diff_options *o)
>  			cflags |= REG_ICASE;
>  		regcomp_or_die(&regex, needle, cflags);
>  		regexp = &regex;
> +
> +		/* diff.c errors on -G and --pickaxe-regex for us */

I had to read this twice; I am guessing that the comment wants to say that this if/else if/else cascade is correct because KIND_G and PICKAXE_REGEX are mutually incompatible (ensured in diff.c). And I think that is true (but as I said, I am not sure if we want to cast in stone that kind-g and regex are mutually exclusive---rather, I'd want to see them eventually orthogonal).

Show 16 quoted lines

> +		if (opts & DIFF_PICKAXE_KIND_G)
> +			fn = diff_grep;
> +		else if (opts & DIFF_PICKAXE_REGEX)
> +			fn = has_changes;
> +		else
> +			BUG("unreachable");
>  	} else if (opts & DIFF_PICKAXE_KIND_S) {
>  		if (o->pickaxe_opts & DIFF_PICKAXE_IGNORE_CASE &&
>  		    has_non_ascii(needle)) {
> @@ -251,10 +260,14 @@ void diffcore_pickaxe(struct diff_options *o)
>  			kwsincr(kws, needle, strlen(needle));
>  			kwsprep(kws);
>  		}
> +		fn = has_changes;
> +	} else if (opts & DIFF_PICKAXE_KIND_OBJFIND) {
> +		fn = NULL;

This is the most valuable line in this patch ;-)  It makes tons of sense.

Show 10 quoted lines

> +	} else {
> +		BUG("unknown pickaxe_opts flag");
>  	}
>  
> -	pickaxe(&diff_queued_diff, o, regexp, kws,
> -		(opts & DIFF_PICKAXE_KIND_G) ? diff_grep : has_changes);
> +	pickaxe(&diff_queued_diff, o, regexp, kws, fn);
>  
>  	if (regexp)
>  		regfree(regexp);