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

[Junio C Hamano <gitster@xxxxxxxxx>]

Ævar Arnfjörð Bjarmason  <avarab@xxxxxxxxx> writes:

> +	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).

> +		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.

> +	} 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);