-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor ipfs get to use CoreAPI #5943
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and on the the two connected PRs.. but I don't consider myself a core API expert by any means.
I have two quick questions:
- Why move unixfile to go-unixfs & why put tar-writer in go-ipfs-files instead of go-unixfs?
- Am I correct in understanding that neither of these two changes are strickly essential to delivering the refactor to make get use CoreAPI?
Non-blocking, just want to understand.
Because However: ipfs/go-unixfs#59 (comment)
We need some way to go from a |
As a note - I decided to drop it as the combo of |
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
7601f27
to
21a05dd
Compare
return err | ||
} | ||
|
||
res.SetLength(uint64(size)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually incorrect but it's an existing bug... #5690
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is not setting this at all, but it will break progress bar... (We could pass this in a separate header)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to. Unfortunately, that'll require a change to the commands library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this doesn't work with sharding. t0260-sharding.sh is failing.
Error is: "Error: cannot write to readonly DAGService" |
The issue is: https://github.com/ipfs/go-unixfs/blob/0eb1ef8f33371b1b9f74f74655b584144919ce91/file/unixfile.go#L121. This is causing the hamt to serialize and save itself. |
It should probably just do |
Ah, right, it breaks the progress bar ._. |
Maybe we should have a separate hamt reader? |
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
21a05dd
to
7c2aa0e
Compare
Refactor ipfs get to use CoreAPI This commit was moved from ipfs/kubo@35aaa98
Depends on: