Unlock files even if an exception occurs while renaming

[korelstar]

Fixes#7009:

Problem

As an app developer, I want to rename a file. Therefore, I call OC\Files\Node\Node.move(...) which calls OC\Files\View.rename(...). If the rename fails (e.g. due to an illegal file name), then an InvalidPathException is thrown. The problem is, that the file is still locked, even if I catch the exception.

Cause

The reason is that OC\Files\View.rename(...) locks the old and new path, but it doesn't unlock them if an exception is thrown, e.g. by verifyPath(...).

Solution

As a solution, I suggest to move the code of OC\Files\View.rename(...) after locking into a try-catch-finally block in order to ensure that the locks are always freed.

The diff looks big, but most oft the changes are just indentation.

[codecov]

Codecov Report

Merging #7014 into master will increase coverage by <.01%.
The diff coverage is 94.11%.

@@ Coverage Diff @@## master #7014 +/- ##
============================================
+ Coverage 50.61% 50.62% +<.01% - Complexity 24297 24298 +1 
============================================
Files 1577 1577 Lines 92922 92925 +3 Branches 1359 1359 ============================================
+ Hits 47037 47039 +2 - Misses 45885 45886 +1

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/View.php	84.11% <94.11%> (+0.04%)	371 <0> (+1)	⬆️
lib/private/Files/Cache/Propagator.php	94.93% <0%> (-1.27%)	16% <0%> (ø)
lib/private/Server.php	83.29% <0%> (-0.13%)	125% <0%> (ø)
lib/private/Security/CertificateManager.php	92.07% <0%> (+0.99%)	39% <0%> (ø)	⬇️

[enoch85]

Finally! This has been so annoying for the past years.

[nickvergessen]

Can you please sign your commits? See https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work for more information.

You can do this manually for now:

git commit --amend -s
git push --force origin <your branch>

[enoch85]

@korelstar If this goes in to 13, could you please backport to 12 and 11 as well?

[korelstar]

@nickvergessen
I've signed the commit. I'm looking forward for reviews. :-)

@enoch85
I don't know about the backporting process, but I think the first step is to get this merged. Nevertheless I would be happy to see this backported and I have no problem in creating separate PRs to the respective stable branch -- if this is the correct way.

[LukasReschke]

@korelstar If this goes in to 13, could you please backport to 12 and 11 as well?

Generally speaking our rule on backports is that we only backport really critical issues (such as data loss) or cases where we experienced major issues at customer deployments. – To be honest, I'm a little bit reluctant to backport any changes to the file system. Even simple changes broke things in the past, so I'm better safe than sorry 😄

[LukasReschke]

@icewind1991 What's your professional opinion here? :)

[enoch85]

Nevertheless I would be happy to see this backported and I have no problem in creating separate PRs to the respective stable branch -- if this is the correct way.

@korelstar Please do :)

[MorrisJobke]

@korelstar For the backport just create a branch based on stable12, cherry-pick the commit from here and then create a PR against stable12 over here on github. We will then review this one as well and merge it. Once you opened the PR you could also remove the "backport-request" label from this PR.

Thanks

[korelstar]

Please see #7144 and #7148 for the backports.