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.

Sign up for GitHub

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

Jump to bottom

mfs: make Root value a Directory #5170

Merged
merged 3 commits into from
Jul 16, 2018
Merged

mfs: make Root value a Directory #5170

merged 3 commits into from
Jul 16, 2018

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Jun 29, 2018 •
edited
Loading

As discussed in #5066 (comment) and #5066 (comment) (different consecutive comments).

mfs: make `Root` value a `Directory`

Make `Root` value explicitly a `Directory` structure instead of the `FSNode`
interface (which also allowed the `File` type). This helps to make the code
easier to reason about: the root of an MFS layout is always a directory, not a
(single) file.

Rename `GetValue()` to `GetDirectory()` to also make it more explicit, the
renamed function now returns a `Directory` so there is no need for type
assertions that were previously done on the `FSNode` interface to check that it
was actually a `Directory`.

`NewRoot()` now doesn't allow to create `Root` structures from DAG nodes that
contain UnixFS files.

mfs: remove unused `Root` variables `node` and `Type`

mfs: remove `DAGService` from `Root`

The `Root` structure now explicitly contains a `Directory` (instead of an
`FSNode` interface), use that `Directory`'s `DAGService` instead of its own
`dserv` variable (which was used only once in `closeChild()`). The `DAGService`
in the `Root` and the `Directory` was the same (passed as an argument in the
`NewRoot` initializer function).

This leaves the `Root` structure with only a `Directory` and a `Republisher` and
allows to better rethink its role and whether if those two structures should be
grouped together (and if that group's name should be `Root`).

@schomatis schomatis added topic/technical debt Topic technical debt topic/MFS Topic MFS labels Jun 29, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jun 29, 2018
@schomatis schomatis self-assigned this Jun 29, 2018
@schomatis schomatis requested a review from Kubuxu as a code owner June 29, 2018 13:59
@ghost ghost added the status/in-progress In progress label Jun 29, 2018
@schomatis schomatis changed the title mfs: make Root value a Directory [WIP] mfs: make Root value a Directory Jun 29, 2018
@schomatis schomatis mentioned this pull request Jun 29, 2018
Closed
@schomatis
mfs: make Root value a Directory
08922d2 
Make `Root` value explicitly a `Directory` structure instead of the `FSNode`
interface (which also allowed the `File` type). This helps to make the code
easier to reason about: the root of an MFS layout is always a directory, not a
(single) file.

Rename `GetValue()` to `GetDirectory()` to also make it more explicit, the
renamed function now returns a `Directory` so there is no need for type
assertions that were previously done on the `FSNode` interface to check that it
was actually a `Directory`.

`NewRoot()` now doesn't allow to create `Root` structures from DAG nodes that
contain UnixFS files.

License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@schomatis schomatis changed the title [WIP] mfs: make Root value a Directory mfs: make Root value a Directory Jun 29, 2018
@schomatis schomatis added need/review Needs a review status/in-progress In progress and removed status/in-progress In progress need/review Needs a review labels Jun 29, 2018
@schomatis
Copy link
Contributor Author

Take advantage of the (now accessible) directory in root and try to remove the DAGService.

schomatis added 2 commits June 29, 2018 12:06
@schomatis
mfs: remove unused Root variables node and Type
ba73385 
License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@schomatis
mfs: remove DAGService from Root
f5e7fe2 
The `Root` structure now explicitly contains a `Directory` (instead of an
`FSNode` interface), use that `Directory`'s `DAGService` instead of its own
`dserv` variable (which was used only once in `closeChild()`). The `DAGService`
in the `Root` and the `Directory` was the same (passed as an argument in the
`NewRoot` initializer function).

This leaves the `Root` structure with only a `Directory` and a `Republisher` and
allows to better rethink its role and whether if those two structures should be
grouped together (and if that group's name should be `Root`).

License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@schomatis
Copy link
Contributor Author

Done. Ready for review.

@schomatis schomatis added need/review Needs a review and removed status/in-progress In progress labels Jun 29, 2018
kevina
kevina reviewed Jun 29, 2018
View reviewed changes
mfs/ops.go
@@ -172,12 +171,11 @@ func Mkdir(r *Root, pth string, opts MkdirOpts) error {
return nil
}

// Lookup extracts the root directory and performs a lookup under it.
// TODO: Now that the root is always a directory, can this function
// be collapsed with `DirLookup`? Or at least be made a method of `Root`?
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is used in many places within the code. I would be okay with making this a method of Root but would not collapse it with DirLookup and DirLookup will work on any directory and not just the root.

kevina
kevina approved these changes Jun 29, 2018
View reviewed changes
Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

LGTM

@Stebalien Stebalien self-requested a review June 29, 2018 17:08
@schomatis schomatis mentioned this pull request Jul 1, 2018
Closed
Stebalien
Stebalien approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I believe this is a step in the right direction (although I think we may not want a Root at all but we can discuss that in the relevant comment).

fuse/ipns/ipns_unix.go
@@ -118,14 +118,7 @@ func loadRoot(ctx context.Context, rt *keyRoot, ipfs *core.IpfsNode, name string

rt.root = root

switch val := root.GetValue().(type) {
Copy link
Member

Choose a reason for hiding this comment

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

So, I need to get better about submitting comments instead of editing them till my browser crashes and they disappear...

This is the one real reason to allow the root to be something other than a directory. However, really, the "root" is just a way to tie a file/directory in an MFS tree to a publisher. I'd rather just:

  1. Hold a directory directly as you suggested here MFS Root vs MFS File/Directory #5066 (comment).
  2. Provide some way to "watch" files/directories.

Currently, we have the childCloser but I wonder if we want a more general system. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I need to get better about submitting comments instead of editing them till my browser crashes and they disappear...

I'm glad I'm not the only one, I'm seriously considering submitting a feature request to GitHub that would allow me to explicitly save my drafts..

Thoughts?

I was considering adding that logic to the directory implementation of childCloser, if the directory has a parent call the parent's closeChild (current behavior), if not (you are the root) call the republisher and tell it things have changed. That would require every directory to be aware of the republisher, either with a pointer to it or some other way (which is not nice). Anyway, I need to keep studying how the republisher works and see if I can come up with something better.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of a more general purpose notification boadcast system where users could register and unregister any number of notifiees (where the parent directory would register itself as a notifiee).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate a bit more on that? I thought the directory was the notifier (I may be misunderstanding the word notifiee, that's the one that receives the notification, right?).

Copy link
Member

@Kubuxu Kubuxu Jul 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Unrelated:
You can save a code comment with a "Start Review", comments will be visible only to you until you send the review.

@schomatis schomatis added RFM and removed need/review Needs a review labels Jul 6, 2018
@whyrusleeping whyrusleeping merged commit 5160724 into ipfs:master Jul 16, 2018
@schomatis schomatis deleted the feat/mfs/root-val-as-dir branch December 13, 2018 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@kevina kevina kevina approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@schomatis schomatis

Labels
RFM topic/MFS Topic MFS topic/technical debt Topic technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@schomatis @Stebalien @kevina @Kubuxu @whyrusleeping