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.