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

From: Tao Klerks <tao@klerks.biz>
Date: Jun 14, 2022, 05:13 UTC
Message-ID: <CAPMMpojdnAMnczJAevqL8GSOb8gvddcSiYfbz0c51oPpn4U0wA@mail.gmail.com>
In-Reply-To: <xmqqleu3au2n.fsf@gitster.g>

On Sat, Jun 11, 2022 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

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

Show 10 quoted lines

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

Show 9 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?
>

Indeed you do (when ignorecase is true of course).

Show 5 quoted lines

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

Show 21 quoted lines

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