Re: [PATCH] apply: support case-only renames in case-insensitive filesystems

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 11, 2022, 19:17 UTC
Message-ID: <xmqqleu3au2n.fsf@gitster.g>
In-Reply-To: <pull.1257.git.1654967038802.gitgitgadget@gmail.com>

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> From: Tao Klerks <tao@klerks.biz>
>
> "git apply" checks, when validating a patch, to ensure that any files
> being added aren't already in the worktree.
>
> When this check runs on a case-only rename, in a case-insensitive
> filesystem, this leads to a false positive - the command fails with an
> error like:
> error: File1: already exists in working directory
>
> Fix this existence check to allow the file to exist, for a case-only
> rename when config core.ignorecase is set.

Hmph, close, but the patch as-posted may be fatally buggy, I think.

At the beginning of the function there is this block:

	const char *old_name = patch->old_name;
	const char *new_name = patch->new_name;
	const char *name = old_name ? old_name : new_name;

which makes us realize that old_name CAN legitimately be NULL. That is true for a creation patch. new_name can also be NULL for a deletion patch.

Show 5 quoted lines

>  	if ((tpatch = in_fn_table(state, new_name)) &&
>  	    (was_deleted(tpatch) || to_be_deleted(tpatch)))
>  		ok_if_exists = 1;
> +	else if (ignore_case && !strcasecmp(old_name, new_name))
> +		ok_if_exists = 1;

You'd get a segfault when the patch is creating a file at new_name, or deleting a file at old_name, wouldn't you?

We need a new test or two to see if a straight creation or deletion patch does work correctly with icase set, before we even dream of handling rename patches. Not having tests for such basic cases is quite surprising, but apparently the above line passed the CI.

>  	else
>  		ok_if_exists = 0;

Having said that, I wonder what the existing check before the new condition is doing? Especially, what is in_fn_table() for and how does it try to do its work?

Reading the big comment before it, it seems that it tries to deal with tricky delete/create case already. With a typechange patch that first removes a regular file "hello.txt" and then creates a symbolic link "hello.txt" is exempted from the "what you are creating should not exist already" rule by using in_fn_table() check. If it tries to create a symlink "Hello.txt" instead, shouldn't we allow it the same way on case-insensitive systems? I do not think in_fn_table() pays attention to "ignore_case" option, so there may be an existing bug there already, regardless of the problem you are trying to address with your patch.

And I wonder if doing case-insensitive match in in_fn_table() lets us cover this new case as well as "fixing" the existing issue.

In any case, here are such two tests to make sure creation and deletion patches on icase systems are not broken by careless mistakes like the one in this patch.

 t/t4114-apply-typechange.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git c/t/t4114-apply-typechange.sh w/t/t4114-apply-typechange.sh
index da3e64f811..e565ad8da1 100755
--- c/t/t4114-apply-typechange.sh
+++ w/t/t4114-apply-typechange.sh
@@ -126,4 +126,44 @@ test_expect_success 'directory becomes symlink' '
 test_debug 'cat patch'
 
 
+test_expect_success 'file becomes nothing' '
+	git checkout -f initial &&
+	test_when_finished "git reset --hard HEAD" &&
+
+	# prepare a patch to remove path "foo"
+	git rm --cached foo &&
+	git diff-index -p --cached HEAD >patch &&
+
+	# such a patch should apply cleanly to the index
+	git reset HEAD &&
+	git apply --cached patch &&
+
+	# and even with icase set.
+	git reset HEAD &&
+	git -c core.ignorecase=true apply --cached patch
+'
+
+test_debug 'cat patch'
+
+test_expect_success 'nothing becomes a file' '
+	git checkout -f initial &&
+	test_when_finished "git reset --hard HEAD" &&
+
+	# prepare a patch to add path "foo"
+	git rm --cached foo &&
+	git diff-index -p --cached -R HEAD >patch &&
+
+	# such a patch should apply cleanly to the index without "foo"
+	git reset HEAD &&
+	git rm --cached foo &&
+	git apply --cached patch &&
+
+	# and even with icase set.
+	git reset HEAD &&
+	git rm --cached foo &&
+	git -c core.ignorecase=true apply --cached patch
+'
+
+test_debug 'cat patch'
+
 test_done