-
-
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
Add 'ipfs file ls …' #1348
Add 'ipfs file ls …' #1348
Conversation
@wking let's use |
Agree. not sure what this would look like yet. |
On Tue, Jun 09, 2015 at 04:08:14PM -0700, Juan Batiz-Benet wrote:
Ok. How much of the code should be renamed from unixfs → files? |
just this command should be files. unixfs is fine in the codebase |
exactly. we use |
To be less confusing to newcomers (the IPFS filesystem isn't Unix-specific anyway, and it isn't even very POSIX-specific [1,2,3]). I'm a bit uncertain about having one name for users and another for devs, but the consensus seems to be that mainaining two names is worth the trouble [4]. We also kicked around: * 'files' (plural), * 'filesystem' (too long), and * 'fs' (redundant after 'ipfs', even though IPFS isn't just about filesystems) on IRC [5 through 6]. I wish there was a more evocative term. I'm never sure where "file" lands on the scale between "filesysytem", "everything is a file", "a single chunk of data with an associated inode". But we can't think of anything better. [1]: #1348 (comment) [2]: #1348 (comment) [3]: https://github.com/ipfs/go-ipfs/pull/1136/files#r29377283 In my response to this (no longer visibile on GitHub): On Wed, Apr 29, 2015 at 01:30:04PM -0700, Juan Batiz-Benet wrote: > > +package fsnode > > i think this package should be called `unixfs` as that's the > abstraction that this is calling to. Will do, although I don't see what's especially Unix-y about these file nodes. [4]: #1348 (comment) [5]: https://botbot.me/freenode/ipfs/2015-06-09/?msg=41428456&page=5 [6]: https://botbot.me/freenode/ipfs/2015-06-09/?msg=41430703&page=5
There were the following issues with your Pull Request
Guidelines are available at: https://github.com/ipfs/community/blob/master/docs/commit-message.md Your feedback on GitCop is welcome on the following issue: ipfs/infra#23 This message was auto-generated by https://gitcop.com |
To be less confusing to newcomers (the IPFS filesystem isn't Unix-specific anyway, and it isn't even very POSIX-specific [1,2,3]). I'm a bit uncertain about having one name for users and another for devs, but the consensus seems to be that mainaining two names is worth the trouble [4]. We also kicked around: * 'files' (plural), * 'filesystem' (too long), and * 'fs' (redundant after 'ipfs', even though IPFS isn't just about filesystems) on IRC [5 through 6]. I wish there was a more evocative term. I'm never sure where "file" lands on the scale between "filesysytem", "everything is a file", "a single chunk of data with an associated inode". But we can't think of anything better. [1]: #1348 (comment) [2]: #1348 (comment) [3]: https://github.com/ipfs/go-ipfs/pull/1136/files#r29377283 In my response to this (no longer visibile on GitHub): On Wed, Apr 29, 2015 at 01:30:04PM -0700, Juan Batiz-Benet wrote: > > +package fsnode > > i think this package should be called `unixfs` as that's the > abstraction that this is calling to. Will do, although I don't see what's especially Unix-y about these file nodes. [4]: #1348 (comment) [5]: https://botbot.me/freenode/ipfs/2015-06-09/?msg=41428456&page=5 [6]: https://botbot.me/freenode/ipfs/2015-06-09/?msg=41430703&page=5 License: MIT Signed-off-by: W. Trevor King <[email protected]>
@jbenet: can you weigh in on the following 'ipfs ls ...' vs. 'ipfs |
Oh interesting. Let me think about it.
Maybe we should have both? so that
and is compatible with tools that are One caveat: does this require downloading the children too? that's usually expensive and worth noting.
I think this is right? @whyrusleeping pls take a look.
dropping a blank line is fine, but we should still have nice CLI output, i.e. it should end with a newline (unless -q or a similar flag demands otherwise)
what is
I think we need universal self-description here. basically IPFS-LD. We'll regret it later otherwise. Let me think about this one more. |
On Thu, Jun 11, 2015 at 08:14:35PM -0700, Juan Batiz-Benet wrote:
In POSIX, the -q option is about hiding control characters 1. Maybe
Yes, although you'll only need to download the first Merkle object to
For non-JSON output this is how it was, is, and ever shall be. This
The command-line argument for which you're getting a result block. $ ipfs file ls File Somewhat similarly: $ ls -l bin/ .bashrc bin/: |
No, i meant
still more expensive. but ok.
we can just add it in the relevant commands
It's getting difficult to follow the formats in the (convoluted) cmdslib marshaller code. the sharness tests are better. perhaps one thing we can do is start compiling the cli interface design into per-command docs we derive the implementations + the sharness tests from. (ideally, we could just feed that to a "spec2sharness" tool that would check each example case or something. May be an upgrade to how sharness does things. |
On Thu, Jun 11, 2015 at 09:36:05PM -0700, Juan Batiz-Benet wrote:
Removing trailing newlines surprises me. Can you point me to the Git
I'd expect “the relevant commands” is “anything called from the
Yup 1. The changes in 30f7819 just make sure we always set it in
That sounds nice, but I'd rather not fall down the ipfs/specs#12 |
@wking i think this is all you need suppose i call
// i think this is all you strictly need
{
"QmXvEg83dD5PvaDUqHV17iXXtTpZMnPrpKR2cGyU3oCK5": {
"Links": [
{
"Name": "gentoo",
"Hash": "QmXAuRiZKYpCvUmFFBBEzAMvvAgw9Aoxcf24DYaWnG43Er",
"Size": 38775183,
"Type": 1
},
{
"Name": "protofs.md",
"Hash": "QmWpn9xUDNNj3XvY4ixMgb8imiDZmGDr1qQLr1DBv8WAnG",
"Size": 4953,
"Type": 2
}
]
}
}
// to make life easier, can:
{
"arguments": {
"/ipfs/Fooohash/bar": "QmXvEg83dD5PvaDUqHV17iXXtTpZMnPrpKR2cGyU3oCK5",
"/ipfs/Barhash": "QmXvEg83dD5PvaDUqHV17iXXtTpZMnPrpKR2cGyU3oCK5",
},
"objects": {
"QmXvEg83dD5PvaDUqHV17iXXtTpZMnPrpKR2cGyU3oCK5": {
"Links": [
{
"Name": "gentoo",
"Hash": "QmXAuRiZKYpCvUmFFBBEzAMvvAgw9Aoxcf24DYaWnG43Er",
"Size": 38775183,
"Type": 1
},
{
"Name": "protofs.md",
"Hash": "QmWpn9xUDNNj3XvY4ixMgb8imiDZmGDr1qQLr1DBv8WAnG",
"Size": 4953,
"Type": 2
}
]
}
}
} |
This is similar to 'ipfs ls ...', but it: * Lists file sizes that match the content size: $ ipfs --encoding=json unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4 { "Objects": [ { "Argument": "/ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4", "Links": [ { "Name": "busybox", "Hash": "QmPbjmmci73roXf9VijpyQGgRJZthiQfnEetaMRGoGYV5a", "Size": 1947624, "Type": 2 } ] } ] } $ ipfs cat /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox | wc -c 1947624 'ipfs ls ...', on the other hand, is using the Merkle-descendant size, which also includes fanout links and the typing information unixfs objects store in their Data: $ ipfs --encoding=json ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4 { "Objects": [ { "Hash": "/ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4", "Links": [ { "Name": "busybox", "Hash": "QmPbjmmci73roXf9VijpyQGgRJZthiQfnEetaMRGoGYV5a", "Size": 1948128, "Type": 2 } ] } ] } * Has a simpler text output corresponding to POSIX ls [1]: $ ipfs unixfs ls /ipfs/QmV2FrBtvue5ve7vxbAzKz3mTdWq8wfMNPwYd8d9KHksCF/gentoo/stage3/amd64/2015-04-02 bin dev etc proc run sys $ ipfs ls /ipfs/QmV2FrBtvue5ve7vxbAzKz3mTdWq8wfMNPwYd8d9KHksCF/gentoo/stage3/amd64/2015-04-02 QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4 1948183 bin/ QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn 4 dev/ QmUz1Z5jnQEjwr78fiMk5babwjJBDmhN5sx5HvPiTGGGjM 1207 etc/ QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn 4 proc/ QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn 4 run/ QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn 4 sys/ The minimal output allows us to start off with POSIX compliance and then add options (which may or may not be POSIX compatible) to adjust the output format as we get a better feel for what we need ([2] through [3]). [1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html [2]: https://botbot.me/freenode/ipfs/2015-06-12/?msg=41724727&page=5 [3]: https://botbot.me/freenode/ipfs/2015-06-12/?msg=41725146&page=5 License: MIT Signed-off-by: W. Trevor King <[email protected]>
Folks operating at the Unix-filesystem level shouldn't care about that level of Merkle-DAG detail. Before this commit we had: $ ipfs unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox: ... several lines of empty-string names ... And with this commit we have: $ ipfs unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox I also reworked the argument-prefixing (object.Argument) in the output marshaller to avoid redundancies like: $ ipfs unixfs ls /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox: /ipfs/QmSRCHG21Sbqm3EJG9aEBo4vS7Fqu86pAjqf99MyCdNxZ4/busybox As a side-effect of this rework, we no longer have the trailing blank line that we used to have after the final directory listing. The new ErrImplementation is like Python's NotImplementedError, and is mostly a way to guard against external changes that would need associated updates in this code. For example, once we see something that's neither a file nor a directory, we'll have to update the switch statement to handle those objects. License: MIT Signed-off-by: W. Trevor King <[email protected]>
To be less confusing to newcomers (the IPFS filesystem isn't Unix-specific anyway, and it isn't even very POSIX-specific [1,2,3]). I'm a bit uncertain about having one name for users and another for devs, but the consensus seems to be that mainaining two names is worth the trouble [4]. We also kicked around: * 'files' (plural), * 'filesystem' (too long), and * 'fs' (redundant after 'ipfs', even though IPFS isn't just about filesystems) on IRC [5 through 6]. I wish there was a more evocative term. I'm never sure where "file" lands on the scale between "filesysytem", "everything is a file", "a single chunk of data with an associated inode". But we can't think of anything better. [1]: #1348 (comment) [2]: #1348 (comment) [3]: https://github.com/ipfs/go-ipfs/pull/1136/files#r29377283 In my response to this (no longer visibile on GitHub): On Wed, Apr 29, 2015 at 01:30:04PM -0700, Juan Batiz-Benet wrote: > > +package fsnode > > i think this package should be called `unixfs` as that's the > abstraction that this is calling to. Will do, although I don't see what's especially Unix-y about these file nodes. [4]: #1348 (comment) [5]: https://botbot.me/freenode/ipfs/2015-06-09/?msg=41428456&page=5 [6]: https://botbot.me/freenode/ipfs/2015-06-09/?msg=41430703&page=5 License: MIT Signed-off-by: W. Trevor King <[email protected]>
Rerolled to use Juan's new suggested JSON output. |
directories := []string{} | ||
for argument := range output.Arguments { | ||
hash := output.Arguments[argument] | ||
object := output.Objects[hash] |
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 check if the object is there. it may not be, and cause a crash below.
object, ok := output.Objects[hash]
if !ok { return error }
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.
On Tue, Jun 16, 2015 at 03:51:31PM -0700, Juan Batiz-Benet wrote:
hash := output.Arguments[argument]
object := output.Objects[hash]
should check if the object is there. it may not be, and cause a
crash below.
Did you actually see a crash? We should be setting Objects[hash] for
every resolved argument 1. That said, I guess it doesn't hurt to
check the status of every dict lookup…
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 haven't tested it, but I see that this is a hole.
- bugs or changes can creep in and violate assumptions
- the CLI may be used against any process implementing the IPFS API (+/- bugs there too). there is zero guarantee it is the same code that is in go-ipfs.
- safety first :) -- the data ought to be validated.
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.
On Tue, Jun 16, 2015 at 04:31:31PM -0700, Juan Batiz-Benet wrote:
- the CLI may be used against any process implementing the IPFS
API. there is zero guarantee it is the same code that is in
go-ipfs.
Where does that split land? I'd expect both the Run and Marshalers
handling to happen on the daemon side, so they need to be in sync with
each other, but not necessarily with any client code.
I'll reroll to check the map-access (since I understand points (1) and
(3)), but if there's some important kernel of wisdom in point (2), I'd
like to clear up my confusion there ;).
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 just mean that there is a json API, and other implementations may match it, but have problems. so even though the Run and Marshalers are related in go-ipfs, it does not mean the cli will always be run against a go-ipfs daemon
Comments above o/ |
Instead of abusing a LsLink for non-directory objects [1]. [1]: #1348 (comment) License: MIT Signed-off-by: W. Trevor King <[email protected]>
@wking this LGTM! thanks. once the ok check is done, this is RFM, i think |
Instead of abusing a LsLink for non-directory objects [1]. [1]: #1348 (comment) License: MIT Signed-off-by: W. Trevor King <[email protected]>
On Thu, Jun 18, 2015 at 05:28:35PM -0700, Juan Batiz-Benet wrote:
I added an ok check for output.Objects[hash] and shifted the |
So I can bind to the Unix-filesystem-layer version of ls that landed with [1]. [1]: ipfs/kubo#1348
So I can bind to the Unix-filesystem-layer version of ls that landed with [1]. I've spun this layer off into it's own file to avoid crowding shell.go. [1]: ipfs/kubo#1348
So I can bind to the Unix-filesystem-layer version of ls that landed with [1]. I've spun this layer off into it's own file to avoid crowding shell.go. [1]: ipfs/kubo#1348
Change the approach to the directory-header control so we can set the Argument value in the JSON response. Stripping the trailing newline from the JSON output is annoying, but looking over [1] I saw no easy way to add a newline to the JSON output. And with the general framework that commands/ attempts to be, it feels a bit funny to customize the JSON output for a command-line program. Perhaps a workable solution is to have the command-line client append newlines to any output that otherwise lacks them? But that seems like a change best left to a separate series. [1]: http://golang.org/pkg/encoding/json/ License: MIT Signed-off-by: W. Trevor King <[email protected]>
This doesn't affect the text output, which was already using a stringified name. The earlier stringification does change the JSON output from an enumeration integer (e.g. 2) to the string form (e.g. "File"). If/when we transition to Merkle-object types named by their hash, we will probably want to revisit this and pass both the type hash and human-readable-but-collision-prone name on to clients. License: MIT Signed-off-by: W. Trevor King <[email protected]>
Discussing this on IRC ([1] through [2]), Jeromy and I decided that we'd really like a way to configure per-command [3] and per-action timeouts, but until we have that we want to leave the minute limit here. We also decided that the use of TODO here instead of the per-command req.Context().Context was a bug, which I'm fixing with this commit. [1]: https://botbot.me/freenode/ipfs/2015-06-12/?msg=41714126&page=4 [2]: https://botbot.me/freenode/ipfs/2015-06-12/?msg=41715618&page=4 [3]: #1325 License: MIT Signed-off-by: W. Trevor King <[email protected]>
Discussion with Juan on IRC ([1] through [2]) lead to this adjusted JSON output. Benefits over the old output include: * deduplication (we only check the children of a given Merkle node once, even if multiple arguments resolve to that hash) * alphabetized output (like POSIX's ls). As a side-effect of this change, I'm also matching GNU Coreutils' ls output (maybe in POSIX?) by printing an alphabetized list of non-directories (one per line) first, with alphabetized directory lists afterwards. [1]: https://botbot.me/freenode/ipfs/2015-06-12/?msg=41725570&page=5 [2]: https://botbot.me/freenode/ipfs/2015-06-12/?msg=41726547&page=5 License: MIT Signed-off-by: W. Trevor King <[email protected]>
We don't want to prefix these results with the argument. If there was only one argument, the unprefixed results are still explicit. License: MIT Signed-off-by: W. Trevor King <[email protected]>
Instead of abusing a LsLink for non-directory objects [1]. [1]: #1348 (comment) License: MIT Signed-off-by: W. Trevor King <[email protected]>
@wking LGTM. I'll run gpe. |
@jbenet every commit appears to have passed with GPE. I'll merge |
yep, LGTM. |
This is mostly to get an interface for listing the file-sizes for Unix
files (vs. the Merkle-ancestor sizes listed by
ipfs ls …
). Forbackground, see #543. For details on the individual commits, see the
commit messages.
The intended space for
ipfs unixfs …
is for folks operating withUnix-filesystem trees (obviously), so the API should tranparently
resolve any fanout. The current Merkle-level commands (
ipfs ls …
,etc.) on the other hand should be operating below the UnixFS level.
They probably shouldn't handle fanout, and they almost certainly
shouldn't be converting Merkle nodes to UnixFS nodes. For example,
ipfs cat …
might print the appended set of all constituent chunks,but it shouldn't be stripping the magic Data wrappers (
Type
,Filesize
,Blocksizes
, … inunixfs_pb.Data
), because ‘ipfs cat …’might be used to access non-UnixFS nodes.
I think there is space between the really-low-level
ipfs object …
API and the UnixFS API we're starting here for commands that only add
fanout and typing to the base Merkle objects. Then the UnixFS
implementation can build on that. For example, the
ipfs unixfs ls …
implementation here doesn't currently handle fanned-out directories.
Blocking issues for building such an intermediate API include generic
typing (vs. the Protobuf magic in
unixfs_pb.Data
, see ipfs/ipfs#36)and generic fanout (see ipfs/specs#10).
There are a few consistency issues with the exising
ipfs ls …
that Ipoint out in the commit messages, and we'll want to resolve those
before merging this PR.