Skip to content

Implement namei command #278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

cdebanil
Copy link

@cdebanil cdebanil commented Apr 2, 2025

Implemented namei command without selinux support. Will add that soon...

@cakebaker
Copy link
Contributor

Can you please run cargo fmt and cargo clippy? Also some tests would be nice ;-)

@cdebanil cdebanil closed this Apr 3, 2025
@cdebanil cdebanil force-pushed the implement-namei-command branch from 9fc8fda to 66fe3b6 Compare April 3, 2025 14:49
@cdebanil cdebanil reopened this Apr 3, 2025
@cdebanil
Copy link
Author

cdebanil commented Apr 3, 2025

Can you please run cargo fmt and cargo clippy? Also some tests would be nice ;-)

I have run cargo fmt and cargo clippy. Also I have added selinux support. Will add tests as soon as possible.

@cdebanil
Copy link
Author

cdebanil commented Apr 5, 2025

I have added tests. Will add windows support soon.

@cdebanil
Copy link
Author

cdebanil commented Apr 6, 2025

Implemented windows support. All checks are passing. Maintainers @cakebaker , please review and merge, if there are no issues.

[dependencies]
uucore = { workspace = true }
clap = { workspace = true }
selinux = { version = "0.1", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this old version of the selinux crate? The current version is 0.5.1.

Copy link
Author

Choose a reason for hiding this comment

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

That is a typo. I have fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please also commit the updated Cargo.lock file?

Comment on lines +15 to +26
#[test]
fn test_fails_on_non_existing_path() {
let (at, mut ucmd) = at_and_ucmd!();
let argmnt = at.plus_as_string("nonexisting");

#[cfg(target_os = "windows")]
let err = "The system cannot find the file specified";
#[cfg(not(target_os = "windows"))]
let err = "No such file or directory";

ucmd.arg(argmnt).fails().code_is(1).stderr_contains(err);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great that you test what happens if a path doesn't exist. What happens in the other case, if a path does exist?

Copy link
Author

Choose a reason for hiding this comment

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

If path does exist then the code runs as expected. Tests for such cases are given in test_long_arg test_mode_arg etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

If path does exist then the code runs as expected

Are you sure? ;-)

I get the following output:

$ cargo run -q namei README.md
f: README.md
 / - No such file or directory (os error 2)
$ echo $?
1

wheras with the original namei I get:

$ namei README.md
f: README.md
 - README.md
$ echo $?
0

Copy link
Author

Choose a reason for hiding this comment

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

I think somewhere i haven't handled absolute vs relative filepaths properly. I will fix it.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @cakebaker it should work now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants