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 'ipfs file ls …' #1348

Merged
merged 9 commits into from
Jun 25, 2015
Merged

Add 'ipfs file ls …' #1348

merged 9 commits into from
Jun 25, 2015

Conversation

wking
Copy link
Contributor

@wking wking commented Jun 9, 2015

This is mostly to get an interface for listing the file-sizes for Unix
files (vs. the Merkle-ancestor sizes listed by ipfs ls …). For
background, see #543. For details on the individual commits, see the
commit messages.

The intended space for ipfs unixfs … is for folks operating with
Unix-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, … in unixfs_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 I
point out in the commit messages, and we'll want to resolve those
before merging this PR.

@jbenet jbenet added the status/in-progress In progress label Jun 9, 2015
@jbenet
Copy link
Member

jbenet commented Jun 9, 2015

@wking let's use ipfs files as the command. ipfs unixfs will be weird to most users.

@jbenet
Copy link
Member

jbenet commented Jun 9, 2015

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.

Agree. not sure what this would look like yet.

@wking
Copy link
Contributor Author

wking commented Jun 9, 2015

On Tue, Jun 09, 2015 at 04:08:14PM -0700, Juan Batiz-Benet wrote:

let's use ipfs files as the command? ipfs unixfs will be weird
to most users.

Ok. How much of the code should be renamed from unixfs → files?
core/commands/files? The current unixfs/ (protobuf definitions,
etc.)? None of it?

@whyrusleeping
Copy link
Member

just this command should be files. unixfs is fine in the codebase

@jbenet
Copy link
Member

jbenet commented Jun 9, 2015

just this command should be files. unixfs is fine in the codebase

exactly.

we use unixfs, but expose ipfs files in the commandline because the cli would be silly otherwise. it could be put in a unixfs standalone tool. (like ipfs name could go into ipns)

wking added a commit that referenced this pull request Jun 9, 2015
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
@GitCop
Copy link

GitCop commented Jun 9, 2015

There were the following issues with your Pull Request

  • Commit: 7c91e3a
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

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

wking added a commit that referenced this pull request Jun 9, 2015
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 <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Jun 11, 2015

@jbenet: can you weigh in on the following 'ipfs ls ...' vs. 'ipfs
file ls ...' consistency issues (I pointed to the commits in my
initial post, but since there haven't been comments on them I thought
I'd call them out again):

  • name markup (e.g. "bin" -> "bin/") vs. explicit type name
    (e.g. "Directory"). See d0ce146
  • dropped minute-timeout context.
    bdefbbb
  • dropped trailing blank line.
    bdefbbb
  • Hash vs. Argument in LsObjects (shows up in JSON output). See
    30f7819
  • string types in LsLink (shows up in JSON output).
    4232e9c

@jbenet
Copy link
Member

jbenet commented Jun 12, 2015

name markup (e.g. "bin" -> "bin/") vs. explicit type name
(e.g. "Directory"). See d0ce146

Oh interesting. Let me think about it.

  • FWIW, ls is very clear: it lists <dir>/ for directories.
  • There's also the d in mode, so it can list the type in some way.

Maybe we should have both? so that ipfs files ls dir -q shows:

file1
file2
dir3/
dir4/
file5

and is compatible with tools that are ls compatible?

One caveat: does this require downloading the children too? that's usually expensive and worth noting.

  • dropped minute-timeout context.
    bdefbbb

I think this is right?

@whyrusleeping pls take a look.

  • dropped trailing blank line.
    bdefbbb

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)

  • Hash vs. Argument in LsObjects (shows up in JSON output). See
    30f7819

what is Argument? I don't follow. I need to look at this PR in depth.

  • string types in LsLink (shows up in JSON output).
    4232e9c

I think we need universal self-description here. basically IPFS-LD. We'll regret it later otherwise. Let me think about this one more.

@wking
Copy link
Contributor Author

wking commented Jun 12, 2015

On Thu, Jun 11, 2015 at 08:14:35PM -0700, Juan Batiz-Benet wrote:

Maybe we should have both? so that ipfs files ls dir -q … is
compatible with tools that are ls compatible?

In POSIX, the -q option is about hiding control characters 1. Maybe
you meant -F or -p?

One caveat: does this require downloading the children too? that's
usually expensive and worth noting.

Yes, although you'll only need to download the first Merkle object to
figure out it's type (e.g. not the entirety of a chunked file or
fanned-out directory).

  • dropped trailing blank line.
    bdefbbb

… i.e. it should end with a newline (unless -q or a similar flag
demands otherwise)

For non-JSON output this is how it was, is, and ever shall be. This
just drops the trailing blank line (e.g. \n\n → \n). I'm happy to
have our default JSON encoder spit out a trailing newline, but I
didn't see an easy way to do that (discussion in 30f7819).

  • Hash vs. Argument in LsObjects (shows up in JSON output). See
    30f7819

what is Argument? I don't follow. I need to look at this PR in
depth.

The command-line argument for which you're getting a result block.
For example:

$ ipfs file ls
:

File

Somewhat similarly:

$ ls -l bin/ .bashrc
lrwxrwxrwx 1 wking wking 51 Nov 25 2013 .bashrc -> /home/wking/src/dotfiles/public/patched-src/.bashrc

bin/:
total 21824
lrwxrwxrwx 1 wking wking 37 Nov 25 2013 bin/dotfiles.sh -> ../src/dotfiles-framework/dotfiles.sh
-rwxr-xr-x 1 wking wking 22347064 Jun 11 21:02 ipfs

@jbenet
Copy link
Member

jbenet commented Jun 12, 2015

In POSIX, the -q option is about hiding control characters [1]. Maybe
you meant -F or -p?

No, i meant -q. In many git commands and in in many ipfs commands -q means quiet and removes unnecessary output (like a trailing newline).

Yes, although you'll only need to download the first Merkle object to
figure out it's type (e.g. not the entirety of a chunked file or
fanned-out directory).

still more expensive. but ok.

I'm happy to have our default JSON encoder spit out a trailing newline

we can just add it in the relevant commands

The command-line argument for which you're getting a result block.
For example:

ls only includes the output when there's multiple to differentiate between. If that's the case here, great.

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.

@wking
Copy link
Contributor Author

wking commented Jun 12, 2015

On Thu, Jun 11, 2015 at 09:36:05PM -0700, Juan Batiz-Benet wrote:

In POSIX, the -q option is about hiding control characters 1. Maybe
you meant -F or -p?

No, i meant -q. In many git commands and in in many ipfs commands
-q means quiet and removes unnecessary output (like a trailing
newline).

Removing trailing newlines surprises me. Can you point me to the Git
command that interprets -q in that way?

I'm happy to have our default JSON encoder spit out a trailing
newline

we can just add it in the relevant commands

I'd expect “the relevant commands” is “anything called from the
command-line”.

The command-line argument for which you're getting a result block.
For example:

ls only includes the output when there's multiple to
differentiate between. If that's the case here, great.

Yup 1. The changes in 30f7819 just make sure we always set it in
the JSON output (even though it's not always set in the text output).

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.

That sounds nice, but I'd rather not fall down the ipfs/specs#12
rabbit hole (machine-parsable specs for interactive systems are hard
;). In the absence of such a command already existing, I think the
best bet is to just iterate on the sharness tests first, and then
leave it to the implementater (who is hopefully involved in the test
discussion) to get the tests to pass.

@jbenet
Copy link
Member

jbenet commented Jun 12, 2015

@wking i think this is all you need

suppose i call

ls Foohash/bar Barhash
// 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 <wking@tremily.us>
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 <wking@tremily.us>
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 <wking@tremily.us>
@wking wking changed the title Add 'ipfs unixfs ls …' Add 'ipfs file ls …' Jun 15, 2015
@wking
Copy link
Contributor Author

wking commented Jun 15, 2015

Rerolled to use Juan's new suggested JSON output.

directories := []string{}
for argument := range output.Arguments {
hash := output.Arguments[argument]
object := output.Objects[hash]
Copy link
Member

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 } 

Copy link
Contributor Author

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…

Copy link
Member

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.

  1. bugs or changes can creep in and violate assumptions
  2. 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.
  3. safety first :) -- the data ought to be validated.

Copy link
Contributor Author

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:

  1. 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 ;).

Copy link
Member

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

@jbenet
Copy link
Member

jbenet commented Jun 16, 2015

Comments above o/

wking added a commit that referenced this pull request Jun 19, 2015
Instead of abusing a LsLink for non-directory objects [1].

[1]: #1348 (comment)

License: MIT
Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Jun 19, 2015

I just pushed fixes for the single directory listing 1 and the JSON
restructuring to avoid abusing LsLink 2. Still to go:

  • Adding missing 'ok' checks 3.

@jbenet
Copy link
Member

jbenet commented Jun 19, 2015

@wking this LGTM! thanks. once the ok check is done, this is RFM, i think

wking added a commit that referenced this pull request Jun 19, 2015
Instead of abusing a LsLink for non-directory objects [1].

[1]: #1348 (comment)

License: MIT
Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Jun 19, 2015

On Thu, Jun 18, 2015 at 05:28:35PM -0700, Juan Batiz-Benet wrote:

once the ok check is done, this is RFM, i think

I added an ok check for output.Objects[hash] and shifted the
output.Arguments value extraction into the range call. These changes
seemed unambiguous enough so I squashed them back into faf83f6
(core/commands/unixfs/ls: Hash-map for Objects, 2015-06-13) instead of
adding them as a new commit.

wking added a commit to wking/ipfs-shell that referenced this pull request Jun 19, 2015
So I can bind to the Unix-filesystem-layer version of ls that landed
with [1].

[1]: ipfs/kubo#1348
wking added a commit to wking/ipfs-shell that referenced this pull request Jun 19, 2015
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
wking added a commit to wking/ipfs-shell that referenced this pull request Jun 19, 2015
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 <wking@tremily.us>
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 <wking@tremily.us>
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 <wking@tremily.us>
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 <wking@tremily.us>
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 <wking@tremily.us>
Instead of abusing a LsLink for non-directory objects [1].

[1]: #1348 (comment)

License: MIT
Signed-off-by: W. Trevor King <wking@tremily.us>
@jbenet
Copy link
Member

jbenet commented Jun 23, 2015

@wking LGTM. I'll run gpe.

@whyrusleeping whyrusleeping mentioned this pull request Jun 23, 2015
34 tasks
@whyrusleeping
Copy link
Member

@jbenet every commit appears to have passed with GPE. I'll merge

whyrusleeping added a commit that referenced this pull request Jun 25, 2015
@whyrusleeping whyrusleeping merged commit 0332f3d into master Jun 25, 2015
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Jun 25, 2015
@jbenet jbenet deleted the tk/unixfs-ls branch June 25, 2015 20:56
@jbenet
Copy link
Member

jbenet commented Jun 25, 2015

yep, LGTM.

@whyrusleeping whyrusleeping mentioned this pull request Jul 1, 2015
44 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.

4 participants