Re: [PATCH 6/9] grep: cache userdiff_driver in grep_source

On Thu, Feb 02, 2012 at 10:34:07AM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

-		grep_attr_lock();
-		drv = userdiff_find_by_path(gs->name);
-		grep_attr_unlock();
-		if (drv && drv->funcname.pattern) {
-			const struct userdiff_funcname *pe = &drv->funcname;
+		grep_source_load_driver(gs);
+		if (gs->driver->funcname.pattern) {
+			const struct userdiff_funcname *pe = &gs->driver->funcname;

When we load driver, gs->driver gets at least "default" driver, so we no
longer need to check for drv != NULL as we used to?  Is that the reason
for the slight difference here?

Yes, exactly.

We could just leave gs->driver NULL instead of looking up "default", and
then use NULL to signal to the calling code that defaults should be
used. But NULL is interpreted by grep_source_load_driver as "we did not
look up the driver yet", so the common case of "no driver" would mean we
accidentally do the lookup multiple times.  The diff_filespec code uses
the same convention to solve the same problem.

Speaking of which, there was some notion in my mind that a "grep_source"
and a "diff_filespec" were very similar objects, and that we could
possibly unify the implementations. I decided against that route with
this series, as it would have involved pretty heavy refactoring of the
diff code to prevent a fairly small amount of code duplication.

-Peff