fix(cmds/add): disallow --wrap with --to-files #10612
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #10611. When we are adding to an MFS folder ("--to-files=folder/"), we append the filename to the folder name. However, when wrapping, the wrapping folder has no name and `path.Base("")` returns `.`. This creates a folder named "." inside the previous one. But mfs operations like `ls` think that "." is the current folder, and does not contemplate the option of a folder of name ".". Dealing with unnamed files in MFS is in general a footgun, so this commits attempts to prohibit that case.
hsanjuan
force-pushed
the
fix/10611-to-files-with-wrap
branch
from
9378dbb to
b320438
Compare
3 tasks
gammazero
approved these changes
Dec 3, 2024
lidel
changed the title
lidel
approved these changes
Dec 3, 2024
| @@ -32,6 +39,4 @@ If you depended on removed ones, please fill an issue to add them to the upstrea | |||
|
|
|||
| ### 📝 Changelog | |||
|
|
|||
| - Fix: unnamed files in MFS. Disallow `--wrap` + `--to-files` when adding ([#10611](https://github.com/ipfs/kubo/issues/10611), [#10612](https://github.com/ipfs/kubo/issues/10612)) | |||
There was a problem hiding this comment.
ℹ️ @hsanjuan fysa the Changelo section is generated during Kubo release (we have cli tool that pulls commits/prs from kubo/boxo/go-libp2p etc), moved this to "Highlights" section.
There was a problem hiding this comment.
Thanks!! Won't forget next time.
Comment on lines
+472
to
+480
| # confirm -w and --to-files are exclusive | ||
| # context: https://github.com/ipfs/kubo/issues/10611 | ||
| test_expect_success "ipfs add -r -w dir --to-files /mfs/subdir5/ errors (-w and --to-files are exclusive)" ' | ||
| ipfs files mkdir -p /mfs/subdir5 && | ||
| test_expect_code 1 ipfs add -r -w test --to-files /mfs/subdir5/ >actual 2>&1 && | ||
| test_should_contain "Error" actual && | ||
| ipfs files rm -r --force /mfs | ||
| ' | ||
|
|
There was a problem hiding this comment.
ℹ️ added basic regression test that guards this behavior
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10611.
When we are adding to an MFS folder ("--to-files=folder/"), we append the filename to the folder name.
However, when wrapping, the wrapping folder has no name and
path.Base("")returns..This creates a folder named "." inside the previous one. But mfs operations like
lsthink that "." is the current folder, and does not contemplate the option of a folder of name ".".Dealing with unnamed files in MFS is in general a footgun, so this commits attempts to prohibit that case.