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 arguments to 'ipfs pin ls' #2208

Merged
merged 8 commits into from
Jan 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
# go-ipfs changelog

### 0.4.0 - 2016-01-31

* Features
* add optional path arguments to 'ipfs pin ls' (@chriscool)

* Incompatible Changes
* the default for '--type' in 'ipfs pin ls' is now "all" (@chriscool)


### 0.3.10 - 2015-12-07

This patch update introduces the 'ipfs update' command which will be used for
Expand Down
159 changes: 111 additions & 48 deletions core/commands/pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ import (

key "github.com/ipfs/go-ipfs/blocks/key"
cmds "github.com/ipfs/go-ipfs/commands"
context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
core "github.com/ipfs/go-ipfs/core"
corerepo "github.com/ipfs/go-ipfs/core/corerepo"
dag "github.com/ipfs/go-ipfs/merkledag"
path "github.com/ipfs/go-ipfs/path"
u "github.com/ipfs/go-ipfs/util"
)

Expand Down Expand Up @@ -158,31 +161,41 @@ var listPinCmd = &cmds.Command{
Tagline: "List objects pinned to local storage",
ShortDescription: `
Returns a list of objects that are pinned locally.
By default, only recursively pinned returned, but others may be shown via the '--type' flag.
By default, all pinned objects are returned, but the '--type' flag or arguments can restrict that to a specific pin type or to some specific objects respectively.
`,
LongDescription: `
Returns a list of objects that are pinned locally.
By default, only recursively pinned returned, but others may be shown via the '--type' flag.
By default, all pinned objects are returned, but the '--type' flag or arguments can restrict that to a specific pin type or to some specific objects respectively.

Use --type=<type> to specify the type of pinned keys to list. Valid values are:
* "direct": pin that specific object.
* "recursive": pin that specific object, and indirectly pin all its decendants
* "indirect": pinned indirectly by an ancestor (like a refcount)
* "all"

With arguments, the command fails if any of the arguments is not a pinned object.
And if --type=<type> is additionally used, the command will also fail if any of the arguments is not of the specified type.

Example:
$ echo "hello" | ipfs add -q
QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
$ ipfs pin ls
QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN recursive
# now remove the pin, and repin it directly
$ ipfs pin rm QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
unpinned QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
$ ipfs pin add -r=false QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
pinned QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN directly
$ ipfs pin ls --type=direct
QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN direct
$ ipfs pin ls QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN
QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN direct
`,
},

Arguments: []cmds.Argument{
cmds.StringArg("ipfs-path", false, true, "Path to object(s) to be listed"),
},
Options: []cmds.Option{
cmds.StringOption("type", "t", "The type of pinned keys to list. Can be \"direct\", \"indirect\", \"recursive\", or \"all\". Defaults to \"recursive\""),
cmds.BoolOption("count", "n", "Show refcount when listing indirect pins"),
Expand All @@ -195,60 +208,37 @@ Example:
return
}

typeStr, found, err := req.Option("type").String()
typeStr, typeStrFound, err := req.Option("type").String()
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
if !found {
typeStr = "recursive"
}

switch typeStr {
case "all", "direct", "indirect", "recursive":
default:
err = fmt.Errorf("Invalid type '%s', must be one of {direct, indirect, recursive, all}", typeStr)
res.SetError(err, cmds.ErrClient)
}

