mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Coffler <j...@taltos.com>
Subject Re: Review Request 60621: Add new stout capability: os::copyfile.
Date Thu, 24 Aug 2017 20:51:12 GMT


> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 26 (patched)
> > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line26>
> >
> >     Oops, can't include a namespace in a header.

I removed this and resolved resulting compilation issues.


> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 30 (patched)
> > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line30>
> >
> >     There's no good reason to return the destination path when successful, as the
same path is already known to the caller.  We can go with a `Try<Nothing>` instead.

Okay, changed.


> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 31 (patched)
> > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line31>
> >
> >     The checks before `cp` are all good (and they are consistent with how the fetcher
currently/coincidentally passes arguments in).  However, these checks should be applied to
the `sourcePath` too.  i.e. Neither the source nor destination can be a directory; or a relative
path.

Yeah, that's good. But the source can be relative, and that's okay. Either we found the source
or we did not. I think that's okay to not be absolute (and, as I recall, a few unit tests
check for that specifically).

Changed code to check source path isn't a directory and that it doesn't end in "/".


> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 46-47 (patched)
> > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line46>
> >
> >     Instead of indexing, we can just use `destinationPath.back()` (C++11 !)

Done.


> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line47>
> >
> >     Don't need to check for backslashes on Posix.

Originally this ran on Windows too, and was #ifdef'ed. Resolved.


- Jeff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/#review182914
-----------------------------------------------------------


On July 3, 2017, 7:29 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
> 
> (Updated July 3, 2017, 7:29 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 7956d1ae392cd3fa560474bc3ac38877cddce857 
>   3rdparty/stout/include/Makefile.am dec7293949e16b6580509c2fd41a851e349fbef4 
>   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 8d881ab7ac571dea7aace269332a856feb7a6c43 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/1/
> 
> 
> Testing
> -------
> 
> See upstream
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message