From 1c98f6f61bed32e8b425283ff22bca63c00f8be8 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/coreapi/pin.go | 2 +- core/corerepo/pinning.go | 4 +-- core/coreunix/add.go | 2 +- pin/pin.go | 58 +++++++++++++++++++++++++-------- pin/pin_test.go | 6 ++-- test/sharness/t0080-repo.sh | 15 ++++++++- test/sharness/t0252-files-gc.sh | 2 +- 8 files changed, 74 insertions(+), 24 deletions(-) diff --git a/core/commands/pin.go b/core/commands/pin.go index c8f455d23f7..1da98571d10 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: []cmdkit.Option{ cmdkit.BoolOption("recursive", "r", "Recursively unpin the object linked to by the specified object(s).").WithDefault(true), + cmdkit.BoolOption("explain", "e", "Check for other pinned objects which could cause specified object(s) to be indirectly pinned").WithDefault(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, cmdkit.ErrNormal) + return + } + + removed, err := corerepo.Unpin(n, req.Context(), req.Arguments(), recursive, explain) if err != nil { res.SetError(err, cmdkit.ErrNormal) return diff --git a/core/coreapi/pin.go b/core/coreapi/pin.go index 50cf7e4f4a2..7e876afc9c3 100644 --- a/core/coreapi/pin.go +++ b/core/coreapi/pin.go @@ -53,7 +53,7 @@ func (api *PinAPI) Ls(ctx context.Context, opts ...caopts.PinLsOption) ([]coreif } func (api *PinAPI) Rm(ctx context.Context, p coreiface.Path) error { - _, err := corerepo.Unpin(api.node, ctx, []string{p.String()}, true) + _, err := corerepo.Unpin(api.node, ctx, []string{p.String()}, true, true) if err != nil { return err } diff --git a/core/corerepo/pinning.go b/core/corerepo/pinning.go index 81c843bc1b6..90b77eac6ac 100644 --- a/core/corerepo/pinning.go +++ b/core/corerepo/pinning.go @@ -58,7 +58,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) { unpinned := make([]*cid.Cid, len(paths)) r := &resolver.Resolver{ @@ -77,7 +77,7 @@ func Unpin(n *core.IpfsNode, ctx context.Context, paths []string, recursive bool return nil, err } - 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 e52c721f032..168627fc351 100644 --- a/core/coreunix/add.go +++ b/core/coreunix/add.go @@ -183,7 +183,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 cff9e4ae5bb..c57eea7d66c 100644 --- a/pin/pin.go +++ b/pin/pin.go @@ -116,7 +116,7 @@ type Pinner interface { // Unpin the given cid. If recursive is true, removes either a recursive or // a direct pin. If recursive is false, only removes a direct pin. - Unpin(ctx context.Context, cid *cid.Cid, recursive bool) error + Unpin(ctx context.Context, cid *cid.Cid, recursive bool, explain 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 @@ -254,29 +254,59 @@ func (p *pinner) Pin(ctx context.Context, node ipld.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 Mode + 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 } - return fmt.Errorf("%s is pinned recursively", c) - case "direct": - p.directPin.Remove(c) - return nil - default: - return fmt.Errorf("%s is pinned indirectly under %s", c, reason) + } 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) + } + } 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 4dc5e3565ee..b74bab256b0 100644 --- a/pin/pin_test.go +++ b/pin/pin_test.go @@ -137,7 +137,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) } @@ -261,7 +261,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) } @@ -269,7 +269,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 0147f4ea985..79b102baeed 100755 --- a/test/sharness/t0080-repo.sh +++ b/test/sharness/t0080-repo.sh @@ -86,11 +86,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 && @@ -100,6 +106,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 f5db0d33bd4..7f513a6214f 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" '