----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42662/#review116600 -----------------------------------------------------------
Fix it, then Ship it! This is looking good! Some nits and we're good to go. src/common/command_utils.hpp (line 48) <https://reviews.apache.org/r/42662/#comment177650> Why untar has a default for 'directory' while tar does not? src/common/command_utils.hpp (line 56) <https://reviews.apache.org/r/42662/#comment177644> Please update the comments accordingly. src/common/command_utils.hpp (line 59) <https://reviews.apache.org/r/42662/#comment177643> s/inputFilePath/input/ src/common/command_utils.cpp (line 19) <https://reviews.apache.org/r/42662/#comment177646> is it used? Please make sure to remove headers that are not used. src/common/command_utils.cpp (line 31) <https://reviews.apache.org/r/42662/#comment177645> is it used? src/common/command_utils.cpp (line 45) <https://reviews.apache.org/r/42662/#comment177649> s/command/path/ to be consistent with subprocess. src/common/command_utils.cpp (lines 55 - 56) <https://reviews.apache.org/r/42662/#comment177648> Instead of saving a lambda, can we just save the string that we want to log? ``` string command = strings::join( ", ", path, strings::join(", ", argv)); ``` src/common/command_utils.cpp (lines 63 - 73) <https://reviews.apache.org/r/42662/#comment177647> You should be able to use strings::join here. src/common/command_utils.cpp (line 143) <https://reviews.apache.org/r/42662/#comment177651> I don't think we need to check if 'directory' is empty or not. Otherwise, do you need to check if 'input' or 'output' is empty or not? src/common/command_utils.cpp (line 161) <https://reviews.apache.org/r/42662/#comment177652> Please align '{' here accordingly. src/common/command_utils.cpp (line 183) <https://reviews.apache.org/r/42662/#comment177653> Ditto on empty check. src/tests/common/command_utils_tests.cpp (line 23) <https://reviews.apache.org/r/42662/#comment177654> Why do you need this? Please remove headers that are not needed please. src/tests/common/command_utils_tests.cpp (line 33) <https://reviews.apache.org/r/42662/#comment177655> we try to avoid using 'using namespace' clause. For this test, I think it's pretty clean and readable to use command::tar, command::untar (i.e., no need for this clause). src/tests/common/command_utils_tests.cpp (line 43) <https://reviews.apache.org/r/42662/#comment177657> I would suggest to rename it to TarTest. Two reasons: 1) we might have 'ar' util in the future 2) it's better to keep the name consistent with the command name to readability and searchability. src/tests/common/command_utils_tests.cpp (line 46) <https://reviews.apache.org/r/42662/#comment177659> s/createTestFileInDir/createTestFile/ InDir is implied by the parameter 'directory' src/tests/common/command_utils_tests.cpp (line 47) <https://reviews.apache.org/r/42662/#comment177658> s/filePath/file/ Path is the type, no need for the redundent Path in variable name. src/tests/common/command_utils_tests.cpp (line 50) <https://reviews.apache.org/r/42662/#comment177660> s/testFilePath/testFile/ src/tests/common/command_utils_tests.cpp (line 54) <https://reviews.apache.org/r/42662/#comment177661> Do you need this temp variable? src/tests/common/command_utils_tests.cpp (lines 93 - 95) <https://reviews.apache.org/r/42662/#comment177662> You can use EXPECT_SOME_EQ("test", os::read(testFile)); src/tests/common/command_utils_tests.cpp (line 103) <https://reviews.apache.org/r/42662/#comment177663> s/testFilePath/testFile/ src/tests/common/command_utils_tests.cpp (lines 122 - 124) <https://reviews.apache.org/r/42662/#comment177664> Ditto on using EXPECT_SOME_EQ src/tests/common/command_utils_tests.cpp (line 135) <https://reviews.apache.org/r/42662/#comment177665> s/testFilePath/testFile/ src/tests/common/command_utils_tests.cpp (lines 157 - 159) <https://reviews.apache.org/r/42662/#comment177667> Ditto on using EXPECT_SOME_EQ src/tests/common/command_utils_tests.cpp (lines 190 - 192) <https://reviews.apache.org/r/42662/#comment177668> Ditto. src/tests/common/command_utils_tests.cpp (line 205) <https://reviews.apache.org/r/42662/#comment177669> s/testFilePath/testFile/ src/tests/common/command_utils_tests.cpp (lines 212 - 213) <https://reviews.apache.org/r/42662/#comment177670> Can you do the indentation like the following so that it's consistent with your style in BZIP2CompressFile ``` AWAIT_ASSERT_READY(tar( testDir, outputTarFile, topDir, Compression::GZIP)); ``` src/tests/common/command_utils_tests.cpp (lines 229 - 231) <https://reviews.apache.org/r/42662/#comment177671> Ditto on using EXPECT_SOME_EQ - Jie Yu On Jan. 26, 2016, 2:18 a.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42662/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2016, 2:18 a.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > ------- > > This common file is a good place to add common command line utilities like > tar, > digests(sha256, sha512, etc). Currently this functionality is spread in the > code > base and this change would enable all those call sites to be replaced with a > common code. > > > Diffs > ----- > > src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e > src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f > src/common/command_utils.hpp PRE-CREATION > src/common/command_utils.cpp PRE-CREATION > src/tests/common/command_utils_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/42662/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jojy Varghese > >
