-
Notifications
You must be signed in to change notification settings - Fork 23
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
[MSHARED-969] Environment variable with null value #68
Conversation
src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
Outdated
Show resolved
Hide resolved
@pzygielo thanks for review, I did fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not talk about popularity neither about reflection, but solely about creating garbage needlessly. I doubt the race condition actually will happen here, as the variable is effectively final at that point.
public void environmentVariableWithNullShouldNotBeAddedToList() { | ||
|
||
Commandline commandline = new Commandline(); | ||
commandline.addEnvironment("TEST_NULL_ENV", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With reard to my previous comments I'd like to ask the OS experts what sense it make to store null
in an environment variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote before Command Line object has local map of environment variable which are provided to Runtime.exec after translate this map to list of name=value
. Translation is done in getEnvironmentVariables.
Please look at my last comment at jira and new test in last update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @slawekjaranowski and @elharo, this does fix potential bugs. I approve the changes. Thanks @slawekjaranowski!
No description provided.