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

pinning + pathresolver: fix pinning/unpinning of sharded directories #3975

Merged
merged 2 commits into from
Jun 13, 2017

Conversation

Stebalien
Copy link
Member

  • Change ResolveToCid to take a Resolver and a NameSystem instead of an ipfs
    Node.
  • Make the pin/unpin methods use a Unixfs path resolver.

Closes: #3974

License: MIT
Signed-off-by: Steven Allen [email protected]

* Change ResolveToCid to take a Resolver and a NameSystem instead of an ipfs
  Node.
* Make the pin/unpin methods use a Unixfs path resolver.

Closes: ipfs#3974

License: MIT
Signed-off-by: Steven Allen <[email protected]>
Copy link
Member

@whyrusleeping whyrusleeping 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
Copy link
Member Author

We may want to hold this pending the discussion in #3910 (comment) (as it's possible to pin IPLD objects).

@whyrusleeping
Copy link
Member

Hrm.... this is true. but the resolve is just being applied to the user given path. It shouldnt have any effect on pinning ipld objects.

cc @Kubuxu @lgierth

@Stebalien
Copy link
Member Author

Can't you path through IPLD objects? Also, whatever happened to /ipld/.... Having the pathing scheme not declared somewhere in the path is kind of terrible.

@whyrusleeping
Copy link
Member

You can path through ipld objects, that works fine. And shard nodes are very specific protobufs, so theres no ambiguity between a path in an ipld node and a path through a sharded node. The only issue happens when you want to address an intermediate shard element directly

@@ -377,13 +378,18 @@ new pin and removing the old one.
return
}

fromc, err := core.ResolveToCid(req.Context(), n, from)
r := &path.Resolver{
Copy link
Member

Choose a reason for hiding this comment

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

This construction is more and more frequent around, it might be good to store it in ipfs node struct or have helper for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the end of the day, I think a better solution would be to avoid this resolver dance altogether. The root of the issue comes from the fact that the path doesn't dictate how it should be interpreted: as an IPLD path through IPLD objects (where the segments are keys) or an IPFS path through potentially sharded IPFS directories (where the segments are directory entries). To work around this:

  1. It's up to commands to chose the initial resolver.
  2. Resolvers do some additional magic under the covers to detect if a node should be treated as an IPLD or IPFS object by looking at it's Cid (they really shouldn't be doing this).

The solution I've proposed is to use separate prefixes: /ipld/... and /ipfs/... (ipfs/notes#248). I believe this is how this was intended to work (when designed) anyways.

If we did this, we could pass around the protocol along with the Cid/relative path so that core.Resolve and core.ResolveToCid could pick the right resolver for the given protocol.

@whyrusleeping whyrusleeping merged commit 2f999d4 into ipfs:master Jun 13, 2017
@Stebalien Stebalien deleted the fix/3974 branch July 12, 2017 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ipns pin add doesn't resolve sharded paths
3 participants