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

Jeff King [off-list ref] writes:

Our callback function does _one_ type of selection (based on a "type"
parameter), but not another (based on the name). That feels
inconsistent, but is also the reason we have this awkward struct.

I wrote a very similar review but did not send it out, as I couldn't
pinpoint where the awkwardness come from exactly.

Part
of my confusion is the name: this is not something to be generically
used with for_each_userdiff_driver(), but rather a type unique to
find_by_namelen() to be passed through the opaque void pointer.

So "struct find_by_namelen_data" would have been a lot more
enlightening.

Yes.  The callback parameter being "void *" is the API, and it is
just this user that uses this particular structure.

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.

quoted

+	if (!strncmp(driver->name, cb_data->k, cb_data->len) &&
+	    !driver->name[cb_data->len]) {
+		cb_data->driver = driver;
+		return -1; /* found it! */
 	}

This "return -1" took me a while to grok, and the comment didn't help
all that much. The point is to stop traversing the list, but "-1" to me
signals error. I think returning "1" might be a bit more idiomatic, but
also a comment that says "tell the caller to stop iterating" would have
been more clear.

And the fact that it becomes the return value of "for_each_()" iterator
does not quite help to use a negative value, implying there was some
error condition X-<.

Perhaps it would make more sense as:

  USERDIFF_DRIVER_TYPE_BUILTIN = 1<<0,
  USERDIFF_DRIVER_TYPE_CUSTOM = 1<<0,
  USERDIFF_DRIVER_TYPE_ALL = USERDIFF_DRIVER_TYPE_BUILTIN | USERDIFF_DRIVER_TYPE_CUSTOM

Or the one caller who wants "ALL" could even do the OR themselves.

Great minds think alike ;-)

I do kind of wonder if there's much value in having a single function
with a type field at all, though, given that there's no overlap in the
implementation. Would separate "for_each_custom" and "for_each_builtin"
functions make sense? And then the existing caller would just call them
sequentially.

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

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

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