This is an automatically generated e-mail. To reply, visit:

Haven't looked at the tests yet. Will do another pass once the existing issues 
are addressed. Thanks Jojy for doing this. This is really useful!

src/common/command_utils.hpp (line 32)

    Can we remove these namespaces? I found the following less verbose and 

src/common/command_utils.hpp (line 34)

    '{' in the next line.

src/common/command_utils.hpp (line 50)

    inputFilePath is a little confusing to readers because it can be a 
directory as well.
    How about renaming it to 'input' since the Path part if pretty much 
redundent as the type is Path.

src/common/command_utils.hpp (line 51)


src/common/command_utils.hpp (line 52)

    Hum, you use 'directory' in the cpp file while using a different name here. 
Can you use 'directory' through out?

src/common/command_utils.hpp (line 63)

    Ditto. s/inputFilePath/input/

src/common/command_utils.hpp (line 64)

    Ditto. s/changeToDirectory/directory/

src/common/command_utils.hpp (lines 70 - 74)

    Can you separate the patches for sha512? We try to avoid big patches.

src/common/command_utils.cpp (line 44)

    I would s/launchCommand/launch/ since you've already had 'command' in 
    command::launch reads much more nicely than command::launchCommand

src/common/command_utils.cpp (line 46)

    We try to avoid using Option<vector<...>> since the semantics is not clear 
between None() and an empty vector.
    For this case, I would suggest we always use the argv version of subprocess 
to avoid escaping spaces.
    static Future<string> launch(
        const string& path,
        const vector<string>& argv)

src/common/command_utils.cpp (line 70)

    Any way to print the agv as well?

src/common/command_utils.cpp (lines 117 - 124)

    See my comments below, no need for this.

src/common/command_utils.cpp (lines 127 - 144)

    See my comments below. No need for this.

src/common/command_utils.cpp (lines 153 - 162)

    I don't think this check is necessary. We are basically checking what 'tar' 
will check later. Can you remove it?

src/common/command_utils.cpp (lines 172 - 176)

    Ditto my comment above. I don't think the check performed in 
`addChangeDirectoryFlag` is necessary.  'tar' will validate that as well. Let's 
keep the function in this file simple (just shelling out).
    No need to the additional helper function here. We also avoid using non 
const references if possible.

src/common/command_utils.cpp (lines 191 - 193)

    What will this print? the type which will an int? Can you introduce a 
helper method to stringify the enum?

src/common/command_utils.cpp (line 228)

    Let's not introduce this helper method. You can just do:
    return launch(path, argv)
      .then([]() { return Nothing(); });

- Jie Yu

On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
> (Updated Jan. 22, 2016, 5:38 p.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

Reply via email to