Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()

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

Simon Ruderich <simon@ruderich.org> writes:

> get_textconv() is called in diff_grep() to determine the textconv driver
> before calling fill_one() and then again in fill_one(). Remove this
> unnecessary call by determining the textconv driver before calling
> fill_one().

If I am reading the code correctly, it is has_changes(), which is
used for "log -S" (not "log -G" that uses diff_grep()), that does
the unnecessary get_textconv() unconditionally.  The way diff_grep() 
divides the work to make fill_one() responsible for filling the
textconv as necessary is internally consistent, and there is no
unnecessary call.

Perhaps...

	The fill_one() function is responsible for finding and
	filling the textconv filter as necessary, and is called by
	diff_grep() function that implements "git log -G<pattern>".

	The has_changes() function calls get_textconv() for two
	sides being compared, before it checks to see if it was
	asked to perform the pickaxe limiting with the -S option.
	Move the code around to avoid this wastage.  After that,
	fill_one() is called to use the textconv.

	By adding get_textconv() to diff_grep() and relieving
	fill_one() of responsibility to find the textconv filter, we
	can avoid calling get_textconv() twice.

Explained that way, it makes me wonder why we cannot fix it the
other way around, that is, not fetching textconv in has_changes()
and instead letting fill_one() to find textconv as needed.

The answer is because has_changes() itself looks at the textconv.

But we have to wonder why it is so.  diff_grep() short-circuits when
the two sides are identical and has_changes() has a similar but
different logic to check if the identical two sides are processed
with the same textconv filter before saying this filepair is
uninteresting.

Shouldn't that logic be unified as well?