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

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

[Jeremiah Mahler]

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

[PATCH v3 1/5] cache: rename cache_name_compare() to name_compare()

[Jeremiah Mahler]

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 <jmmahler@gmail.com>
---
 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

Re: [PATCH v3 1/5] cache: rename cache_name_compare() to name_compare()

[Jonathan Nieder]

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.

[PATCH v3 2/5] tree-walk.c: remove name_compare() function

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---

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

Re: [PATCH v3 2/5] tree-walk.c: remove name_compare() function

[Jonathan Nieder]

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

Re: [PATCH v3 2/5] tree-walk.c: remove name_compare() function

[Jeremiah Mahler]

Jonathan,

On Wed, Jun 18, 2014 at 12:03:59PM -0700, Jonathan Nieder wrote:
> 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.
> 
Makes sense. I will add it to the log message.

> Thanks,
> Jonathan

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

[PATCH v3 3/5] unpack-trees.c: remove name_compare() function

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---

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

Re: [PATCH v3 3/5] unpack-trees.c: remove name_compare() function

[Jonathan Nieder]

Jeremiah Mahler wrote:

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

Same thoughts as patch 2/5. :)

Thanks,
Jonathan

[PATCH v3 4/5] dir.c: rename to name_compare()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---

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

Re: [PATCH v3 4/5] dir.c: rename to name_compare()

[Jonathan Nieder]

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

[PATCH v3 5/5] name-hash.c: rename to name_compare()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 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

Re: [PATCH v3 5/5] name-hash.c: rename to name_compare()

[Jonathan Nieder]

Jeremiah Mahler wrote:

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

Same thoughts as patch 4/5.

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

[Jonathan Nieder]

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

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

[Jeremiah Mahler]

Jonathan,

On Wed, Jun 18, 2014 at 12:14:07PM -0700, Jonathan Nieder wrote:
> 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.
> 
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

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

[Jeff King]

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

> > 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.