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 <[email protected]>
ipfs · Aug 31, 2018 · 9831231 · 9831231
1 parent 59805f0
commit 9831231
Show file tree

Hide file tree

Showing 8 changed files with 74 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,15 +261,15 @@ 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)
 	}
 
 	assertPinned(t, p, aKeys[0], "A0 should still be pinned through B")
 	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
@@ -86,11 +86,17 @@ test_expect_success "pinning directly should fail now" '
 '
 
 test_expect_success "'ipfs pin rm -r=false <hash>' 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 <hash>' 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
@@ -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" '