keys := make(map[string]RefKeyObject)
if typeStr == "direct" || typeStr == "all" {
for _, k := range n.Pinning.DirectKeys() {
keys[k.B58String()] = RefKeyObject{
Type: "direct",
}
if typeStrFound {
switch typeStr {
case "all", "direct", "indirect", "recursive":
default:
err = fmt.Errorf("Invalid type '%s', must be one of {direct, indirect, recursive, all}", typeStr)
res.SetError(err, cmds.ErrClient)
return
}
} else {
typeStr = "all"
}
if typeStr == "indirect" || typeStr == "all" {
ks := key.NewKeySet()
for _, k := range n.Pinning.RecursiveKeys() {
nd, err := n.DAG.Get(n.Context(), k)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
err = dag.EnumerateChildren(n.Context(), n.DAG, nd, ks)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}

}
for _, k := range ks.Keys() {
keys[k.B58String()] = RefKeyObject{
Type: "indirect",
}
}
}
if typeStr == "recursive" || typeStr == "all" {
for _, k := range n.Pinning.RecursiveKeys() {
keys[k.B58String()] = RefKeyObject{
Type: "recursive",
}
}
var keys map[string]RefKeyObject

if len(req.Arguments()) > 0 {
keys, err = pinLsKeys(req.Arguments(), typeStr, req.Context(), n)
} else {
keys, err = pinLsAll(typeStr, req.Context(), n)
}

res.SetOutput(&RefKeyList{Keys: keys})
if err != nil {
res.SetError(err, cmds.ErrNormal)
} else {
res.SetOutput(&RefKeyList{Keys: keys})
}
},
Type: RefKeyList{},
Marshalers: cmds.MarshalerMap{
Expand Down Expand Up @@ -282,3 +272,76 @@ type RefKeyObject struct {
type RefKeyList struct {
Keys map[string]RefKeyObject
}

func pinLsKeys(args []string, typeStr string, ctx context.Context, n *core.IpfsNode) (map[string]RefKeyObject, error) {

keys := make(map[string]RefKeyObject)

for _, p := range args {
dagNode, err := core.Resolve(ctx, n, path.Path(p))
if err != nil {
return nil, err
}

k, err := dagNode.Key()
if err != nil {
return nil, err
}

pinType, pinned, err := n.Pinning.IsPinnedWithType(k, typeStr)
if err != nil {
return nil, err
}

if !pinned {
return nil, fmt.Errorf("Path '%s' is not pinned", p)
}

switch pinType {
case "direct", "indirect", "recursive", "internal":
default:
pinType = "indirect through " + pinType
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, it looks like pinLsKeys = ipfs resolve | isPinnedWithType.
I wonder why is this not in IsPinnedWithType?

gc.GC() in pin/gc/gc.go uses a kind of ipfs pin ls --type=all (in Descendants and ColoredSet), so there could be a refactor here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rht if you ask why the switch pinType { ... } is not part of IsPinnedWithType then my answer is that it would be inefficient for IsPinnedWithType users which are internal functions to have to parse "indirect through XXX" if they want to get XXX.

If there are many IsPinnedWithType users that want "indirect through XXX" then we can add IsPinnedWithTypePretty that just calls IsPinnedWithType and does what the switch pinType { ... } does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About refactoring with pin/gc/gc.go it does not look simple to me, and as ipfs pin ls --type=all already existed before this PR, it could have been done before. So it is independent and maybe you can open a new issue to suggest that refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are many IsPinnedWithType users that want "indirect through XXX" then we can add IsPinnedWithTypePretty that just calls IsPinnedWithType and does what the switch pinType { ... } does.

It could be the other way around, e.g. scripts parsing the output of ipfs pin ls would use IsPinnedWithType so that rows are mainly space separated.

}
keys[k.B58String()] = RefKeyObject{
Type: pinType,
}
}

return keys, nil
}

func pinLsAll(typeStr string, ctx context.Context, n *core.IpfsNode) (map[string]RefKeyObject, error) {

keys := make(map[string]RefKeyObject)

AddToResultKeys := func(keyList []key.Key, typeStr string) {
for _, k := range keyList {
keys[k.B58String()] = RefKeyObject{
Type: typeStr,
}
}
}

if typeStr == "direct" || typeStr == "all" {
AddToResultKeys(n.Pinning.DirectKeys(), "direct")
}
if typeStr == "indirect" || typeStr == "all" {
ks := key.NewKeySet()
for _, k := range n.Pinning.RecursiveKeys() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ctx: https://github.com/ipfs/go-ipfs/pull/2208/files#r50369858, this line until L340 could be replaced with gc.Descendants.

Copy link
Contributor

Choose a reason for hiding this comment

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

(if you don't mind to put the refactor in this pr; nvm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the last commit, thanks for the suggestion.

nd, err := n.DAG.Get(ctx, k)
if err != nil {
return nil, err
}
err = dag.EnumerateChildren(n.Context(), n.DAG, nd, ks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Listing indirect keys recursively appears only in here, while I think it should be the case in pinLsKeys's indirect as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, this is much like the ipfs ls and ipfs refs/ipfs refs -r case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pinLsKeys calls IsPinnedWithType() which does something like the above for indirect keys. See pin/pin.go around line 203.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chriscool IsPinnedWithType() does have hasChild, which is recursive. However dag.EnumerateChildren also adds all the child keys into a key.KeySet. The keyset is what is needed here for recursive ls. It is orthogonal to this PR, but just to point it out (of the possibility of ipfs pin ls -r $hash or ipfs pin refs -r and deduplications).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then please open another issue to suggest adding the -r flag, and by the way ipfs pin ls also has a --count flag but it is not used.

Copy link
Contributor

Choose a reason for hiding this comment

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

redirect: #2225

if err != nil {
return nil, err
}
}
AddToResultKeys(ks.Keys(), "indirect")
}
if typeStr == "recursive" || typeStr == "all" {
AddToResultKeys(n.Pinning.RecursiveKeys(), "recursive")
}

return keys, nil
}
39 changes: 32 additions & 7 deletions pin/pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const (

type Pinner interface {
IsPinned(key.Key) (string, bool, error)
IsPinnedWithType(key.Key, string) (string, bool, error)
Pin(context.Context, *mdag.Node, bool) error
Unpin(context.Context, key.Key, bool) error

Expand Down Expand Up @@ -126,7 +127,7 @@ func (p *pinner) Pin(ctx context.Context, node *mdag.Node, recurse bool) error {
func (p *pinner) Unpin(ctx context.Context, k key.Key, recursive bool) error {
p.lock.Lock()
defer p.lock.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should reuse the PinMode constants from line 31 or so. We would need to add Indirect and Any to the list, but it might be cleaner than using string constants (The string constants are my fault)

Copy link
Member

Choose a reason for hiding this comment

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

I can also change this later in a refactor, its somewhat tangental to your changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I would prefer to do that in a later refactor.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on constants (ok later too)

reason, pinned, err := p.isPinned(k)
reason, pinned, err := p.isPinnedWithType(k, "all")
if err != nil {
return err
}
Expand Down Expand Up @@ -159,22 +160,46 @@ func (p *pinner) isInternalPin(key key.Key) bool {
func (p *pinner) IsPinned(k key.Key) (string, bool, error) {
p.lock.RLock()
defer p.lock.RUnlock()
return p.isPinned(k)
return p.isPinnedWithType(k, "all")
}

// isPinned is the implementation of IsPinned that does not lock.
func (p *pinner) IsPinnedWithType(k key.Key, typeStr string) (string, bool, error) {
p.lock.RLock()
defer p.lock.RUnlock()
return p.isPinnedWithType(k, typeStr)
}

// isPinnedWithType is the implementation of IsPinnedWithType that does not lock.
// intended for use by other pinned methods that already take locks
func (p *pinner) isPinned(k key.Key) (string, bool, error) {
if p.recursePin.HasKey(k) {
func (p *pinner) isPinnedWithType(k key.Key, typeStr string) (string, bool, error) {
switch typeStr {
case "all", "direct", "indirect", "recursive", "internal":
default:
err := fmt.Errorf("Invalid type '%s', must be one of {direct, indirect, recursive, internal, all}", typeStr)
return "", false, err
}
if (typeStr == "recursive" || typeStr == "all") && p.recursePin.HasKey(k) {
return "recursive", true, nil
}
if p.directPin.HasKey(k) {
if typeStr == "recursive" {
return "", false, nil
}

if (typeStr == "direct" || typeStr == "all") && p.directPin.HasKey(k) {
return "direct", true, nil
}
if p.isInternalPin(k) {
if typeStr == "direct" {
return "", false, nil
}

if (typeStr == "internal" || typeStr == "all") && p.isInternalPin(k) {
return "internal", true, nil
}
if typeStr == "internal" {
return "", false, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

i wonder how much would break if something was pinned both recursively and directly (i.e. if the pinset was such that...)

Copy link
Member

Choose a reason for hiding this comment

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

We don't allow something to be pinned both directly and recursively

Copy link
Member

Choose a reason for hiding this comment

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

right, "if the pinset was such that..." -- i.e. there was a rogue pinset constructed.

how we fail in these scenarios is important. rogue malicious content can be crafted by anyone. its ok to have bad behavior in these failures, but the degree of badness / UX deterioration can certainly be safe or unsafe depending on how we handle it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here if something is pinned both directly and recursively, we just answer that it is pinned recursively. The changes in this PR don't change this answer. The same answer was given previously.

If you are worried about that it's probably better to create a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be redirected to an issue as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rht: I am not sure what @jbenet is worried about, so I don't know exactly what issue should be created. If you have an idea about what could be wrong or improved, please create the issue.


// Default is "indirect"
for _, rk := range p.recursePin.GetKeys() {
rnd, err := p.dserv.Get(context.Background(), rk)
if err != nil {
Expand Down
1 change: 0 additions & 1 deletion test/bin/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,4 @@
# Do not ignore the following special scripts
!checkflags
!continueyn
!ipfs-pin-stat
!verify-go-fmt.sh
30 changes: 0 additions & 30 deletions test/bin/ipfs-pin-stat

This file was deleted.

25 changes: 9 additions & 16 deletions test/sharness/t0081-repo-pinning.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,23 @@ test_description="Test ipfs repo pinning"

. lib/test-lib.sh



test_pin_flag() {
object=$1
ptype=$2
expect=$3

echo "test_pin_flag" $@
echo "test_pin_flag" "$@"

ipfs-pin-stat "$object" | egrep "\b$ptype\b"
actual=$?

if [ "$expect" = "true" ]; then
if [ "$actual" != "0" ]; then
echo "$object should be pinned $ptype ($actual)"
return 1
fi
if ipfs pin ls --type="$ptype" "$object" >actual
then
test "$expect" = "true" && return
test_fsh cat actual
return
else
if [ "$actual" != "1" ]; then
echo "$object should NOT be pinned $ptype ($actual)"
return 1
fi
test "$expect" = "false" && return
test_fsh cat actual
return
fi
return 0
}

test_pin() {
Expand Down