Add arguments to 'ipfs pin ls' #2208

chriscool · 2016-01-16T15:28:48Z

This makes it possible to pass path arguments to ipfs pin ls to know if some specific objects are pinned.

To enable that IsPinnedWithType(key.Key, string) (string, bool, error) is added to the Pinner interface.

The benefits are that test/bin/ipfs-pin-stat can be removed, and t0081 is now faster (around 18 seconds now, versus 50 seconds before this PR on my machine).

whyrusleeping · 2016-01-17T06:26:17Z

core/commands/pin.go

-		if typeStr == "direct" || typeStr == "all" {
-			for _, k := range n.Pinning.DirectKeys() {
+
+		if (argCount > 0) {


[style] I would just inline len(req.Arguments()) here instead of declaring an extra variable for it

Ok, will do.

whyrusleeping · 2016-01-17T06:36:02Z

A few comments there, and also make sure to run go fmt on your code, go allows parentheses on if statements, but theyre not part of the official style so we always leave them off (and go fmt automatically removes them)

chriscool · 2016-01-17T07:45:59Z

Ok, I will push something cleaned up using go fmt soon.
Thanks!

chriscool · 2016-01-17T07:48:04Z

Done!

chriscool · 2016-01-17T09:44:59Z

The Travis CI build failure is the usual "no output received for 10 minutes".

jbenet · 2016-01-17T19:15:43Z

test/bin/ipfs-pin-stat

@@ -1,30 +0,0 @@
-#!/bin/sh


curious why this test is no longer needed?

It was not a test. It was a script used only by t0081.
Now we can do the same thing with ipfs pin ls <arg>... so this script is not needed any more.

oh ok i see it now.

jbenet · 2016-01-17T19:35:33Z

comments above, LGTM otherwise

chriscool · 2016-01-17T22:19:45Z

Ok, here is a new version. I think all the comments have been taken into account.

chriscool · 2016-01-18T04:53:25Z

The Travis CI an Circle CI failures are timeouts: "command make test_sharness_expensive took more than 10 minutes since last output"

jbenet · 2016-01-18T19:11:47Z

core/commands/pin.go

+			keys, err = pinLsKeys(req.Arguments(), typeStr, req.Context(), n)
+		} else {
+			if !typeStrFound {
+				typeStr = "recursive"


i think the default for typeStr should be the same in both cases (either "all" or "recursive") i fear that having them be different will confuse users?

Yeah, it would be more logical for the default to be the same in both cases, but when there are args it is natural to have the default be "all" and setting it to "all" when there are no args is a bit backward incompatible, so I preferred to avoid it.

Now if you are ok for the default being "all" in both case I can change that.

when there are args it is natural to have the default be "all"

yeah.

setting it to "all" when there are no args is a bit backward incompatible

yeah... :(

i think ipfs pin ls makes sense to default to all, too. we can put a notice and make sure to put it in the changelog. it would be nice to have a "incompatible changes" section that people could see easier.

Ok, I change the default to "all" and put something about that in the changelog.

Looking at CHANGELOG.md on GitHub, it seems that there is nothing about v0.3.11 and v0.4.0 in it. Are we still supposed to update it?

@chriscool yes we are.

@whyrusleeping why was the changelog not updated when you released 0.3.11?

@jbenet a rebased version is now available with a new commit that changes the default to "all" and adds entries to the changelog for v0.4.0.

It is more generic to be able to pass a pin type argument. License: MIT Signed-off-by: Christian Couder <[email protected]>

License: MIT Signed-off-by: Christian Couder <[email protected]>

chriscool · 2016-01-20T21:19:43Z

There is this error with Circle CI:

expecting success: 
        ipfs cat "$PCOUT/$name" >tpcp_out &&
        ipfs cat "$target" >tpcp_exp &&
        test_cmp tpcp_exp tpcp_out

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x40 pc=0x5b4fbe]

goroutine 1 [running]:
github.com/ipfs/go-ipfs/repo/fsrepo.(*FSRepo).GetStorageUsage.func1(0xc8200bc7e0, 0x5d, 0x0, 0x0, 0x2b019bdc4270, 0xc82068c0c0, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/repo/fsrepo/fsrepo.go:565 +0x3e
path/filepath.walk(0xc8200bc1e0, 0x52, 0x2b019c202890, 0xc8200b42d0, 0xc8201797c0, 0x0, 0x0)
    /usr/local/go/src/path/filepath/path.go:370 +0x415
path/filepath.walk(0xc8200101ea, 0x48, 0x2b019c202890, 0xc820016af0, 0xc8201797c0, 0x0, 0x0)
    /usr/local/go/src/path/filepath/path.go:374 +0x4fc
path/filepath.Walk(0xc8200101ea, 0x48, 0xc8201797c0, 0x0, 0x0)
    /usr/local/go/src/path/filepath/path.go:396 +0xe1
github.com/ipfs/go-ipfs/repo/fsrepo.(*FSRepo).GetStorageUsage(0xc820018940, 0x2, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/repo/fsrepo/fsrepo.go:567 +0xa7
github.com/ipfs/go-ipfs/repo.(*ref).GetStorageUsage(0xc8200e4780, 0x411358, 0x0, 0x0)
    <autogenerated>:22 +0x5c
github.com/ipfs/go-ipfs/core/corerepo.(*GC).maybeGC(0xc8206e4580, 0x2b019bdc4998, 0xc820018900, 0xc, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/core/corerepo/gc.go:162 +0x65
github.com/ipfs/go-ipfs/core/corerepo.ConditionalGC(0x2b019bdc4998, 0xc820018900, 0xc820116000, 0xc, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/core/corerepo/gc.go:158 +0x86
github.com/ipfs/go-ipfs/core/commands.glob.func18(0x2b019bdc48c0, 0xc820298240, 0x2b019bdc4a28, 0xc820014150)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/core/commands/cat.go:48 +0x2a8
github.com/ipfs/go-ipfs/commands.(*Command).Call(0x1429e40, 0x2b019bdc48c0, 0xc820298240, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/commands/command.go:110 +0x471
main.callCommand(0x2b019bdc4998, 0xc820018840, 0x2b019bdc48c0, 0xc820298240, 0x1429e40, 0x1418e80, 0x0, 0x0, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/cmd/ipfs/main.go:343 +0xa5c
main.(*cmdInvocation).Run(0xc820018800, 0x2b019bdc4998, 0xc820018840, 0x0, 0x0, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/cmd/ipfs/main.go:186 +0x1e5
main.main()
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/cmd/ipfs/main.go:151 +0x65b

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:1696 +0x1

goroutine 18 [chan receive]:
github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/codahale/metrics.init.1.func2()
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/codahale/metrics/metrics.go:321 +0x73
created by github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/codahale/metrics.init.1
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/codahale/metrics/metrics.go:328 +0x7b

goroutine 19 [syscall]:
os/signal.loop()
    /usr/local/go/src/os/signal/signal_unix.go:22 +0x18
created by os/signal.init.1
    /usr/local/go/src/os/signal/signal_unix.go:28 +0x37

goroutine 20 [select]:
github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.(*MirrorWriter).logRoutine(0xc8200ee580)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:71 +0x33c
created by github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.NewMirrorWriter
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:38 +0xe2

goroutine 8 [select, locked to thread]:
runtime.gopark(0x1088c00, 0xc82002c728, 0xecb298, 0x6, 0x43cd18, 0x2)
    /usr/local/go/src/runtime/proc.go:185 +0x163
runtime.selectgoImpl(0xc82002c728, 0x0, 0x18)
    /usr/local/go/src/runtime/select.go:392 +0xa64
runtime.selectgo(0xc82002c728)
    /usr/local/go/src/runtime/select.go:212 +0x12
runtime.ensureSigM.func1()
    /usr/local/go/src/runtime/signal1_unix.go:227 +0x353
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:1696 +0x1

goroutine 29 [runnable]:
github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess.(*process).doClose.func1(0x0, 0x0, 0x0)
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess/impl-mutex.go:208
created by github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess.(*process).doClose
    /home/ubuntu/.go_project/src/github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/goprocess/impl-mutex.go:221 +0x309
not ok 50 - output looks good

jbenet · 2016-01-20T22:01:48Z

I've never seen that error. weird. cc @whyrusleeping @rht

whyrusleeping · 2016-01-21T02:39:14Z

that panic is because the error is not checked in that function: (repo/fsrepo/fsrepo.go:565)

    err = filepath.Walk(pth, func(p string, f os.FileInfo, err error) error {
        du += uint64(f.Size())
        return nil
    })

whyrusleeping · 2016-01-21T02:39:53Z

you can probably ignore it, and we can fix it in another PR (file a bug for it).

Or you can maybe check the error, but i'm not sure how it propogates, so maybe its really best for another PR

rht · 2016-01-21T03:21:36Z

'is it reproducible?' no, I tested locally and didn't see any err, and so it is not deterministic.

Explaining what had possibly happened: between ipfs cat "$PCOUT/$name" >tpcp_out && and ipfs cat "$target" >tpcp_exp &&, a gc from the first ipfs cat happened which finished in the middle of of the next gc.

rht · 2016-01-21T04:46:21Z

core/commands/pin.go

+			if err != nil {
+				return nil, err
+			}
+			err = dag.EnumerateChildren(n.Context(), n.DAG, nd, ks)


Listing indirect keys recursively appears only in here, while I think it should be the case in pinLsKeys's indirect as well.

nvm, this is much like the ipfs ls and ipfs refs/ipfs refs -r case.

pinLsKeys calls IsPinnedWithType() which does something like the above for indirect keys. See pin/pin.go around line 203.

@chriscool IsPinnedWithType() does have hasChild, which is recursive. However dag.EnumerateChildren also adds all the child keys into a key.KeySet. The keyset is what is needed here for recursive ls. It is orthogonal to this PR, but just to point it out (of the possibility of ipfs pin ls -r $hash or ipfs pin refs -r and deduplications).

Ok, then please open another issue to suggest adding the -r flag, and by the way ipfs pin ls also has a --count flag but it is not used.

redirect: #2225

rht · 2016-01-21T04:51:25Z

Why is context.Background used in https://github.com/ipfs/go-ipfs/blob/c49dcffce2233689ff1f6dfd003a5545338ebc5e/pin/gc/gc.go#L72-L88 ? I don't know if this would imply dangling gc processes before the next ipfs cat. I find it probably since I have seen an ipfs add exiting early before a disk write (and flush) finishes.

rht · 2016-01-21T04:59:24Z

core/commands/pin.go

+		switch pinType {
+		case "direct", "indirect", "recursive", "internal":
+		default:
+			pinType = "indirect through " + pinType


To me, it looks like pinLsKeys = ipfs resolve | isPinnedWithType.
I wonder why is this not in IsPinnedWithType?

gc.GC() in pin/gc/gc.go uses a kind of ipfs pin ls --type=all (in Descendants and ColoredSet), so there could be a refactor here.

@rht if you ask why the switch pinType { ... } is not part of IsPinnedWithType then my answer is that it would be inefficient for IsPinnedWithType users which are internal functions to have to parse "indirect through XXX" if they want to get XXX.

If there are many IsPinnedWithType users that want "indirect through XXX" then we can add IsPinnedWithTypePretty that just calls IsPinnedWithType and does what the switch pinType { ... } does.

About refactoring with pin/gc/gc.go it does not look simple to me, and as ipfs pin ls --type=all already existed before this PR, it could have been done before. So it is independent and maybe you can open a new issue to suggest that refactoring.

If there are many IsPinnedWithType users that want "indirect through XXX" then we can add IsPinnedWithTypePretty that just calls IsPinnedWithType and does what the switch pinType { ... } does.

It could be the other way around, e.g. scripts parsing the output of ipfs pin ls would use IsPinnedWithType so that rows are mainly space separated.

chriscool · 2016-01-21T07:29:11Z

Issue #2220 should take care of the bug found by the Circle CI run.

rht · 2016-01-21T08:27:00Z

core/commands/pin.go

+	}
+	if typeStr == "indirect" || typeStr == "all" {
+		ks := key.NewKeySet()
+		for _, k := range n.Pinning.RecursiveKeys() {


ctx: https://github.com/ipfs/go-ipfs/pull/2208/files#r50369858, this line until L340 could be replaced with gc.Descendants.

(if you don't mind to put the refactor in this pr; nvm)

Done in the last commit, thanks for the suggestion.

chriscool · 2016-01-21T17:19:52Z

@rht with the last patch that uses gc.Descendants(), t0080-repo.sh fails so I will remove it.

The error is:

expecting success: 

    ipfs refs "$MBLOCKHASH" >refsout &&
    ipfs refs -r "$HASH_WELCOME_DOCS" >>refsout &&
    sed -i"~" "s/\(.*\)/\1 indirect/g" refsout &&
    ipfs pin ls --type=indirect >indirectpins &&
    test_sort_cmp refsout indirectpins

> diff -u refsout_sorted indirectpins_sorted
--- refsout_sorted  2016-01-21 16:12:03.988386529 +0000
+++ indirectpins_sorted 2016-01-21 16:12:03.988386529 +0000
@@ -1,10 +1,13 @@
 QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB indirect
 QmS4GvdzPvGN2o7UcmtSRemtFBEETRoiGRYPiuqysmke6q indirect
 QmTumTjvcYCAvRRwQ8sDRxh8ezmrcr88YFU7iYNroGGTBZ indirect
+QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn indirect
 QmVb5AfSF3XdNoujRZRE2EJ3ygioKwYzbnRtzw2nswXP3t indirect
 QmWJRfqw8pP3LPcBWgUEwLkVouoETi8NsZYMnboRaBqxt1 indirect
 QmWdWMdr95FiXpR5gw2mycx5Fz9qgTiWAu4GAZ6LHSvkX5 indirect
 QmY5heUM5qgRubMDD1og9fhCPA6QdkMp3QCwd4s7gJsyE7 indirect
 QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
+QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG indirect
 QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V indirect
+QmaPFp3MR6EtgZt3c3ihyQHkctZ49azUF2d7PvRzBNFbAm indirect
 QmdncfsVm2h5Kqq9hPmU7oAVX2zTSVP3L869tgTbPYnsha indirect

not ok 26 - 'ipfs pin ls --type=indirect' is correct

rht · 2016-01-21T17:34:46Z

ic, it is because gc.Descendants includes the recursive keys.

jbenet · 2016-01-21T20:39:25Z

@whyrusleeping think we should merge it. im a bit wary of merging something with a known crash. since it's not really in this code (i.e unrelated) i'll merge this. 0.4.0 people, please update and warn me if you see any crashing at all.

maybe we can put effort on finding and solving #2220

Add arguments to 'ipfs pin ls'

chriscool added the RFCR label Jan 16, 2016

whyrusleeping reviewed Jan 17, 2016
View reviewed changes

chriscool force-pushed the add-arguments-to-pin-ls branch from 86bbe5f to b36bf37 Compare January 17, 2016 07:46

jbenet reviewed Jan 17, 2016
View reviewed changes

chriscool force-pushed the add-arguments-to-pin-ls branch from b36bf37 to aada69c Compare January 17, 2016 22:04

jbenet reviewed Jan 18, 2016
View reviewed changes

jbenet mentioned this pull request Jan 18, 2016

Sprint January 11th ipfs/team-mgmt#79

Closed

chriscool added 7 commits January 18, 2016 21:37

pin/pin: replace isPinned() with isPinnedWithType()

b770c88

It is more generic to be able to pass a pin type argument. License: MIT Signed-off-by: Christian Couder <[email protected]>

core/commands/pin: refactor 'pin ls'

c89e3c2

License: MIT Signed-off-by: Christian Couder <[email protected]>

core/commands/pin: make 'ipfs pin ls' accept path args

4244f71

License: MIT Signed-off-by: Christian Couder <[email protected]>

core/commands/pin: refactor listing all pins

d87ece1

License: MIT Signed-off-by: Christian Couder <[email protected]>

core/commands/pin: update 'ipfs pin ls' documentation

f5f923a

License: MIT Signed-off-by: Christian Couder <[email protected]>

t0081: use 'ipfs pin ls <arg>' in test_pin_flagi()

893ab6d

License: MIT Signed-off-by: Christian Couder <[email protected]>

test/bin: remove ipfs-pin-stat

2dd4f40

License: MIT Signed-off-by: Christian Couder <[email protected]>

chriscool added the RFM label Jan 18, 2016

core/commands/pin: change the default for --type to 'all'

29830da

License: MIT Signed-off-by: Christian Couder <[email protected]>

chriscool force-pushed the add-arguments-to-pin-ls branch from aada69c to 29830da Compare January 20, 2016 20:34

rht reviewed Jan 21, 2016
View reviewed changes

chriscool mentioned this pull request Jan 21, 2016

Panic when doing 'ipfs cat' #2220

Closed

rht reviewed Jan 21, 2016
View reviewed changes

chriscool mentioned this pull request Jan 21, 2016

CHANGELOG.md should be updated for v0.3.11 and v0.4.0 #2222

Closed

chriscool force-pushed the add-arguments-to-pin-ls branch from 7e89859 to 29830da Compare January 21, 2016 17:22

jbenet added a commit that referenced this pull request Jan 21, 2016

Merge pull request #2208 from ipfs/add-arguments-to-pin-ls

9c1375f

Add arguments to 'ipfs pin ls'

jbenet merged commit 9c1375f into master Jan 21, 2016

jbenet deleted the add-arguments-to-pin-ls branch January 21, 2016 20:39

jbenet mentioned this pull request Jan 25, 2016

Sprint January 19th ipfs/team-mgmt#83

Closed

Stebalien mentioned this pull request Mar 15, 2016

Pin command fixes #2477

Merged

Add arguments to 'ipfs pin ls' #2208

Add arguments to 'ipfs pin ls' #2208

Conversation

chriscool commented Jan 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jan 17, 2016

chriscool commented Jan 17, 2016

chriscool commented Jan 17, 2016

chriscool commented Jan 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Jan 17, 2016

chriscool commented Jan 17, 2016

chriscool commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscool commented Jan 20, 2016

jbenet commented Jan 20, 2016

whyrusleeping commented Jan 21, 2016

whyrusleeping commented Jan 21, 2016

rht commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rht commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscool commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscool commented Jan 21, 2016

rht commented Jan 21, 2016

jbenet commented Jan 21, 2016