[PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

Subsystems: the rest

STALE3139d

6 messages, 4 authors, 2018-02-09 · open the first message on its own page

[PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

From: Ben Peart <hidden>
Date: 2018-02-08 00:42:22

Correct the pointer arithmetic in adjust_dirname_case() so that it calls
find_dir_entry() with the correct string length.  Previously passing in
"dir1/foo" would pass a length of 6 instead of the correct 4.  This resulted in
find_dir_entry() never finding the entry and so the subsequent memcpy that would
fold the name to the version with the correct case never executed.

Add a test to validate the corrected behavior with name folding of directories.

Signed-off-by: Ben Peart <redacted>
---

Notes:
    Base Ref: v2.16.1
    Web-Diff: https://github.com/benpeart/git/commit/2944970f4e
    Checkout: git fetch https://github.com/benpeart/git adjust_dirname-v1 && git checkout 2944970f4e

 name-hash.c           |  6 +++---
 t/t0050-filesystem.sh | 12 +++++++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/name-hash.c b/name-hash.c
index 45c98db0a0..2ddbb72647 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -696,12 +696,12 @@ void adjust_dirname_case(struct index_state *istate, char *name)
 		if (*ptr == '/') {
 			struct dir_entry *dir;
 
-			ptr++;
-			dir = find_dir_entry(istate, name, ptr - name + 1);
+			dir = find_dir_entry(istate, name, ptr - name);
 			if (dir) {
 				memcpy((void *)startPtr, dir->name + (startPtr - name), ptr - startPtr);
-				startPtr = ptr;
+				startPtr = ptr + 1;
 			}
+			ptr++;
 		}
 	}
 }
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..219c96594c 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '
 	git merge topic
 '
 
-
+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+	git reset --hard initial &&
+	mkdir -p dir1 &&
+	mkdir -p dir1/dir2 &&
+	touch dir1/dir2/a &&
+	touch dir1/dir2/b &&
+	git add dir1/dir2/a &&
+	git add dir1/DIR2/b &&
+	camel=$(git ls-files | grep dir2) &&
+	test $(echo "$camel" | wc -l) = 2
+'
 
 test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
 	git reset --hard initial &&
base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa
-- 
2.15.0.windows.1

Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

From: David Turner <hidden>
Date: 2018-02-08 01:02:39

On Wed, 2018-02-07 at 19:41 -0500, Ben Peart wrote:
Correct the pointer arithmetic in adjust_dirname_case() so that it
calls
find_dir_entry() with the correct string length.  Previously passing
in
"dir1/foo" would pass a length of 6 instead of the correct 4.  This
resulted in
find_dir_entry() never finding the entry and so the subsequent memcpy
that would
fold the name to the version with the correct case never executed.

Add a test to validate the corrected behavior with name folding of
directories.
Looks good to me.

Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

From: Torsten Bögershausen <hidden>
Date: 2018-02-08 17:22:10

On Wed, Feb 07, 2018 at 07:41:56PM -0500, Ben Peart wrote:
[]
quoted hunk
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..219c96594c 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '
 	git merge topic
 '
 
-
+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+	git reset --hard initial &&
+	mkdir -p dir1 &&
+	mkdir -p dir1/dir2 &&
+	touch dir1/dir2/a &&
+	touch dir1/dir2/b &&
+	git add dir1/dir2/a &&
+	git add dir1/DIR2/b &&
+	camel=$(git ls-files | grep dir2) &&
+	test $(echo "$camel" | wc -l) = 2
+'
 
There is nothing wrong with with "wc -l", but:
a more new-style would probably use test_line_count() here.

My personal favorite would be to spell out what we expect and run a diff.
When it fails, we can see what fails, and the function would look
like this:


test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
	git reset --hard initial &&
	mkdir -p dir1 &&
	mkdir -p dir1/dir2 &&
	touch dir1/dir2/a &&
	touch dir1/dir2/b &&
	git add dir1/dir2/a &&
	git add dir1/DIR2/b &&
	git ls-files | grep dir2 | sort >actual &&
	cat >expected <<-\EOF &&
	dir1/dir2/a
	dir1/dir2/b
	EOF
	test_cmp expected actual
'


Re: [PATCH v1] name-hash: properly fold directory names in adjust_dirname_case()

From: Ben Peart <hidden>
Date: 2018-02-08 17:45:26


On 2/8/2018 12:21 PM, Torsten Bögershausen wrote:
On Wed, Feb 07, 2018 at 07:41:56PM -0500, Ben Peart wrote:
[]
quoted
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..219c96594c 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -80,7 +80,17 @@ test_expect_success 'merge (case change)' '
  	git merge topic
  '
  
-
+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+	git reset --hard initial &&
+	mkdir -p dir1 &&
+	mkdir -p dir1/dir2 &&
+	touch dir1/dir2/a &&
+	touch dir1/dir2/b &&
+	git add dir1/dir2/a &&
+	git add dir1/DIR2/b &&
+	camel=$(git ls-files | grep dir2) &&
+	test $(echo "$camel" | wc -l) = 2
+'
  
There is nothing wrong with with "wc -l", but:
a more new-style would probably use test_line_count() here.

