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

From: Jeff King <peff@peff.net>
Date: Apr 4, 2013, 21:11 UTC
Message-ID: <20130404211110.GB25811@sigill.intra.peff.net>
In-Reply-To: <7vvc82ggy3.fsf@alter.siamese.dyndns.org>

On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote:

Show 36 quoted lines

> 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?

I think it would be OK to perform the same optimization in diff_grep; I do not recall offhand why I didn't do so when touching this code last time, so it may have just been because I didn't think of it.

But I do not think fill_one is the right interface for it. The reason has_changes calls get_textconv separately is that we do not want to fill the buffer (which may be expensive) if we can avoid it. So the correct sequence is:

  1. Find out textconv_one.

  2. Find out textconv_two.

  3. Check "!hashcmp(one->sha1, two->sha1) && textconv_one == textconv_two";
     if true, then we know the content we are about to compare will be
     identical, and we can return early.

  4. Otherwise, retrieve the content for one (respecting textconv_one).

  5. Retrieve the content for two (respecting textconv_two).

You cannot implement the optimization looking only at one side (obviously), but you also need to split the textconv lookup from the "fill" procedure if you want the optimization to come at the right place.

If you turned fill_one into "fill_both_sides" then you could share the code between diff_grep/pickaxe and do it in the right order in the helper.

-Peff