Repository navigation
chown: set group to owner's login group on "OWNER:" - #15204
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The shared parser incorrectly changes --from=OWNER: filtering semantics.
1 open finding
What changed in this PR
Updates chown OWNER: to apply the owner’s login group.
Changes:
- Resolves login groups during owner-spec parsing.
- Adds unit and integration coverage.
| File | Description |
|---|---|
src/uu/chown/src/chown.rs |
Implements login-group resolution. |
tests/by-util/test_chown.rs |
Tests valid and invalid owner specifications. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+223
to
+228
| if !user.is_empty() && group.is_empty() && spec != user { | ||
| // A separator was given but the group was not ("owner:"), so the | ||
| // group is set to the owner's login group. | ||
| return match Passwd::locate(user) { | ||
| // Lookup must occur through name, not ID, to be valid here. | ||
| Ok(passwd) if passwd.name == user => Ok((Some(passwd.uid), Some(passwd.gid))), |
sylvestre
reviewed
Oct 9, 2026
| return match Passwd::locate(user) { | ||
| // Lookup must occur through name, not ID, to be valid here. | ||
| Ok(passwd) if passwd.name == user => Ok((Some(passwd.uid), Some(passwd.gid))), | ||
| _ => Err(USimpleError::new( |
Contributor
There was a problem hiding this comment.
for an unknown user GNU says "invalid user: 'nosuchuser:'", not "invalid spec". Could you please keep the invalid-user error here and only use invalid spec for the numeric case?
Contributor
Author
There was a problem hiding this comment.
When I try it out I get this:
❯ gnuchown someone: file
gnuchown: invalid spec: ‘someone:’
So I think the "invalid spec" message is correct in this case.
sylvestre
reviewed
Oct 9, 2026
sylvestre
reviewed
Oct 9, 2026
| .arg("auserthatdoesntexist:") | ||
| .arg("f") | ||
| .fails() | ||
| .stderr_contains("chown: invalid spec: 'auserthatdoesntexist:'"); |
Contributor
There was a problem hiding this comment.
this locks in the wrong message, GNU prints "invalid user" here
"chown USER:" left the group unchanged instead of setting it to USER's login group like in GNU.
| let user_name = String::from(result.stdout_str().trim()); | ||
| assert!(!user_name.is_empty()); | ||
|
|
||
| let result = scene.cmd("id").arg("-gn").run(); |
Comment on lines
199
to
+203
| /// and the owner or group can be specified either as an ID or a | ||
| /// name. The `sep` argument specifies which character to use as a | ||
| /// separator between the owner and group; calling code should set | ||
| /// this to `':'`. | ||
| /// this to `':'`. If a separator is given but the group is omitted | ||
| /// (as in `"owner:"`), the group is set to the owner's login group. |
|
GNU testsuite comparison: |
Contributor
|
Thanks for your PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
chown OWNER: FILEchanged the owner but left the group unchanged,while GNU chown sets the group to OWNER's login group. This fixes that
and adds test.
Fixes #12751