Fix VFS crash and false conflict on local new. #3449

allexzander · 2021-06-16T18:59:25Z

Signed-off-by: allexzander blackslayer4@gmail.com

This should do it. Just need to test a couple of times more and maybe come up with some additional scenarios...
Having said that, it works well now when creating new folder and still works as expected when renaming on 2 clients.

mgallien

Works for me but please see my comments

mgallien · 2021-06-17T14:59:36Z

src/libsync/discovery.cpp

+        const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local);
+
+        // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet
+        const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityError::NoSuchItem;


I have been having an hard time understanding what is being done in this line
I understand that we are using some implicit conversion of OCC::Result but I really wonder why we even have those implicit conversions ?
Can you please make it explicit here ?
I was really wondering why the enum value could be used in boolean context

I was really wondering why the enum value could be used in boolean context

@mgallien I guess this is by the design of Result and enums. But yeah, readability could use some improvement.

mgallien · 2021-06-17T15:07:07Z

src/libsync/discovery.cpp

+        // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet
+        const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityError::NoSuchItem;
+
+        const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : PinState::Unspecified;


same comment about the implicit conversion

mgallien · 2021-06-17T15:07:48Z

src/libsync/discovery.cpp

+
+        const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : PinState::Unspecified;
+
+        if (!isFilePlaceHolder && !folderAvailability && !folderPinState) {


see my previous comment, I would have an hard time explaining what this check is doing
please make it more obvious such that a comment would not be needed

allexzander · 2021-06-18T12:21:24Z

Works for me but please see my comments

@mgallien I am using more explicit code now. The reason implicit conversion works is because Result is implemented as a union and it also implements operator bool, so, it can be implicitly checked for validity. I have added a method to allow explicit chek for no-error.

mgallien

could you reduce the scope of the change to just fix the case of directory creation or write the list of scenario needed to test that change

mgallien · 2021-06-23T09:02:59Z

src/libsync/discovery.cpp

@@ -896,46 +896,68 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
    auto postProcessLocalNew = [item, localEntry, path, this]() {


I fail to understand why we need such complexity here when owncloud client is much simple https://github.com/owncloud/client/blob/master/src/libsync/discovery.cpp#L837*
I am under the impression that this shows that the fixes are being done at the wrong place

after all that code was added to perform a last chance clean up of placeholder files that should not be present and we are now doing a lot of stuff needing comment to be understood
we should simplify or we will end up with code that we can no longer understand and maintain

I fail to understand why we need such complexity here when owncloud client is much simple https://github.com/owncloud/client/blob/master/src/libsync/discovery.cpp#L837*
I am under the impression that this shows that the fixes are being done at the wrong place

@mgallien I am pretty sure, OwnCloud would have the same problem happening. It is just, nobody complaint about simultaneous renaming/moving of virtual files/folders.

allexzander · 2021-06-29T08:14:10Z

@mgallien Test cases https://cloud.nextcloud.com/apps/files/?dir=/DesktopTeam&fileid=1351251

mgallien · 2021-07-01T07:32:21Z

src/libsync/discovery.cpp

+        const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local);
+
+        // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet
+        const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityResult(Vfs::AvailabilityError::NoSuchItem);


if I understand correctly your modification, this should be called folderPlaceHolderAvailability ?

@mgallien done

Signed-off-by: allexzander <blackslayer4@gmail.com>

…ne-only folder based on its pinState. Signed-off-by: allexzander <blackslayer4@gmail.com>

Signed-off-by: allexzander <blackslayer4@gmail.com>

nextcloud-desktop-bot · 2021-07-01T09:08:54Z

AppImage file: Nextcloud-PR-3449-99f6e822906ba47670593dc91ee1a601ee0fcdcc-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.

allexzander requested review from FlexW, mgallien and camilasan June 16, 2021 18:59

mgallien reviewed Jun 17, 2021

View reviewed changes

allexzander marked this pull request as ready for review June 22, 2021 08:43

allexzander force-pushed the bugfix/fix-vfs-crash-and-false-conflict-on-local-new branch from 937eb1a to 5ed1e87 Compare June 22, 2021 08:47

mgallien added the 2. to review label Jun 22, 2021

allexzander requested a review from mgallien June 23, 2021 05:46

mgallien reviewed Jun 23, 2021

View reviewed changes

allexzander requested a review from mgallien June 29, 2021 08:14

mgallien reviewed Jul 1, 2021

View reviewed changes

allexzander added 5 commits July 1, 2021 11:59

Fix VFS crash and false conflict on local new.

48aefc1

Signed-off-by: allexzander <blackslayer4@gmail.com>

Refactoring.

c598214

Signed-off-by: allexzander <blackslayer4@gmail.com>

Do not process non-directories and non-virtual files. Wipe empty onli…

22eba41

…ne-only folder based on its pinState. Signed-off-by: allexzander <blackslayer4@gmail.com>

Fix review comments.

6ce5279

Signed-off-by: allexzander <blackslayer4@gmail.com>

Fix review comments.

99f6e82

Signed-off-by: allexzander <blackslayer4@gmail.com>

allexzander force-pushed the bugfix/fix-vfs-crash-and-false-conflict-on-local-new branch from 086fed8 to 99f6e82 Compare July 1, 2021 08:59

allexzander requested a review from mgallien July 1, 2021 09:00

mgallien approved these changes Jul 1, 2021

View reviewed changes

allexzander merged commit dca7b8d into master Jul 1, 2021

allexzander deleted the bugfix/fix-vfs-crash-and-false-conflict-on-local-new branch July 1, 2021 10:33

pasbec mentioned this pull request Aug 27, 2022

[Bug]: Some virtual files cannot be sync'd: "Conflict when uploading a file. It's going to get removed!" #4636

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VFS crash and false conflict on local new. #3449

Fix VFS crash and false conflict on local new. #3449

allexzander commented Jun 16, 2021

mgallien left a comment

mgallien Jun 17, 2021

allexzander Jun 17, 2021

mgallien Jun 17, 2021

mgallien Jun 17, 2021

allexzander commented Jun 18, 2021

mgallien left a comment

mgallien Jun 23, 2021

mgallien Jun 23, 2021

allexzander Jul 1, 2021

allexzander commented Jun 29, 2021

mgallien Jul 1, 2021

allexzander Jul 1, 2021

nextcloud-desktop-bot commented Jul 1, 2021


		const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : PinState::Unspecified;

		if (!isFilePlaceHolder && !folderAvailability && !folderPinState) {

		@@ -896,46 +896,68 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
		auto postProcessLocalNew = [item, localEntry, path, this]() {

Fix VFS crash and false conflict on local new. #3449

Fix VFS crash and false conflict on local new. #3449

Conversation

allexzander commented Jun 16, 2021

mgallien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allexzander commented Jun 18, 2021

mgallien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allexzander commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Jul 1, 2021