-
-
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
Pin ls traverses all indirect pins #6705
Conversation
Ideally we should merge the tests (ipfs/interface-go-ipfs-core#47) before merging this so that we can reference the tests from go-ipfs. However, I'm not picky on ordering here as long as the tests actually end up running on the CoreAPI code. |
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 whole point of deduplicating code is to simplify things. Something is wrong if the code size blows up by a factor of 1.5x.
A lot of the complexity/lines of code comes from using channels instead of an emit function in Also, the point of the deduplication here was to make testing easier (easier to test CoreAPI then commands lib) and reduce the chance of introducing weird bugs due to having fixed code in one place but not another. |
if err != nil { | ||
return nil, err | ||
go func() { | ||
defer close(ch) |
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.
Given the way this function is currently called, this can hide errors: we can observe this channel closing before we read the error off the error channel.
It's generally safer to just use a "result" channel. That makes it easier to call this function as the for x := range myCh
syntax can be used.
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.
Alternatively, we can close both the error and normal channels and, instead of listening on the error channel, run err := <-errCh
after finishing the normal loop.
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 I did what you asked here, lmk.
core/coreapi/pin.go
Outdated
err := merkledag.Walk(ctx, merkledag.GetLinksWithDAG(dag), k, func(c cid.Cid) bool { | ||
r := indirectKeys.Visit(c) | ||
if r && keys.Visit(c) { | ||
ch <- &pinInfo{ |
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 can block.
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.
now checks on context
@aschmahmann bump (when you get a chance). |
219cf2d
to
1e6c67c
Compare
@Stebalien I think we should just squash these commits when we're ready for merge. Lmk and we can finish any last issues with this PR. |
…ecedence change - a direct/recursive pin is now labeled as such even if also indirectly pinned.
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.
Squashed and added a comment.
core/coreapi/pin.go
Outdated
for _, v := range keys { | ||
out = append(out, v) | ||
} | ||
if typeStr != "all" { |
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 needs a comment explaining why we're doing this.
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 also inverted the test to if typeStr == "indirect"
to make it clear that we're skipping this because we only care about indirect pins.
pin ls
soon and then this will be easy)Changes 1 and 3 need to be tested, and I will submit a PR for a CoreAPI test (if we want a sharness test we can do that as well, but it seems like overkill). Since the CoreAPI tests all live in interface-go-ipfs-core this test should probably be there as well.
@Stebalien