> On Nov. 3, 2017, 11:29 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> > The general pattern is to just include the reason for an error, and to
> > not include any information the caller already has, because otherwise the
> > callers will double log:
> >
> > ```
> > Try<Nothing> copy = copyfile(source, destination);
> >
> > if (copy.isError()) {
> > return ("Failed to copy '" + source + "'"
> > " to '" + destination + "': " + copy.error();
> > }
> > ```
> >
> > The current code would log:
> >
> > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space
> > left"
> >
> > Can you guys do a sweep for this issue in the windows related code that
> > has been added?
I'm not sure I follow. Are you saying the _caller_ should always write the
actual error message, versus the _callee_ preparing it here?
I guess in your example, I don't get why the user of `os::copyfile` would add
`"Failed to copy..."` instead of just doing:
```
if (copy.isError()) {
return Error(copy.error());
}
```
And then the error message is only written once, in `os::copyfile`.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/#review190043
-----------------------------------------------------------
On Oct. 19, 2017, 2:32 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 2:32 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
> -------
>
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
>
>
> 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/6/
>
>
> Testing
> -------
>
> See upstream
>
> Note that Joe made some changes to this, I ended up taking his changes as is.
>
>
> Thanks,
>
> Jeff Coffler
>
>
