Throw error if unset argument starts with '=' for env command #6172

Carbrex · 2024-04-01T15:27:02Z

Output

Before:

$ cargo run env | grep ZZZ 
ZZZ=zzz
$ cargo run env -u=ZZZ | grep ZZZ

(No output because ZZZ was unset)

After:

$ cargo run env | grep ZZZ
ZZZ=zzz
$ cargo run env -u=ZZZ | grep ZZZ
env: cannot unset '=ZZZ': Invalid argument
Try 'target/debug/coreutils env --help' for more information.

Tests:

Before:

$ cargo test test_env_with_gnu_reference_unset_invalid_variables

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 70 filtered out; finished in 0.00s


running 1 test
test test_env::test_env_with_gnu_reference_unset_invalid_variables ... FAILED

failures:

---- test_env::test_env_with_gnu_reference_unset_invalid_variables stdout ----
run: /home/geet/debian-gsoc/coreutils/target/debug/coreutils env -u=2EKt
thread 'test_env::test_env_with_gnu_reference_unset_invalid_variables' panicked at tests/by-util/test_env.rs:578:10:
Command was expected to fail.
stdout = LC_ALL=C
TZ=UTC

 stderr = 
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_env::test_env_with_gnu_reference_unset_invalid_variables

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2875 filtered out; finished in 0.00s

After:

$ cargo test test_env_with_gnu_reference_unset_invalid_variables

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 70 filtered out; finished in 0.00s


running 1 test
test test_env::test_env_with_gnu_reference_unset_invalid_variables ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 2875 filtered out; finished in 0.01s

src/uu/env/src/env.rs

BenWiederhake · 2024-04-01T15:31:26Z

Hi again! I made a mistake and merged your other PR a bit too quickly, which is why the other maintainers had to do a force-push. Sorry ^^'

I'd be happy to help you sort out this mess in case you get stuck with git rebase. I'm available to chat in the uutils Discord chat.

Carbrex · 2024-04-01T15:32:30Z

Hi again! I made a mistake and merged your other PR a bit too quickly, which is why the other maintainers had to do a force-push. Sorry ^^'

I'd be happy to help you sort out this mess in case you get stuck with git rebase. I'm available to chat in the uutils Discord chat.

Thanks for your help I will try to fix it and if I fail I will certainly ask you on discord.

sylvestre · 2024-04-01T15:44:26Z

Please rebase your work
Thanks

Carbrex · 2024-04-01T15:59:21Z

Thanks for your patience! Its ready to be reviewed now.
I was not able to remove this Add test for ignoring time-style if time not defined commit using rebase as it shows some error.

src/uu/env/src/env.rs

Fix clippy error Fix formatting Simplify unset prefix handling Also include '=' in invalid_arg cargo fmt

sylvestre · 2024-04-01T17:01:07Z

please fix the lint

Carbrex · 2024-04-01T17:03:08Z

please fix the lint

I think all the lint tests have passed, haven't they?

BenWiederhake · 2024-04-01T18:14:40Z

tests/by-util/test_env.rs

@@ -7,6 +7,7 @@
 #[cfg(target_os = "linux")]
 use crate::common::util::expected_result;
 use crate::common::util::TestScenario;
+#[rustfmt::skip]


I'm very surprised that this is necessary, and can't reproduce it. In fact, all CI so far always was green, so this is not a systemic issue. Also, removing the leading :: would break the code, so any program that suggests just removing them without replacement is obviously wrong.

Which rustfmt version are you using, and how exactly do you invoke it?

I am doing cargo fmt --all and when I run that it removes :: in front of env this resulting in error.

$ rustfmt --version rustfmt 1.7.0-stable ( )

please remove these skips :)

if needed, we will fix them into a different PR

Huh, that is very weird.

$ cargo +stable fmt --version # I have the same version as you rustfmt 1.7.0-stable (07dca48 2024-02-04) $ cargo +stable fmt --all --check # It does not report anything $ cargo +stable fmt --all # And running it in the same manner … $ git diff # … does not cause any file changes. $

If the formatting issues cause pre-commit to fail, you can skip it completely via git commit --no-verify. However, that's just a temporary solution.

sylvestre · 2024-04-01T18:53:37Z

Seems that it needs more work:
With this PR:

./target/debug/coreutils env -i -0 -u=kQ4dALb1 A=0d CZj=lYr
env: cannot unset '=kQ4dALb1': Invalid argument

With GNU:

$ /usr/bin/env -i -0 -u=kQ4dALb1 A=0d CZj=lYr
A=0dCZj=lYr%

Carbrex · 2024-04-01T19:02:54Z

Seems that it needs more work: With this PR:

./target/debug/coreutils env -i -0 -u=kQ4dALb1 A=0d CZj=lYr
env: cannot unset '=kQ4dALb1': Invalid argument

With GNU:

$ /usr/bin/env -i -0 -u=kQ4dALb1 A=0d CZj=lYr
A=0dCZj=lYr%

