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 <voker57@gmail.com>

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

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
 		}

core/coreunix/add.go

@@ -197,7 +197,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
 		}

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 {

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,15 +256,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")

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 <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 &&
@@ -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
 '

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" '