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

Check wehter file is locked before we move #8768

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

TheOneRing
Copy link
Member

No description provided.

@TheOneRing TheOneRing force-pushed the work/require_acces_before_move branch 3 times, most recently from 43d2ff5 to b60b543 Compare June 22, 2021 07:53
@TheOneRing

This comment has been minimized.

@TheOneRing TheOneRing force-pushed the work/require_acces_before_move branch 2 times, most recently from fde9956 to 1bd2cc7 Compare June 22, 2021 09:33
Copy link
Contributor

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

src/common/filesystembase.cpp Outdated Show resolved Hide resolved
@@ -191,6 +196,11 @@ bool FileSystem::uncheckedRenameReplace(const QString &originFileName,
}

#else //Q_OS_WIN
if (FileSystem::isFileLocked(originFileName, FileSystem::LockMode::Exclusive)) {
*errorString = QCoreApplication::tr("FileSystem", "Can't rename %1, the file is currently in use").arg(originFileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, in this function errorString is used completely unchecked.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an assert, as the function required an error parameter from the start

if (error)
*error = tr("File %1 is currently in use").arg(fn);
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

before this new block, touchedFile() is emitted. However, if the file is locked, we do not touch anything, so the emits should probably be moved below.

emit propagator()->seenLockedFile(fn, FileSystem::LockMode::Exclusive);
done(SyncFileItem::SoftError, tr("%1 is currently in use").arg(fn));
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder who/what removes the tmp file of the download that is stopped here. But there are already other checks that interrupt the download, so that is probably handled elsewhere. Otherwise that would leave tmp files behind, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd doubt it but that something for a different review :D

@TheOneRing TheOneRing force-pushed the work/require_acces_before_move branch from 1bd2cc7 to 94f8d0e Compare June 22, 2021 10:12
@TheOneRing TheOneRing force-pushed the work/require_acces_before_move branch from 94f8d0e to 4650029 Compare June 22, 2021 10:19
@sonarcloud
Copy link

sonarcloud bot commented Jun 22, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing force-pushed the work/require_acces_before_move branch from 4650029 to b7ed176 Compare June 22, 2021 11:42
Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

nice.

@TheOneRing TheOneRing merged commit cd2a7a9 into 2.8 Jun 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the work/require_acces_before_move branch June 22, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants