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

Add carve utility for updating the index of a car{v1,v2} file #219

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

willscott
Copy link
Member

run as
carve index -c CarMultihashIndexSorted test.car

@willscott willscott requested a review from masih September 7, 2021 14:43
Copy link
Contributor

@mvdan mvdan left a 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

carve/carve.go Outdated Show resolved Hide resolved
carve/carve.go Outdated Show resolved Hide resolved
carve/carve.go Outdated Show resolved Hide resolved
Copy link
Member

@masih masih left a 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 Show resolved Hide resolved
carve/carve.go Outdated Show resolved Hide resolved
carve/carve.go Outdated Show resolved Hide resolved
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))
Copy link
Member

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

Copy link
Member Author

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))
Copy link
Member

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
Copy link
Member

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?

Copy link
Member Author

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

carve/carve.go Outdated Show resolved Hide resolved
@willscott
Copy link
Member Author

I'm noting that running:

go run ./cmd/car i ./testdata/sample-v1.car > x.car

from the v2 directory results in the error:

expected 1 as the cid version number, got: 4018

I wonder if that means this isn't striding over blocks properly / ends up misaligned in the v1 case

@willscott willscott merged commit 2b19e2e into master Sep 8, 2021
@willscott willscott deleted the carve branch September 8, 2021 12:45
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

3 participants