chgrp: add option --from #7129
Conversation
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
I added a similar function |
|
indeed but as it uses ChrootError, it will require a bunch of refactor for 4 lines |
|
GNU testsuite comparison: |
jfinkels
left a comment
jfinkels
left a comment
There was a problem hiding this comment.
One minor comment, but it makes sense to me!
| const USAGE: &str = help_usage!("chgrp.md"); | ||
|
|
||
| fn parse_gid_from_str(group: &str) -> Result<u32, String> { | ||
| if let Some(gid_str) = group.strip_prefix(':') { |
There was a problem hiding this comment.
If the option is --from=:foo, could foo be a group name? This code assumes it is always a numeric group ID, I think. The docs (for chown, but I think it applies to chgrp also) seem to imply that it can be a group name: https://www.gnu.org/software/coreutils/manual/html_node/chown-invocation.html
Also, the docs for chgrp say
Change a file’s ownership only if it has current attributes specified by old-owner. old-owner has the same form as new-owner described above.
new-owner is described in the chown documentation as allowing anything of the form [USER] [: [GROUP]]. If that's true, then we should check for user names and IDs too.
All of these can be fixed separately (and common code refactored from chgrp, chown, and chroot), I just wanted to share my understanding of the GNU documentation.
There was a problem hiding this comment.
Probably yeah
I think it can be done as a follow up :)
| #[test] | ||
| #[cfg(not(target_vendor = "apple"))] | ||
| fn test_from_with_invalid_group() { | ||
| let (at, mut ucmd) = at_and_ucmd!(); | ||
| at.touch("test_file"); | ||
| #[cfg(not(target_os = "android"))] | ||
| let err_msg = "chgrp: invalid group: 'nonexistent_group'\n"; | ||
| #[cfg(target_os = "android")] | ||
| let err_msg = "chgrp: invalid group: 'staff'\n"; | ||
|
|
||
| ucmd.arg("--from") | ||
| .arg("nonexistent_group") | ||
| .arg("staff") | ||
| .arg("test_file") | ||
| .fails() | ||
| .stderr_is(err_msg); | ||
| } |
There was a problem hiding this comment.
This test fails on my machine. And I don't know if the error message is deliberately different from GNU chgrp. With GNU it is about an invalid user and not about an invalid group.
$ cargo run --features=unix -q chgrp --from nonexistent_group staff README.md
chgrp: invalid group: 'staff'
$ chgrp --from nonexistent_group staff README.md
chgrp: invalid user: ‘nonexistent_group’
|
GNU testsuite comparison: |
|
Did you see that a bunch of CI jobs fail? |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
missed it, thanks! |
Great :) |
3 participants
Should make tests/chgrp/from pass