git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 19, 2014, 18:03 UTC
Message-ID
<xmqqionwhj54.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1403165242-16849-2-git-send-email-jmmahler@gmail.com>
Jeremiah Mahler <jmmahler@gmail.com> writes:
Show 5 quoted lines
> 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.

Show 9 quoted lines
> 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.

Show 48 quoted lines
> 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.
Previous: Jeremiah MahlerNext: Junio C Hamano
Message 3 of 6 in “cleanup duplicate name_compare() functions”
  1. cleanup duplicate name_compare() functionsJeremiah Mahler, Jun 19, 2014
  2. cleanup duplicate name_compare() functionsJeremiah Mahler, Jun 19, 2014
  3. Junio C HamanoJun 19, 2014
  4. Junio C HamanoJun 19, 2014
  5. Jeremiah MahlerJun 20, 2014
  6. Jeremiah MahlerJun 19, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.