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

[kevina]

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

Closes #2811

Part of #2634

License: MIT
Signed-off-by: Kevin Atkinson [email protected]

[kevina]

Added test case.

[whyrusleeping]

This doesnt handle the case where i ipfs add -r A B and A and B are directories.

I think that instead of skipping using the mfs root, we rework how the 'pin finalize' works. We need to be able to convey what we want eventually pinned to the adder.

Maybe we do a logically separate add for each input? (that seems to make the most sense to me)

[kevina]

You are right it doesn't. Handling that case will be very tricky due to how the request is sent to the server. For now, I think we should detect and disallow adding multiple directories at once.

I don't particularly like the idea of using an mfs root when it is not necessary. In a script for the filestore (add-dir, see the README (section Adding all files in a directory) for info on using it) I am adding 1000s of unrelated files (about as many as I can get away with before running into system limits on the size of the command line) at the same time, the mfs root in those cases is completely irrelevant.

[kevina]

Also, we talked a lot about this in #2811. I tried one request per file in that commit and it turned out to me more code so I went with what I was currently using.

To see how an alternative GetLinks() is provided see filestore/support/dagservice.go in #2634.

[kevina]

@whyrusleeping I recently pushed another test case to cover the other problem caused by using an MFS Root as summarized in #2811

For now, I think we should detect and disallow adding multiple directories at once.

Due to the way "-r" is handled this will need to be done outside of the add command. The directory content are already expanded and there is no way for the add command to see which ones where specified on the command line.

[whyrusleeping]

@kevina the Files object we get in the command request is an array of all the top level files the user is trying to add right?

This needs to be verified, but i think the structure looks something like:

ipfs add -r dir
Files = [
    dir{...}
]

ipfs add file
Files = [
    file={...}
]

ipfs add fileA fileB
Files = [
    fileA = {...}
    fileB = {...}
]

ipfs add -r dirA dirB
Files = [
    dirA = {...}
    dirB = {...}
]

(still need to confirm that, but) If thats the case, we can just iterate over the immediate children of the Files object and run 'add and pin' on each of them individually.

[kevina]

@whyrusleeping I did some checks to double check. The Files object in the PreRun is as you described. However, when the request is sent to the server the directory contents are expended and there is no way to tell what was specified on the command line.

For now the easiest thing would be to detect the situation in the PreRun and abort with an clean error. Agreed?

[kevina]

@whyrusleeping, okay, as discussed over IRC, I added a test to disallow multiple directories and filed an new issue.

Hopefully this is good to go.

[kevina]

@whyrusleeping I would like you to consider giving this pull request some serious considering. Not using a MFS root for single file adds turns out to required for my filestore to function. Here is what happens in a single file add without this patch:

1. A small file is added (let say the hash is QmXXYZ) and it is added to the blockstore as a FilestoreNode node, so far no problem. But then:
2. PinRoot is then called, this calls adder.RootNode() that retrieves the same node from the blockstore (with the hash QmXXYZ), however now the node is a ProtoNode.
3. PinRoot readds the block to the blockstore. If the blockstore is the special filestore blockstore it will error out because it doesn't know what to do with it.

If the MFS root is not used than 2 and 3 do not happen when adding single files and there is no problem.

For now, as a hack, I will ignore a ProtoNode if the block already exists in the filestore.

(And in case there is any confusion I can't just blindly accept FilestoreNode and pass on ProtoNode as that will lead to storing the same block twice, once in the filestore and once in the normal datastore.)

Steps (2) and (3) seam like a horrible waste of time even without the problems it causes the filestore.

[whyrusleeping]

In which cases will the mfs root actually be a file? I think most cases it will be a directory node, or at least an intermediate node. And thats fine to have in the normal blockstore anyway. You only really care about having leaf nodes (data containing nodes) in the filestore, right?

[kevina]

adder.RootNode() will return the hash of the leaf node when adding a single small file with just a Leaf. https://github.com/ipfs/go-ipfs/blob/master/core/coreunix/add.go#L147:

func (adder *Adder) RootNode() (node.Node, error) {
...
    // if not wrapping, AND one root file, use that hash as root.
    if !adder.Wrap && len(root.Links()) == 1 {
        nd, err := root.Links()[0].GetNode(adder.ctx, adder.dagService)
        if err != nil {
            return nil, err
        }

        root = nd
    }
...
}

[kevina]

Also, although not strictly relevant to this problem, I also store non-leaf nodes related to a file in the filestore. They are used for maintenance operations and to be able to easily list the files stored in it by only listing the roots. I do not store directory nodes in the filestore.

[whyrusleeping]

Lets loop back around and see about getting this issue fixed

[magik6k]

This code was rewritten at least once since this PR was opened, feel free to reopen if it's still relevant.