-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
e737b06
to
d263d81
Compare
There was a problem hiding this 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.
I can't reproduce your error. Let's just wait on @lidel's review on this one. |
There was a problem hiding this 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
063556c
to
6b45c92
Compare
I'm now hitting @Jorropo's error |
This fails when running it through the daemon, fixing. |
It seems the iterator for (I could try to repackage what was iterated but seems the cleaner approach is to fix this upstream.) |
Blocked on ipfs/go-ipfs-files#53. |
We can't easily fix ipfs/go-ipfs-files#53 so working around it here. |
7319e5b
to
d614316
Compare
@lidel Unified loops to use the iterator only once. |
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. |
c37cee8
to
ca2b36b
Compare
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/
ca2b36b
to
9bd0117
Compare
There was a problem hiding this 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
- If the destination has a trailing slash, we ensure that the directory exists and is a dir and not a file.
"ipfs add test/ -r --pin=false --to-files /" is fixed now
Closes #8504.
Sharness missing: #8504 (comment)
Example: