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

Fix MacOS UTF-8 normalization issue #4957

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Conversation

xavi-b
Copy link
Contributor

@xavi-b xavi-b commented Sep 19, 2022

Fix #4950

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #4957 (5057fd9) into master (38ae92b) will decrease coverage by 2.17%.
The diff coverage is 100.00%.

❗ Current head 5057fd9 differs from pull request most recent head 5113802. Consider uploading reports for the commit 5113802 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4957      +/-   ##
==========================================
- Coverage   59.28%   57.12%   -2.17%     
==========================================
  Files         143      138       -5     
  Lines       18445    17234    -1211     
==========================================
- Hits        10936     9845    -1091     
+ Misses       7509     7389     -120     
Impacted Files Coverage Δ
src/common/syncjournaldb.cpp 77.45% <100.00%> (-1.24%) ⬇️

... and 78 files with indirect coverage changes

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

This looks fine to me though why not implement this for all systems as a precautionary measure?

Would like to get your feedback before merging @mgallien @allexzander @camilasan

@mgallien
Copy link
Collaborator

@xavi-b thanks for your contribution

why do even need to use such a phash column with possibilities for troubles when we can just use the path column ?
I would rather remove the use for the phash column rather than trying to make it more robust just in case we can identify some of the problems of using it

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

@mgallien Say we merge this for now, and get ride of hash down the road?

Signed-off-by: xavi-b <developer@xavi-b.fr>
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-4957-5113802daf38afdf5934fcd210e74513b22decd8-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@claucambra claucambra merged commit 4429635 into nextcloud:master Apr 24, 2023
@claucambra
Copy link
Collaborator

/backport to stable-3.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: MacOS Virtual File with french character issue
4 participants