Address PR #19 review comments: naming, test coverage, and documentation#21
Conversation
Co-authored-by: bwalsh <47808+bwalsh@users.noreply.github.com>
Co-authored-by: bwalsh <47808+bwalsh@users.noreply.github.com>
Co-authored-by: bwalsh <47808+bwalsh@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR applies follow-up review feedback from PR #19, refining the OptimalChunkSize algorithm documentation, tightening test coverage and naming around multipart parts calculation, and improving observability and comments in common constants.
Changes:
- Expanded and clarified the engineering note in
docs/optimal-chunk-size.mdto document the newOptimalChunkSizebehavior, test strategy, and cleaned up markdown artifacts. - Strengthened
TestOptimalChunkSizeinclient/upload/utils_test.gowith additional boundary and scaled-range cases, renamedwantChunks→wantParts, and fixed the parts calculation for zero-byte files. - Updated
client/common/constants.goto fix spelling in comments, remove dead code, add logging when git config lookup fails, and normalize error message formatting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/optimal-chunk-size.md | Documents the new OptimalChunkSize algorithm, its behavior across size ranges, and provides a concrete test plan and examples. |
| client/upload/utils_test.go | Extends and renames the table-driven tests to validate chunk sizes and part counts across edge cases and scaled ranges, including the 0-byte behavior. |
| client/upload/utils.go | Minor cleanup in OptimalChunkSize, keeping implementation aligned with the documented algorithm. |
| client/common/constants.go | Corrects comment spelling, adds a warning log on git config read failure for multipart min chunk size, removes obsolete commented code, and standardizes error message formatting in GetLfsCustomTransferInt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| name: "0 bytes", | ||
| fileSize: 0, | ||
| wantChunkSize: 1 * common.MB, | ||
| wantParts: 0, | ||
| }, | ||
| { | ||
| name: "1MB", | ||
| fileSize: 1 * common.MB, | ||
| wantChunkSize: 1 * common.MB, | ||
| wantParts: 1, | ||
| }, |
There was a problem hiding this comment.
The test table exercises the 0-byte, exact-threshold, and +1 byte boundary cases, but it does not include the documented -1 byte boundaries or a negative fileSize, even though docs/optimal-chunk-size.md ("Test design notes" #3 and #4) explicitly calls for those cases. To fully match the documented test plan and guard against off-by-one or fallback regressions, consider adding subtests for fileSize < 0 and for each X - 1 byte threshold (e.g., 100 MB - 1 B, 1 GB - 1 B, etc.).
Applies all actionable feedback from PR #19 review thread #3690473082.
Code Quality
wantChunks→wantParts,chunks→parts(consistent with multipart upload terminology)>%q<→%q(conventional Go error format)maximun→maximum,terrabytes→terabytesTest Coverage
Expanded
TestOptimalChunkSizefrom 7 to 15 cases covering all thresholds and boundaries:fileSize == 0Observability
Added logging when git config fails:
Documentation
Added algorithm description to NEW section matching OLD section structure:
\-→-)💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.