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

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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)
> >         {
> >                 ...
> 
> How about this one instead (to be squashed into 4/4)?
> 
> The updated wording directly addresses the puzzlement I initially
> felt "This returns error() which is always negative, so comparing
> (A, B) would say A < B, while comparing (B, A) would say B < A.
> Would it cause a problem in the caller?" while reading the function
> by being explicit that the sign does not matter.