threads / patch / 36952

v3, 5 partscleanup duplicate name_compare() functions

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

## tl;dr

15 messages between Jun 18, 2014 and Jun 19, 2014. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Jeremiah Mahler· Jun 18, 2014, 18:45 UTC · lore

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
Jeremiah Mahler· Jun 18, 2014, 18:45 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to 2 files +14 −11

cache.h, read-cache.c

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
Jonathan Nieder· Jun 18, 2014, 19:11 UTC · re: Jeremiah Mahler · lore

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

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· Jun 18, 2014, 18:45 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to tree-walk.c +0 −10
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
Jonathan Nieder· Jun 18, 2014, 19:03 UTC · re: Jeremiah Mahler · lore

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

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.
Show 6 quoted lines
> ---
> 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· Jun 19, 2014, 08:04 UTC · re: Jonathan Nieder · lore

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

Jonathan,
On Wed, Jun 18, 2014 at 12:03:59PM -0700, Jonathan Nieder wrote:
Show 17 quoted lines
> 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
Jeremiah Mahler· Jun 18, 2014, 18:45 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to unpack-trees.c +0 −11
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
Jeremiah Mahler· Jun 18, 2014, 18:45 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to dir.c +1 −2
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
Jonathan Nieder· Jun 18, 2014, 19:09 UTC · re: Jeremiah Mahler · lore

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

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· Jun 18, 2014, 18:45 UTC · re: Jeremiah Mahler · lore

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

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(-)
Show changes to name-hash.c +1 −1
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
Jonathan Nieder· Jun 18, 2014, 19:14 UTC · re: Jeremiah Mahler · lore

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

Jeremiah Mahler wrote:
Show 14 quoted lines
> 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

Jeremiah Mahler· Jun 19, 2014, 08:04 UTC · re: Jonathan Nieder · lore

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

Jonathan,
On Wed, Jun 18, 2014 at 12:14:07PM -0700, Jonathan Nieder wrote:
Show 22 quoted lines
> 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
Jeff King· Jun 19, 2014, 08:54 UTC · re: Jeremiah Mahler · lore

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

On Thu, Jun 19, 2014 at 01:04:32AM -0700, Jeremiah Mahler wrote:
Show 7 quoted lines
> > 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.

← back to recent threads