-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add carve utility for updating the index of a car{v1,v2} file #219
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.
Any particular reason you made another module? this could live in go-car/v2 as far as I can tell, like in v2/cmd/carve
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'd love to reduce the code gymnastics in the CLI and capture some of them in convenient APIs.
Blocker: index marshalling
carve/carve.go
Outdated
Aliases: []string{"i"}, | ||
Usage: "write out the car with an index", | ||
Action: func(c *cli.Context) error { | ||
r, err := carv2.OpenReader(c.Args().Get(0)) |
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.
Would be nice to have a named flag for this, e.g. -i
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 this is fine as a positional argument
carve/carve.go
Outdated
|
||
outStream := os.Stdout | ||
if c.Args().Len() >= 2 { | ||
outStream, err = os.Create(c.Args().Get(1)) |
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.
Would be nice to have a named flag for this, e.g. -o
carve/carve.go
Outdated
} | ||
defer r.Close() | ||
|
||
var idx index.Index |
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.
We may have been given a CARv2 that has the desired index already; would it make sense to check for that and skip extra work if not needed?
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 in this context i'm fine without that optimization
I'm noting that running:
from the
I wonder if that means this isn't striding over blocks properly / ends up misaligned in the v1 case |
run as
carve index -c CarMultihashIndexSorted test.car