From: Junio C Hamano <gitster@pobox.com>
To: "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Tao Klerks <tao@klerks.biz>
Subject: Re: [PATCH] apply: support case-only renames in case-insensitive filesystems
Date: Sat, 11 Jun 2022 12:17:20 -0700 [thread overview]
Message-ID: <xmqqleu3au2n.fsf@gitster.g> (raw)
In-Reply-To: <pull.1257.git.1654967038802.gitgitgadget@gmail.com> (Tao Klerks via GitGitGadget's message of "Sat, 11 Jun 2022 17:03:58 +0000")
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
> 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
next prev parent reply other threads:[~2022-06-11 19:17 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-11 17:03 [PATCH] apply: support case-only renames in case-insensitive filesystems Tao Klerks via GitGitGadget
2022-06-11 19:17 ` Junio C Hamano [this message]
2022-06-12 23:35 ` Junio C Hamano
2022-06-14 6:22 ` Tao Klerks
2022-06-15 11:24 ` Tao Klerks
2022-06-14 5:13 ` Tao Klerks
2022-06-18 0:45 ` Junio C Hamano
2022-06-18 15:34 ` Tao Klerks
2022-06-12 23:30 ` Junio C Hamano
2022-06-13 18:12 ` Junio C Hamano
2022-06-14 6:26 ` Tao Klerks
2022-06-14 6:16 ` Tao Klerks
2022-06-19 16:10 ` [PATCH v2 0/3] RFC: " Tao Klerks via GitGitGadget
2022-06-19 16:10 ` [PATCH v2 1/3] t4141: test "git apply" with core.ignorecase Junio C Hamano via GitGitGadget
2022-06-19 16:10 ` [PATCH v2 2/3] reset: new failing test for reset of case-insensitive duplicate in index Tao Klerks via GitGitGadget
2022-06-19 16:10 ` [PATCH v2 3/3] apply: support case-only renames in case-insensitive filesystems Tao Klerks via GitGitGadget
2022-10-10 4:09 ` [PATCH v2 0/3] RFC: " Tao Klerks
2023-05-28 9:59 ` [PATCH v3 0/3] " Tao Klerks via GitGitGadget
2023-05-28 9:59 ` [PATCH v3 1/3] t4142: test "git apply" with core.ignorecase Junio C Hamano via GitGitGadget
2023-05-28 9:59 ` [PATCH v3 2/3] reset: new failing test for reset of case-insensitive duplicate in index Tao Klerks via GitGitGadget
2023-05-28 9:59 ` [PATCH v3 3/3] apply: support case-only renames in case-insensitive filesystems Tao Klerks via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqleu3au2n.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=tao@klerks.biz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).