Skip to content

Conversation

@ggeorgg
Copy link

@ggeorgg ggeorgg commented May 21, 2018

Also delete symlinked files, so that the updater does not stop when deleting a non-empty directory.

What do you think of this pull-request? @MorrisJobke

ggeorgg added 2 commits May 17, 2018 23:14
Symlinks are not handled correct. $fileInfo->getRealPath() delivers an empty result for symlink files. Therefore I added another if to check if it is a link.
Also delete symlinked files, so that the updater does not stop when deleting a non-empty directory.
@MorrisJobke
Copy link
Member

Code wise this looks good 👍 Haven't tested yet. What are the exact steps to reproduce? Link some files inside Nextcloud? How did you manage to get there?

@MorrisJobke
Copy link
Member

Also could you update https://github.com/nextcloud/updater/blob/master/lib/Updater.php with the exact same changes? Then the CI job also get's green.

Symlinks are not handled correct. $fileInfo->getRealPath() delivers an empty result for symlink files. Therefore I added another if to check if it is a link.
A part of issue #158 can be solved with this pull-request.
@ggeorgg
Copy link
Author

ggeorgg commented May 24, 2018

Steps to reproduce:
Use Open SUSE Tumbleweed and install nextcloud via
sudo zypper in nextcloud
Point your browser to your nextcloud installation and select update channel daily. Reload the page and try to update your instance with the web based updater.
The update process will fail due to files which couldn't be removed beforehand by the updater itself. The files which cause this problem are symlinks that are generated at the installation because of
https://build.opensuse.org/package/view_file/openSUSE:Factory/nextcloud/nextcloud.spec?expand=1
Line 164: %fdupes -s $RPM_BUILD_ROOT/%{apache_serverroot}/%{name}
The problem is solved on my machine if we check for symlinks, files and folders in the delete functions.

@kesselb
Copy link
Contributor

kesselb commented Aug 23, 2018

Basically this patch should fix an issue the open suse packager introduced by replacing duplicate files with symlinks right?

@ggeorgg
Copy link
Author

ggeorgg commented Aug 23, 2018

Exactly.

@kesselb
Copy link
Contributor

kesselb commented Aug 23, 2018

@ggeorgg thanks! code looks good.

@kesselb
Copy link
Contributor

kesselb commented Aug 23, 2018

Build fails because your branch is a bit outdated. Could you merge latest changes from nextcloud/updater?

Get newest master branch from nextcloud updater
Updapte branch to latest nextcloud updater master
@ggeorgg
Copy link
Author

ggeorgg commented Aug 25, 2018

I just merged the latest nextcloud updater repository into my own branch "patch-1". It may look a bit clustered now with my commits. Hope this will not be a problem. I am not quite sure if this is how it works. If it was not correct what I have done, please give me a hint on how to proceed ;)

@kesselb
Copy link
Contributor

kesselb commented Aug 25, 2018

OK. Sorry there is one more thing. You need to sign off your commits (see this link for more information https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for)

You have two ways to do this:

A) I would rebase (interactive) the branch to updater/master, squash all commits into one commit, sign these commit off and force push them back.
B) Create a new branch from updater/master, apply your patch again, sign off and open a new pr.

@ggeorgg ggeorgg closed this Aug 26, 2018
@ggeorgg ggeorgg deleted the patch-1 branch August 26, 2018 18:34
@ggeorgg
Copy link
Author

ggeorgg commented Aug 27, 2018

Unfortunatley I was not able to clean up the pull request. I opened a new one here: #180.

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.

3 participants