feat(ipfs-car): avoid recursive writes #175
Conversation
fix #174 if output file exists in source path, just overwrite it, but don't include it in the packed car file
autonome
left a comment
autonome
left a comment
There was a problem hiding this comment.
looks good, thanks! added a couple of minor non-blocking suggestions
| let fullOutputPath | ||
| if (opts.output) { | ||
| fullOutputPath = path.resolve(opts.output) | ||
| } |
There was a problem hiding this comment.
| let fullOutputPath | |
| if (opts.output) { | |
| fullOutputPath = path.resolve(opts.output) | |
| } | |
| const fullOutputPath = opts.output !== null ? path.resolve(opts.output) : null; |
- stricter check
- remove unnecessary modifiableness
There was a problem hiding this comment.
I'm not convinced this will work. It's worth checking but I don't think opts.output is ever null. It might be undefined or the empty string.
| ? await filesFromPaths(paths, { | ||
| hidden, | ||
| filter: (filePath) => { | ||
| const include = !fullOutputPath || filePath !== fullOutputPath |
There was a problem hiding this comment.
| const include = !fullOutputPath || filePath !== fullOutputPath | |
| const include = fullOutputPath !== null || filePath !== fullOutputPath |
- stricter check
alanshaw
left a comment
alanshaw
left a comment
There was a problem hiding this comment.
I don't have super strong opinions here but I think I'd prefer an error. If I output to a CAR file in the source directory that I wanted to keep, then that CAR is now not in the new CAR and is no longer in the source directory (it got overwritten). It is lost forever.
agree that it's better to bail than to make an assumption that results in permanent data loss. however, the caller has themselves pointed to the output file. if they wanted to keep that file, why would they have made it the output file? |
|
People make mistakes! |
ok first do no harm then, bail loudly ftw low priority would be a --force or --overwrite for the not-a-mistake case |
fix #174
if output file exists in source path, just overwrite it, but don't include it in the packed car file
requires storacha/files-from-path#46