From e66b774faa8c426f97aec72ff72f4ccb2d5dd05b Mon Sep 17 00:00:00 2001 From: Iaroslav Gridin Date: Sun, 15 Jan 2017 19:13:46 +0200 Subject: [PATCH] Make pin rm not try to find pinned ancestors by default Also make a switch to pin rm to allow old behavior. Trying to look up pins which interfere with requested unpin can be an extremely costly operation and typically is not required by user. License: MIT Signed-off-by: Iaroslav Gridin --- core/commands/pin.go | 9 +++++- core/corerepo/pinning.go | 4 +-- core/coreunix/add.go | 2 +- pin/pin.go | 54 +++++++++++++++++++++++++-------- pin/pin_test.go | 6 ++-- test/sharness/t0080-repo.sh | 15 ++++++++- test/sharness/t0252-files-gc.sh | 2 +- 7 files changed, 70 insertions(+), 22 deletions(-) diff --git a/core/commands/pin.go b/core/commands/pin.go index d774dcbe02e..62c60454211 100644 --- a/core/commands/pin.go +++ b/core/commands/pin.go @@ -184,6 +184,7 @@ collected if needed. (By default, recursively. Use -r=false for direct pins.) }, Options: []cmds.Option{ cmds.BoolOption("recursive", "r", "Recursively unpin the object linked to by the specified object(s).").Default(true), + cmds.BoolOption("explain", "e", "Check for other pinned objects which could cause specified object(s) to be indirectly pinned").Default(false), }, Type: PinOutput{}, Run: func(req cmds.Request, res cmds.Response) { @@ -200,7 +201,13 @@ collected if needed. (By default, recursively. Use -r=false for direct pins.) return } - removed, err := corerepo.Unpin(n, req.Context(), req.Arguments(), recursive) + explain, _, err := req.Option("explain").Bool() + if err != nil { + res.SetError(err, cmds.ErrNormal) + return + } + + removed, err := corerepo.Unpin(n, req.Context(), req.Arguments(), recursive, explain) if err != nil { res.SetError(err, cmds.ErrNormal) return diff --git a/core/corerepo/pinning.go b/core/corerepo/pinning.go index aad23d09093..13d812e8bfe 100644 --- a/core/corerepo/pinning.go +++ b/core/corerepo/pinning.go @@ -67,7 +67,7 @@ func Pin(n *core.IpfsNode, ctx context.Context, paths []string, recursive bool) return out, nil } -func Unpin(n *core.IpfsNode, ctx context.Context, paths []string, recursive bool) ([]*cid.Cid, error) { +func Unpin(n *core.IpfsNode, ctx context.Context, paths []string, recursive bool, explain bool) ([]*cid.Cid, error) { var unpinned []*cid.Cid r := &path.Resolver{ @@ -88,7 +88,7 @@ func Unpin(n *core.IpfsNode, ctx context.Context, paths []string, recursive bool ctx, cancel := context.WithCancel(ctx) defer cancel() - err = n.Pinning.Unpin(ctx, k, recursive) + err = n.Pinning.Unpin(ctx, k, recursive, explain) if err != nil { return nil, err } diff --git a/core/coreunix/add.go b/core/coreunix/add.go index 8539bf1087c..63d0a217097 100644 --- a/core/coreunix/add.go +++ b/core/coreunix/add.go @@ -194,7 +194,7 @@ func (adder *Adder) PinRoot() error { } if adder.tempRoot != nil { - err := adder.pinning.Unpin(adder.ctx, adder.tempRoot, true) + err := adder.pinning.Unpin(adder.ctx, adder.tempRoot, true, false) if err != nil { return err } diff --git a/pin/pin.go b/pin/pin.go index c73d3dd7ba9..976227fcf79 100644 --- a/pin/pin.go +++ b/pin/pin.go @@ -85,7 +85,7 @@ type Pinner interface { IsPinned(*cid.Cid) (string, bool, error) IsPinnedWithType(*cid.Cid, PinMode) (string, bool, error) Pin(context.Context, node.Node, bool) error - Unpin(context.Context, *cid.Cid, bool) error + Unpin(context.Context, *cid.Cid, bool, bool) error // Update updates a recursive pin from one cid to another // this is more efficient than simply pinning the new one and unpinning the @@ -207,30 +207,58 @@ func (p *pinner) Pin(ctx context.Context, node node.Node, recurse bool) error { var ErrNotPinned = fmt.Errorf("not pinned") // Unpin a given key -func (p *pinner) Unpin(ctx context.Context, c *cid.Cid, recursive bool) error { +func (p *pinner) Unpin(ctx context.Context, c *cid.Cid, recursive bool, explain bool) error { p.lock.Lock() defer p.lock.Unlock() - reason, pinned, err := p.isPinnedWithType(c, Any) + var pinMode PinMode + if recursive { + pinMode = Recursive + } else { + pinMode = Direct + } + _, pinned, err := p.isPinnedWithType(c, pinMode) if err != nil { return err } - if !pinned { - return ErrNotPinned - } - switch reason { - case "recursive": + if pinned { if recursive { p.recursePin.Remove(c) return nil } else { + p.directPin.Remove(c) + return nil + } + } else if recursive { + _, pinned, err := p.isPinnedWithType(c, Direct) + if err != nil { + return err + } + if pinned { + p.directPin.Remove(c) + return nil + } + } + + if explain { + reason, pinned, err := p.isPinnedWithType(c, Any) + if err != nil { + return err + } + if !pinned { + return ErrNotPinned + } + switch reason { + case "recursive": return fmt.Errorf("%s is pinned recursively", c) + default: + return fmt.Errorf("%s is pinned indirectly under %s", c, reason) } - case "direct": - p.directPin.Remove(c) - return nil - default: - return fmt.Errorf("%s is pinned indirectly under %s", c, reason) + } else if recursive { + return fmt.Errorf("%s is not pinned recursively or directly", c) + } else { + return fmt.Errorf("%s is not pinned directly", c) } + } func (p *pinner) isInternalPin(c *cid.Cid) bool { diff --git a/pin/pin_test.go b/pin/pin_test.go index 57683eef9f2..1f5d41bbfb7 100644 --- a/pin/pin_test.go +++ b/pin/pin_test.go @@ -134,7 +134,7 @@ func TestPinnerBasic(t *testing.T) { assertPinned(t, p, dk, "pinned node not found.") // Test recursive unpin - err = p.Unpin(ctx, dk, true) + err = p.Unpin(ctx, dk, true, false) if err != nil { t.Fatal(err) } @@ -256,7 +256,7 @@ func TestIsPinnedLookup(t *testing.T) { assertPinned(t, p, bk, "B should be pinned") // Unpin A5 recursively - if err := p.Unpin(ctx, aKeys[5], true); err != nil { + if err := p.Unpin(ctx, aKeys[5], true, false); err != nil { t.Fatal(err) } @@ -264,7 +264,7 @@ func TestIsPinnedLookup(t *testing.T) { assertUnpinned(t, p, aKeys[4], "A4 should be unpinned") // Unpin B recursively - if err := p.Unpin(ctx, bk, true); err != nil { + if err := p.Unpin(ctx, bk, true, false); err != nil { t.Fatal(err) } assertUnpinned(t, p, bk, "B should be unpinned") diff --git a/test/sharness/t0080-repo.sh b/test/sharness/t0080-repo.sh index e1bf97c3a48..196380e02c8 100755 --- a/test/sharness/t0080-repo.sh +++ b/test/sharness/t0080-repo.sh @@ -84,11 +84,17 @@ test_expect_success "pinning directly should fail now" ' ' test_expect_success "'ipfs pin rm -r=false ' should fail" ' - echo "Error: $HASH is pinned recursively" >expected4 && + echo "Error: $HASH is not pinned directly" >expected4 && test_must_fail ipfs pin rm -r=false "$HASH" 2>actual4 && test_cmp expected4 actual4 ' +test_expect_success "'ipfs pin rm -e -r=false ' should fail and explain why" ' + echo "Error: $HASH is pinned recursively" >expected11 && + test_must_fail ipfs pin rm -e -r=false "$HASH" 2>actual11 && + test_cmp expected11 actual11 +' + test_expect_success "remove recursive pin, add direct" ' echo "unpinned $HASH" >expected5 && ipfs pin rm -r "$HASH" >actual5 && @@ -98,6 +104,13 @@ test_expect_success "remove recursive pin, add direct" ' test_expect_success "remove direct pin" ' echo "unpinned $HASH" >expected6 && + ipfs pin rm -r=false "$HASH" >actual6 && + test_cmp expected6 actual6 +' + +test_expect_success "remove direct pin with pin rm -r" ' + echo "unpinned $HASH" >expected6 && + ipfs pin add -r=false "$HASH" ipfs pin rm "$HASH" >actual6 && test_cmp expected6 actual6 ' diff --git a/test/sharness/t0252-files-gc.sh b/test/sharness/t0252-files-gc.sh index 10928c4247a..531b7aff505 100755 --- a/test/sharness/t0252-files-gc.sh +++ b/test/sharness/t0252-files-gc.sh @@ -34,7 +34,7 @@ test_expect_success "gc okay after adding incomplete node -- prep" ' ipfs repo gc && # will remove /adir/file1 and /adir/file2 but not /adir test_must_fail ipfs cat $FILE1_HASH && ipfs files cp /ipfs/$ADIR_HASH /adir && - ipfs pin rm $ADIR_HASH + ipfs pin rm -r=false $ADIR_HASH ' test_expect_success "gc okay after adding incomplete node" '