-
-
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
ipfs files add #8637
ipfs files add #8637
Conversation
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.
Thank you @coryschwartz, amazing to see this happening so soon! ❤️
I took it for a spin and works as expected. Subjectively, it feels like it was always part of the ipfs files
commands, which is a good sign in my book.
Given that the feature request was written by me, asked for more eyes to review this, mainly to ensure ergonomics feel intuitive to the majority of users from different backgrounds.
For now, only two actionable asks:
- rebase on latest master – this should fix interop tests
- add a basic sharness test
- for now just add a file via
ipfs add
andipfs files add
with default parameters and confirm they (1) produce the same CID (2) the low-level pin is not created whenipfs files add
is used
- for now just add a file via
@lidel : it looks like your asks have been addressed. Can you retract your "request changes" please? @schomatis : can you please give this a once over and then merge? @lidel wanted another opinion on the ergonomics. |
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.
This looks like a giant code duplication. If it does what was requested I'm not standing in the way of landing this but I can't approve it, especially since we're duplicating one of the ugliest pieces of the CLI code: ipfs add
.
@coryschwartz : I'm assuming you won't have bandwidth to drive this forward. Totally understood. It helped exposed some issues for doing this write. I'm going to close this for now so it's clear in the issue that someone else can take a crack at this. |
fixes: #8504
adds command
ipfs files add
using mostly the same parameters as inipfs add
This is functionally equivilent to running
ipfs add
followed byifps files cp
with some slightly different defautls.IPFS pinning is not enabled by default, although it is an option.
Keeping with the pattern established for other files command,
-p
is for parent creation. The progress bar is moved to-P
(capital) instead. Seems unfortunate; if this doesnt make sense, let me know what would be better.There is no wrap option. Although, I could be convinced that it should be included here. When adding a directory recursively, files are "wrapped" implicitly by the MFS directory that contains them.
Following the convention from cp command, if the destination ends in a forward slash, files are added under an MFS directory such that many files can be added like this:
The two encantations, although similar, result in a different dag for the containing directory. This is a subtle difference that might be confusing, maybe there is a better way to go about it.
Recursive adds
Adding several files with implicit directory creation
Results: