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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2021, 04:50 UTC
Message-ID: <xmqqtup1yx2f.fsf@gitster.g>
In-Reply-To: <patch-05.11-64ea5e8443f-20210324T014604Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 5 quoted lines

> +struct for_each_userdiff_driver_cb {
> +	const char *k;
> +	size_t len;
> +	struct userdiff_driver *driver;
> +};

Makes me wonder if we want to rename s/k/name/;

Show 13 quoted lines

> +static int userdiff_find_by_namelen_cb(struct userdiff_driver *driver,
> +				       enum userdiff_driver_type type, void *priv)
>  {
> +	struct for_each_userdiff_driver_cb *cb_data = priv;
> +
> +	if (!strncmp(driver->name, cb_data->k, cb_data->len) &&
> +	    !driver->name[cb_data->len]) {
> +		cb_data->driver = driver;
> +		return -1; /* found it! */
>  	}
> -	return NULL;
> +	return 0;
> +}

Makes sense.

> +static struct userdiff_driver *userdiff_find_by_namelen(const char *k, size_t len)
> +{
> +	struct for_each_userdiff_driver_cb udcbdata = { .k = k, .len = len, .driver = NULL };

No need to explicitly spell the zero initialization. Wrapping it like this:

	struct for_each_userdiff_driver_cb udcbdata = {
		.k = k, .len = len
	};

would avoid the overlong line.

> +
> +	for_each_userdiff_driver(userdiff_find_by_namelen_cb,
> +				 USERDIFF_DRIVER_TYPE_UNSPECIFIED, &udcbdata);
> +	return udcbdata.driver;

Show 10 quoted lines

> @@ -373,3 +385,28 @@ struct userdiff_driver *userdiff_get_textconv(struct repository *r,
>  
>  	return driver;
>  }
> +
> +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)) {

I presume that the concrete ones are bitmask (i.e. BUILTIN occupies bit #0 while CUSTOM occupies bit #1, or something like that). Then

    #define USERDIFF_DRIVER_TYPE_UNSPECIFIED (-1)

would make this (and the other) condition far easier to read, i.e.

	if (type & USERDIFF_DRIVER_TYPE_CUSTOM) {
		... if the caller wants to iterate over "custom" drivers
		... do these things