core/commands/add: Change add() to only accept a single reader

[wking]

The change to an array of readers comes from e096060
(refactor(core/commands2/add) split loop, 2014-11-06), where it's used
to setup readers for each path in the argument list. However, since
6faeee8 (cmds2/add: temp fix for -r. horrible hack, 2014-11-11) the
argument looping moved outside of add() and into Run(), so we can drop
the multiple-reader support from add().

Adding a file can create multiple nodes (e.g. the splitter can chunk
the file into several blocks), but:

1. we were only appending a single node per reader to our returned
   list, and
2. we are only using the final node in that returned list,

so this commit also adjusts add() to return a single node reference
instead on an array of nodes.

I'm having trouble running the test suite locally:

$ make test
cd cmd/ipfs && go build -i
go test ./...
go/src/github.com/ipfs/fs-repo-migrations/ipfs-1-to-2/go-datastore/lru/datastore.go:6:2: no buildable Go source files in /…/go/src/github.com/ipfs/fs-repo-migrations/ipfs-1-to-2/golang-lru
can't load package: package github.com/ipfs/go-ipfs/go/src/golang.org/x/net/context: code in directory /…/go/src/github.com/ipfs/go-ipfs/go/src/golang.org/x/net/context expects import "golang.org/x/net/context"
can't load package: package github.com/ipfs/go-ipfs/go/src/golang.org/x/net/dict: code in directory /…/go/src/github.com/ipfs/go-ipfs/go/src/golang.org/x/net/dict expects import "golang.org/x/net/dict"
…

so someone more familiar with building and running the test suite
should put this through its paces before merging.

[whyrusleeping]

you need to make sure your local repo for go-ipfs is properly placed within your GOPATH for your testing to work properly.

[wking]

On Thu, Apr 23, 2015 at 10:56:23AM -0700, Jeromy Johnson wrote:

you need to make sure your local repo for go-ipfs is properly placed
within your GOPATH for your testing to work properly.

It's at $GOPATH/src/github.com/ipfs/go-ipfs, which seems right to me.

[wking]

Hmm, there are some very similar looking functions in core/coreunix/add.go. Should I be updating them too? Removing the versions in core/commands/add.go and using the coreunix versions instead? Removing the versions in coreunix and using the commands versions wherever the coreunix versions are being used? How are these pieces supposed to fit together?

[wking]

On Thu, Apr 23, 2015 at 10:56:23AM -0700, Jeromy Johnson wrote:

you need to make sure your local repo for go-ipfs is properly placed
within your GOPATH for your testing to work properly.

Blowing away my GOPATH and starting over with:

$ go get github.com/ipfs/go-ipfs
$ cd src/github.com/ipfs/go-ipfs/
$ make test

has things working. So I must have tangled up my GOPATH with my
original setup.

[whyrusleeping]

@wking I really think we should try and reduce code duplication between core/commands/add and core/coreunix/add. exporting the functions from coreunix should be fine IMO

[wking]

On Fri, Apr 24, 2015 at 10:24:25AM -0700, Jeromy Johnson wrote:

@wking I really think we should try and reduce code duplication
between core/commands/add and core/coreunix/add. exporting the
functions from coreunix should be fine IMO

coreunix.add is so low-level. I think we want to shift the whole of
core.commands.add.addFile into coreunix.add and just call that from
commands.add. The main change would be the need to pass through
outChan, progress, and wrap. I need to look up optional parameters in
Go to figure out how that would work (I guess we can always set
pointers to nil…).

[wking]

On Fri, Apr 24, 2015 at 10:57:12AM -0700, W. Trevor King wrote:

I need to look up optional parameters in Go to figure out how that
would work (I guess we can always set pointers to nil…).

Looks like explicit nils or alternative function names are the way to
go. The other workarounds 1 are too awkward or specialized to be
worth the trouble. So do we want:

func addFile(n _core.IpfsNode, file files.File, out chan interface{}, progress bool, wrap bool) (_dag.Node, error)

or:

func addFile(n *core.IpfsNode, file files.File)
func addFileProgress(n *core.IpfsNode, file files.File, progress bool)
…

We already have coreunix.AddWrapped:

func AddWrapped(n *core.IpfsNode, r io.Reader, filename string) (string, *merkledag.Node, error)

but that's not going to work well with metadata. I think we should
drop coreunix.AddWrapped and handle that in core.commands.add. And
then have a single addTree (which handles directories and
recursion like the current addFile) like:

func addTree(n _core.IpfsNode, file files.File, out chan interface{}) (_dag.Node, error)

with sufficient details sent down the out channel to attach a progress
bar if thte caller feels like it. My grasp of this is still a bit
wobbly, so let me know if that doesn't sound workable ;).

[whyrusleeping]

I think that interface feels a little closer to what we want.. I'll think about it some. maybe @jbenet has some ideas too.

We really want to fix the entire add command, its not pretty right now.

[jbenet]

This is internal code to the add command, so i don't care much. This seems like reduction of complexity, and 👍 LGTM.

---

(to be clear, in the core/shell case though -- the api we export to users -- it should match the cli as close as possible. so .Add(...) there should take multiple readers (i don't think it does now), just like ipfs add file1 file2 file3 works. doing it variadic instead of array based, makes both .Add(r1) and .Add(manyRdrs...) work, so maybe that's nice).