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

Outputs added directories as they are traversed. #2531

Merged
merged 9 commits into from
Apr 18, 2016

Conversation

hackergrrl
Copy link
Contributor

This also fixes a bug where adding with -n would prevent directories from
being outputted. Addresses #2530.

Bonus: directories no longer appear at the end of the list for ipfs add --
they're in order! 👯

@hackergrrl
Copy link
Contributor Author

(I still have some tests to fix up, now that the add order has changed.)

@whyrusleeping
Copy link
Member

No. The change to output directories at the end of the add was a conscious decision, files in a directory may be added before or after the directory itself is 'added' so the hash of the directory may change. No order is guaranteed about the stream of objects being added in an add

@hackergrrl
Copy link
Contributor Author

Ah, this makes sense now. Consider a case like:

$ ipfs add foo/bar foo/baz

We need to create the foo dir when foo/bar is created, but we can't output its hash yet, since after foo/baz is added it will have changed -- so directories definitely need to have their hash computed last.

However, this doesn't mean we must output them at the end: we can still perform a formatting step post-add that will provide a more traditional output structure (subfiles THEN containing dir).

@hackergrrl hackergrrl force-pushed the output-dirs-on-add branch 3 times, most recently from edeca42 to bace3ec Compare April 7, 2016 02:55
@hackergrrl
Copy link
Contributor Author

I ended up modifying the existing logic to use MFS for traversal instead of relying on the dag service directly. I had to tweak how mfs.Directory was doing caching to ensure that the children are available after being added by AddNode, but otherwise this went pretty smoothly after I grokked the code structure.

@hackergrrl hackergrrl added the need/review Needs a review label Apr 7, 2016
@whyrusleeping whyrusleeping added need_tests and removed need/review Needs a review labels Apr 7, 2016
@hackergrrl hackergrrl force-pushed the output-dirs-on-add branch 2 times, most recently from 96fc1d1 to 312aef9 Compare April 8, 2016 04:10
child, err := root.Links[0].GetNode(adder.ctx, adder.node.DAG)
name = rootNode.Links[0].Name

root, err = adder.mr.GetValue().(*mfs.Directory).Child(name)
Copy link
Member

Choose a reason for hiding this comment

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

we'll want to do some type checking here:

dir, ok := adder.mr.GetValue().(*mfs.Directory)
if !ok {
   // errrorrrr
}

root, err = dir.Child(name)
...

@hackergrrl
Copy link
Contributor Author

🎉 Ready for merge!

@@ -200,21 +207,35 @@ func (adder *Adder) Finalize() (*dag.Node, error) {
return nil, err
}

return root, nil
rootNode, err = root.GetNode()
Copy link
Member

Choose a reason for hiding this comment

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

can actually just do return root.GetNode() its a little cleaner looking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed -- changed.

License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
This reverts commit dfd98f2.

License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
@hackergrrl
Copy link
Contributor Author

hackergrrl commented Apr 18, 2016

Anything else you need done in here, @whyrusleeping?

@whyrusleeping
Copy link
Member

Nope, this LGTM

@whyrusleeping whyrusleeping merged commit 2509631 into ipfs:master Apr 18, 2016
@hackergrrl hackergrrl mentioned this pull request May 19, 2016
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.

2 participants