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

feat(cmds/add): --to-files option as files cp #8927

Merged
merged 10 commits into from
Sep 21, 2022

Conversation

schomatis
Copy link
Contributor

Closes #8504.

Sharness missing: #8504 (comment)

Example:

ipfs add p2p -r --to-files /

# added QmWSpN5PR7rkcnHQAWU8AvrAnqUtZnDfSzfFd9r5xnh7DJ p2p/listener.go
# added QmSj2YofPhB4i6LQwfBhdZZv5h7p1NNK8sTWYxT7xsKyUx p2p/local.go
# added QmaUuTcmN4y8G2E8i14U7VstZyrF6RrydbUanYkXt2hPXg p2p/p2p.go
# added QmTDkjKwN6ZxndDUnyLjdDF1yBxjr2TzBmmq2sDSgP82TE p2p/remote.go
# added QmQHnDfEdHDQPCksGNiY9asadBYMkmcQ5xKULog8FZLVJ3 p2p/stream.go
# added QmWw69M8XVQEV4tUQVYeUK9Er1v7M32X6Lz41Zg6ttzdnE p2p
 
ipfs files ls /p2p

# listener.go
# local.go
# p2p.go
# remote.go
# stream.go

@schomatis schomatis requested a review from lidel May 2, 2022 16:11
@schomatis schomatis self-assigned this May 2, 2022
@schomatis schomatis force-pushed the schomatis/feat/cmds/add/to-files branch 2 times, most recently from e737b06 to d263d81 Compare May 2, 2022 20:25
Jorropo
Jorropo previously requested changes May 3, 2022
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM thx, this looks good, however:

I can't get it working:

$ ./ipfs add test/ -r --pin=false --to-files /
Error: expected a file argument

Whatever I've tried, I get Error: expected a file argument.
My MFS is empty right now.

Also this could use some sharness tests.

core/commands/add.go Outdated Show resolved Hide resolved
core/commands/add.go Show resolved Hide resolved
@schomatis
Copy link
Contributor Author

I can't reproduce your error. Let's just wait on @lidel's review on this one.

@lidel lidel added this to the go-ipfs 0.14 milestone May 16, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schomatis lgtm, I think if you rebase and add some basic sharness tests it will be ready to go.

(I've added some --help text around ipfs add --to-files and ipfs files write)

ps. fwiw I was also unable to reproduce @Jorropo's problem

@schomatis schomatis force-pushed the schomatis/feat/cmds/add/to-files branch from 063556c to 6b45c92 Compare July 21, 2022 23:53
@schomatis
Copy link
Contributor Author

I'm now hitting @Jorropo's error Error: expected a file argument. Not sure if this is related to the rebase, but in any case I'll delve deeper into the add command.

@BigLep BigLep modified the milestones: kubo 0.14, kubo 0.15 Jul 22, 2022
@schomatis
Copy link
Contributor Author

This fails when running it through the daemon, fixing.

@schomatis
Copy link
Contributor Author

schomatis commented Jul 25, 2022

It seems the iterator for multipartDirectory somehow consumes the state of the directory from where it's derived and we can't call it twice:

https://github.com/ipfs/go-ipfs-files/blob/88b4692b4642719bb8459a966f4b9a3da57c6e2d/multipartfile.go#L218-L221

(I could try to repackage what was iterated but seems the cleaner approach is to fix this upstream.)

@schomatis
Copy link
Contributor Author

Blocked on ipfs/go-ipfs-files#53.

@schomatis
Copy link
Contributor Author

We can't easily fix ipfs/go-ipfs-files#53 so working around it here.

@schomatis schomatis force-pushed the schomatis/feat/cmds/add/to-files branch from 7319e5b to d614316 Compare August 4, 2022 00:32
@schomatis schomatis requested a review from lidel August 4, 2022 17:31
@schomatis
Copy link
Contributor Author

@lidel Unified loops to use the iterator only once.

@lidel lidel removed this from the kubo 0.15 milestone Sep 1, 2022
@ajnavarro
Copy link
Member

ajnavarro commented Sep 9, 2022

@Jorropo I tested the exact same command as you did and it is working fine. Can you have another look, please?
@lidel I see that there are some sharness tests. Do we need more of them?

@ajnavarro ajnavarro self-assigned this Sep 9, 2022
@BigLep
Copy link
Contributor

BigLep commented Sep 20, 2022

2022-09-20 conversation: @lidel will check to see if more sharness tests are needed. If there are more tests needed, @lidel will specify them and @ajnavarro will do them. If there is sufficient coverage @lidel will merge.

@lidel lidel mentioned this pull request Sep 20, 2022
@lidel lidel force-pushed the schomatis/feat/cmds/add/to-files branch from c37cee8 to ca2b36b Compare September 21, 2022 14:39
this adds bunch of tests that guard UX around importing multiple files
into MFS, and the logic around trailing slash to indicate if the MFS
destination if a directory.

If the destination has a trailing slash, we ensure that the directory
exists and is a dir and not a file. this allows us to support
adding multipl files into MFS dir:

ipfs add file1.txt file2.txt --to-files /some/mfs/dir/
@lidel lidel force-pushed the schomatis/feat/cmds/add/to-files branch from ca2b36b to 9bd0117 Compare September 21, 2022 14:44
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected – this is a real quality of life improvement.

  • Improved helptext (--help)
  • Added tests that guard UX around importing multiple files into MFS.
  • Improved logic around trailing slash to indicate if the MFS destination if a directory
    • If the destination has a trailing slash, we ensure that the directory exists and is a dir and not a file.
      This allows us to support adding multiple files into MFS, as long the destination is a dir and ends with slash:
      $ ipfs add file1.txt file2.txt --to-files /at/some/mfs/dir/
      ...
      $ ipfs files ls /at/some/mfs/dir/
      file1.txt
      file1.txt

@lidel lidel enabled auto-merge (squash) September 21, 2022 15:34
@lidel lidel dismissed Jorropo’s stale review September 21, 2022 16:08

"ipfs add test/ -r --pin=false --to-files /" is fixed now

@lidel lidel merged commit 9e5d0aa into master Sep 21, 2022
@lidel lidel deleted the schomatis/feat/cmds/add/to-files branch September 21, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ipfs add --to-files=/path/in/mfs
5 participants