[v3,1/1] abspath_part_inside_repo: respect core.fileMode

Commit Message

Linus Arver via GitGitGadget Dec. 24, 2018, 10:56 p.m. UTC

From: Johannes Schindelin <[e-mail]>

If the file system is case-insensitive, we really must be careful to
ignore differences in case only.

This fixes https://github.com/git-for-windows/git/issues/735

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 setup.c        | 6 +++---
 t/t3700-add.sh | 7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Dec. 25, 2018, 3:06 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> diff --git a/setup.c b/setup.c
> index 1be5037f12..291bfb2128 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -39,7 +39,7 @@ static int abspath_part_inside_repo(char *path)
>  	off = offset_1st_component(path);
>  
>  	/* check if work tree is already the prefix */
> -	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
> +	if (wtlen <= len && !fspathncmp(path, work_tree, wtlen)) {
>  		if (path[wtlen] == '/') {
>  			memmove(path, path + wtlen + 1, len - wtlen);
>  			return 0;
> @@ -59,7 +59,7 @@ static int abspath_part_inside_repo(char *path)
>  		path++;
>  		if (*path == '/') {
>  			*path = '\0';
> -			if (strcmp(real_path(path0), work_tree) == 0) {
> +			if (fspathcmp(real_path(path0), work_tree) == 0) {
>  				memmove(path0, path + 1, len - (path - path0));
>  				return 0;
>  			}
> @@ -68,7 +68,7 @@ static int abspath_part_inside_repo(char *path)
>  	}
>  
>  	/* check whole path */
> -	if (strcmp(real_path(path0), work_tree) == 0) {
> +	if (fspathcmp(real_path(path0), work_tree) == 0) {
>  		*path0 = '\0';
>  		return 0;
>  	}

So the idea is that the path to the top level of the working tree
must be compared with fspath[n]cmp() to what was given.  After
stripping that prefix, the caller uses the result just like it uses
a non-absolute path, which is why the necessary changes are isolated
to this function.

Makes sense.

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 37729ba258..be582a513b 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -402,4 +402,11 @@ test_expect_success 'all statuses changed in folder if . is given' '
>  	test $(git ls-files --stage | grep ^100755 | wc -l) -eq 0
>  '
>  
> +test_expect_success CASE_INSENSITIVE_FS 'path is case-insensitive' '
> +	path="$(pwd)/BLUB" &&
> +	touch "$path" &&
> +	downcased="$(echo "$path" | tr A-Z a-z)" &&
> +	git add "$downcased"
> +'

One problem with the above test is that it leaves it unspecified if
the resulting index entry is "blub" or "BLUB".  Shouldn't we verify
that "git add" adds an expected path to the index, instead of
blindly trusting that it says "Yeah, I did as I was told" with its
exit status?  Would we be adding 'blub' as that is what we told
'git' to add, or would it be 'BLUB' as that is what exists on the
filesystem that is case insensitive but case preserving?

On a project whose participants all are on case insensitive
filesystems, the above does not matter by definition, but once a
project wants to work with their case sensitive friends, it starts
to matter.

Other than that, looks good to me.

Thanks.


> +
>  test_done

Junio C Hamano Dec. 25, 2018, 8:46 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> the resulting index entry is "blub" or "BLUB".  Shouldn't we verify
> that "git add" adds an expected path to the index, instead of
> blindly trusting that it says "Yeah, I did as I was told" with its
> exit status?  Would we be adding 'blub' as that is what we told
> 'git' to add, or would it be 'BLUB' as that is what exists on the
> filesystem that is case insensitive but case preserving?

Needless to say, the last part of the above is a mere thetorical
question, and I am not questioning the established behaviour or
suggesting to "improve" it.  On a case insensitive filesystem, we
trust what readdir() gave us (but match them with pathspec case
insensitively) for a new path that is not in the index.  When we
update the contents of a path that is already in the index, we
preserve the case in the index, even when readdir() reports the same
path in different case (iow, we trust the case in the index more
than what readdir() gives us)..

What I am wondering in the above is if we should document that in
the test, perhaps with a simple

	git ls-files blub >actual &&
	echo BLUB >expect &&
	test_cmp expect actual

or something like that.

Johannes Schindelin Jan. 18, 2019, 12:55 p.m. UTC | #3

Hi Junio,

first of all, my apologies for the incorrect commit message (you missed
it, too, though): it is not about respecting core.fileMode, but about
core.ignoreCase.

On Tue, 25 Dec 2018, Junio C Hamano wrote:

> Junio C Hamano <[e-mail]> writes:
> 
> > the resulting index entry is "blub" or "BLUB".  Shouldn't we verify
> > that "git add" adds an expected path to the index, instead of
> > blindly trusting that it says "Yeah, I did as I was told" with its
> > exit status?  Would we be adding 'blub' as that is what we told
> > 'git' to add, or would it be 'BLUB' as that is what exists on the
> > filesystem that is case insensitive but case preserving?
> 
> Needless to say, the last part of the above is a mere thetorical
> question, and I am not questioning the established behaviour or
> suggesting to "improve" it.  On a case insensitive filesystem, we
> trust what readdir() gave us (but match them with pathspec case
> insensitively) for a new path that is not in the index.  When we
> update the contents of a path that is already in the index, we
> preserve the case in the index, even when readdir() reports the same
> path in different case (iow, we trust the case in the index more
> than what readdir() gives us)..
> 
> What I am wondering in the above is if we should document that in
> the test, perhaps with a simple
> 
> 	git ls-files blub >actual &&
> 	echo BLUB >expect &&
> 	test_cmp expect actual
> 
> or something like that.

Actually, I would interpret the description in our documentation to mean
that Git does not want to trust what readdir() gave us but heed what the
user said, i.e. when `BLUB` is added as `git add blub`, Git really should
think that it is `blub`, not `BLUB` (i.e. the exact opposite of what you
suggest). From Documentation/config/core.txt's section about
`core.ignoreCase`:

	For example, if a directory listing finds "makefile" when Git
	expects "Makefile", Git will assume it is really the same file,
	and continue to remember it as "Makefile".

Ciao,
Dscho

Patch

diff --git a/setup.c b/setup.c
index 1be5037f12..291bfb2128 100644
--- a/setup.c
+++ b/setup.c
@@ -39,7 +39,7 @@  static int abspath_part_inside_repo(char *path)
 	off = offset_1st_component(path);
 
 	/* check if work tree is already the prefix */
-	if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
+	if (wtlen <= len && !fspathncmp(path, work_tree, wtlen)) {
 		if (path[wtlen] == '/') {
 			memmove(path, path + wtlen + 1, len - wtlen);
 			return 0;
@@ -59,7 +59,7 @@  static int abspath_part_inside_repo(char *path)
 		path++;
 		if (*path == '/') {
 			*path = '\0';
-			if (strcmp(real_path(path0), work_tree) == 0) {
+			if (fspathcmp(real_path(path0), work_tree) == 0) {
 				memmove(path0, path + 1, len - (path - path0));
 				return 0;
 			}
@@ -68,7 +68,7 @@  static int abspath_part_inside_repo(char *path)
 	}
 
 	/* check whole path */
-	if (strcmp(real_path(path0), work_tree) == 0) {
+	if (fspathcmp(real_path(path0), work_tree) == 0) {
 		*path0 = '\0';
 		return 0;
 	}
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 37729ba258..be582a513b 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -402,4 +402,11 @@  test_expect_success 'all statuses changed in folder if . is given' '
 	test $(git ls-files --stage | grep ^100755 | wc -l) -eq 0
 '
 
+test_expect_success CASE_INSENSITIVE_FS 'path is case-insensitive' '
+	path="$(pwd)/BLUB" &&
+	touch "$path" &&
+	downcased="$(echo "$path" | tr A-Z a-z)" &&
+	git add "$downcased"
+'
+
 test_done