You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oodt.apache.org by Michael Starch <MD...@gmail.com> on 2013/01/29 22:25:51 UTC
Review Request: Suggested Fix for JIRA Issue OODT-553
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9142/
-----------------------------------------------------------
Review request for oodt and Chris Mattmann.
Description
-------
This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec "env".
Diffs
-----
/trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711
/trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711
Diff: https://reviews.apache.org/r/9142/diff/
Testing
-------
Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set.
Thanks,
Michael Starch
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
Posted by Chris Mattmann <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9142/#review16042
-----------------------------------------------------------
Ship it!
Ship It!
- Chris Mattmann
On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9142/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 9:25 p.m.)
>
>
> Review request for oodt and Chris Mattmann.
>
>
> Description
> -------
>
> This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec "env".
>
>
> Diffs
> -----
>
> /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711
> /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711
>
> Diff: https://reviews.apache.org/r/9142/diff/
>
>
> Testing
> -------
>
> Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set.
>
>
> Thanks,
>
> Michael Starch
>
>
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
Posted by Michael Starch <MD...@gmail.com>.
> On Jan. 30, 2013, 2:09 a.m., Chris Mattmann wrote:
> > /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java, line 77
> > <https://reviews.apache.org/r/9142/diff/1/?file=253029#file253029line77>
> >
> > Hey Mike, here we are no longer calling preProcessInputStream -- doesn't that do envVarReplace?
Chris, the only thing that this function does is "line = line.replaceAll("\\\\", "\\\\\\\\");". There is no envVarReplace here. I did some quick tests to see if this was an issue, but I will test this again to see if I can get a better idea of any implications here.
Are you experiencing any specific problems?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9142/#review15817
-----------------------------------------------------------
On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9142/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 9:25 p.m.)
>
>
> Review request for oodt and Chris Mattmann.
>
>
> Description
> -------
>
> This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec "env".
>
>
> Diffs
> -----
>
> /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711
> /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711
>
> Diff: https://reviews.apache.org/r/9142/diff/
>
>
> Testing
> -------
>
> Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set.
>
>
> Thanks,
>
> Michael Starch
>
>
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
Posted by Chris Mattmann <ma...@apache.org>.
> On Jan. 30, 2013, 2:09 a.m., Chris Mattmann wrote:
> >
>
> Michael Starch wrote:
> The line "line = line.replaceAll("\\\\", "\\\\\\\\");" is used to allow the call "Properties.load(InputStream)". The reason for this is the load command expects '\' to escape something, and will drop invalid escapes. Therefore '\' must become "\\" to be properly loaded without causing unwanted escapes or being ignored.
>
> This is all handled by java on the back end when calling getEnvironment i.e. any env variable, complete with any '\'s is already properly read in.
>
> I cannot get the new method to fail on any environment variable containing series of 1 to 7 '\'s against the old implementation in a regression test. Therefore, I expect the above implementation to function properly.
Thanks Mike OK that makes sense to me. I'll go ahead and apply your patch thank you!
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9142/#review15817
-----------------------------------------------------------
On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9142/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 9:25 p.m.)
>
>
> Review request for oodt and Chris Mattmann.
>
>
> Description
> -------
>
> This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec "env".
>
>
> Diffs
> -----
>
> /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711
> /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711
>
> Diff: https://reviews.apache.org/r/9142/diff/
>
>
> Testing
> -------
>
> Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set.
>
>
> Thanks,
>
> Michael Starch
>
>
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
Posted by Michael Starch <MD...@gmail.com>.
> On Jan. 30, 2013, 2:09 a.m., Chris Mattmann wrote:
> >
The line "line = line.replaceAll("\\\\", "\\\\\\\\");" is used to allow the call "Properties.load(InputStream)". The reason for this is the load command expects '\' to escape something, and will drop invalid escapes. Therefore '\' must become "\\" to be properly loaded without causing unwanted escapes or being ignored.
This is all handled by java on the back end when calling getEnvironment i.e. any env variable, complete with any '\'s is already properly read in.
I cannot get the new method to fail on any environment variable containing series of 1 to 7 '\'s against the old implementation in a regression test. Therefore, I expect the above implementation to function properly.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9142/#review15817
-----------------------------------------------------------
On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9142/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 9:25 p.m.)
>
>
> Review request for oodt and Chris Mattmann.
>
>
> Description
> -------
>
> This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec "env".
>
>
> Diffs
> -----
>
> /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711
> /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711
>
> Diff: https://reviews.apache.org/r/9142/diff/
>
>
> Testing
> -------
>
> Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set.
>
>
> Thanks,
>
> Michael Starch
>
>
Re: Review Request: Suggested Fix for JIRA Issue OODT-553
Posted by Chris Mattmann <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9142/#review15817
-----------------------------------------------------------
/trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java
<https://reviews.apache.org/r/9142/#comment34072>
Hey Mike, here we are no longer calling preProcessInputStream -- doesn't that do envVarReplace?
- Chris Mattmann
On Jan. 29, 2013, 9:25 p.m., Michael Starch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9142/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2013, 9:25 p.m.)
>
>
> Review request for oodt and Chris Mattmann.
>
>
> Description
> -------
>
> This is a review for a patch for JIRA issue OODT-553 (https://issues.apache.org/jira/browse/OODT-553). It fixes the EnvUtilities. to use System.getEnvironment and not exec "env".
>
>
> Diffs
> -----
>
> /trunk/commons/src/main/java/org/apache/oodt/commons/exec/EnvUtilities.java 1439711
> /trunk/commons/src/test/org/apache/oodt/commons/exec/TestEnvUtilities.java 1439711
>
> Diff: https://reviews.apache.org/r/9142/diff/
>
>
> Testing
> -------
>
> Wrote several new unit tests that test this on unix systems only as it requires USER and HOME env vars to be set.
>
>
> Thanks,
>
> Michael Starch
>
>