On Sat, Jun 11, 2022 at 9:17 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Tao Klerks via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Tao Klerks <tao@xxxxxxxxxx> > > > > "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.
