Add Command #27
Add Command #27
Conversation
new add command
| ### `ExeDat:add_file` | ||
|
|
||
| ```lua | ||
| function ExeDat:add_file(path: string, contents: Buffer) | ||
| ``` | ||
|
|
||
| appends the file data into the .dat file. | ||
|
|
There was a problem hiding this comment.
- "Append" implies that the .dat file contents are meaningfully ordered. Is that actually the case? If not, I would suggest replacing "append" with "add".
- It may be worth adding a note to the documentation for
write_filethat it fails if the file does not exist. (Although now, it doesn't anymore - see my other comments.) - Please uppercase the first letter of the sentence.
| for file in self.overlaid_files.keys() { | ||
| if let None=self.base.zr.file_names().find(|&x| x==file) | ||
|
|
||
| { | ||
| log::info!("adding {}", file); | ||
| zw.start_file( | ||
| file, | ||
| zip::write::FileOptions::default(), | ||
| )?; | ||
| zw.write_all(self.overlaid_files.get(file).unwrap())?; | ||
| } |
There was a problem hiding this comment.
Please run cargo fmt to fix formatting
| pub fn add<'a>(&'a mut self,path: &str,contents: Vec<u8>) -> Result<(),std::io::Error> { | ||
| self.overlaid_files.insert(path.to_string(), contents); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This function is effectively a copy of write but without the forward/backward slash sanitization and directory check. That has a couple of likely undesired implications:
- You can call
add_filewith the exact path of an existing file, and it will happily overwrite the file. - You can call
add_filewith the path of an existing file but with some of the forward/backward slashes flipped, and it will happily go ahead and do it. Which, I'm not sure what will actually happen if you try to do that in a zip file, but it's probably not a supported use case. - You can pass
add_filethe path of a directory and it will happily try to overwrite it, which will probably fail. - You can pass
write_filethe path of a file that doesn't exist yet, and it will add it with all the slashes flipped. This is now possible because your extension ofpack_intomakes it no longer ignore all the non-existent files inoverlaid_files.
I think the add function needs an extra check to avoid accidentally overwriting existing files/directories, taking forward/backward slash sanitization in mind. Similarly, we should extend the write function with a check for existing files.
My suggestion would be to:
- Extend the
correct_path()function to add a second file exists check after the slashes have been swapped, and change the return type fromStringtoOption<String>. Then it can return eitherSome(value), which means that the file exists andvalueis the canonical path; orNone(), which means the file doesn't exist regardless of the forward/backward slashes. - In
add, return anErrorifcorrect_path(path)returnsSome. - In
write, return anErrorifcorrect_path(path)returnsNone. (In addition to the current directory check.)
| } | ||
| } | ||
|
|
||
| for file in self.overlaid_files.keys() { |
There was a problem hiding this comment.
By changing get in self.overlaid_files.get(entry.name()) in the above loop to remove, you can avoid this zr.file_names().find(...) check altogether. Because then the previous loop will remove all the already-existing files from overlaid_files as it replaces them.
Aside from simplifying the code, this might also speed things up because zr.file_names().find(...) is an O(n) operation called in a loop.
Might also be worth adding a code comment here that the first loop is replacing existing files and the second loop is adding new files.
2 participants
This fork adds an add command to add new files to the dats. For example, if someone wants to add new fonts after modifying the config file to load more files.
Example usage:
exe1_dat:add_file("exe1/data/ui/font_eng_01.pak", exe1_font_eng_01_pak)Result: