Skip to content

Directory.CreateDirectory: create missing parents using default UnixFileMode. #74002

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.

Sign up for GitHub

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
jozkee merged 7 commits into from
Aug 19, 2022
Merged

Directory.CreateDirectory: create missing parents using default UnixFileMode. #74002

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b0acc3c
Directory.CreateDirectory: create missing parents using default UnixF…
tmds Aug 16, 2022
253e68a
Simplify TarHelpers.CreateDirectory.
tmds Aug 17, 2022
daed79e
PR feedback.
tmds Aug 18, 2022
03858aa
AssertFileModeEquals: don't skip on Android.
tmds Aug 19, 2022
7f3d1bd
Always overwite directory metadata.
tmds Aug 19, 2022
5271907
TarHelpers.Windows: remove CreateDirectory overwriteMetadata.
tmds Aug 19, 2022
e1cf9d4
Fix Assert indentation.
tmds Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Always overwite directory metadata.
  • Loading branch information
@tmds
tmds committed Aug 19, 2022
commit 7f3d1bd1fd07b0205bf2ef888e02b2582d308ad0
8 changes: 4 additions & 4 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,12 +287,12 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o

if (EntryType == TarEntryType.Directory)
{
TarHelpers.CreateDirectory(fileDestinationPath, Mode, overwrite, pendingModes);
TarHelpers.CreateDirectory(fileDestinationPath, Mode, pendingModes);
}
else
{
// If it is a file, create containing directory.
TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, overwrite, pendingModes);
TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, pendingModes);
ExtractToFileInternal(fileDestinationPath, linkTargetPath, overwrite);
}
}
Expand All @@ -309,13 +309,13 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b

if (EntryType == TarEntryType.Directory)
{
TarHelpers.CreateDirectory(fileDestinationPath, Mode, overwrite, pendingModes);
TarHelpers.CreateDirectory(fileDestinationPath, Mode, pendingModes);
return Task.CompletedTask;
}
else
{
// If it is a file, create containing directory.
TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, overwrite, pendingModes);
TarHelpers.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!, mode: null, pendingModes);
return ExtractToFileInternalAsync(fileDestinationPath, linkTargetPath, overwrite, cancellationToken);
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public int Compare (string? x, string? y)
internal static SortedDictionary<string, UnixFileMode>? CreatePendingModesDictionary()
=> new SortedDictionary<string, UnixFileMode>(s_reverseStringComparer);

internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool overwriteMetadata, SortedDictionary<string, UnixFileMode>? pendingModes)
internal static void CreateDirectory(string fullPath, UnixFileMode? mode, SortedDictionary<string, UnixFileMode>? pendingModes)
{
// Minimal permissions required for extracting.
const UnixFileMode ExtractPermissions = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute;
Expand All @@ -61,8 +61,8 @@ internal static void CreateDirectory(string fullPath, UnixFileMode? mode, bool o

if (Directory.Exists(fullPath))
{
// Apply permissions to an existing directory when we're overwriting metadata.
if (mode.HasValue && overwriteMetadata)
// Apply permissions to an existing directory.
if (mode.HasValue)
Copy link
Member

@jozkee jozkee Aug 19, 2022

Choose a reason for hiding this comment

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

I know tar overwrites metadata for existing directories by default, but it feels counterintuitive for our API to do that when overwriteFiles is false. I really don't know what's best here.

Suggested change
if (mode.HasValue)
if (mode.HasValue && overwriteMetadata)

Copy link
Member Author

@tmds tmds Aug 19, 2022

Choose a reason for hiding this comment

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

tar has this behavior.

       --overwrite
              Overwrite existing files when extracting.

       --overwrite-dir
              Overwrite metadata of existing directories when extracting (default).

--overwrite control in .NET is through overwrite.
.NET doesn't have an arg for --overwrite-dir but the behavior matches with the tar default.

I think we're good.

{
// Ensure we have sufficient permissions to extract in the directory.
bool hasExtractPermissions = (mode.Value & ExtractPermissions) == ExtractPermissions;
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ public void UnixFileModes(bool overwrite)

// Directory modes that are out-of-order are still applied.
Assert.True(Directory.Exists(outOfOrderDirPath), $"{outOfOrderDirPath}' does not exist.");
AssertFileModeEquals(outOfOrderDirPath, overwrite ? TestPermission4 : CreateDirectoryDefaultMode);
AssertFileModeEquals(outOfOrderDirPath, TestPermission4);
}

[Theory]
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectoryAsync.File.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ public async Task UnixFileModes_Async(bool overwrite)

// Directory modes that are out-of-order are still applied.
Assert.True(Directory.Exists(outOfOrderDirPath), $"{outOfOrderDirPath}' does not exist.");
AssertFileModeEquals(outOfOrderDirPath, overwrite ? TestPermission4 : CreateDirectoryDefaultMode);
AssertFileModeEquals(outOfOrderDirPath, TestPermission4);
}

[Fact]
Expand Down