From: Tao Klerks <tao@klerks.biz>
To: Junio C Hamano <gitster@pobox.com>
Cc: Tao Klerks via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH] apply: support case-only renames in case-insensitive filesystems
Date: Tue, 14 Jun 2022 07:13:31 +0200 [thread overview]
Message-ID: <CAPMMpojdnAMnczJAevqL8GSOb8gvddcSiYfbz0c51oPpn4U0wA@mail.gmail.com> (raw)
In-Reply-To: <xmqqleu3au2n.fsf@gitster.g>
On Sat, Jun 11, 2022 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.
>
Yes indeed, very much so.
> 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.
>
Yep, I was aware of the nulls, but I was unaware that passing nulls
into "strcasecmp()" was a bad thing to do. I just assumed a non-zero
comparison result would ensue.
> > 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?
>
Indeed you do (when ignorecase is true of course).
> 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.
>
This is where I made some very bad assumptions: I only manually ran
the new "t4141-apply-case-insensitive-rename.sh" test, and assumed
that the test suite ran against linux, windows, and OSX, with the
latter two running on case-insensitive filesystems. I assumed that
both case-sensitive and case-insensitive code paths would be tested by
the complete CI suite.
The OSX tests were not running for me at all in GitGitGadget (seems to
be an ongoing struggle), but I assumed that everything was still
tested in case-insensitive mode because of the windows suite. It looks
like that was wrong, although I still don´t know how/why.
Had I run "t4114-apply-typechange.sh" (or probably some others in the
41XX range) on the OSX environment where I happen to have developed
this weekend, I would have seen the failures immediately.
> > 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.
>
Yep, I confirmed that as you expect, it does fix the issue I set out
to fix, and as you noted also fixes other (slightly more obscure?)
existing issues with "git apply" on case-insensitive filesystems.
This time I tested all of t41XX on a case-insensitive system, and the
CI process ran in GitLab, presumably on case-sensitive filesystems
only.
I'm not sure what more to look out for, and will note as much in the
patch v2 comments.
> 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.
I have a question related to this:
*Do* we expect to run the full test suite on case-insensitive systems
in gitlab, or do we expect to need to add explicit "-C
core.ignorecase" tests as you have done here? The latter seems risky
both because the behavior is not representatively tested (because it's
still actually running in a case-sensitive filesystem), and because
it's hard to predict all the things that should be explicitly retested
in this way.
I don't think these specific tests were necessary, and I guess they
are replaced by later ones in this thread, so I will ignore this bit
specifically.
Thanks for the careful review, my apologies for the careless patch.
next prev parent reply other threads:[~2022-06-14 5:13 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
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 [this message]
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=CAPMMpojdnAMnczJAevqL8GSOb8gvddcSiYfbz0c51oPpn4U0wA@mail.gmail.com \
--to=tao@klerks.biz \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/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).