Re: [PATCH v4 05/10] userdiff: add and use for_each_userdiff_driver()

From: Jeff King <peff@peff.net>
Date: Mar 24, 2021, 20:43 UTC
Message-ID: <YFukY9XZkO1vRYZC@coredump.intra.peff.net>
In-Reply-To: <xmqqsg4kxr38.fsf@gitster.g>

On Wed, Mar 24, 2021 at 12:57:15PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> And while "type" could also be made a part of this structure and
> have the API always iterate over all entries regardless of "type",
> i.e. the callback function could be the one responsible for finding
> the entries in the table for one particular type, it is understandable
> that potential callers can be helped by having the pre-filtering
> based on the "type" thing on the API side.

Yeah. We _could_ also have the for-each function filter by name (if present). And then quit when it finds a matching name (because it knows the names are supposed to be unique).

That is the opposite of:

> Or a single for_each_driver() that gives <name, length, type> tuple
> to the callback function, iterating over all drivers regardless of
> the type.

I'd be fine with that, too. I don't have a huge preference either way, but it does feel like it would fix the weird asymmetry (though as I said, I don't think the asymmetry is _wrong_, and it may not be worth over-engineering this tiny corner of the interface).

Thanks for spelling out both of these directions clearly. My response to Ævar was a little muddled as I was having trouble figuring out just what it was that left me so puzzled by the patch. :)

By the way, one thing I wondered about all of this: could we have an entry in both the custom and builtin lists? I think the answer is "no", because any custom config for a builtin type would get placed inside the existing struct in the builtin_drivers array (which is sadly a reason we must leak any old string values when we parse diff.*.funcname, etc; we don't know if they are string literals or heap values from previously parsed config).

I also notice that the "builtin" for-each does not count the boolean "diff" or "!diff" attribute structs. That is perhaps reasonable, as they do not have interesting names nor values in the first place. I do still wonder whether "builtin vs custom" is even a useful distinction (i.e., those builtins are less builtin_drivers[] than drivers[]).

-Peff