Skip to content

python-stdlib/tarfile: Fixed permissions when adding to archive. #799

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

Merged

Conversation

ubidefeo
Copy link
Contributor

@ubidefeo ubidefeo commented Feb 5, 2024

This change fixes issue #797 which I opened a couple of days ago.
More context to be found there.

This is my first PR in which I tried to be neater, hope it'll get reviewed by someone :)

thanks
ubi

@ubidefeo
Copy link
Contributor Author

ubidefeo commented Feb 5, 2024

I changed the value from hex to oct in order to avoid the variety of formats 😄

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

One small request, but this change LGTM. Thanks @ubidefeo!

@@ -63,7 +64,7 @@ def skip(self):
class TarInfo:
def __init__(self, name=""):
self.name = name
self.mode = _S_IFDIR if self.name[-1] == "/" else _S_IFREG
self.mode = _S_IFDIR if os.stat(name)[0] == 0x4000 else _S_IFREG
Copy link
Member

Choose a reason for hiding this comment

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

What happens if name doesn't exist on disk? In the case that you are extracting from a tar file, the files most likely do not exist on disk, so this will now raise an exception because the file isn't found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @dpgeorge
I didn't think of that at all 🤔

would you be ok if I added a try/except condition?
I like the flexibility offered by not having to rely on what the user hands in.

In my code I always test if the fs item exists, but I neglected to do it in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dug deeper and probably have to handle archive and expand in two different ways.
During expansion it does need to rely on the name coming from the item in the archive 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dpgeorge @projectgus
please take a look at my last commit, I think I found a compromise 😄

@@ -1,6 +1,7 @@
"""Subset of cpython tarfile class methods needed to decode tar files."""

import uctypes
import os
Copy link
Member

Choose a reason for hiding this comment

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

This import shouldn't be needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true!
fixing it now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!
I really appreciate your time, @dpgeorge 🙏🏼

@dpgeorge dpgeorge force-pushed the bugfix/tarfile-write-permissions branch from 9b64934 to 8058b29 Compare February 8, 2024 23:44
@dpgeorge dpgeorge merged commit 8058b29 into micropython:master Feb 8, 2024
@dpgeorge
Copy link
Member

dpgeorge commented Feb 8, 2024

Thanks for updating. Rebased and merged.

@ubidefeo ubidefeo deleted the bugfix/tarfile-write-permissions branch February 9, 2024 05:27
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.

3 participants