----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/#review187346 -----------------------------------------------------------
The commit message needs to be in the past tense, and generally you can ignore mentioning stout as the files imply it (though it still gets mentioned in commits a lot). E.g.:: > Added `os::copyfile(from, to)`. > This patch... Currently the description is a copy of the summary (which happens when the commit body is left empty). This should usually be avoided (the exception being trivial commits). 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 16 (patched) <https://reviews.apache.org/r/60621/#comment264288> What was this included for? My only guess is `WIFEXITED`? 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 29-31 (patched) <https://reviews.apache.org/r/60621/#comment264281> Can this comment document why we don't support a directory as the destination path? 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 33-34 (patched) <https://reviews.apache.org/r/60621/#comment264287> Per the style guide: > We use snake_case for variable names in the libprocess and stout libraries. I'll let this issue cover the rest of the review. 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 39-40 (patched) <https://reviews.apache.org/r/60621/#comment264280> s/and/nor/g s/a directory/directories/g 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 43-45 (patched) <https://reviews.apache.org/r/60621/#comment264283> This sounds to me like the problem isn't `stat::isdir` but that we didn't test for existence first, which would have avoided this edge case. 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 48-49 (patched) <https://reviews.apache.org/r/60621/#comment264282> This is an exacty copy of the message above; let's not duplicate it. The error conditions can be checked in the same expression. 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 52-53 (patched) <https://reviews.apache.org/r/60621/#comment264284> Cool, but this should probably be in the header comment, it's a pretty important exception. 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 54 (patched) <https://reviews.apache.org/r/60621/#comment264289> I didn't see `stout/path` included. IWYU. 3rdparty/stout/include/stout/os/posix/copyfile.hpp Lines 60-71 (patched) <https://reviews.apache.org/r/60621/#comment264285> Do we have to use `cp`? Generally we should prefer just using C++ to shelling out for something like copying a file, especially if we're implementing a stout function like `os::copyfile`. 3rdparty/stout/include/stout/os/windows/copyfile.hpp Lines 29-32 (patched) <https://reviews.apache.org/r/60621/#comment264286> Since this implementation _doesn't_ use the equivalent of `cp`, it makes even more sense to be consistent in the POSIX implementation. 3rdparty/stout/include/stout/os/windows/copyfile.hpp Lines 52-53 (patched) <https://reviews.apache.org/r/60621/#comment264290> s/Posix/POSIX/g This is not apparent in the POSIX implementation at all. Is it just a quirk of `cp` or are we calling it specially or what? 3rdparty/stout/include/stout/os/windows/copyfile.hpp Lines 56-59 (patched) <https://reviews.apache.org/r/60621/#comment264291> This is probably cleaner as `if (!::CopyFileW` since the Windows `BOOL` and `FALSE` types are so weird. 3rdparty/stout/tests/os/copyfile_tests.cpp Lines 33 (patched) <https://reviews.apache.org/r/60621/#comment264292> This is being repeated... and it's already part of the base class: https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L64 - Andrew Schwartzmeyer On Oct. 5, 2017, 9:03 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60621/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2017, 9:03 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and > Li Li. > > > Bugs: MESOS-6705 > https://issues.apache.org/jira/browse/MESOS-6705 > > > Repository: mesos > > > Description > ------- > > Add new stout capability: os::copyfile. > > > Diffs > ----- > > 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 > 3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 > 3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION > 3rdparty/stout/tests/CMakeLists.txt > 6e5773f1e03671de7ac007caf49edd0f1cd7aedd > 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60621/diff/2/ > > > Testing > ------- > > See upstream > > Note that Joe made some changes to this, I ended up taking his changes as is. > > > Thanks, > > Jeff Coffler > >
