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

Inspect differences between files/storage/local.php and files/storage/mappedlocal.php #12460

Closed
nickvergessen opened this issue Nov 26, 2014 · 4 comments · Fixed by #12469
Closed
Assignees
Milestone

Comments

@nickvergessen
Copy link
Contributor

There are some smaller differences which need to be inspected, whether they are intentional or not.

In the end, if the local.php uses $this->buildPath($path) instead of $this->datadir . $path (see #12462) a lot of the code duplication could be removed, which would be a nice step to avoid problems as listed below.

public function stat($path)

local calls clearstatcache(); in the beginning, mappedlocal does not call it at all.
@icewind1991 you added this to local.php: 283c10f

public function touch($path, $mtime = null)

local checks whether the file exists and is updateable before touching it

        if ($this->file_exists($path) and !$this->isUpdatable($path)) {
            return false;
        }

@icewind1991 and @DeepDiver1975 you added this to local.php 258ad38 and d069ee8

public function free_space($path)

local.php

        $space = @disk_free_space($this->buildPath($path));
        if ($space === false || is_null($space)) {
            return \OCP\Files\FileInfo::SPACE_UNKNOWN;
        }
        return $space;

mappedlocal.php

        return @disk_free_space($this->buildPath($path));

@icewind1991 you added this to local.php ed83597 and 25370fc

public function hasUpdated($path, $time)

local.php

        if ($this->file_exists($path)) {
            return $this->filemtime($path) > $time;
        } else {
            return true;
        }

mappedlocal.php

        return $this->filemtime($path) > $time;

@Xenopathic you add this to local.php eeee9ea

public function isLocal()

local.php is true, which should be also the case for mappedlocal.php when looking at the description:

        // the common implementation returns a temporary file by
        // default, which is not local
        return false;

@PVince81 you added that to local.php with 788c854

Can the mentioned people please have a look at the respective commits and say whether there is a reason, why the change should not also be applied to mappedlocal.php?
Because I can not find a reason and would therefor port the changes.

@RobinMcCorkell
Copy link
Member

To be honest, the change I made was only to local.php because that was where the issue was reported. I see no reason it can't be ported to mappedlocal.php, but I'd say that @icewind1991 is probably the person to confirm this.

@icewind1991
Copy link
Contributor

It would make more sense imo to get rid of MappedLocal and instead inject a mapper in the local storage (which on non-win systems would just do the $this->data.$path)

@nickvergessen
Copy link
Contributor Author

@icewind1991 im on the way to deduplicate this, but in order to do so, I need to be sure which changes are intentional to only be in one or the other.

@PVince81
Copy link
Contributor

@nickvergessen for 788c854 I might have missed MappedLocal as I didn't understand it well back then. But I think it's fine to have it there as well.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants