-
-
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
implement ipfs files command #1769
Conversation
48bb130
to
ea69975
Compare
License: MIT Signed-off-by: Jeromy <[email protected]>
ea69975
to
de41a9f
Compare
terrible luck with tests... lets try that again. |
|
what looks better?
or
|
is vendoring broken? the circleci tests look odd |
} | ||
|
||
res.SetOutput(&Object{ | ||
Hash: k.B58String(), |
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.
probably want more here, maybe:
- size
- cumulative size
-
of files (if dir)
is vendoring broken? the circleci tests look odd |
return | ||
default: | ||
res.SetError(errors.New("unrecognized type"), cmds.ErrNormal) | ||
} |
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 put this (the meat) ls
into the mfs
module (or a subpkg like mfs/cmds
or something. i like how @rht has been moving add and cat into core. these Commands
should really just be glue between the commands
library, and the functionality. (i.e. so that users who import IPFS as a library could use them)
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 is just glue though, its converting the output from the mfs libs into the output we use in the cmds lib.
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.
it's doing something meaningful, like selecting what to do depending on the Directory or FIle, etc. this is the meat of ls, even though ls is small. and we should be able to call ls from importing Go 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.
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.
you mean walkFn and walk (for ls -R
)
|
||
switch { | ||
case err == ds.ErrNotFound || val == nil: | ||
nd = &merkledag.Node{Data: unixfs.FolderPBData()} |
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.
Should this be uio.NewEmptyDirectory()
? as in https://github.com/ipfs/go-ipfs/blob/master/core/corehttp/gateway_handler.go#L277
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.
oh, yeah. that works too.
f46286e
to
0dbc807
Compare
pretty sure circleCI cant handle PRs that arent based on master. |
|
||
for _, o := range out.Entries { | ||
if long { | ||
fmt.Fprintf(buf, "%s\t%s\t%d\n", o.Name, o.Hash, o.Size) |
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.
You might want to try tabwriter next time :)
plan9
|
return | ||
} | ||
|
||
res.SetOutput(&Object{ |
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.
ipfs object stat
also has LinksSize
, and if Blocks
should be NumLinks
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.
Except for the hash, why not reuse Stat()
as in ipfs object stat
? And the marshalers as well.
0dbc807
to
67b15a7
Compare
good tests! write fails need a bit of improvement:
also:
this is looking pretty good to me! 👍 excited to land this |
License: MIT Signed-off-by: Jeromy <[email protected]>
67b15a7
to
d69891f
Compare
@jbenet Alright, those are some tests! |
@jbenet thoughts here? |
@whyrusleeping investigating why the circle-ci tests are failing. for one, trying to run > go get github.com/ipfs/go-log
# github.com/ipfs/go-log
./oldlog.go:20: unknown logrus.TextFormatter field 'TimestampFormat' in struct literal
./oldlog.go:21: unknown logrus.TextFormatter field 'TimestampFormat' in struct literal
nvm. outdated logrus. fixed. |
mfs (
|
(this is in |
so rad! Thank you for putting all of this together so quickly @whyrusleeping !! :D |
I actually moved it forward from the distant future of |
Oh sorry, whoops! |
|
||
var FilesCmd = &cmds.Command{ | ||
Helptext: cmds.HelpText{ | ||
Tagline: "Manipulate unixfs files", |
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.
@whyrusleeping will we use unixfs, files and mfs interchangeably? (looking also at the remaining help descriptions for the commands)
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.
good question. i'm really not sure what the cleanest thing to do is. Posix files + dirs we use today across all OSes are unix derived, hence unixfs.
i think this should not be named mfs
, but {unix, unixfs, files}
. (mfs
will be confusing to people unless we really push it and use that name everywhere. i think the other thing -- with mounts -- is likely better target to keep for the mfs
name anyway. let's just say ipfs unix
or ipfs files
)
@jbenet
@diasdavid @mappum Heres the mfs api, Its gonna make things pretttty cooooool.
License: MIT
Signed-off-by: Jeromy [email protected]