Re: [PATCH v3 1/1] abspath_part_inside_repo: respect core.fileMode

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 25, 2018, 03:06 UTC
Message-ID: <xmqqftumb8zv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <b935e11d21fc2a34953d1fc651ea09f1a4c1a769.1545692162.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 31 quoted lines

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

Show 14 quoted lines

> 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