Jeremiah Mahler <jmmahler@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html