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

@@ -194,6 +194,7 @@ collected if needed. (By default, recursively. Use -r=false for direct pins.)
 	},
 	Options: []cmdkit.Option{
 		cmdkit.BoolOption(pinRecursiveOptionName, "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) {
@@ -216,7 +217,13 @@ collected if needed. (By default, recursively. Use -r=false for direct pins.)
 			return
 		}
 
-		removed, err := corerepo.Unpin(n, api, 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, api, req.Context(), req.Arguments(), recursive, explain)
 		if err != nil {
 			res.SetError(err, cmdkit.ErrNormal)
 			return

core/coreapi/pin.go

@@ -53,12 +53,12 @@ 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, api.core(), ctx, []string{p.String()}, true)
+	_, err := corerepo.Unpin(api.node, api.core(), ctx, []string{p.String()}, true, true)
 	if err != nil {
 		return err
 	}
 
-	return api.node.Pinning.Flush()
+	return nil
 }
 
 func (api *PinAPI) Update(ctx context.Context, from coreiface.Path, to coreiface.Path, opts ...caopts.PinUpdateOption) error {

core/corerepo/pinning.go

@@ -51,7 +51,7 @@ func Pin(n *core.IpfsNode, api iface.CoreAPI, ctx context.Context, paths []strin
 	return out, nil
 }
 
-func Unpin(n *core.IpfsNode, api iface.CoreAPI, ctx context.Context, paths []string, recursive bool) ([]cid.Cid, error) {
+func Unpin(n *core.IpfsNode, api iface.CoreAPI, ctx context.Context, paths []string, recursive bool, explain bool) ([]*cid.Cid, error) {
 	unpinned := make([]cid.Cid, len(paths))
 
 	for i, p := range paths {
@@ -65,7 +65,7 @@ func Unpin(n *core.IpfsNode, api iface.CoreAPI, ctx context.Context, paths []str
 			return nil, err
 		}
 
-		err = n.Pinning.Unpin(ctx, k.Cid(), recursive)
+		err = n.Pinning.Unpin(ctx, k.Cid(), recursive, explain)
 		if err != nil {
 			return nil, err
 		}

core/coreunix/add.go

@@ -178,7 +178,7 @@ func (adder *Adder) PinRoot() error {
 	}
 
 	if adder.tempRoot.Defined() {
-		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

@@ -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
@@ -266,29 +266,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 {

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

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
 '

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