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 #132 removes the temporary file #166

Merged
merged 2 commits into from Nov 13, 2018
Merged

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Nov 13, 2018

How to test?

Just run the standard tests.

make check

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, this unlinks the file but it is then used later in the function

@@ -48,6 +48,7 @@ def decrypt_submission_or_reply(filepath, target_filename, home_dir,
logger.error("GPG error: {}".format(msg))

os.unlink(err.name)
os.unlink(out.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we can move these two lines to right before we return from this function? so that in both cases these files are cleaned up

out is a tempfile.NamedTemporaryFile created with delete=True,
(default) so it'll get up deleted on disk once the file is closed.

err is a tempfile.NamedTemporaryFile created with delete=False,
so we'll need to delete it after closing it.

This commit just makes sure both err and out are deleted after use.
It also adds some comments for clarity.
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see discussion in #132, but basically: turns out out.name is automatically cleaned up. i added a commit just clarifying this and making sure that err is cleaned up in the else stanza (should be empty but it doesn't hurt to explicitly delete)

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good catch/comments on delete=False behavior

@heartsucker
Copy link
Contributor

heartsucker commented Nov 13, 2018

Why not just use a context manager or try/finally for this? Seems like this is error prone.

(edit: this is just a nit)

@redshiftzero
Copy link
Contributor

yepppp that is also good and equivalent. I just wanted to at least add explanatory comments (for the benefit of auditors) and make a minimal diff change for alpha. @joshuathayer pointed out in the issue that this entire file is all modified in the replies branch anyway so we'll be favoring the changes in there

@redshiftzero redshiftzero merged commit 3019b68 into master Nov 13, 2018
@redshiftzero redshiftzero deleted the remove_the_temporary branch November 13, 2018 21:43
legoktm pushed a commit that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants