Make pin rm not try to find pinned ancestors by default

[Voker57]

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]

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

[Voker57]

@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]

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

[Voker57]

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?

[whyrusleeping]

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 ?

[Voker57]

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

[kevina]

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.

[kevina]

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]

@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]

@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]

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

[whyrusleeping]

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

[Kubuxu]

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

[kevina]

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]

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]

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

[kevina]

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]

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]

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

[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.

[Voker57]

Appears to be already changed during some overhaul.

[Stebalien]

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