Junio, On Thu, Jun 19, 2014 at 11:03:03AM -0700, Junio C Hamano wrote: > 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) > 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@xxxxxxxxx http://github.com/jmahler -- 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