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

[Jeremiah Mahler <jmmahler@gmail.com>]

Junio,

On Thu, Jun 19, 2014 at 11:03:03AM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > Both unpack-trees.c and read-cache.c have their own name_compare()
> > function, which are identical.  And read-cache.c has a
> > cache_name_compare() function which is nearly identical to
> > name_compare() [1].  The cache_name_compare() function is not specific
> > to a cache, other than by being part of cache.h.
> 
> 'other than by designed to be used only for comparing names in the
> cache entries' is probably more accurate, I would think.
> 
> > Generalize the cache_name_compare() function by renaming it to
> > name_compare().  Simplify the cache_name_stage_compare() function using
> > name_compare().  Then change the few instances which used
> > cache_name_compare() to name_compare() [2].
> >
> > [1] cache_name_compare() is not identical to name_compare().  The former
> >     returns +1, -1, whereas the latter returns +N, -N.  But there is no
> >     place where name_compare() is used that needs the magnitude so this
> >     difference does not alter its behavior.
> 
> 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?
> 
> > [2] The instances where cache_name_compare() is used have nothing to do
> >     with a cache.  The new name, name_compare(), makes it clear that no
> >     cache is involved.
> 
> This is redundant and should be dropped, as you already said "is not
> specific to a cache" earlier.
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  cache.h        |  2 +-
> >  dir.c          |  3 +--
> >  name-hash.c    |  2 +-
> >  read-cache.c   | 23 +++++++++++++----------
> >  tree-walk.c    | 10 ----------
> >  unpack-trees.c | 11 -----------
> >  6 files changed, 16 insertions(+), 35 deletions(-)
> >
> > diff --git a/cache.h b/cache.h
> > index c498a30..e3205fe 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1027,7 +1027,7 @@ extern int validate_headref(const char *ref);
> >  
> >  extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);
> >  extern int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);
> > -extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);
> > +extern int name_compare(const char *name1, size_t len1, const char *name2, size_t len2);
> >  extern int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2);
> >  
> >  extern void *read_object_with_reference(const unsigned char *sha1,
> > diff --git a/dir.c b/dir.c
> > index 797805d..e65888d 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -1354,8 +1354,7 @@ static int cmp_name(const void *p1, const void *p2)
> >  	const struct dir_entry *e1 = *(const struct dir_entry **)p1;
> >  	const struct dir_entry *e2 = *(const struct dir_entry **)p2;
> >  
> > -	return cache_name_compare(e1->name, e1->len,
> > -				  e2->name, e2->len);
> > +	return name_compare(e1->name, e1->len, e2->name, e2->len);
> >  }
> >  
> >  static struct path_simplify *create_simplify(const char **pathspec)
> > diff --git a/name-hash.c b/name-hash.c
> > index be7c4ae..e2bea88 100644
> > --- a/name-hash.c
> > +++ b/name-hash.c
> > @@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
> >  	 * Always do exact compare, even if we want a case-ignoring comparison;
> >  	 * we do the quick exact one first, because it will be the common case.
> >  	 */
> > -	if (len == namelen && !cache_name_compare(name, namelen, ce->name, len))
> > +	if (len == namelen && !name_compare(name, namelen, ce->name, len))
> >  		return 1;
> 
> The existing code is somewhat strange; while the update is correct
> in the context of this patch, it may further want to be fixed in a
> later patch to either
> 
> 	!name_compare(name, namelen, ce->name, len)
> 
> or
> 
> 	len == namelen && !memcmp(name, ce->name, len)
> 
I did not notice that, good catch.  Since that line is going to be
changed I can make a short fixup patch before the main patch and avoid
the rename.

> The patch text looks good.
> 
> Thanks.

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