Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree

From: John Keeping <john@keeping.me.uk>
Date: May 13, 2013, 14:59 UTC
Message-ID: <20130513145948.GR2299@serenity.lan>
In-Reply-To: <7v38tros3y.fsf@alter.siamese.dyndns.org>

On Mon, May 13, 2013 at 07:46:09AM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> John Keeping <john@keeping.me.uk> writes:
> 
> > On Mon, May 13, 2013 at 06:53:29AM -0700, Junio C Hamano wrote:
> >> John Keeping <john@keeping.me.uk> writes:
> >> 
> >> > The caching layer could also introduce false positives though, which is
> >> > more serious.  If you cache patch IDs with a pathspec restriction ...
> >> 
> >> What?  What business does patch-id have with pathspec-limited diff
> >> generation?  You do not rebase or cherry-pick with pathspec, so
> >> unless you are populating the patch-id cache at a wrong point (like,
> >> say whenevern "git show $commit" is run), I am not sure why pathspec
> >> limit becomes even an issue.
> >
> > revision.c::cherry_pick_list() sets the pathspec to what was specified
> > in the revision options.  It's done that since commit 36d56de (Fix
> > --cherry-pick with given paths, 2007-07-10) and t6007 tests that it
> > works.
> 
> Then the caching should be automatically turned off when pathspec is
> given.

That was my first thought, but since we can be affected by other diff options set in the user's config as well it ended up being simpler to include it in the options hash and use that.

This has the advantage that you get the benefit of the cache if you run "git log --cherry-mark" with the same paths more than once. In my testing the cache is beneficial as soon as you examine more than one similar range (e.g. master...feature-A and then master...feature-B).