Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
None => ok((recent.unwrap_or(older), recent.map(|_| older)))mapping is correct but quite… · Newls -loutput.… · Newolderis used both as the name of the expected older-format string and also as the… · New
What changed in this PR
Fixes ls custom time-style parsing so that two-format strings interpret the first line as the non-recent (older) format and the second line as the recent format, while keeping single-format behavior and handling empty halves consistently across --time-style and TIME_STYLE.
Changes:
- Reverse mapping for
+FORMAT1\nFORMAT2soFORMAT1applies to non-recent andFORMAT2to recent files. - Preserve single-format behavior and support empty “older”/“recent” halves.
- Add tests covering option vs
TIME_STYLE, single vs two formats, and empty halves.
| File | Description |
|---|---|
| tests/by-util/test_ls.rs | Updates existing expectations and adds coverage for custom two-format ordering and empty halves via option/env. |
| src/uu/ls/src/config.rs | Adjusts parse_time_style parsing/mapping logic for custom +... time styles to match intended ordering. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
GNU testsuite comparison: |
| match it.next() { | ||
| None => ok((recent, older)), | ||
| Some(_) => Err(LsError::TimeStyleParseError(String::from(field))), | ||
| let older = it.next().unwrap_or_default(); |
There was a problem hiding this comment.
older is passed as the recent format just below, could you rename it to first? it reads wrong otherwise
There was a problem hiding this comment.
Renamed the first parsed format to first in c0212f7. It is used for both timestamps when there is no newline, and for older timestamps when there is one.
| } | ||
|
|
||
| #[test] | ||
| fn test_ls_custom_time_style_recent_and_older() { |
There was a problem hiding this comment.
this overlaps quite a bit with test_ls_time_styles, could the new empty-half cases go there instead? thanks
There was a problem hiding this comment.
Moved the empty-half and environment cases into test_ls_time_styles in c0212f7, reusing its recent and old fixtures. The focused test and clippy pass.
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | three_39_bit_primes |
419.2 ms | 647.9 ms | -35.3% |
| ❌ | Simulation | du_wide_tree[(5000, 500)] |
18.4 ms | 18.9 ms | -3.01% |
| ⚡ | Simulation | complex_relative_date |
400.3 µs | 288.2 µs | +38.86% |
| ⚡ | Simulation | five_38_bit_primes |
2.1 s | 1.8 s | +22.09% |
| ⚡ | Simulation | thirteen_39_bit_primes |
9.3 s | 8.8 s | +5.89% |
| ⚡ | Simulation | du_deep_tree[(100, 3)] |
2.2 ms | 2.1 ms | +3.28% |
| ⚡ | Simulation | ls_recursive_deep_tree[(200, 2)] |
3.4 ms | 3.3 ms | +3.19% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ddy314:fix/ls-custom-time-order (c0212f7) with main (b947458)
Footnotes
-
54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Custom time styles put the non-recent format before the newline and the recent format after it. Reverse the current mapping, retain single-format behavior, and cover empty halves through both the option and TIME_STYLE.
Refs #14727 (the two-format ordering; other strftime cases remain).
Validation: 186 ls tests passed, 1 ignored; Clippy and rustfmt passed.
AI-assisted with Codex.