-
-
Notifications
You must be signed in to change notification settings - Fork 150
Improve max cli arguments length calculation #1518
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1518 +/- ##
==========================================
- Coverage 91.98% 91.97% -0.01%
==========================================
Files 88 88
Lines 18374 18390 +16
==========================================
+ Hits 16901 16914 +13
- Misses 1473 1476 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📦 Cargo Bloat ComparisonBinary size change: +0.44% (22.5 MiB → 22.6 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the calculation of maximum CLI argument lengths by adopting a more accurate approach from the argmax library. The changes primarily focus on better accounting for pointer sizes, environment variables, and platform-specific limitations.
Changes:
- Refactored CLI argument length calculation to use more precise size accounting with pointer overhead and null terminators
- Extracted static
ARG_MAXandPAGE_SIZEvalues usinglibc::sysconffor Unix systems - Updated
Partitionsiterator to track remaining argument length instead of computing current length - Made an unrelated syntax fix in the Haskell language module
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/prek/src/run.rs | Core refactoring of CLI argument length calculation with new helper functions and improved platform-specific handling |
| crates/prek/src/languages/haskell.rs | Unrelated syntax fix changing closure parameter order in get_or_try_init call |
Comments suppressed due to low confidence (1)
crates/prek/src/run.rs:220
- The panic message uses the old calculation method
filename.as_os_str().len() + 1instead of the newarg_size()function. This creates an inconsistency where:
- The check uses
arg_size(filename)to determine if a file fits (line 206) - The panic message reports the size using a different calculation (line 220)
This inconsistency could confuse users because the reported size won't match what the code actually checked. The panic message should use arg_size(filename) for consistency and accuracy.
// is too long to fit in the command line by itself.
Adapted from https://github.com/sharkdp/argmax