SuperCuber
Add copy target type
Closes #77.
Implements a new copy target type.
This is a much bigger change than I had expected, so there may be bugs and things I've missed. I've marked the PR as a draft for now, as there are still some things that I think should be discussed:
- It is currently not possible to compare the contents of non-UTF-8 files as all files are read to a string using
std::fs::read_to_string(). So, non-UTF-8 files end up asFileState(File(None)). We could read files into a bytes vector instead usingstd::fs::read(), but this would require changes in a lot of places. - Currently, if symlinks are not enabled,
symbolicandautomatictargets fallback totemplate. I think it would be better to fallback tocopyinstead, though this could potentially be breaking. Forautomatictargets, we can usefs.is_template()to determine whether to usecopyortemplate; whereas forsymbolic, we would just default tocopy. - Hinting to use
copyin the error for non-UTF-8 templates, as you suggested, would be nice, but I wasn't sure how to fit this in the code.
On a side note, there are a lot of clippy warnings about references. Were those left intentionally?
Other than my one comment this looks very good. I commend you for even adding tests to it :D Regarding your points:
FileState::File(None)was a temporary placeholder for not being able to represent nonutf files. I think this should becomeFileState::TextFile(String)andFileState::BinaryFile(Vec). Then you would use the appropriate variant here. Should probably use a different function thanread_to_stringthere.- I agree with this. This shouldn't be breaking either - all files that are of the symlink type are either explicitly selected that way or are detected that way, so having them be
copyshould in fact increase correctness for cases where the user disabled templating using type=symlink but they were being templated anyways. Whoopsie. - After doing point 1, there should be a some place where you will need to handle
FileState::BinaryFilein a match where it's expected to be a template from the context. Maybe incompare_template? I guess this will be easier to find with a compiler error pointing at it :P - For the clippy warnings, I guess those checks were added after the code was written :P Strange that they don't show up in CI though...
Two more things before this can be released:
- [ ] Wiki needs to be updated to reflect all changes
- [ ] A short entry needs to be added to the release description
FileState::File(None)was a temporary placeholder for not being able to represent nonutf files. I think this should becomeFileState::TextFile(String)andFileState::BinaryFile(Vec). Then you would use the appropriate variant here. Should probably use a different function thanread_to_stringthere.
I tried doing this, but this resulted in a lot of boilerplate in the comparison functions (compare_symlink(), compare_copy() and compare_template(). I've changed FileState::File(Option<String>) to FileState::File(Vec<u8>) instead, which I think works better.
I noticed that there are some instances where we read the file contents for a second time using read_to_string() even though the file has already been read into FileState. I believe this can be optimized away, but that's probably beyond the scope of this PR.
Two more things before this can be released:
- [ ] Wiki needs to be updated to reflect all changes
- [ ] A short entry needs to be added to the release description
Seems like Github doesn't let me do this from a PR. I think you'll have to do it on your side.
Just realized that there's a bug with updating copy targets. We can only detect that there is a difference between the source and target, but we cannot detect which one was modified. So, modifying the source and then trying to update results in dotter telling us that the target has been modified.
Template targets compare the cached file and the target file, and because we can assume that the cached file is unmodified (since users shouldn't be modifying the cache directly), the target file must have been modified if there is a change detected. We can't do the same for copy targets because we don't copy the file to the cache.
I think there are several approaches we can take from here:
- Always overwrite the target
- Use a similar method to templates — copy to the cache, and then compare that with the target
- Store timestamps in the cache, and then when comparing, compare with the target's modification timestamp rather than file contents
The second method is probably the easiest to implement, but making an additional copy isn't very efficient storage-wise. The first method may upset users who accidentally modified the target file instead of the source file.
I'm leaning towards the third method as it would eliminate the inefficiency of comparing files byte by byte. We can apply this to template targets as well (~~and maybe symbolic targets, though I'm not sure how modification timestamps work with symlinks~~ forgot that we can just check the symlink's target). We can use .modified() in std::fs::Metadata for this.
Sorry for the late reply, for some reason I wasn't subscribed to the PR so I didn't see your comment.
Regarding your solution 3, I think this is an interesting idea, although it seems like there's some nuance to how SystemTimes should be compared.
I was thinking instead of storing the timestamp as the content of the file you could have an empty file that is touched right after the target is updated - this way you can just compare the timestamps of the two files to see which one was modified last (should be cache). I don't see a reason why this can't be extended to templates as well.
The big issue I see with this is we now need to handle migrating between someone using the old cache format and the new one.
One idea I have is to bail with an error if you find a non-empty file in the cache and tell the user to delete cache/ and cache.toml.
Sorry for the late reply, for some reason I wasn't subscribed to the PR so I didn't see your comment.
No worries! It's been a busy past few weeks for me so I wouldn't have been able to work on this anyway.
Regarding your solution 3, I think this is an interesting idea, although it seems like there's some nuance to how
SystemTimes should be compared.
Yeah, it's certainly a nuance, but I assume time drift is a very rare occurrence. There's also the fact that users can modify the timestamps. But since duration_since() returns a Result, I imagine we can just warn the user and ask for confirmation if it happens to err.
I was thinking instead of storing the timestamp as the content of the file you could have an empty file that is touched right after the target is updated - this way you can just compare the timestamps of the two files to see which one was modified last (should be cache).
Ah, I was actually thinking of serializing the timestamps into cache.toml. Creating empty files would certainly work, but I think it's more tidy to store the timestamps in cache.toml, and we'd have less interactions with the filesystem. What do you think?
euration_since() returns a result
Now that I think about it, the Err case is the one that means that the file was modified. So time drift is probably undetectable... Meh, worst-case scenario the user will force deploy without understanding why.
Store the timestamps in cache.toml
This is an idea I haven't considered. It does seem pretty good, and it means we can remove cache folder altogether. (if serde supports SystemTime of course)
It's looking like the scope of this change is pretty big though - maybe enough to have its own "Remove cache folder" pull request... However implementing the copy type in the same way as template then modifying it after could be more needless work.
What do you think?
I don't think this change is neccesary - if the hook isn't a template then it will just make a file that is the same as the original...
IMO this complicates the function significantly (adds a Filesystem for example) for a small performance boost in case the hook isn't a template (which it probably is for most use cases)
I added this change because users would get the File {:?} is specified as 'template' but is not a templated file. Consider using 'copy' instead. warning (in perform_template_deploy()) if the hook didn't contain any templates, which is confusing since users cannot change the target type of hooks. You're right that it complicates the function, but I can't think of any other way of suppressing that message at the moment.
if serde supports SystemTime of course
Just checked, and seems like it does!
It's looking like the scope of this change is pretty big though - maybe enough to have its own "Remove cache folder" pull request... However implementing the copy type in the same way as template then modifying it after could be more needless work.
What do you think?
Yeah, I think it's big enough to have its own PR. We could temporarily put this PR on hold, and merge the "remove cache folder" PR first. And then finally rebase this PR on top. That way, no needless work would need to be done.
@SuperCuber @PlayerNameHere Any update about this PR?
Seems like we both lost interest/need in this.
@SuperCuber @PlayerNameHere Any update about this PR?
Seems like we both lost interest/need in this.
Yes this feature is very important for me.
You could build some os part(archlinux) with the sudo cp option
@SuperCuber @PlayerNameHere Any update about this PR?
Seems like we both lost interest/need in this.
Yes this feature is very important for me.
You could build some os part(archlinux) with the sudo cp option
You're welcome to open another PR.
Any updates on this? A copy feature would be really appreciated.
This is not being worked on. PRs are welcome :)
Hey, really sorry for ghosting this. I had been really busy with life and did not have the time to work on this PR. Unfortunately, I don't plan to continue working on this as I don't really have a need for it anymore. That said, if anyone would like to work on this, please feel free to use the code in this PR.