Skip to content

dircolors: remove unsafe#12228

Open
oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:dirc-unsafe-env
Open

dircolors: remove unsafe#12228
oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:dirc-unsafe-env

Conversation

@oech3
Copy link
Copy Markdown
Contributor

@oech3 oech3 commented May 11, 2026

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2026 •
edited
Loading

GNU testsuite comparison:

Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/follow-name (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/retry (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.
Note: The gnu test tests/tail/pid is now being skipped but was previously passing.
Skip an intermittent issue tests/pr/bounded-memory (was skipped on 'main', now failing)

@oech3 oech3 force-pushed the dirc-unsafe-env branch from 25f4f5b to 50a6eaf Compare May 11, 2026 04:47
@oech3 oech3 marked this pull request as ready for review May 11, 2026 05:07
@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented May 11, 2026

Good work. This is exactly the kind of unsafe worth removing. I think most uses of env::set_var and env::remove_var are unnecessary if we refactor the tests accordingly.

xtqqczze
xtqqczze reviewed May 11, 2026
View reviewed changes
Comment thread src/uu/dircolors/src/dircolors.rs Outdated
@oech3 oech3 force-pushed the dirc-unsafe-env branch from 50a6eaf to 6c7ed3b Compare May 11, 2026 13:17
@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented May 11, 2026

I think fn run_gnu_cmd can use Command::env to remove unsafes.

@xtqqczze
Copy link
Copy Markdown
Contributor

xtqqczze commented May 11, 2026

Yeah, we should definitely do that instead of mutating the environment. Regardless of the unsafe aspect, mutating global environment state makes the code harder to reason about and can interfere with optimizations.

@xtqqczze xtqqczze mentioned this pull request May 11, 2026
Open
@oech3 oech3 force-pushed the dirc-unsafe-env branch 3 times, most recently from 21993e0 to 895f7ea Compare May 14, 2026 10:00
xtqqczze
xtqqczze reviewed May 14, 2026
View reviewed changes
Comment thread src/uu/dircolors/src/dircolors.rs
@oech3 oech3 force-pushed the dirc-unsafe-env branch from 895f7ea to cb24fa9 Compare May 15, 2026 16:37
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 15, 2026

Merging this PR will improve performance by 13.71%

⚡ 2 improved benchmarks
✅ 315 untouched benchmarks
⏩ 46 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation ls_recursive_balanced_tree[(6, 4, 15)] 50.9 ms 48.9 ms +4.02%
⚡ Memory cp_recursive_deep_tree[(120, 4)] 699.2 KB 562.5 KB +24.31%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.

Comparing oech3:dirc-unsafe-env (cb24fa9) with main (d09f351)

Open in CodSpeed

Footnotes

  1. 46 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. ↩

xtqqczze
xtqqczze reviewed May 15, 2026
View reviewed changes
Comment thread src/uu/dircolors/src/dircolors.rs
pub fn guess_syntax() -> OutputFmt {
match env::var("SHELL") {
Ok(ref s) if !s.is_empty() => {
pub fn guess_syntax(env: Option<OsString>) -> OutputFmt {
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
pub fn guess_syntax(env: Option<OsString>) -> OutputFmt {
pub fn guess_syntax<P: AsRef<Path>>(path: Option<P>) -> OutputFmt {

We can make this more type-safe and avoid .into() at the call sites.

Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze May 15, 2026

Choose a reason for hiding this comment

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

Then x.as_os_str().is_empty() until path_is_empty is stabilised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@xtqqczze xtqqczze xtqqczze left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@oech3 @xtqqczze