Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> Hi Junio,
>
> On Fri, 29 Jul 2016, Junio C Hamano wrote:
>
>> Kevin Willford <kcwillford@xxxxxxxxx> writes:
>>
>> > static int patch_id_cmp(struct patch_id *a,
>> > struct patch_id *b,
>> > - void *keydata)
>> > + struct diff_options *opt)
>> > {
>> > + if (is_null_sha1(a->patch_id) &&
>> > + commit_patch_id(a->commit, opt, a->patch_id, 0))
>> > + return error("Could not get patch ID for %s",
>> > + oid_to_hex(&a->commit->object.oid));
>> > + if (is_null_sha1(b->patch_id) &&
>> > + commit_patch_id(b->commit, opt, b->patch_id, 0))
>> > + return error("Could not get patch ID for %s",
>> > + oid_to_hex(&b->commit->object.oid));
>> > return hashcmp(a->patch_id, b->patch_id);
>> > }
>>
>> These error returns initially looks slightly iffy in that in general
>> the caller of any_cmp_fn() wants to know how a/b compares, but by
>> returning error(), it always says "a is smaller than b".
>
> I am to blame, as this is my design.
>
> And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
> and >0 for comparisons, when hashmaps try to avoid any order.
Perhaps hashmap API needs fixing in the longer term not to call this
type hashmap_cmp_fn; instead it should lose cmp and say something
like hashmap_eq_fn or something.
> Do you want a note in the commit message about this "abuse" of a negative
> return value, or a code comment?
I do not think negative (or non-zero) return is an "abuse" at all.
It is misleading in the context of the function whose name has "cmp"
in it, but that is not the fault of this function, rather, the
breakage is more in the API that calls a function that wants to know
only equality a "cmp". A in-code comment before the function name
may be appropriate:
/*
* hashmap API calls hashmap_cmp_fn, but it only wants
* "does the key match the entry?" with 0 (matches) and
* non-zero (does not match).
*/
static int patch_id_match(const struct patch_id *ent,
const struct patch_id *key,
const void *keydata)
{
...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html