Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #1341: clean up temp file with sudo uploads if destination path… #1586

Closed
wants to merge 1 commit into from

Conversation

@UkuLoskit
Copy link

@UkuLoskit UkuLoskit commented Apr 7, 2017

… is invalid

@UkuLoskit
Copy link
Author

@UkuLoskit UkuLoskit commented May 12, 2017

Any comments on this one?

@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 12, 2017

(for easier forward referencing: #1341)

Not sure how this got past me originally...looks all right at a glance, thanks!

Wondering if try/finally makes more sense here, it's normally what one wants to do in this sort of spot (i.e. my mental alarm bells went off at except BaseException), and since one uses rm -f, it will not error even if the mv succeeded.

Only downside is in the success case it's one extra shell command that is not strictly necessary, but I don't know if that bothers me a ton. Readability vs efficiency, I guess.

@bitprophet bitprophet added this to the 1.12.3 milestone May 12, 2017
@UkuLoskit
Copy link
Author

@UkuLoskit UkuLoskit commented May 14, 2017

Thanks for the quick feedback, @bitprophet. This was exactly my line of reasoning that calling rm always would be a bit wasteful, for example in a scenario where you are uploading a large file tree.

I could still change it, if you'd like.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 18, 2017

I think I'm okay with it as-is for now; as much as I enjoy beating people over the head with the readability clause, an extra almost-never-useful shell session inside something this frequently used & which would execute unconditionally, just leaves a bad taste.

bitprophet added a commit that referenced this pull request Nov 27, 2018
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Nov 27, 2018

Merged into 1.14 branch.

@bitprophet bitprophet closed this Nov 27, 2018
ojrac added a commit to demiurgestudios/fabric that referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.