Skip to content

Conversation

slawekjaranowski
Copy link
Member

No description provided.

@slawekjaranowski
Copy link
Member Author

@pzygielo thanks for review, I did fix

Copy link

@mkarg mkarg left a 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);
Copy link

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?

Copy link
Member Author

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.

Copy link

@MartinKanters MartinKanters left a 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!

@MartinKanters MartinKanters merged commit 8b8d586 into apache:master Apr 16, 2021
@slawekjaranowski slawekjaranowski deleted the fix-969 branch April 16, 2021 13:37
@jira-importer
Copy link

Resolve #282

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.

6 participants