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

implement ipfs files command #1769

Merged
merged 2 commits into from
Oct 4, 2015
Merged

implement ipfs files command #1769

merged 2 commits into from
Oct 4, 2015

Conversation

whyrusleeping
Copy link
Member

@jbenet
image

@diasdavid @mappum Heres the mfs api, Its gonna make things pretttty cooooool.

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

@jbenet jbenet added the status/in-progress In progress label Sep 30, 2015
@whyrusleeping whyrusleeping force-pushed the mfs-api-redux branch 2 times, most recently from 48bb130 to ea69975 Compare September 30, 2015 05:42
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
@whyrusleeping
Copy link
Member Author

terrible luck with tests... lets try that again.

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

pretttty cooooool.
-- @diasdavid

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

what looks better?

ipfs unix read
ipfs unix write
ipfs unix mv
ipfs unix cp
ipfs unix ls
ipfs unix mkdir
ipfs unix stat
ipfs unix rm

or

ipfs files read
ipfs files write
ipfs files mv
ipfs files cp
ipfs files ls
ipfs files mkdir
ipfs files stat
ipfs files rm

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

is vendoring broken? the circleci tests look odd

https://circleci.com/gh/ipfs/go-ipfs/1391

}

res.SetOutput(&Object{
Hash: k.B58String(),
Copy link
Member

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)

@jbenet
Copy link
Member

jbenet commented Sep 30, 2015

is vendoring broken? the circleci tests look odd

https://circleci.com/gh/ipfs/go-ipfs/1391

return
default:
res.SetError(errors.New("unrecognized type"), cmds.ErrNormal)
}
Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, agreed with @jbenet. This is the kind of walk you usually don't want to take again and again.

Copy link
Contributor

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)

@ghost ghost mentioned this pull request Sep 30, 2015
88 tasks

switch {
case err == ds.ErrNotFound || val == nil:
nd = &merkledag.Node{Data: unixfs.FolderPBData()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

@whyrusleeping
Copy link
Member Author

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

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 :)

@rht
Copy link
Contributor

rht commented Oct 1, 2015

plan9

mv file1 file2
mv file ... directory

http://man.cat-v.org/plan_9/1/cp

return
}

res.SetOutput(&Object{
Copy link
Contributor

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

Copy link
Contributor

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.

@jbenet
Copy link
Member

jbenet commented Oct 2, 2015

good tests! write fails need a bit of improvement:

  • on failures: check things haven't actually changed

also:

  • some silly whitespace things
  • path len(0) check

this is looking pretty good to me! 👍 excited to land this

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
@whyrusleeping
Copy link
Member Author

@jbenet Alright, those are some tests!

@whyrusleeping
Copy link
Member Author

@jbenet thoughts here?

@jbenet
Copy link
Member

jbenet commented Oct 4, 2015

@whyrusleeping investigating why the circle-ci tests are failing.

for one, trying to run make vendor, need to get ipfs/go-log:

> 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

forget to push or something?

nvm. outdated logrus. fixed.

jbenet added a commit that referenced this pull request Oct 4, 2015
implement ipfs files command
@jbenet jbenet merged commit b2695b0 into dev0.4.0 Oct 4, 2015
@jbenet jbenet removed the status/in-progress In progress label Oct 4, 2015
@jbenet jbenet deleted the mfs-api-redux branch October 4, 2015 08:16
@jbenet
Copy link
Member

jbenet commented Oct 4, 2015

mfs (ipfs files for now) has shipped!

solid work from @whyrusleeping to land this.

remaining questions:

  • this is still an experimental command that might change a bit
  • play with the UI/UX and let us know how it is.
  • let us know what you prefer: ipfs files or ipfs unix.
    • ipfs unix may be more accurate/less misleading even if a bit more confusing at first
    • ipfs mfs -- could use this for merkledag-fs or something. (but mfs may be reserved for the "with mounts and hooks" one)

@jbenet
Copy link
Member

jbenet commented Oct 4, 2015

(this is in 0.8.3-dev, 0.8.3 will be out soon)

@daviddias
Copy link
Member

so rad! Thank you for putting all of this together so quickly @whyrusleeping !! :D

@whyrusleeping
Copy link
Member Author

(this is in 0.8.3-dev, 0.8.3 will be out soon)

I actually moved it forward from the distant future of 0.8.3 to the nearer future of 0.4.0

@jbenet
Copy link
Member

jbenet commented Oct 5, 2015

Oh sorry, whoops!


var FilesCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Manipulate unixfs files",
Copy link
Member

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)

Copy link
Member

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)

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.

5 participants