Ok thanks I will look into it.

Carbrex · 2024-04-01T19:12:53Z

These are some other results with GNU that don't match:

$ env -0 -u=kQ4dALb1 - A=0d CZj=lYr
A=0dCZj=lYr
$ env -0 -u=kQ4dALb1 - A=0d CZj=lYr 
A=0dCZj=lYr 
$ env -u=kQ4dALb1 - A=0d CZj=lYr 
A=0d
CZj=lYr
$ env -i -u=kQ4dALb1  A=0d CZj=lYr
A=0d
CZj=lYr

Carbrex · 2024-04-01T19:19:54Z

I am converting this to a draft pr until I fix these.

BenWiederhake · 2024-04-02T11:20:25Z

src/uu/env/src/env.rs

+        // clap automatically removes '=' from the beginning of the argument, so we need to check for it
+        for arg in &original_args {
+            let arg = arg.to_string_lossy();
+            let prefix = "-u=";


Wouldn't that break something like env echo gotcha -u=hello ?

Also, it definitely wouldn't catch env -vu=hello.

Suggestion: Instead of trying to parse the arguments again, is there a way to examine matches to determine what the exact argument was that leads clap to believe something? This should be much more robust.

Nice catch, it really breaks it. Also, I have found some more cases where it breaks things. I am not sure of what approach I should do, because it looks like I am trying to shoehorn too much of if conditions to match the gnu behavior. Some help would be appreciated.

Also, it definitely wouldn't catch env -vu=hello.

Suggestion: Instead of trying to parse the arguments again, is there a way to examine matches to determine what the exact argument was that leads clap to believe something? This should be much more robust.

I tried doing something with clap but it turns out that clap removes '=' before the argument by itself.
$ cargo run env -u=kQ4dALb1 A=0d CZj=lYr

This is what matches print:

matches: ArgMatches { valid_args: ["ignore-environment", "chdir", "null", "file", "unset", "debug", "split-string", "vars", "help", "version"], valid_subcommands: [], args: FlatMap { keys: ["unset", "vars", "ignore-environment", "null", "debug"], values: [MatchedArg { source: Some(CommandLine), indices: [2], type_id: Some(std::ffi::os_str::OsString), vals: [[AnyValue { inner: std::ffi::os_str::OsString }]], raw_vals: [["kQ4dALb1"]], ignore_case: false }, MatchedArg { source: Some(CommandLine), indices: [3, 4], type_id: Some(std::ffi::os_str::OsString), vals: [[AnyValue { inner: std::ffi::os_str::OsString }], [AnyValue { inner: std::ffi::os_str::OsString }]], raw_vals: [["A=0d"], ["CZj=lYr"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [5], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [6], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }, MatchedArg { source: Some(DefaultValue), indices: [7], type_id: Some(bool), vals: [[AnyValue { inner: bool }]], raw_vals: [["false"]], ignore_case: false }] }, subcommand: None }

Carbrex · 2024-04-02T11:30:33Z

Some more deviations from gnu are

$ env D=ddsa - A=0d CZj=lYr  
env: ‘-’: No such file or directory
                                                                                       
$ cargo run env D=ddsa - A=0d CZj=lYr 
D=ddsa
A=0d
CZj=lYr

$ env -- -u=kQ4dALb1 A=0d  CZj=lYr      
(Prints all environment variables)

After some testing I have found that gnu works if there is -i or '-' before or after '-u=adsfas'.
Also, when there is '--' before '-u=adfs'.
Some help would be appreciated on what should I do.

github-actions · 2024-05-21T11:51:18Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/timeout/timeout is no longer failing!

sylvestre · 2024-06-04T21:56:03Z

is it ready to be reviewed? Thanks

Carbrex · 2024-06-04T22:06:36Z

No, sorry it isn't ready for review. This pr wont work because of so many weird behavior by gnu as mentioned in my previous comments.
But I guess the tests might be useful. I might write some more tests if needed?

sylvestre · 2024-06-05T05:59:40Z

Sure sounds good :)

…patch-2-6165

Carbrex · 2024-06-05T08:50:52Z

I think I have added enough tests.

BenWiederhake

Thanks for collecting and implementing the test cases! Currently, CI fails because the tests are run (and fail), therefore this cannot be merged. Also, I have the impression that some of these commands should behave differently, are you sure about the expected results? (See below)

tests/by-util/test_env.rs

sylvestre reviewed Apr 1, 2024

View reviewed changes

src/uu/env/src/env.rs Outdated Show resolved Hide resolved

Carbrex marked this pull request as draft April 1, 2024 15:37

Carbrex force-pushed the patch-2-6165 branch from fbdcb12 to ee62b8e Compare April 1, 2024 15:40

Add test for ignoring time-style if time not defined

3a13d3e

Carbrex force-pushed the patch-2-6165 branch 4 times, most recently from bd361f7 to 403da10 Compare April 1, 2024 15:58

Carbrex marked this pull request as ready for review April 1, 2024 15:58

sylvestre reviewed Apr 1, 2024

View reviewed changes

src/uu/env/src/env.rs Outdated Show resolved Hide resolved

Carbrex force-pushed the patch-2-6165 branch from 3979bd1 to 9877c2f Compare April 1, 2024 16:14

Throw error if unset argument starts with '=' for env command

4e5b764

Fix clippy error Fix formatting Simplify unset prefix handling Also include '=' in invalid_arg cargo fmt

Carbrex force-pushed the patch-2-6165 branch from 9877c2f to 4e5b764 Compare April 1, 2024 16:20

BenWiederhake reviewed Apr 1, 2024

View reviewed changes

Remove rust fmt skips

8b863f3

Carbrex marked this pull request as draft April 1, 2024 19:20

Allow = with --unset but not with -u

75548be

BenWiederhake reviewed Apr 2, 2024

View reviewed changes

useless

0f05b6b

Merge branch 'main' into patch-2-6165

7e8c767

sylvestre and others added 6 commits June 5, 2024 07:59

Merge branch 'main' into patch-2-6165

cf1d5e8

Merge remote-tracking branch 'refs/remotes/origin/patch-2-6165' into …

c612e74

…patch-2-6165

Add more tests

59b5c64

Remove not working env -u code

ccb0162

Add unset tests with --unset

9e0b4cd

Some more tests for env -u

da441b7

Carbrex marked this pull request as ready for review June 5, 2024 08:51

BenWiederhake requested changes Jun 10, 2024

View reviewed changes

tests/by-util/test_env.rs Outdated Show resolved Hide resolved

tests/by-util/test_env.rs Outdated Show resolved Hide resolved

tests/by-util/test_env.rs Show resolved Hide resolved

Ignore the unset var test for env and also fix a test with -0

4d0e31f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw error if unset argument starts with '=' for env command #6172

Throw error if unset argument starts with '=' for env command #6172

Carbrex commented Apr 1, 2024 •

edited

BenWiederhake commented Apr 1, 2024

Carbrex commented Apr 1, 2024

sylvestre commented Apr 1, 2024

Carbrex commented Apr 1, 2024 •

edited

sylvestre commented Apr 1, 2024

Carbrex commented Apr 1, 2024

BenWiederhake Apr 1, 2024

Carbrex Apr 1, 2024

sylvestre Apr 1, 2024

BenWiederhake Apr 1, 2024

sylvestre commented Apr 1, 2024

Carbrex commented Apr 1, 2024

Carbrex commented Apr 1, 2024

Carbrex commented Apr 1, 2024

BenWiederhake Apr 2, 2024

BenWiederhake Apr 2, 2024

Carbrex Apr 2, 2024

Carbrex Apr 2, 2024 •

edited

Carbrex commented Apr 2, 2024

github-actions bot commented May 21, 2024

sylvestre commented Jun 4, 2024

Carbrex commented Jun 4, 2024 •

edited

sylvestre commented Jun 5, 2024

Carbrex commented Jun 5, 2024

BenWiederhake left a comment

Throw error if unset argument starts with '=' for env command #6172

Are you sure you want to change the base?

Throw error if unset argument starts with '=' for env command #6172

Conversation

Carbrex commented Apr 1, 2024 • edited

Output

Tests:

BenWiederhake commented Apr 1, 2024

Carbrex commented Apr 1, 2024

sylvestre commented Apr 1, 2024

Carbrex commented Apr 1, 2024 • edited

sylvestre commented Apr 1, 2024

Carbrex commented Apr 1, 2024

BenWiederhake Apr 1, 2024

Choose a reason for hiding this comment

Carbrex Apr 1, 2024

Choose a reason for hiding this comment

sylvestre Apr 1, 2024

Choose a reason for hiding this comment

BenWiederhake Apr 1, 2024

Choose a reason for hiding this comment

sylvestre commented Apr 1, 2024

Carbrex commented Apr 1, 2024

Carbrex commented Apr 1, 2024

Carbrex commented Apr 1, 2024

BenWiederhake Apr 2, 2024

Choose a reason for hiding this comment

BenWiederhake Apr 2, 2024

Choose a reason for hiding this comment

Carbrex Apr 2, 2024

Choose a reason for hiding this comment

Carbrex Apr 2, 2024 • edited

Choose a reason for hiding this comment

Carbrex commented Apr 2, 2024

github-actions bot commented May 21, 2024

sylvestre commented Jun 4, 2024

Carbrex commented Jun 4, 2024 • edited

sylvestre commented Jun 5, 2024

Carbrex commented Jun 5, 2024

BenWiederhake left a comment

Choose a reason for hiding this comment

Carbrex commented Apr 1, 2024 •

edited

Carbrex commented Apr 1, 2024 •

edited

Carbrex Apr 2, 2024 •

edited

Carbrex commented Jun 4, 2024 •

edited