Make pin rm not try to find pinned ancestors by default by Voker57 · Pull Request #3600 · ipfs/kubo

Voker57 · 2017-01-15T17:17:35Z

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.

whyrusleeping · 2017-01-17T18:05:09Z

@Voker57 Is this for the case where youre unpinning something 'recursively' and its pinned directly? I'm unsure the exact usecase here.

Voker57 · 2017-01-17T20:28:37Z

@whyrusleeping it's for the case when you're trying to unpin something which is not directly pinned: currently, in that case ipfs checks all the pins to see if they affect the requested nodes, and that can take huge amounts of time in case of big pinned items.

whyrusleeping · 2017-02-14T22:47:00Z

test/sharness/t0080-repo.sh

 test_expect_success "remove direct pin" '
 	echo "unpinned $HASH" >expected6 &&
-	ipfs pin rm "$HASH" >actual6 &&
+	ipfs pin rm -r=false "$HASH" >actual6 &&


This behaviour shouldnt change though, ipfs pin rm should remove either a recursive or a direct pin

But then there is no way to remove only recursive pins. -r will remove direct pins as well. Is that intended way for -r to behave?

Hrm... That was the behavior we initially intended to have. The idea was to mimic the unix rm command:

rm file #works rm dir #doesnt work rm -r file #works rm -r dir #works

This does bring up the equivalent of having an rmdir analogue though.

What do you think @jbenet @Kubuxu @lgierth ?

It's not mimicing rm -r anyway, since it will not delete included pins.

I agree that rm is a bad model. The type of a pin does not say anything about the type of a file. In addition I image most pins are recursive and direct pins are only used in special cases.

I partly take back what I said. I left another comment.

whyrusleeping

This is a good optimization, However I think we should default the explain flag to true, so that we retain current behavior by default, and also make sure that we don't change the behavior of having ipfs pin rm remove direct pins as well as recursive ones.

Voker57 · 2017-02-14T23:23:50Z

@whyrusleeping IMHO default behavior should be "do not explain", since it involves potentially extremely costly operation and chances that user needs resulting information are quite low. Trying to unpin a certain block/file? Then they should know ahead to run pin rm -e to determine pin source. My target case is unpinning already unpinned hash, and I think this is more likely to happen.

whyrusleeping · 2017-02-16T23:59:01Z

@Voker57 Okay, you've convinced me that having do not explain be the default is the right thing to do. That feels more natural to me now

whyrusleeping · 2017-02-16T23:59:39Z

(bumping to 0.4.7 as getting feedback on changing api behaviour will take a few days)

whyrusleeping · 2017-03-24T18:31:09Z

@Kubuxu @lgierth @kevina could I get you guys to review this?

Kubuxu · 2017-03-24T18:43:59Z

Regarding code LGTM, regarding UI, I am not sure but this probably applies to current UI too.

kevina · 2017-03-24T19:36:15Z

This very closely relates to #3796. Give me a few days to try this p.r. out and verify the current and changed behavior of of ipfs pin rm.

kevina · 2017-03-27T22:02:03Z

i don't like the change in behavior pin --recursive=true should remove both recursive a direct pins, pin --recursive=false should only remove direct pins.

Voker57 · 2017-03-27T23:30:48Z

How to remove only recursive pins them? Technically they are completely unrelated, imo interface implying they are is misleading.

kevina · 2017-03-28T00:24:42Z

I'm not sure, but, in my opinion there should be a way to remove a pin regardless of it's type. We should not change the default behavior in any case.

Perhaps the recursive option should be removed, and replaced with a --type option that defaults to any, this will make it more explicit what is going on. Or perhaps we can keep the recursive option as is and in addition add a --type option.

@whyrusleeping (and others) opinions?

Voker57 · 2017-03-28T00:53:39Z

It's not related to PR anyways, i just changed it because I thought current behavior is not intentional and test was not clear on that. I'll remove the change if -r is indeed supposed to remove both pins.

kevina · 2017-03-28T04:06:28Z

Without --explain enabled doing a ipfs pin rm on a direct pin will now return Error: not pinned which is misleading at best.

mistake

Kubuxu

@kevina is this change still relevant?
It seems to me that checking all pins when removing one pin is a bit over the top.

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>

