Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi file add does not work as I would expect #2811

Open
kevina opened this issue Jun 6, 2016 · 9 comments
Open

Multi file add does not work as I would expect #2811

kevina opened this issue Jun 6, 2016 · 9 comments
Labels
kind/bug A bug in existing code (including security flaws) status/deferred Conscious decision to pause or backlog topic/commands:add Topic commands:add

Comments

@kevina
Copy link
Contributor

kevina commented Jun 6, 2016

It seams that even if -w is not specified, files added via ipfs add <file1> <file2> ... are added to a mfs directory. As I see it this leads to two unexpected behaviors:

  1. The command will fail if a file with the same name is added twice, even if they are separate files:
$ mkdir a b
$ echo "Hello World!" > a/hello
$ echo 'HELLO WORLD!' > b/hello
$ ipfs add a/hello b/hello
added QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG hello
Error: directory already has entry by that name
  1. The files are pinned indirectly via the created directory
$ mv b/hello b/hello2
$ ipfs add  a/hello b/hello2 
added QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG hello
added QmemWc3tw1HEM5oTTE1bxwvjKkn1yGFKJF1zVCZXD5wXgi hello2
$ ipfs pin rm QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG
Error: QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG is pinned indirectly under QmXpzvr3QHQ2Yxwb264HsNQsASJkD7jLhFBArCy6DX5iKV

Without -w the behavior I would expect is that each file is added and pinned individually.

@whyrusleeping
Copy link
Member

hrm... interesting. That does seem like a bug

@whyrusleeping whyrusleeping added the kind/bug A bug in existing code (including security flaws) label Jun 6, 2016
@whyrusleeping
Copy link
Member

I guess the 'proper' solution here would be to make each parameter to add be its own separate 'add' call

@kevina
Copy link
Contributor Author

kevina commented Jun 6, 2016

So if the node is online, the client should make one request per file? Or are you thinking of something else.

Note that when "-r" is specified and the node is online, ipfs add -r dir/ translates to ipfs add -r dir dir/file1 dir/file2 ....

@kevina
Copy link
Contributor Author

kevina commented Jun 6, 2016

@whyrusleeping an easier solution would be to disallow multiple files when -r is specified and then if neither -w and -r is specified add and pin each file individually without adding the files to any directory.

I am hacking on this code now and think I can implement the above solution in a few hours.

kevina added a commit to ipfs-filestore/go-ipfs that referenced this issue Jun 6, 2016
If neither "-w" or "-r" is specified don't use an mfs-root.  Add and
pin each file separately.

Towards ipfs#2811

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina
Copy link
Contributor Author

kevina commented Jun 6, 2016

@whyrusleeping I implemented a solution on my filestore branch (see 293e848). If neither '-w' or '-r' is specified it does not use an mfs-root and adds and pins each file individually. Pins are not flushed until the end to avoid a major slowdown.

@whyrusleeping
Copy link
Member

@kevina i think the solution is simpler than that. I believe all we need to do is change the addAllAndPin method to use a separate adder for each input argument. (although it might not be as simple as i'm imagining). i'm hoping we can avoid further complicating the adder logic that way

@kevina
Copy link
Contributor Author

kevina commented Jun 6, 2016

@whyrusleeping I can try that, but for performance reasons it is important that pins are not flushed until the very end. Otherwise we lose most of the benefit of combining multiple files in a single command. In my informal tests the performance difference between flushing the pins each time and waiting to the end is an order of magnitude. There may also may be some performance lost due to the adding of an completely unnecessary directory object for each file.

kevina added a commit to ipfs-filestore/go-ipfs that referenced this issue Jun 9, 2016
Towards ipfs#2811.

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina
Copy link
Contributor Author

kevina commented Jun 9, 2016

@whyrusleeping I implemented your solution in commit e4b48dd. I might be able to refactor the addAllAndPin to make this a little nicer. I modified PinRoot so that flushing can be delayed to the end. Otherwise, performance will suffer.

I like my original solution better as it avoids the unnecessary mfs code altogether when directories are not being added. With your solution a unnecessary directory entry is being created and then left in the datastore.

My very informal test thought, indicate that the performance is about the same in either solution.

@whyrusleeping whyrusleeping added the help wanted Seeking public contribution on this issue label Sep 14, 2016
kevina added a commit to ipfs-filestore/go-ipfs that referenced this issue Sep 25, 2016
If neither "-w" or "-r" is specified don't use an mfs-root.  Add and
pin each file separately.

Closes ipfs#2811

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@Kubuxu Kubuxu added status/in-progress In progress and removed help wanted Seeking public contribution on this issue labels Sep 25, 2016
@kevina
Copy link
Contributor Author

kevina commented Sep 25, 2016

@whyrusleeping #3258 implemented the solution I am currently using. I would prefer we just use that for now. The adder is likely to undergo a rewrite soon for #3172.

kevina added a commit that referenced this issue Sep 30, 2016
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 <[email protected]>
kevina added a commit that referenced this issue Sep 30, 2016
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 <[email protected]>
kevina added a commit that referenced this issue Oct 3, 2016
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 <[email protected]>
kevina added a commit that referenced this issue Oct 3, 2016
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 <[email protected]>
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Nov 28, 2016
@whyrusleeping whyrusleeping added the status/ready Ready to be worked label Nov 28, 2016
@Stebalien Stebalien added status/deferred Conscious decision to pause or backlog and removed status/ready Ready to be worked labels Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) status/deferred Conscious decision to pause or backlog topic/commands:add Topic commands:add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants