Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle case only file name updates on case insensitive file systems #18865

Merged

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Mar 1, 2022

Description:

Fixes #18724

Background

During an update, any files that already exist in the installation destination but no longer exist in the update package will be identified and removed.

The process is:

  1. Copy the new files.
  2. Check for obsolete files that exist in the installation target but are no longer part of the package.
  3. Delete the obsolete files.

This causes a problem when a file is renamed with only a change in case, eg. Myfile to MyFile.

For case sensitive systems this works:

  1. The new MyFile is copied without interfering with the old Myfile
  2. Myfile is identified as not being in the source package and noted for deletion.
  3. The obsolete Myfile is correctly unlinked without interfering with the new MyFile.

For case insensitive system this fails:

  1. The new MyFile is copied and overwrites the old Myfile as the name is treated as the same.
  2. Myfile is identified as not being in the source package and noted for deletion.
  3. The new MyFile is incorrectly deleted as the name is treated as the same, this results in a missing file.

Fix

To fix this a new method has been added which checks if the file system is case insensitive by writing a file with one case and checking if it exists using another case. If the file is reported to exist under a differently cased filename then the file system is assumed to be case insensitive.

When checking for files which exist in the installation but not in the source package and a case insensitive file system is detected then the file name comparison will be performed in a case insensitive manner, this results in the following behaviour:

  1. The new MyFile is copied and overwrites the old Myfile as the name is treated as the same.
  2. MyFile in the source package is compared case insensitively with the existing Myfile and considered to exist in the source package, so it is not noted for deletion.
  3. No attempt is made to delete Myfile and as it has been overwritten with the new MyFile, resulting in the new file in the correct place.

Review

@bx80 bx80 added Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review labels Mar 1, 2022
@bx80 bx80 added this to the 4.8.0 milestone Mar 1, 2022
@bx80 bx80 self-assigned this Mar 1, 2022
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and tests work (using the case insensitive filesystem path).

@tsteur do you maybe want to take a look at this solution?

bx80 and others added 2 commits March 2, 2022 11:38
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
@justinvelluppillai justinvelluppillai changed the base branch from 4.x-dev to next_release March 7, 2022 01:19
@justinvelluppillai justinvelluppillai merged commit aa71a21 into next_release Mar 8, 2022
@justinvelluppillai justinvelluppillai deleted the m-18724-case-insensitive-file-delete branch March 8, 2022 00:39
@justinvelluppillai justinvelluppillai removed the Needs Review PRs that need a code review label Mar 8, 2022
justinvelluppillai added a commit that referenced this pull request Mar 8, 2022
* Update version to 4.8.0-rc1

* Fix possible warning in Url::isValidHost (#18887)

* Handle case only file name updates on case insensitive file systems (#18865)

* Added unit test for case-sensitive unlink

* Use case-insensitive comparison in directoryDiff() if a case-insensitive filesystem is detected

* Fix misleading method name

* Update tests/PHPUnit/Unit/FilesystemTest.php

Co-authored-by: Justin Velluppillai <justin@innocraft.com>

* Update tests/PHPUnit/Unit/FilesystemTest.php

Co-authored-by: Justin Velluppillai <justin@innocraft.com>

Co-authored-by: Justin Velluppillai <justin@innocraft.com>

* Update version to 4.8.0 (#18893)

Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.7.0 After triggering the automatic update a fatal error occurres
2 participants