add: Allow proper adding of multiple directories with a single add call

[whyrusleeping]

Resolves #2811
License: MIT
Signed-off-by: Jeromy why@ipfs.io

[whyrusleeping]

This might be adding more complexity than is required for the fix, i'm gonna take another look at it

[whyrusleeping]

The issue here is when adding directories by more than a direct reference: ipfs add -r A/B, previously, it would have the same output as cd A && ipfs add -r B, now it outputs everything prefixed with A, and then outputs the hash of A itself.

[kevina]

Relates to #3258.

[whyrusleeping]

Alright, the decision has been made. This behaviour will be the new 'correct' behaviour:

ipfs add -r A/B
added QmBlah A/B/foo
added QmAbcd A/B
added QmZyxv A

Adding a 'path' will also add all components of the path going upwards to the 'path' root. This is mimicing the behaviour of tar, and make it far easier to reason about the behaviour of the command.

[Kubuxu]

It might be breaking change, as adding file in subdirectory would return just its hash, see:

ipfs add gx/HEAD
added QmWeKwYTKwBwVd7AKioXTmtpLFxTk3MBBk2ef2JtwCccAi HEAD

on which users can depend.

Also how it is supposed to work with -q.

[kevina]

I strongly agree with @Kubuxu on this one. The behavior without "-r" should not change. If it does it will cause me all sorts of grief.

[whyrusleeping]

Okay, so i need to go back to focusing on ipld related stuff... still havent gotten this fixed entirely. If anyone else wants to take this over that would be cool, otherwise it will take me a few more days to get back to it.

[kevina]

@whyrusleeping, how about a different approach. Rather than trying to deal with the new IsRoot() property inside the add command, how about using IsRoot() to reconstruct the original directory structure before passing it to add? Basically in the addAllAndPin loop only pass top-level files/directories to AddFile.

[kevina]

@whyrusleeping if you are okay with my idea, I will see what I can do, but I will likely do so by adding commits to #3258.

The idea of only passing top-level files/directories could clean up the pinning logic quite a bit.

[whyrusleeping]

@kevina i like that idea, but i'm not sure we can do it easily with the limitation that files come over the wire in no particular order. If you still think you can handle that, then by all means please do try that

[kevina]

@whyrusleeping, is the a way to fix the order so files come in the order specified on the command line?

[kevina]

@whyrusleeping more to point, would anything break by fixing the order and can you give me a hint on what needs to be done?

[whyrusleeping]

@kevina the problem is we arent always dealing with a command line. Files can come in through the API, and we can't guarantee anything about that

[kevina]

@whyrusleeping, notes from IRC Oct 14:

whyrusleeping: in #3300, is the new IsRoot() method used?
I am trying to figure out what exactly you are attempting to do
If files can indeed be specified in any order than than the IsRoot() need to be used to weed out the original roots when multiple directories are specified
I think I know what to do provided you managed to set IsRoot() correctly...

[kevina]

@whyrusleeping @Kubuxu

This is becoming really complicated. I am not even sure what this p.r. is trying to solve. I would prefer we let #3258 in, that solves the issue with correctly adding and pinning multiple files and close this, or at least bump this from the ipfs 0.4.5 milestone.

That being said, as this seams to be holding up #3258 from getting in I am willing to work on correctly adding multiple directories (what I assume this pull request first set out to do). By multiple directories I mean adding and pinning each directory specified on the command line.

First off because of how directories are passed in we need to make sure we can distinguish which directories where specified on the command line. That is what I think the files IsRoot() method is trying to do, but the implementation does not look complete. I will make sure this works first.

Secondly, again because of how files and directories are passed in when -r is specified, adding a path should also add all components of the path going upwards. When -w is used this also makes sense. When neither -r or -w is used this breaks lots of things. Instead I propose in that case just the full path is is outputted and none of the the components (before only the filename we be outputted) @whyrusleeping do you agree with this?

@whyrusleeping do you want me to go ahead and try to implement this as outlined above, or can we table this for now and let #3258 in? It I do implement it I will do it by adding commits to #3258.

[kevina]

I should add that when files are added via the filestore FullPath() will always be an absolute path. So making a directory for ever component will very likely created unwanted directories. I can live with this for "-r" but not when just adding files. Also outputting more than one line for each file added will make parsing the results more difficult.

[whyrusleeping]

@kevina Yeah, i think we should finish the IsRoot() code so we can make this all work. Basically what i'm trying to fix here is what i'm adding tests for. Adding multiple directories in the same invocation, and not enountering the same name overlap issues as we currently have.

[kevina]

@whyrusleeping to be clear you want me to implement this as outlined above?

I decided to go ahead and solve an important limitation with the filestore (ipfs-filestore#12) so it will be a couple of days before I can get back to this.

[kevina]

@whyrusleeping, I'm afraid this is going to need more thought before it gets implemented, especially in regard to pinning.

For example what is the expected behavior if I do:

  ipfs add -r dir/dir1 dir/dir2

what exactly do you want pinned? As a user I would expect dir1 and dir2 to be pinned, but your plan of "tar like" behavior complicates this, should dir also be added and pinned?

Similarly, exactly how is this

  ips add --with-dirs dir/filea dir/fileb

suppose to behave. What exactly will be pinned?

[kevina]

In short I think "tar like" behavior is okay with the "-w" or "--wrap-with-directory" option, without the "-w" option, than I as a user, would expect each directly to be added and pinned individually. The expected behavior seams incompatible with the tar like behavior that you want. (However, maybe I am missing something completely obvious, in which case I apologize in advance.)

[whyrusleeping]

ipfs add -r dir/dir1 dir/dir2

In this case, i would expect a single pin that contains both objects.

[kevina]

ipfs add -r dir/dir1 dir/dir2

In this case, i would expect a single pin that contains both objects.

Than what about

ipfs add -r dir1/dir dir2/dir

Two separate pins?

This is getting really complicated.

[whyrusleeping]

Nope, one pin.

[whyrusleeping]

One pin per add invocation

[kevina]

That is what "-w" does, what about without "-w"?

[whyrusleeping]

With and without -w. One pin per add invocation

[kevina]

I really don't like that behavior. What about when adding files.

Are you trying to say "-w" should just be the default now?

[kevina]

Note: this issue is basically being replaced by #3342.

[whyrusleeping]

dropping from 0.4.5 milestone, lets reprioritize after we get that out

[Stebalien]

Closing as "completely out of date".