[PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

Subsystems: the rest

STALE1803d

5 messages, 3 authors, 2021-08-30 · open the first message on its own page

[PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

From: René Scharfe <hidden>
Date: 2021-08-28 21:30:57

Call fspathcmp() instead of open-coding it.  This shortens the code and
makes it less repetitive.

Signed-off-by: René Scharfe <redacted>
---
 merge-recursive.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 3355d50e8a..840599fd53 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,
 	a = container_of(eptr, const struct path_hashmap_entry, e);
 	b = container_of(entry_or_key, const struct path_hashmap_entry, e);

-	if (ignore_case)
-		return strcasecmp(a->path, key ? key : b->path);
-	else
-		return strcmp(a->path, key ? key : b->path);
+	return fspathcmp(a->path, key ? key : b->path);
 }

 /*
--

2.33.0

Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

From: Taylor Blau <hidden>
Date: 2021-08-29 20:21:25

Hi René,

On Sat, Aug 28, 2021 at 11:30:49PM +0200, René Scharfe wrote:

quoted hunk

Call fspathcmp() instead of open-coding it.  This shortens the code and
makes it less repetitive.

Signed-off-by: René Scharfe <redacted>
---
 merge-recursive.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 3355d50e8a..840599fd53 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,
 	a = container_of(eptr, const struct path_hashmap_entry, e);
 	b = container_of(entry_or_key, const struct path_hashmap_entry, e);

-	if (ignore_case)
-		return strcasecmp(a->path, key ? key : b->path);
-	else
-		return strcmp(a->path, key ? key : b->path);
+	return fspathcmp(a->path, key ? key : b->path);
 }

Looks obviously right to me. I found another spot in
t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the
same way. But this looks fine with or without the following diff:

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 36ff07bd4b..ab34bdfecd 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c

@@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,
        e1 = container_of(eptr, const struct test_entry, ent);
        e2 = container_of(entry_or_key, const struct test_entry, ent);

-       if (ignore_case)
-               return strcasecmp(e1->key, key ? key : e2->key);
-       else
-               return strcmp(e1->key, key ? key : e2->key);
+       return fspathcmp(e1->key, key ? key : e2->key);
 }

 static struct test_entry *alloc_test_entry(unsigned int hash,

Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

From: Jeff King <hidden>
Date: 2021-08-29 21:00:21

On Sun, Aug 29, 2021 at 04:21:21PM -0400, Taylor Blau wrote:

quoted hunk

quoted

+++ b/merge-recursive.c

@@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,

[...]

Looks obviously right to me. I found another spot in
t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the
same way. But this looks fine with or without the following diff:

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 36ff07bd4b..ab34bdfecd 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c

@@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,
        e1 = container_of(eptr, const struct test_entry, ent);
        e2 = container_of(entry_or_key, const struct test_entry, ent);

-       if (ignore_case)
-               return strcasecmp(e1->key, key ? key : e2->key);
-       else
-               return strcmp(e1->key, key ? key : e2->key);
+       return fspathcmp(e1->key, key ? key : e2->key);
 }

 static struct test_entry *alloc_test_entry(unsigned int hash,

Maybe also:

diff --git a/dir.c b/dir.c
index 03c4d21267..ee46290cbb 100644
--- a/dir.c
+++ b/dir.c

@@ -669,9 +669,7 @@ int pl_hashmap_cmp(const void *unused_cmp_data,
 			 ? ee1->patternlen
 			 : ee2->patternlen;
 
-	if (ignore_case)
-		return strncasecmp(ee1->pattern, ee2->pattern, min_len);
-	return strncmp(ee1->pattern, ee2->pattern, min_len);
+	return fspathncmp(ee1->pattern, ee2->pattern, min_len);
 }
 
 static char *dup_and_filter_pattern(const char *pattern)

This is fun. :)

-Peff

Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

From: René Scharfe <hidden>
Date: 2021-08-30 15:09:44

Am 29.08.21 um 22:21 schrieb Taylor Blau:

quoted hunk

Hi René,

On Sat, Aug 28, 2021 at 11:30:49PM +0200, René Scharfe wrote:

quoted

Call fspathcmp() instead of open-coding it.  This shortens the code and
makes it less repetitive.

Signed-off-by: René Scharfe <redacted>
---
 merge-recursive.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 3355d50e8a..840599fd53 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,
 	a = container_of(eptr, const struct path_hashmap_entry, e);
 	b = container_of(entry_or_key, const struct path_hashmap_entry, e);

-	if (ignore_case)
-		return strcasecmp(a->path, key ? key : b->path);
-	else
-		return strcmp(a->path, key ? key : b->path);
+	return fspathcmp(a->path, key ? key : b->path);
 }

Looks obviously right to me. I found another spot in
t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the
same way. But this looks fine with or without the following diff:

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 36ff07bd4b..ab34bdfecd 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c

@@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,
        e1 = container_of(eptr, const struct test_entry, ent);
        e2 = container_of(entry_or_key, const struct test_entry, ent);

-       if (ignore_case)
-               return strcasecmp(e1->key, key ? key : e2->key);
-       else
-               return strcmp(e1->key, key ? key : e2->key);
+       return fspathcmp(e1->key, key ? key : e2->key);
 }

 static struct test_entry *alloc_test_entry(unsigned int hash,

That's a local variable named "ignore_case", not the one declared in
environment.c that fspathcmp() uses, so this would change the behavior.
The helper code does not include cache.h, so this is not even a case of
variable shadowing, just two different variables for similar purposes
in different places having the same name.

René

Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()

From: Jeff King <hidden>
Date: 2021-08-30 18:20:19

On Mon, Aug 30, 2021 at 05:09:34PM +0200, René Scharfe wrote:

quoted

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 36ff07bd4b..ab34bdfecd 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c

@@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,
        e1 = container_of(eptr, const struct test_entry, ent);
        e2 = container_of(entry_or_key, const struct test_entry, ent);

-       if (ignore_case)
-               return strcasecmp(e1->key, key ? key : e2->key);
-       else
-               return strcmp(e1->key, key ? key : e2->key);
+       return fspathcmp(e1->key, key ? key : e2->key);
 }

 static struct test_entry *alloc_test_entry(unsigned int hash,

That's a local variable named "ignore_case", not the one declared in
environment.c that fspathcmp() uses, so this would change the behavior.
The helper code does not include cache.h, so this is not even a case of
variable shadowing, just two different variables for similar purposes
in different places having the same name.

Yikes, good catch. Perhaps it's overkill, but I wonder if a comment
like:

  /*
   * Do not use fspathcmp() here; our behavior depends on the local
   * ignore_case variable, not the usual Git-wide global.
   */

would help.

I double-checked the spot I suggested. I think it is actually using the
global (though I got it right through sheer luck).

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help