[PATCH v3 0/5] cleanup duplicate name_compare() functions

Version 3 of the patch series to cleanup duplicate name_compare()
functions (previously was 'add strnncmp() function' [1]).  

This version goes in a slightly different direction than the previous
version.  Before I was trying to add a strnncmp() function so I could
remove duplicate copies of the name_compare() function in tree-walk.c
and unpack-trees.c.  But then Torsten Bögershausen pointed out that
there is a cache_name_compare() function which is nearly identical to
name_compare() [2]*.

* 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() was used that needed the magnitude so this
  change would not alter its behavior.

So I decided why not generalize the name of cache_name_compare() by
renaming it to  name_compare(), since it doesn't do anything with
caches, other than being part of cache.h and read-cache.c.  Then the
duplicate name_compare() functions can be removed and the few places
that used cache_name_compare() can be renamed to name_compare().

It cleans up the code with a minimal number of changes.  It keeps
existing functions instead of creating new ones.  And there are several
other functions in cache.h that are similarly named '*name_compare' so
it follows the already established style.

Also, the name_compare() now uses memcmp() as it did originally instead
of using strncmp() as it did in the last version.

[1]: http://marc.info/?l=git&m=140299051431479&w=2

[2]: http://marc.info/?l=git&m=140300329403706&w=2

Jeremiah Mahler (5):
  cache: rename cache_name_compare() to name_compare()
  tree-walk.c: remove name_compare() function
  unpack-trees.c: remove name_compare() function
  dir.c: rename to name_compare()
  name-hash.c: rename to name_compare()

 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(-)

-- 
2.0.0

The cache_name_compare() function is not specific to a cache.
Make its name more general by renaming it to name_compare().

Simplify cache_name_stage_compare() via name_compare().
Where lengths are involved, change int to size_t.

Signed-off-by: Jeremiah Mahler <redacted>
---
 cache.h      |  2 +-
 read-cache.c | 23 +++++++++++++----------
 2 files changed, 14 insertions(+), 11 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/read-cache.c b/read-cache.c
index 9f56d76..158241d 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -434,18 +434,26 @@ int df_name_compare(const char *name1, int len1, int mode1,
 	return c1 - c2;
 }
 
-int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2)
+int name_compare(const char *name1, size_t len1, const char *name2, size_t len2)
 {
-	int len = len1 < len2 ? len1 : len2;
-	int cmp;
-
-	cmp = memcmp(name1, name2, len);
+	size_t min_len = (len1 < len2) ? len1 : len2;
+	int cmp = memcmp(name1, name2, min_len);
 	if (cmp)
 		return cmp;
 	if (len1 < len2)
 		return -1;
 	if (len1 > len2)
 		return 1;
+	return 0;
+}
+
+int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2)
+{
+	int cmp;
+
+	cmp = name_compare(name1, len1, name2, len2);
+	if (cmp)
+		return cmp;
 
 	if (stage1 < stage2)
 		return -1;

@@ -454,11 +462,6 @@ int cache_name_stage_compare(const char *name1, int len1, int stage1, const char
 	return 0;
 }
 
-int cache_name_compare(const char *name1, int len1, const char *name2, int len2)
-{
-	return cache_name_stage_compare(name1, len1, 0, name2, len2, 0);
-}
-
 static int index_name_stage_pos(const struct index_state *istate, const char *name, int namelen, int stage)
 {
 	int first, last;

-- 
2.0.0

Rename the call to cache_name_compare() to name_compare().

Signed-off-by: Jeremiah Mahler <redacted>
---

Notes:
    This is a case where cache_name_compare() was used even though it had
    nothing to do with a cache.  The new name makes it clear that no cache
    is involved.

 dir.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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)

-- 
2.0.0

Rename the call to cache_name_compare() to name_compare().

Signed-off-by: Jeremiah Mahler <redacted>
---
 name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
 
 	if (!icase)

-- 
2.0.0

Remove the duplicate name_compare() function and use the one provided by
read-cache.c.

Signed-off-by: Jeremiah Mahler <redacted>
---

Notes:
    There is one small difference between the old function and the new one.
    The old one returned -N and +N whereas the new one returns -1 and +1.
    However, there is no place where the magnitude was needed, so this
    change will not alter its behavior.

 unpack-trees.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 4a9cdf2..c4a97ca 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -629,17 +629,6 @@ static int unpack_failed(struct unpack_trees_options *o, const char *message)
 	return -1;
 }
 
