Re: [PATCH 03/11] sha1-array: provide oid_array_filter

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2018, 17:20 UTC
Message-ID: <xmqqsh2m1r1d.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180904230149.180332-4-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> +/* Call fn for each oid, and retain it if fn returns 0, remove it otherwise */
> +int oid_array_filter(struct oid_array *array,
> +		     for_each_oid_fn fn,
> +		     void *cbdata);

Comparing this with object_array_filter(), which I think this was modeled after, I notice that this is much harder to remember how to use it correctly.

    void object_array_filter(struct object_array *array,
                             object_array_each_func_t want, void *cb_data)

It is clear to a potential user of this function who needs to implement the callback function what the function should return, because it calls it "want" (as opposed to "fn"). The function must answer "I want to keep this? Yes/no?".

Also polarity of oid_array_filter()'s callback is different. It retains elements for which "fn" returns false.

So one step of improvement may be to rename "fn" to "reject", but as we do not have any caller of this new function yet, perhaps match the polarity and call it "want"?

I think "x_filter" is perhaps trying to match other language's filter(), e.g. Python's

	>>> filter(lambda x: x % 2 == 0, range(0,10))
	[0, 2, 4, 6, 8]

so "say yes if you want to keep it" may be more familiar than "say yes if you want to filter it out".