chgrp: add option --from #7129
chgrp: add option --from #7129
Conversation
sylvestre
force-pushed
the
chgrp
branch
2 times, most recently
from
e94d84d to
78cff4f
Compare
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
chgrp
branch
5 times, most recently
from
1b3db36 to
f195b52
Compare
|
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: |
| @@ -19,6 +19,24 @@ use std::os::unix::fs::MetadataExt; | |||
| const ABOUT: &str = help_about!("chgrp.md"); | |||
| 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.
| #[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’
| return; | ||
| } | ||
|
|
||
| at.touch("test_file"); |
There was a problem hiding this comment.
I think this file is not needed as it isn't used anywhere else in this test.
| at.touch("test_file"); |
Should make tests/chgrp/from pass