feat: add TarUtilities tar creation methods#354
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
+ Coverage 91.76% 92.17% +0.41%
==========================================
Files 64 65 +1
Lines 2755 2925 +170
Branches 364 386 +22
==========================================
+ Hits 2528 2696 +168
Misses 138 138
- Partials 89 91 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds internal tar-creation helpers to support the ongoing FileStore implementation breakdown, plus a focused xUnit test suite validating basic tar entry construction behavior.
Changes:
- Introduces
TarUtilitieswithTarDirectoryAsync, directory entry creation, and default Unix mode helpers. - Adds unit tests covering directory/file entries, timestamps (reproducible vs non-reproducible), uid/gid portability, and prefix normalization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/OrasProject.Oras/Content/File/TarUtilities.cs | Implements tar writing for a directory tree, including reproducible timestamp option and basic symlink handling. |
| tests/OrasProject.Oras.Tests/Content/File/TarUtilitiesTest.cs | Adds unit tests validating tar contents and metadata produced by TarUtilities. |
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
af1c4a1 to
6d39ddb
Compare
akashsinghal
left a comment
There was a problem hiding this comment.
Caution
This review was generated by Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT.
The implementation is solid — prefix validation, symlink-aware traversal, OS-aware separator normalization, and deterministic ordering are all well done. Good test coverage across the major paths. A few observations below.
|
Many lines in both files are wrapped at ~55-60 columns, well below the repo's target of ~100 (hard limit 120). This makes the code harder to scan. A few examples from // Currently (wrapped at ~55 cols):
var dirs =
Directory.GetDirectories(currentDir); // 61 chars unwrapped
var relativePath = Path.GetRelativePath(
sourceDirectory, dir); // 74 chars unwrapped
symEntry.ModificationTime =
DateTimeOffset.UnixEpoch; // 72 chars unwrapped
using var tarWriter = new TarWriter(
outputStream, leaveOpen: true); // 71 chars unwrapped
await tarWriter.WriteEntryAsync(
rootEntry, cancellationToken)
.ConfigureAwait(false); // 90 chars unwrappedAll of these fit comfortably on a single line under 100 columns. Same pattern throughout the test file. Consider unwrapping lines that are under ~100 columns to improve readability. |
|
Test consolidation opportunity — several tests share identical setups and could be merged without losing coverage:
Tests that should stay separate (unique coverage): This would bring the count from ~18 to ~11-12 tests with equivalent coverage and less duplicated setup code. |
- Add exception docs to TarDirectoryAsync for ArgumentException and DirectoryNotFoundException - Document recursion depth assumption and symlink target preservation behavior in WalkDirectoryAsync remarks - Clarify GetUnixFileMode returns hardcoded portable modes (755/644), not actual filesystem permissions Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
- Unwrap lines in TarUtilities.cs to target ~100 columns - Consolidate related tests in TarUtilitiesTest.cs: merge BasicFiles+SetsUidGidToZero+PrefixWithoutSlash, merge WithSubdirectories+DeterministicOrder+Reproducible_SetsEpoch, merge CreateDirectoryEntry tests into Theory, merge GetUnixFileMode tests into Theory Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
002271f to
7f416cb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
- Add Path.IsPathRooted check plus cross-platform HasDrivePrefix guard (e.g. C:/abs, C:\abs) since Path.IsPathRooted does not recognise drive letters on non-Windows platforms - Add test cases for C:/abs and C:\abs prefixes Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
|
Caution This review was generated by Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT. Second-round review of PR #354 All 30 prior threads addressed. Test consolidation (18 → 11 methods), lazy enumeration for non-reproducible mode, symlink handling for directories, prefix validation improvements, and better docs — clean iteration. Two observations: 1.
2. nit: Otherwise SGTM. The symlink handling (both file and directory), reproducible mode with deterministic ordering, and prefix validation are well-implemented. |
|
@wangxiaoxuan273 could you address the final comments on this PR? |
Description
Part of #37
Add tar archive creation utilities as part of the FileStore implementation breakdown.
Changes
TarUtilities.TarDirectoryAsync— creates a tar archive from a directory, writing entries to an output stream. Supports:reproducible: true)TarUtilities.CreateDirectoryEntry— creates aPaxTarEntryfor a directory with correct mode and optional epoch timestampTarUtilities.GetUnixFileMode— returns standard Unix file modes: 755 for directories, 644 for filesTests (13 test methods)
TarDirectoryAsync: basic files, subdirectories, reproducible timestamps, non-reproducible timestamps, empty directory, file content preservation, uid/gid zero, prefix normalizationCreateDirectoryEntry: slash appending, preserving trailing slash, reproducible timestampGetUnixFileMode: directory returns 755, file returns 644Context
Part 4 of the FileStore PR breakdown (#328), per sajayantony's suggested split. This PR has no dependencies and can be reviewed independently.