Re: [PATCH 2/2] pickaxe: use textconv for -S counting

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

Jeff King <peff@peff.net> writes:

> We currently just look at raw blob data when using "-S" to
> pickaxe. This is mostly historical, as pickaxe predates the
> textconv feature. If the user has bothered to define a
> textconv filter, it is more likely that their search string will be
> on the textconv output, as that is what they will see in the
> diff (and we do not even provide a mechanism for them to
> search for binary needles that contain NUL characters).

Oookay, I suppose...

>  static int has_changes(struct diff_filepair *p, struct diff_options *o,
>  		       regex_t *regexp, kwset_t kws)
>  {
> +	struct userdiff_driver *textconv_one = get_textconv(p->one);
> +	struct userdiff_driver *textconv_two = get_textconv(p->two);
> +	mmfile_t mf1, mf2;
> +	int ret;
> +
>  	if (!o->pickaxe[0])
>  		return 0;
>  
> -	if (!DIFF_FILE_VALID(p->one)) {
> -		if (!DIFF_FILE_VALID(p->two))
> -			return 0; /* ignore unmerged */

What happened to this part that avoids showing nonsense for unmerged
paths?

> +	/*
> +	 * If we have an unmodified pair, we know that the count will be the
> +	 * same and don't even have to load the blobs. Unless textconv is in
> +	 * play, _and_ we are using two different textconv filters (e.g.,
> +	 * because a pair is an exact rename with different textconv attributes
> +	 * for each side, which might generate different content).
> +	 */
> +	if (textconv_one == textconv_two && diff_unmodified_pair(p))
> +		return 0;

I am not sure about this part that cares about the textconv.

Wouldn't the normal "git diff A B" skip the filepair that are
unmodified in the first place at the object name level without even
looking at the contents (see e.g. diff_flush_patch())?

Shouldn't this part of the code emulating that behaviour no matter
what textconv filter(s) are configured for these paths?