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

mfs: remove unused Flush() from Root #5172

Closed
wants to merge 1 commit into from
Closed

mfs: remove unused Flush() from Root #5172

wants to merge 1 commit into from

Conversation

schomatis
Copy link
Contributor

Towards clarifying #5066.


mfs: remove unused `Flush()` from `Root`

The unused `Flush()` function in the `Root` structure helps create the misguided
impression that `Root` implements the `mfs.FSNode` interface (which it doesn't).

The unused `Flush()` function in the `Root` structure helps create the misguided
impression that `Root` implements the `mfs.FSNode` interface (which it doesn't).

License: MIT
Signed-off-by: Lucas Molas <[email protected]>
@schomatis schomatis added topic/technical debt Topic technical debt topic/MFS Topic MFS labels Jul 1, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jul 1, 2018
@schomatis schomatis self-assigned this Jul 1, 2018
@schomatis schomatis requested a review from Kubuxu as a code owner July 1, 2018 17:45
@ghost ghost added the status/in-progress In progress label Jul 1, 2018
@schomatis schomatis added need/review Needs a review and removed status/in-progress In progress labels Jul 1, 2018
@Stebalien
Copy link
Member

Really, shouldn't we be calling this function on exit?

@schomatis
Copy link
Contributor Author

It seems that Root's Flush() was replaced by FlushMemFree() (#4509) which uses the Directory Flush() function (but not Root's).

@Stebalien
Copy link
Member

We replaced the existing use of Flush with FlushFreeMem because we wanted to free memory. However, that doesn't mean that Flush isn't otherwise useful (as FlushFreeMem is very unsafe).

@schomatis
Copy link
Contributor Author

that doesn't mean that Flush isn't otherwise useful

Sure, I'm don't want to imply we shouldn't use Flush(), just that at this point in time this function doesn't seem to be called anywhere in the code base and generates confusion when trying to understand the role of the MFS root, that was my main motivation to remove it, but I'm not against discussing in a separate issue to include it back in the code path. Would you prefer to keep it and close this PR?

@Stebalien
Copy link
Member

Would you prefer to keep it and close this PR?

Yes. A lot of the interfaces in go-ipfs are built with the intention of extracting them into a separate library at some point. This is definitely true for MFS.

@schomatis schomatis closed this Jul 3, 2018
@schomatis schomatis deleted the fix/mfs/remove-root-flush branch July 3, 2018 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review topic/MFS Topic MFS topic/technical debt Topic technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants