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

Remove usage of merkledag.Link.Node pointer outside of merkledag #1916

Closed
wants to merge 242 commits into from

Conversation

mildred
Copy link
Contributor

@mildred mildred commented Oct 30, 2015

This prepares for inclusion of IPLD where the Node pointer might not be publicly accessible of even be there. We should probably move away from using the struct attributes and use functions to access them instead. This will make it easier to plug in IPLD in place of merkledag.

Obsoletes #1902 (PR on the wrong branch, and this can't be changed)

see also discussion in ipld/go-ipld-deprecated#14

@jbenet jbenet added the status/in-progress In progress label Oct 30, 2015
@mildred
Copy link
Contributor Author

mildred commented Oct 30, 2015

From there, we might want to split the Node type in two separate types for different use cases:

  • ipld.Node which contains the raw Node object (possibly still encoded), and the logic to decode it
  • merkledag.Node (or any other name) which contains just the decoded links of the node, not anything extra. Plus a reference to ipld.Node.
  • more types as we need to do more things. Like unixfs, IPNS/IPRS...

The data flow would be different. Instead of decoding everything of the IPLD node in a generic and unoptimized data structure (recursive maps basically), we could get both a non decoded data structure (the ipld.Node) and an optimized data structure (merkledag.Node) that would be efficient for link traversing.

The drawback of this approach is that we could not directly encode the optimized data structure back to the wire format (JSON, CBOR or ProtoBuf) because we would lose the bits we didn't decode first. This would apply only to modifying existing nodes, not to creating new nodes.

@whyrusleeping this might explain a bit of the discussion we had in ipld/go-ipld-deprecated#14

@whyrusleeping whyrusleeping force-pushed the dev0.4.0 branch 3 times, most recently from 764bef9 to 1bbc472 Compare November 11, 2015 18:04
@rht
Copy link
Contributor

rht commented Nov 17, 2015

@mildred needs a rebase

@GitCop
Copy link

GitCop commented Nov 27, 2015

There were the following issues with your Pull Request

  • Commit: a1dc966
    • Invalid signoff. Commit message must end with
      License: MIT
      Signed-off-by: .* <.*>

Guidelines and a script are available to help. Your feedback on GitCop is welcome on this issue.


This message was auto-generated by https://gitcop.com

@@ -78,12 +78,13 @@ it contains, with the following format:
Links: make([]LsLink, len(dagnode.Links)),
}
for j, link := range dagnode.Links {
link.Node, err = link.GetNode(req.Context(), node.DAG)
var linkNode *merkledag.Node
linkNode, err = link.GetNode(req.Context(), node.DAG)
Copy link
Member

Choose a reason for hiding this comment

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

keep it as one line, using the := instead of a var declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to override the err variable. using := is not equivalent because it creates a new err variable scoped in the for loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fail to see the reason for reusing the err var here:
if an err happens inside the loop, the function returns right away, and if the loop is done, it means the err is nil -- same behavior for both reused / scoped errs.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, theres no reason not to create a new err in the inner scope. thats what we want to do.

Copy link
Member

Choose a reason for hiding this comment

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

reusing err values across scopes causes a lot of errors, has bitten us in the past already

@mildred
Copy link
Contributor Author

mildred commented Nov 27, 2015

See also ipld/go-ipld-deprecated#17 which is where I'd like to go next to fit IPLD within IPFS in terms of API.

@ghost ghost added need_signoff topic/merkledag Topic merkledag labels Dec 22, 2015
@whyrusleeping whyrusleeping force-pushed the dev0.4.0 branch 2 times, most recently from 68b9745 to b0a8591 Compare December 28, 2015 14:36
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
whyrusleeping and others added 11 commits January 12, 2016 18:02
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
See ipfs#1296

License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
combine multiple bootstrap addrs into single peer info
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Kubuxu and others added 10 commits February 14, 2016 23:45
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@gmail.com>
Do not install gx if user explicitly didn't ask for it
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
add a pause to fix timing on t0065
Capitalized Merkle, added single quotes, periods
Normalised Example heading, added dollar sign to examples
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@gmail.com>
Add information about installing gx into readme
RichardLitt and others added 12 commits February 17, 2016 13:12
License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
See @dignifiedquire comments in ipfs-inactive/http-api-spec#45

License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
License: MIT
Signed-off-by: Richard Littauer <richard.littauer@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
This prepares for inclusion of IPLD where the Node pointer won't be there.

License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
This function work only with protocol buffer encoding. To make this clear,
rename the function.

License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
@mildred
Copy link
Contributor Author

mildred commented Feb 18, 2016

Just to be sure, you asked me to rebase on master. Unfortunately this PR is set to merge in dev0.4.0. Either I close this PR and reopen one that merges to master, or I should rebase to dev0.4.0.

That's why we see all the commits here.

@chriscool
Copy link
Contributor

These days there are some v0.4.0 related tags on master. It looks like the last one is v0.4.0-rc2, so there should not be a big difference and I think it's better to rebase on master.

@mildred
Copy link
Contributor Author

mildred commented Feb 18, 2016

Closed in favor of #2359

@mildred mildred closed this Feb 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress In progress topic/merkledag Topic merkledag
Projects
None yet
Development

Successfully merging this pull request may close these issues.