Re: [PATCH v4] cleanup duplicate name_compare() functions

[Jeremiah Mahler <jmmahler@gmail.com>]

Junio,

On Thu, Jun 19, 2014 at 11:29:21AM -0700, Junio C Hamano wrote:
> On Thu, Jun 19, 2014 at 11:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > You chose to use the one that loses the information by unifying
> > these two into the variant that only returns -1/0/+1.  We know that
> > it does not matter for the current callers, but is it expected that
> > no future callers will benefit by having the magnitude information?
> 
> Heh, I was being silly, partly fooled by your reference to
> "magnitude".
> 
> You are not losing information at all, because the caller cannot
> tell if the return value came from an earlier memcmp(), whose only
> guarantee is that the sign of the returned value is all that
> matters, or from the later subtraction between lengths.
> 
> So unifying to the -1/0/+1 variant is entirely justifiable.  It is
> just your rationale was a bit misleading.
> 
>     We often represent our strings as a counted string, i.e. a pair of
>     the pointer to the beginning of the string and its length, and the
>     string may not be NUL terminated to that length.
> 
>     To compare a pair of such counted strings, unpack-trees.c and
>     read-cache.c implement their own name_compare() functions
>     identically.  In addition, cache_name_compare() function in
>     read-cache.c is nearly identical.  The only difference is when one
>     string is the prefix of the other string, in which case the former
>     returns -1/+1 to show which one is longer and the latter returns the
>     difference of the lengths to show the same information.
> 
>     Unify these three functions by using the implementation from
>     cache_name_compare().  This does not make any difference to the
>     existing and future callers, as they must be paying attention only
>     to the sign of the returned value (and not the magnitude) because
>     the original implementations of these two functions return values
>     returned by memcmp(3) when the one string is not a prefix of the
>     other string, and the only thing memcmp(3) guarantees its callers is
>     the sign of the returned value, not the magnitude.
> 
> or something like that, perhaps?

Yes, that looks good.  It is a bit clearer than my message.  I like how
you used "the prefix of the other string" to describe when the two
functions behave differently.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler