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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Mar 24 2021, Jeff King wrote:

> On Wed, Mar 24, 2021 at 02:48:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Refactor the userdiff_find_by_namelen() function so that a new
>> for_each_userdiff_driver() API function does most of the work.
>> 
>> This will be useful for the same reason we've got other for_each_*()
>> API functions as part of various APIs, and will be used in a follow-up
>> commit.
>
> The refactorings up to here all made sense, but TBH this one makes the
> code more confusing to follow to me.
>
> Perhaps part of it is just that the diff is messy, but I had to read it
> several times to understand what's going on. Here's what I think were
> the tricky parts:
>
>> -static struct userdiff_driver *userdiff_find_by_namelen(const char *k, size_t len)
>> +struct for_each_userdiff_driver_cb {
>> +	const char *k;
>> +	size_t len;
>> +	struct userdiff_driver *driver;
>> +};
>
> 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.  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.
>
> The fact that callbacks are awkward in general in C might not be
> solvable, at least not without duplicating some iteration code.
>
>> +static int userdiff_find_by_namelen_cb(struct userdiff_driver *driver,
>> +				       enum userdiff_driver_type type, void *priv)
>>  {
>> [...]
>> +	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.

*nod*

Also thanks for all the reviewing so far both, I'm not replying to all
of it point-by-point here, will respond with a re-roll at some point.

>> +int for_each_userdiff_driver(each_userdiff_driver_fn fn,
>> +			     enum userdiff_driver_type type, void *cb_data)
>> +{
>> +	int i, ret;
>> +	if (type & (USERDIFF_DRIVER_TYPE_UNSPECIFIED | USERDIFF_DRIVER_TYPE_CUSTOM)) {
>> +
>> +		for (i = 0; i < ndrivers; i++) {
>> +			struct userdiff_driver *drv = drivers + i;
>> +			ret = fn(drv, USERDIFF_DRIVER_TYPE_CUSTOM, cb_data);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +	if (type & (USERDIFF_DRIVER_TYPE_UNSPECIFIED | USERDIFF_DRIVER_TYPE_BUILTIN)) {
>> +
>> +		for (i = 0; i < ARRAY_SIZE(builtin_drivers); i++) {
>> +			struct userdiff_driver *drv = builtin_drivers + i;
>> +			ret = fn(drv, USERDIFF_DRIVER_TYPE_BUILTIN, cb_data);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +	return 0;
>> +}
>
> I spent a while scratching my head at these types, and what they would
> be used for, since this caller doesn't introduce any. Looking at patch 7
> helped, though it's unclear to me why we need to distinguish between
> custom and builtin drivers there. As you note there, nobody calls
> list-custom-drivers nor list-drivers. And if we haven't configured
> anything, then wouldn't list-drivers be the same as list-builtin-drivers?
> Or for the purposes of that test, if we _did_ configure something,
>
>   As an aside, it feels like this is something we ought to be able to
>   ask git-config about, rather than having a test-helper. This is
>   basically "baked-in" config, and if we represented it as such, and
>   parsed it into a struct just like regular config, then probably "git
>   config --list --source" could be used to find it (and differentiate it
>   from user-provided config). Possible downsides:
>
>     1. Would people find it confusing that "git config --list" suddenly
>        gets way bigger? Maybe we'd want an "--include-baked-in" option
>        or something.
>
>     2. Is the cost of parsing the config measurably bad? Obviously a
>        user could provide the same content and we'd have to parse it,
>        but there's a lot more rules here than most users would probably
>        provide.

Also:

 3. Only the PATTERNS() macro translates as-is to config syntax. We
    don't have a way to do what IPATTERN() does in the config syntax
    currently.

    We could add a ifuncname and xifuncname or whatever for it I guess,
    but currently the ICASE behavior in the C code is magic.

>> +enum userdiff_driver_type {
>> +	USERDIFF_DRIVER_TYPE_UNSPECIFIED = 1<<0,
>> +	USERDIFF_DRIVER_TYPE_BUILTIN = 1<<1,
>> +	USERDIFF_DRIVER_TYPE_CUSTOM = 1<<2,
>> +};
>
> I was confused by these being bits, because some of them seem mutually
> exclusive (e.g., UNSPECIFIED and anything else).
>
> 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.
>
> 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.
>
> I dunno. I know a lot of this is nit-picking, and I don't think there's
> anything incorrect in this patch. I just found it surprisingly hard to
> read for something that purports to be refactoring / cleaning the code.
>
> -Peff