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

fsutil: Explicit parent directory creation #76

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

woky
Copy link
Contributor

@woky woky commented Jun 21, 2023

Currently fsutil.Create() always creates missing parent directories with
0755 mode. While this is handy for some use cases, during extraction we
want to create every path with attributes from the tarball and having
parent directories created automatically hides the bugs where we fail to
create some directories from the tarball.

One such bug is when a glob matches. In this case, the directories
before wildcards are created by os.MkdirAll() in fsutil.Create().

Don't fix the bug now but make the creation of the parent directories
explicit in CreateOptions with the Dirs attribute. Later, we will drop
it from call sites where appropriate.

Currently fsutil.Create() always creates missing parent directories with
0755 mode. While this is handy for some use cases, during extraction we
want to create every path with attributes from the tarball and having
parent directories created automatically hides the bugs where we fail to
create some directories from the tarball.

One such bug is when a glob matches. In this case, the directories
before wildcards are created by os.MkdirAll() in fsutil.Create().

Don't fix the bugs now but make the creation of the parent directories
explicit in CreateOptions with the Dirs attribute. Later, we will drop
it from call sites where appropriate.
@woky woky force-pushed the pub/fsutil-explicit-parents branch from bd81485 to f85d1ee Compare June 22, 2023 04:44
@cjdcordeiro cjdcordeiro removed the Priority Look at me first label Jun 22, 2023
@woky woky added the Priority Look at me first label Aug 8, 2023
Copy link
Member

@TheSignPainter98 TheSignPainter98 left a comment

Choose a reason for hiding this comment

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

Code looks good, just one naming and comment suggestion :)

internal/fsutil/create.go Outdated Show resolved Hide resolved
@woky woky force-pushed the pub/fsutil-explicit-parents branch from 332c567 to e723ac2 Compare August 9, 2023 19:55
@woky woky requested a review from jnsgruk August 10, 2023 11:24
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

LGTM. Can we please just get the docs fixed? Happy to merge this as-is if you'd rather do that separately.

internal/fsutil/create.go Outdated Show resolved Hide resolved
internal/fsutil/create.go Outdated Show resolved Hide resolved
@niemeyer niemeyer added the Reviewed Supposedly ready for tuning or merging label Aug 28, 2023
@cjdcordeiro
Copy link
Collaborator

thanks @niemeyer .

LGTM. Can we please just get the docs fixed? Happy to merge this as-is if you'd rather do that separately.

@woky let's fix it here if possible, and only then merge ;)

@woky woky force-pushed the pub/fsutil-explicit-parents branch from 0703150 to cacbdf7 Compare August 29, 2023 10:17
@woky woky requested a review from niemeyer August 29, 2023 10:19
@woky
Copy link
Contributor Author

woky commented Aug 29, 2023

thanks @niemeyer .

LGTM. Can we please just get the docs fixed? Happy to merge this as-is if you'd rather do that separately.

@woky let's fix it here if possible, and only then merge ;)

Updated comments.

Copy link
Member

@jnsgruk jnsgruk left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for the iteration @woky :)

@jnsgruk jnsgruk merged commit f235273 into canonical:main Aug 29, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first Reviewed Supposedly ready for tuning or merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants