Adder: Fix multi-file add so it works as expected.

If neither "-w" or "-r" is specified don't use an mfs-root.  Add and
pin each file separately.

Closes #2811

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>

core/commands/add.go

@@ -135,6 +135,7 @@ You can now refer to the added file in a gateway, like so:
 		silent, _, _ := req.Option(silentOptionName).Bool()
 		chunker, _, _ := req.Option(chunkerOptionName).String()
 		dopin, _, _ := req.Option(pinOptionName).Bool()
+		recursive, _, _ := req.Option(cmds.RecLong).Bool()
 
 		if hash {
 			nilnode, err := core.NewNode(n.Context(), &core.BuildCfg{
@@ -160,7 +161,8 @@ You can now refer to the added file in a gateway, like so:
 		outChan := make(chan interface{}, 8)
 		res.SetOutput((<-chan interface{})(outChan))
 
-		fileAdder, err := coreunix.NewAdder(req.Context(), n.Pinning, n.Blockstore, dserv)
+		useRoot := wrap || recursive
+		fileAdder, err := coreunix.NewAdder(req.Context(), n.Pinning, n.Blockstore, dserv, useRoot)
 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return

core/coreunix/add.go

 package coreunix
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -64,14 +65,8 @@ type AddedObject struct {
 	Bytes int64  `json:",omitempty"`
 }
 
-func NewAdder(ctx context.Context, p pin.Pinner, bs bstore.GCBlockstore, ds dag.DAGService) (*Adder, error) {
-	mr, err := mfs.NewRoot(ctx, ds, unixfs.EmptyDirNode(), nil)
-	if err != nil {
-		return nil, err
-	}
-
-	return &Adder{
-		mr:         mr,
+func NewAdder(ctx context.Context, p pin.Pinner, bs bstore.GCBlockstore, ds dag.DAGService, useRoot bool) (*Adder, error) {
+	adder := &Adder{
 		ctx:        ctx,
 		pinning:    p,
 		blockstore: bs,
@@ -82,8 +77,17 @@ func NewAdder(ctx context.Context, p pin.Pinner, bs bstore.GCBlockstore, ds dag.
 		Trickle:    false,
 		Wrap:       false,
 		Chunker:    "",
-	}, nil
+	}
 
+	if useRoot {
+		mr, err := mfs.NewRoot(ctx, ds, unixfs.EmptyDirNode(), nil)
+		if err != nil {
+			return nil, err
+		}
+		adder.mr = mr
+	}
+
+	return adder, nil
 }
 
 // Internal structure for holding the switches passed to the `add` call
@@ -130,6 +134,10 @@ func (adder Adder) add(reader io.Reader) (*dag.Node, error) {
 }
 
 func (adder *Adder) RootNode() (*dag.Node, error) {
+	if adder.mr == nil {
+		return nil, nil
+	}
+
 	// for memoizing
 	if adder.root != nil {
 		return adder.root, nil
@@ -153,6 +161,10 @@ func (adder *Adder) RootNode() (*dag.Node, error) {
 }
 
 func (adder *Adder) PinRoot() error {
+	if adder.mr == nil {
+		return nil
+	}
+
 	root, err := adder.RootNode()
 	if err != nil {
 		return err
@@ -179,6 +191,13 @@ func (adder *Adder) PinRoot() error {
 }
 
 func (adder *Adder) Finalize() (*dag.Node, error) {
+	if adder.mr == nil && adder.Pin {
+		err := adder.pinning.Flush()
+		return nil, err
+	} else if adder.mr == nil {
+		return nil, nil
+	}
+
 	root := adder.mr.GetValue()
 
 	// cant just call adder.RootNode() here as we need the name for printing
@@ -250,7 +269,7 @@ func (adder *Adder) outputDirs(path string, fsn mfs.FSNode) error {
 func Add(n *core.IpfsNode, r io.Reader) (string, error) {
 	defer n.Blockstore.PinLock().Unlock()
 
-	fileAdder, err := NewAdder(n.Context(), n.Pinning, n.Blockstore, n.DAG)
+	fileAdder, err := NewAdder(n.Context(), n.Pinning, n.Blockstore, n.DAG, true)
 	if err != nil {
 		return "", err
 	}
@@ -278,7 +297,7 @@ func AddR(n *core.IpfsNode, root string) (key string, err error) {
 	}
 	defer f.Close()
 
-	fileAdder, err := NewAdder(n.Context(), n.Pinning, n.Blockstore, n.DAG)
+	fileAdder, err := NewAdder(n.Context(), n.Pinning, n.Blockstore, n.DAG, true)
 	if err != nil {
 		return "", err
 	}
@@ -302,7 +321,7 @@ func AddR(n *core.IpfsNode, root string) (key string, err error) {
 // the directory, and and error if any.
 func AddWrapped(n *core.IpfsNode, r io.Reader, filename string) (string, *dag.Node, error) {
 	file := files.NewReaderFile(filename, filename, ioutil.NopCloser(r), nil)
-	fileAdder, err := NewAdder(n.Context(), n.Pinning, n.Blockstore, n.DAG)
+	fileAdder, err := NewAdder(n.Context(), n.Pinning, n.Blockstore, n.DAG, true)
 	if err != nil {
 		return "", nil, err
 	}
@@ -324,23 +343,30 @@ func AddWrapped(n *core.IpfsNode, r io.Reader, filename string) (string, *dag.No
 	return gopath.Join(c.String(), filename), dagnode, nil
 }
 
-func (adder *Adder) addNode(node *dag.Node, path string) error {
-	// patch it into the root
-	if path == "" {
-		path = node.Cid().String()
-	}
+func (adder *Adder) pinOrAddNode(node *dag.Node, path string) error {
+	if adder.Pin && adder.mr == nil {
+
+		adder.pinning.PinWithMode(node.Cid(), pin.Recursive)
+
+	} else if adder.mr != nil {
+
+		// patch it into the root
+		if path == "" {
+			path = node.Cid().String()
+		}
+
+		dir := gopath.Dir(path)
+		if dir != "." {
+			if err := mfs.Mkdir(adder.mr, dir, true, false); err != nil {
+				return err
+			}
+		}
 
-	dir := gopath.Dir(path)
-	if dir != "." {
-		if err := mfs.Mkdir(adder.mr, dir, true, false); err != nil {
+		if err := mfs.PutNode(adder.mr, path, node); err != nil {
 			return err
 		}
-	}
 
-	if err := mfs.PutNode(adder.mr, path, node); err != nil {
-		return err
 	}
-
 	if !adder.Silent {
 		return outputDagnode(adder.Out, path, node)
 	}
@@ -384,7 +410,7 @@ func (adder *Adder) addFile(file files.File) error {
 			return err
 		}
 
-		return adder.addNode(dagnode, s.FileName())
+		return adder.pinOrAddNode(dagnode, s.FileName())
 	}
 
 	// case for regular file
@@ -401,10 +427,14 @@ func (adder *Adder) addFile(file files.File) error {
 	}
 
 	// patch it into the root
-	return adder.addNode(dagnode, file.FileName())
+	return adder.pinOrAddNode(dagnode, file.FileName())
 }
 
 func (adder *Adder) addDir(dir files.File) error {
+	if adder.mr == nil {
+		return errors.New("cannot add directories without mfs root")
+	}
+
 	log.Infof("adding directory: %s", dir.FileName())
 
 	err := mfs.Mkdir(adder.mr, dir.FileName(), true, false)

core/coreunix/add_test.go

@@ -56,7 +56,7 @@ func TestAddGCLive(t *testing.T) {
 
 	errs := make(chan error)
 	out := make(chan interface{})
-	adder, err := NewAdder(context.Background(), node.Pinning, node.Blockstore, node.DAG)
+	adder, err := NewAdder(context.Background(), node.Pinning, node.Blockstore, node.DAG, true)
 	if err != nil {
 		t.Fatal(err)
 	}

test/sharness/t0040-add-and-cat.sh

@@ -285,7 +285,7 @@ test_expect_success "'ipfs add' with stdin input succeeds" '
 
 test_expect_success "'ipfs add' output looks good" '
 	HASH="QmZDhWpi8NvKrekaYYhxKCdNVGWsFFe1CREnAjP1QbPaB3" &&
-	echo "added $HASH $HASH" >expected &&
+	echo "added $HASH " >expected &&
 	test_cmp expected actual
 '
 

test/sharness/t0046-multifile-add.sh

@@ -20,11 +20,11 @@ test_expect_success "add files all at once" '
 	ipfs add -q fileA fileB fileC > hashes
 '
 
-test_expect_failure "unpin one of the files" '
+test_expect_success "unpin one of the files" '
 	ipfs pin rm `head -1 hashes` > pin-out
 '
 
-test_expect_failure "unpin output looks good" '
+test_expect_success "unpin output looks good" '
 	echo "unpinned `head -1 hashes`" > pin-expect
 	test_cmp pin-expect pin-out
 '
@@ -36,8 +36,18 @@ test_expect_success "create files with same name but in different directories" '
 	echo BA > dirB/fileA
 '
 
-test_expect_failure "add files with same name but in different directories" '
+test_expect_success "add files with same name but in different directories" '
 	ipfs add -q dirA/fileA dirB/fileA > hashes
 '
 
+cat <<EOF | LC_ALL=C sort > cat-expected
+AA
+BA
+EOF
+
+test_expect_success "check that both files are added" '
+	cat hashes | xargs ipfs cat | LC_ALL=C sort > cat-actual
+	test_cmp cat-expected cat-actual
+'
+
 test_done

test/sharness/t0080-repo.sh

@@ -29,11 +29,6 @@ test_expect_success "'ipfs repo gc' succeeds" '
 	ipfs repo gc >gc_out_actual
 '
 
-test_expect_success "'ipfs repo gc' looks good (patch root)" '
-	PATCH_ROOT=QmQXirSbubiySKnqaFyfs5YzziXRB5JEVQVjU6xsd7innr &&
-	grep "removed $PATCH_ROOT" gc_out_actual
-'
-
 test_expect_success "'ipfs repo gc' doesnt remove file" '
 	ipfs cat "$HASH" >out &&
 	test_cmp out afile
@@ -104,8 +99,7 @@ test_expect_success "remove direct pin" '
 
 test_expect_success "'ipfs repo gc' removes file" '
 	ipfs repo gc >actual7 &&
-	grep "removed $HASH" actual7 &&
-	grep "removed $PATCH_ROOT" actual7
+	grep "removed $HASH" actual7
 '
 
 test_expect_success "'ipfs refs local' no longer shows file" '
@@ -114,8 +108,7 @@ test_expect_success "'ipfs refs local' no longer shows file" '
 	grep "QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y" actual8 &&
 	grep "$EMPTY_DIR" actual8 &&
 	grep "$HASH_WELCOME_DOCS" actual8 &&
-	test_must_fail grep "$HASH" actual8 &&
-	test_must_fail grep "$PATCH_ROOT" actual8
+	test_must_fail grep "$HASH" actual8
 '
 
 test_expect_success "adding multiblock random file succeeds" '