bootstrap: (half) merge cp_r and cp_filtered #111734
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Looking at 2 uses of |
…ernal which will call remove_file itself
| self.create_dir(&dst); | ||
| self.recurse_(&path, &dst, &relative, filter); | ||
| } else { | ||
| let _ = fs::remove_file(&dst); |
There was a problem hiding this comment.
Can you say more about removing these lines? What is replacing them / why is it safe to do that?
There was a problem hiding this comment.
I've described why this line can be removed in commit's title:
dont call remove_file in cp_filtered, as self.copy will call copy_internal which will call remove_file itself
Hm, looks like i mean recurse_ function, not cp_filtered
There was a problem hiding this comment.
Comment about remove_dir_all: #111734 (comment)
| if self.config.dry_run() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
So this dry_run is only difference.
There was a problem hiding this comment.
But we may break things if we remove it? I'm not sure we want to do this clean up, seems like these functions are distinct if similar today.
I don't think this is valid reasoning. We don't want to write this code against current usage, we want it to work against future usages as well -- which may have a destination directory that does exist. |
Mark-Simulacrum
added
S-waiting-on-author
|
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks! FYI: when a PR is ready for review, send a message containing |
|
☔ The latest upstream changes (presumably #116196) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Dylan-DPC
added
S-inactive
Half-way merges cp_r and cp_filtered functions, as there some different points:
cp_randcp_filtereddifference:skip running if dry_run =>
cp_r: yes,cp_filtered: noremoves target dir if it exist (i.e. can't be used to copy files into dst dir from multiple sources, (or no?)) =>
cp_r: no,cp_filtered: yesCurrently this two places didn't changed.