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

[Junio C Hamano <gitster@pobox.com>]

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)

The patch text looks good.

Thanks.