My personal favorite would be to spell out what we expect and run a diff.
When it fails, we can see what fails, and the function would look
like this:
I agree with you completely that this is a better format and is easier 
to read.  All the new tests I've been writing follow this same pattern.

In this particular test file, I opted (for better and for worse) to 
stick with the style of all the other tests rather than 1) have this one 
test be very different than all the others or 2) rewriting all the 
existing tests in the new style.
test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
	git reset --hard initial &&
	mkdir -p dir1 &&
	mkdir -p dir1/dir2 &&
	touch dir1/dir2/a &&
	touch dir1/dir2/b &&
	git add dir1/dir2/a &&
	git add dir1/DIR2/b &&
	git ls-files | grep dir2 | sort >actual &&
	cat >expected <<-\EOF &&
	dir1/dir2/a
	dir1/dir2/b
	EOF
	test_cmp expected actual
'

[PATCH v2] name-hash: properly fold directory names in adjust_dirname_case()

From: Ben Peart <hidden>
Date: 2018-02-08 19:24:05

Correct the pointer arithmetic in adjust_dirname_case() so that it calls
find_dir_entry() with the correct string length.  Previously passing in
"dir1/foo" would pass a length of 6 instead of the correct 4.  This resulted in
find_dir_entry() never finding the entry and so the subsequent memcpy that would
fold the name to the version with the correct case never executed.

Add a test to validate the corrected behavior with name folding of directories.

Signed-off-by: Ben Peart <redacted>
---

Notes:
    Base Ref: v2.16.1
    Web-Diff: https://github.com/benpeart/git/commit/477da4602c
    Checkout: git fetch https://github.com/benpeart/git adjust_dirname-v2 && git checkout 477da4602c
    
    ### Interdiff (v1..v2):
    
    diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
    index 219c96594c..0e4e51b79a 100755
    --- a/t/t0050-filesystem.sh
    +++ b/t/t0050-filesystem.sh
    @@ -82,14 +82,18 @@ test_expect_success 'merge (case change)' '
    
     test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
     	git reset --hard initial &&
    -	mkdir -p dir1 &&
     	mkdir -p dir1/dir2 &&
    -	touch dir1/dir2/a &&
    -	touch dir1/dir2/b &&
    +	echo > dir1/dir2/a &&
    +	echo > dir1/dir2/b &&
     	git add dir1/dir2/a &&
     	git add dir1/DIR2/b &&
    -	camel=$(git ls-files | grep dir2) &&
    -	test $(echo "$camel" | wc -l) = 2
    +	git ls-files >actual &&
    +	cat >expected <<-\EOF &&
    +		camelcase
    +		dir1/dir2/a
    +		dir1/dir2/b
    +	EOF
    +	test_cmp expected actual
     '
    
     test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
    
    ### Patches

 name-hash.c           |  6 +++---
 t/t0050-filesystem.sh | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/name-hash.c b/name-hash.c
index 45c98db0a0..2ddbb72647 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -696,12 +696,12 @@ void adjust_dirname_case(struct index_state *istate, char *name)
 		if (*ptr == '/') {
 			struct dir_entry *dir;
 
-			ptr++;
-			dir = find_dir_entry(istate, name, ptr - name + 1);
+			dir = find_dir_entry(istate, name, ptr - name);
 			if (dir) {
 				memcpy((void *)startPtr, dir->name + (startPtr - name), ptr - startPtr);
-				startPtr = ptr;
+				startPtr = ptr + 1;
 			}
+			ptr++;
 		}
 	}
 }
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..0e4e51b79a 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -80,7 +80,21 @@ test_expect_success 'merge (case change)' '
 	git merge topic
 '
 
-
+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+	git reset --hard initial &&
+	mkdir -p dir1/dir2 &&
+	echo > dir1/dir2/a &&
+	echo > dir1/dir2/b &&
+	git add dir1/dir2/a &&
+	git add dir1/DIR2/b &&
+	git ls-files >actual &&
+	cat >expected <<-\EOF &&
+		camelcase
+		dir1/dir2/a
+		dir1/dir2/b
+	EOF
+	test_cmp expected actual
+'
 
 test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
 	git reset --hard initial &&
base-commit: 8279ed033f703d4115bee620dccd32a9ec94d9aa
-- 
2.15.0.windows.1

Re: [PATCH v2] name-hash: properly fold directory names in adjust_dirname_case()

From: Torsten Bögershausen <hidden>
Date: 2018-02-09 05:43:48

On Thu, Feb 08, 2018 at 02:23:33PM -0500, Ben Peart wrote:

[]
-
+test_expect_success CASE_INSENSITIVE_FS 'add directory (with different case)' '
+	git reset --hard initial &&
+	mkdir -p dir1/dir2 &&
+	echo > dir1/dir2/a &&
+	echo > dir1/dir2/b &&
Thanks for working on this-
One and a half style nits:
The ">" should be close to the file name:
	echo >dir1/dir2/a &&
But then we don't even look into the file, so that the "\" produced by echo is not needed.
A simple 
	>dir1/dir2/a &&
is all that is needed.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help