Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2016, 17:08 UTC
Message-ID: <xmqqk2fzf69w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1608021244450.79248@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 8 quoted lines

>> 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.
>
> Maybe.
>
> But to make sure: you do not expect Kevin to do that in the context of
> this here patch series, right?

I think I already answered this in the message you are responding to.

> 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)
        {
                ...