-/* NEEDSWORK: give this a better name and share with tree-walk.c */
-static int name_compare(const char *a, int a_len,
-			const char *b, int b_len)
-{
-	int len = (a_len < b_len) ? a_len : b_len;
-	int cmp = memcmp(a, b, len);
-	if (cmp)
-		return cmp;
-	return (a_len - b_len);
-}
-
 /*
  * The tree traversal is looking at name p.  If we have a matching entry,
  * return it.  If name p is a directory in the index, do not return

-- 
2.0.0

Remove the duplicate name_compare() function and use the one provided by
read-cache.c.

Signed-off-by: Jeremiah Mahler <redacted>
---

Notes:
    There is one small difference between the old function and the new one.
    The old one returned -N and +N whereas the new one returns -1 and +1.
    However, there is no place where the magnitude was needed, so this
    change will not alter its behavior.

 tree-walk.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 4dc86c7..5dd9a71 100644
--- a/tree-walk.c
+++ b/tree-walk.c

@@ -144,16 +144,6 @@ struct tree_desc_x {
 	struct tree_desc_skip *skip;
 };
 
-static int name_compare(const char *a, int a_len,
-			const char *b, int b_len)
-{
-	int len = (a_len < b_len) ? a_len : b_len;
-	int cmp = memcmp(a, b, len);
-	if (cmp)
-		return cmp;
-	return (a_len - b_len);
-}
-
 static int check_entry_match(const char *a, int a_len, const char *b, int b_len)
 {
 	/*

-- 
2.0.0

Jeremiah Mahler wrote:

Remove the duplicate name_compare() function and use the one provided by
read-cache.c.

I'd squash this into patch 1/5.

---
Notes:
    There is one small difference between the old function and the new one.
    The old one returned -N and +N whereas the new one returns -1 and +1.
    However, there is no place where the magnitude was needed, so this
    change will not alter its behavior.

This is useful information for anyone looking back at the patch in the
future, so it belongs above the three-dash divider.

Thanks,
Jonathan

Jeremiah Mahler wrote:

 unpack-trees.c | 11 -----------
 1 file changed, 11 deletions(-)

Same thoughts as patch 2/5. :)

Thanks,
Jonathan

Jeremiah Mahler wrote:

    This is a case where cache_name_compare() was used even though it had
    nothing to do with a cache.  The new name makes it clear that no cache
    is involved.

That's a perfect sort of thing to put in the commit message. ;-)

Unlike patches 2 and 3, this could make sense to me as a separate
patch from 1/5.  Except... how does git work at all with patch 1 and
without this patch?  I thought that patch removed the public
cache_name_compare function.

Would it make sense to delay the removal of cache_name_compare until a
patch at the end of the series?

The patch is small enough that squashing into patch 1 seems fine, too.

[...]

Rename the call to cache_name_compare() to name_compare().

It's not actually renaming but calling a different function, right?
So I'd say something like

	read_directory: use name_compare instead of cache_name_compare

	This is a case where cache_name_compare() was used even though it had
	nothing to do with a cache.  The new name makes it clear that no cache
	is involved.

	No functional change intended.

Thanks,
Jonathan

Jeremiah Mahler wrote:

 name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Same thoughts as patch 4/5.

Jeremiah Mahler wrote:

The cache_name_compare() function is not specific to a cache.
Make its name more general by renaming it to name_compare().

Sounds reasonable.

Jeremiah Mahler wrote:

Jeremiah Mahler (5):
  cache: rename cache_name_compare() to name_compare()
  tree-walk.c: remove name_compare() function
  unpack-trees.c: remove name_compare() function
  dir.c: rename to name_compare()
  name-hash.c: rename to name_compare()

 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(-)

After looking at the patches I suspect this should be a single patch.
That way it's bisectable, and the changes outside of read-cache.c are
small enough that it's not too much of a burden to review as a single
patch.

The code change looked good.

Thanks and hope that helps,
Jonathan

Jonathan,

On Wed, Jun 18, 2014 at 12:03:59PM -0700, Jonathan Nieder wrote:

Jeremiah Mahler wrote:

quoted

Remove the duplicate name_compare() function and use the one provided by
read-cache.c.

I'd squash this into patch 1/5.

quoted

---
Notes:
    There is one small difference between the old function and the new one.
    The old one returned -N and +N whereas the new one returns -1 and +1.
    However, there is no place where the magnitude was needed, so this
    change will not alter its behavior.

This is useful information for anyone looking back at the patch in the
future, so it belongs above the three-dash divider.

Makes sense. I will add it to the log message.

Thanks,
Jonathan

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

Jonathan,

On Wed, Jun 18, 2014 at 12:14:07PM -0700, Jonathan Nieder wrote:

Jeremiah Mahler wrote:

quoted

Jeremiah Mahler (5):
  cache: rename cache_name_compare() to name_compare()
  tree-walk.c: remove name_compare() function
  unpack-trees.c: remove name_compare() function
  dir.c: rename to name_compare()
  name-hash.c: rename to name_compare()

 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(-)

After looking at the patches I suspect this should be a single patch.
That way it's bisectable, and the changes outside of read-cache.c are
small enough that it's not too much of a burden to review as a single
patch.

That would be a pain to bisect if the partial application of the patch
set left the system in a broken state.  Good suggestion.

The code change looked good.

Thanks and hope that helps,
Jonathan

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

On Thu, Jun 19, 2014 at 01:04:32AM -0700, Jeremiah Mahler wrote:

quoted

After looking at the patches I suspect this should be a single patch.
That way it's bisectable, and the changes outside of read-cache.c are
small enough that it's not too much of a burden to review as a single
patch.

That would be a pain to bisect if the partial application of the patch
set left the system in a broken state.  Good suggestion.

One trick I use, especially when refactoring, is to use an interactive
rebase to test each commit in isolation, like:

  GIT_EDITOR='sed -i "/^pick .*/aexec make -j8 test"' git rebase -i

After picking each commit, that will run the tests on each one[1]. If it
fails, the rebase will pause. You can fix any problems, test to your
satisfaction, "commit --amend", and then "rebase --continue" to keep
going.

-Peff

[1] Of course it can be rather time-consuming for a large series. I
    often just compile-test at first, and then do a final "make test"
    pass when I think everything is right.