feat: fix get-dag and add version=1 option #260
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rvagg
force-pushed
the
rvagg/cmd-get-dag-improve
branch
3 times, most recently
from
b270be5
to
daace98
Compare
|
i think this needs a rebase now. 🤭 |
willscott
approved these changes
Oct 21, 2021
There was a problem hiding this comment.
this looks good. Maybe update the flag name for index so that we're consistent in using version=n as the way to do this :) https://github.com/ipld/go-car/blob/master/cmd/car/car.go#L89-L92
rvagg
force-pushed
the
rvagg/cmd-get-dag-improve
branch
from
daace98
to
7e59d53
Compare
|
wew, ended up bailing on a nasty rebase and cherry-picked my way out of it; should be good now, I've replaced |
rvagg
force-pushed
the
rvagg/cmd-get-dag-improve
branch
from
52a83cb
to
f5de7b8
Compare
rvagg
force-pushed
the
rvagg/cmd-get-dag-improve
branch
from
f5de7b8
to
3f9a436
Compare
willscott
approved these changes
Oct 22, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was just noticing in #246 that
get-dagdoesn't seem quite right so went about trying to address it, then in the process I'm realising that I don't have much use for CARv2 with much of the stuff I'm interested in and not having the option to make v1 CARs is a bit annoying, so I added a-versionto it as well.The current form seems to write out every matching node in a selector, not block, which will get pretty gnarly with complex blocks and a match-all and likely not what the user wants. This version uses the selector for the walk only (so it can be an explore, not a match) and uses the block loader to indicate what blocks are visited. It's not entirely unreasonable that a user may want to match individual nodes and turn them into blocks, but I think it's very unlikely desired behaviour and they're thinking in terms of whole blocks contained within the DAG described by a selector that will walk that DAG.
This needs tests, the sample v1 and wrapped v2 are kind of funky, with inlines and some weird stuff that I'm not sure how to account for properly in tests so I was thinking about making a simple DAG that can have selectors run over.
I also wanted to get the same thing for
ipfs dag exportwhich is exclusively CARv1 (atm anyway): ipfs/kubo#8506 - I was making way for selectors in there too but figure that maybe this is the best place to experiment with some of that crazy, including questions of what to do with excessively linked DAGs that make the traversal too onerous.