Voker57 · 2019-06-03T14:30:07Z

Appears to be already changed during some overhaul.

Stebalien · 2019-06-03T23:41:52Z

This was fixed in #6311. I had no idea there was an existing PR.

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 548580b to 8e15b98 Compare January 15, 2017 17:18

Voker57 force-pushed the feat/pin-rm-explain-optional branch 3 times, most recently from 924b691 to 8f559bd Compare January 19, 2017 14:09

Kubuxu added this to the ipfs 0.4.6 milestone Jan 23, 2017

whyrusleeping reviewed Feb 14, 2017

View reviewed changes

whyrusleeping requested changes Feb 14, 2017

View reviewed changes

whyrusleeping modified the milestones: Ipfs 0.4.7, ipfs 0.4.6 Feb 16, 2017

whyrusleeping modified the milestones: Ipfs 0.4.8, Ipfs 0.4.7 Mar 11, 2017

whyrusleeping requested review from a user, Kubuxu and kevina March 24, 2017 18:30

whyrusleeping modified the milestones: Ipfs 0.4.9, Ipfs 0.4.8 Mar 24, 2017

kevina mentioned this pull request Mar 28, 2017

'pin rm' in the context of broken pins #3796

Open

kevina self-requested a review May 1, 2017 07:14

Voker57 force-pushed the feat/pin-rm-explain-optional branch 3 times, most recently from 1d2549c to 97f3725 Compare May 7, 2017 11:15

whyrusleeping modified the milestones: Ipfs 0.4.10, Ipfs 0.4.9 May 8, 2017

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 97f3725 to b673c0d Compare June 27, 2017 17:57

Voker57 force-pushed the feat/pin-rm-explain-optional branch from b673c0d to 5172e69 Compare July 14, 2017 22:17

magik6k modified the milestones: Ipfs 0.4.10, Ipfs 0.4.11 Jul 28, 2017

whyrusleeping modified the milestones: Ipfs 0.4.12, Ipfs 0.4.11 Aug 28, 2017

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 5172e69 to 9e39d43 Compare September 22, 2017 13:53

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 9e39d43 to 9ed32d5 Compare October 20, 2017 11:56

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 9ed32d5 to 05fd877 Compare November 1, 2017 14:10

whyrusleeping modified the milestones: Ipfs 0.4.12, go-ipfs 0.4.13 Nov 14, 2017

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 05fd877 to 7641c94 Compare December 16, 2017 04:46

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 7641c94 to 166d313 Compare January 19, 2018 20:14

whyrusleeping removed this from the go-ipfs 0.4.14 milestone Feb 10, 2018

Voker57 mentioned this pull request Mar 2, 2018

Named pins & pins stored in datastore #4757

Closed

5 tasks

Kubuxu reviewed Mar 6, 2018

View reviewed changes

Voker57 force-pushed the feat/pin-rm-explain-optional branch from 166d313 to 1c98f6f Compare September 9, 2018 08:39

Voker57 closed this Jun 3, 2019

Uh oh!

Conversation

Voker57 commented Jan 15, 2017

Uh oh!

whyrusleeping commented Jan 17, 2017

Uh oh!

Voker57 commented Jan 17, 2017

Uh oh!

whyrusleeping Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

Voker57 Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

whyrusleeping Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

Voker57 Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

kevina Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

kevina Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

whyrusleeping left a comment

Choose a reason for hiding this comment

Uh oh!

Voker57 commented Feb 14, 2017

Uh oh!

whyrusleeping commented Feb 16, 2017

Uh oh!

whyrusleeping commented Feb 16, 2017

Uh oh!

whyrusleeping commented Mar 24, 2017

Uh oh!

Kubuxu commented Mar 24, 2017

Uh oh!

kevina commented Mar 24, 2017

Uh oh!

kevina commented Mar 27, 2017

Uh oh!

Voker57 commented Mar 27, 2017

Uh oh!

kevina commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Voker57 commented Mar 28, 2017

Uh oh!

kevina commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kubuxu left a comment

Choose a reason for hiding this comment

Uh oh!

Voker57 commented Jun 3, 2019

Uh oh!

Stebalien commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kevina commented Mar 28, 2017 •

edited

Loading

kevina commented Mar 28, 2017 •

edited

Loading