-
Notifications
You must be signed in to change notification settings - Fork 167
feat: ipfs-unixfs get or "walk over anything" #189
Conversation
Ok(s) => s, | ||
Err(e) => { | ||
eprintln!("IPFS_PATH is not set or could not be read: {}", e); | ||
std::process::exit(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.
nit: unwrap_or_else
can be used here
Now that I have the http api and conformance tests going in another branch based on this, and there'll be a lot of rework of unmerged code, I'll probably scrap this review and open a new one a bit later. |
It appears I ended up just continuing where I left off. I'll try to write a better PR description before ready for review. |
This seems to be ready for review, with the linked tar-rs issue and such! Even the CI agrees: only transient failures left (macos sigsegv and nightly probably timeouts -- 23min without a test log). |
|
||
#[derive(Debug)] | ||
enum GetError { | ||
NonUtf8Symlink, |
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.
Wouldn't we want to provide the underlying bytes of a symlink that is not UTF-8? It might be "mangled" in a way that is not visible to the user, so if this is not immediately thrown only when given a direct symlink, it would be a good idea to indicate where the issue is.
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 error will actually be eaten by hyper
as it currently doesn't support returning trailer headers. No one would ever see the bytes. The tar
functionality is not exposed in the ipfs
crate. This is by design, we currently can't do it using current tar-rs
interfaces.
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 "design" idea was that we could provide the means to get
to local filesystem, which we almost do with ipfs::unixfs::ll::walk::Walker
(or ipfs_unixfs::walk::Walker
). Exporting to filesystem safely is a bit different task which I don't want to grow this massive PR into.
two buffers would allow better reusing, but only with concurrency.
this does not hit the buffer cycling cases but hits all other file cases.
this is probably being overly strict but at least there will not be any misunderstandings.
also the file was forgotten.
added hints about users preferring to use `ipfs_unixfs::walk::Walker`.
with patching we can run all of our outstanding work.
Including mostly comment fixes and removal of an extra &mut. Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
docs, comments and minor API changes again. Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
dcb939e
to
0c5ad59
Compare
Slight rebasing, merging. |
Add
/get
endpoint, which passes modified conformance tests (PR not yet created). There are "a few" commits throughout the days but I sadly did not find a way to make this any smaller.Most importantly this adds
ipfs_unixfs::walk::Walker
which can be used to walk the whole dag-pb/unixfs tree in a fashion useful when exporting files to filesystem or creating a tarball out of those. If you look at earlier commits, this used to beipfs_unixfs::dir::walk::Walker
. Initially it had a much less ergonomic API as well, but it has been ironed out.This PR includes some minor changes to path walking to fix the issues created by refactoring
ipfs_unixfs::UnexpectedNodeType
and eyeballing the code correctness.Reviewing:
/get
implementationMaking the Walker api more ergonomic resulted in a few more internal
unwrap
's which I think are all good.Pending in this PR:
Later:
unifyInnerKind
inwalk.rs
Root versions and use Cids in all of them (or none of them)Entry
values atContinuedWalk
level; not sure how to do this without it becoming a self-referential struct, perhapsWalker::continue_walk
could becomeWalker::continue_walk(&mut self, ...)
? The current item could always be moved to theContinuedWalk
.IpfsPath
andipfs_http::v0::refs::walk_path
over toipfs
underipfs::Ipfs::resolve
and merge theIpfsPath
, leave only supported featuresipfs::Ipfs::{get, add}
which only work on single block filesOpen questions:
is the fact that this contains code fromCreated issue Exposing currently private builder.rs, header.rs fns alexcrichton/tar-rs#235tar
clearly enough marked, should we add notices elsewhere? An external fork oftar
would be one way as the approach taken here is wildly different from async-tar or tokio-tar, which switch from std::io traits to the async versions.