Re: Review Request 50673: Made semantics of `os::rmdir` consistent between POSIX and Windows.

[Joseph Wu]

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


Ship it!




With the cleanups in ( https://reviews.apache.org/r/52848/ ), this is good to 
go!

- Joseph Wu


On Aug. 1, 2016, 2:24 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50673/
> ---
> 
> (Updated Aug. 1, 2016, 2:24 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Bugs: MESOS-5942
> https://issues.apache.org/jira/browse/MESOS-5942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will fix 2 known bugs in the Windows implementation of
> `os::rmdir`, as chronicled in MESOS-5942, namely:
> 
> 1. Calling `os::rmdir` with a file argument (rather than a directory)
>and the `recursive` parameter set to `true` will fail on Windows,
>but succeed on POSIX.
> 
> The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`,
> the behavior of which depends on the arguments the caller passes in. If
> the formal parameter `recursive` is set to `true`, then the semantics
> are meant to be`rm -r`; if `false`, the semantics are meant to be
> `::rmdir`.
> 
> The implications of this are somewhat subtle: `::rmdir` will error out
> if you try to delete (e.g.) regular files, while `rm -r` will happily
> delete them.
> 
> On Windows, we currently always have `::rmdir`-style semantics, in that
> we if you pass a path that points at a file to `os::rmdir`, it will not
> delete that file.
> 
> This commit will reverse this, and move make the Windows implementation
> semantically identical to the POSIX implementation (at least in this
> regard).
> 
> 2. Recursively deleting nested directories fails on windows when
>`removeRoot` is set to `false`.
> 
> Currently if you set the `removeRoot` parameter to `false`, the Windows
> implementation of `os::rmdir` will fail to delete a directory inside a
> directory. The reason is that we are propagating the `removeRoot` flag
> to the recursive calls to `os::rmdir`. The implication of this is that
> the recursive call will *not* delete the nested directory (since
> `removeRoot` is `false`).
> 
> This commit will fix this by setting `removeRoot` to `true` in recursive
> calls to `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> b74bf7153a15435ce424880df84901c349dee216 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
> 
> Diff: https://reviews.apache.org/r/50673/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 50673: Made semantics of `os::rmdir` consistent between POSIX and Windows.

[Joseph Wu]

> On Aug. 2, 2016, 4:03 p.m., Joseph Wu wrote:
> > 3rdparty/stout/tests/os/rmdir_tests.cpp, lines 116-117
> > 
> >
> > Since these two sets are only modified once within the test, why not do:
> > ```
> >   const hashset expectedRootListing = { newDirectoryName };
> >   const hashset expectedSubListing = { newFileName };
> > ```
> > 
> > I realize this is the pattern within the other tests in this file.  So 
> > perhaps you can change the pattern in a subsequent review :)

Added: https://reviews.apache.org/r/52848/


- Joseph


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


On Aug. 1, 2016, 2:24 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50673/
> ---
> 
> (Updated Aug. 1, 2016, 2:24 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Bugs: MESOS-5942
> https://issues.apache.org/jira/browse/MESOS-5942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will fix 2 known bugs in the Windows implementation of
> `os::rmdir`, as chronicled in MESOS-5942, namely:
> 
> 1. Calling `os::rmdir` with a file argument (rather than a directory)
>and the `recursive` parameter set to `true` will fail on Windows,
>but succeed on POSIX.
> 
> The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`,
> the behavior of which depends on the arguments the caller passes in. If
> the formal parameter `recursive` is set to `true`, then the semantics
> are meant to be`rm -r`; if `false`, the semantics are meant to be
> `::rmdir`.
> 
> The implications of this are somewhat subtle: `::rmdir` will error out
> if you try to delete (e.g.) regular files, while `rm -r` will happily
> delete them.
> 
> On Windows, we currently always have `::rmdir`-style semantics, in that
> we if you pass a path that points at a file to `os::rmdir`, it will not
> delete that file.
> 
> This commit will reverse this, and move make the Windows implementation
> semantically identical to the POSIX implementation (at least in this
> regard).
> 
> 2. Recursively deleting nested directories fails on windows when
>`removeRoot` is set to `false`.
> 
> Currently if you set the `removeRoot` parameter to `false`, the Windows
> implementation of `os::rmdir` will fail to delete a directory inside a
> directory. The reason is that we are propagating the `removeRoot` flag
> to the recursive calls to `os::rmdir`. The implication of this is that
> the recursive call will *not* delete the nested directory (since
> `removeRoot` is `false`).
> 
> This commit will fix this by setting `removeRoot` to `true` in recursive
> calls to `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> b74bf7153a15435ce424880df84901c349dee216 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
> 
> Diff: https://reviews.apache.org/r/50673/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 50673: Made semantics of `os::rmdir` consistent between POSIX and Windows.

[Joseph Wu]

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




3rdparty/stout/tests/os/rmdir_tests.cpp (line 105)


Per the next comment (below this one), this line would become:
```
EXPECT_EQ(hashset::EMPTY, listfiles(tmpdir));
```



3rdparty/stout/tests/os/rmdir_tests.cpp (lines 116 - 117)


Since these two sets are only modified once within the test, why not do:
```
  const hashset expectedRootListing = { newDirectoryName };
  const hashset expectedSubListing = { newFileName };
```

I realize this is the pattern within the other tests in this file.  So 
perhaps you can change the pattern in a subsequent review :)


- Joseph Wu


On Aug. 1, 2016, 2:24 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50673/
> ---
> 
> (Updated Aug. 1, 2016, 2:24 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Bugs: MESOS-5942
> https://issues.apache.org/jira/browse/MESOS-5942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will fix 2 known bugs in the Windows implementation of
> `os::rmdir`, as chronicled in MESOS-5942, namely:
> 
> 1. Calling `os::rmdir` with a file argument (rather than a directory)
>and the `recursive` parameter set to `true` will fail on Windows,
>but succeed on POSIX.
> 
> The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`,
> the behavior of which depends on the arguments the caller passes in. If
> the formal parameter `recursive` is set to `true`, then the semantics
> are meant to be`rm -r`; if `false`, the semantics are meant to be
> `::rmdir`.
> 
> The implications of this are somewhat subtle: `::rmdir` will error out
> if you try to delete (e.g.) regular files, while `rm -r` will happily
> delete them.
> 
> On Windows, we currently always have `::rmdir`-style semantics, in that
> we if you pass a path that points at a file to `os::rmdir`, it will not
> delete that file.
> 
> This commit will reverse this, and move make the Windows implementation
> semantically identical to the POSIX implementation (at least in this
> regard).
> 
> 2. Recursively deleting nested directories fails on windows when
>`removeRoot` is set to `false`.
> 
> Currently if you set the `removeRoot` parameter to `false`, the Windows
> implementation of `os::rmdir` will fail to delete a directory inside a
> directory. The reason is that we are propagating the `removeRoot` flag
> to the recursive calls to `os::rmdir`. The implication of this is that
> the recursive call will *not* delete the nested directory (since
> `removeRoot` is `false`).
> 
> This commit will fix this by setting `removeRoot` to `true` in recursive
> calls to `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> b74bf7153a15435ce424880df84901c349dee216 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
> 
> Diff: https://reviews.apache.org/r/50673/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 50673: Made semantics of `os::rmdir` consistent between POSIX and Windows.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat and Joseph Wu.


Bugs: MESOS-5942
https://issues.apache.org/jira/browse/MESOS-5942


Repository: mesos


Description
---

This commit will fix 2 known bugs in the Windows implementation of
`os::rmdir`, as chronicled in MESOS-5942, namely:

1. Calling `os::rmdir` with a file argument (rather than a directory)
   and the `recursive` parameter set to `true` will fail on Windows,
   but succeed on POSIX.

The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`,
the behavior of which depends on the arguments the caller passes in. If
the formal parameter `recursive` is set to `true`, then the semantics
are meant to be`rm -r`; if `false`, the semantics are meant to be
`::rmdir`.

The implications of this are somewhat subtle: `::rmdir` will error out
if you try to delete (e.g.) regular files, while `rm -r` will happily
delete them.

On Windows, we currently always have `::rmdir`-style semantics, in that
we if you pass a path that points at a file to `os::rmdir`, it will not
delete that file.

This commit will reverse this, and move make the Windows implementation
semantically identical to the POSIX implementation (at least in this
regard).

2. Recursively deleting nested directories fails on windows when
   `removeRoot` is set to `false`.

Currently if you set the `removeRoot` parameter to `false`, the Windows
implementation of `os::rmdir` will fail to delete a directory inside a
directory. The reason is that we are propagating the `removeRoot` flag
to the recursive calls to `os::rmdir`. The implication of this is that
the recursive call will *not* delete the nested directory (since
`removeRoot` is `false`).

This commit will fix this by setting `removeRoot` to `true` in recursive
calls to `os::rmdir`.


Diffs
-

  3rdparty/stout/include/stout/os/windows/rmdir.hpp 
b74bf7153a15435ce424880df84901c349dee216 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ffe234baac305e26b5a29cffcdd310350d10167e 

Diff: https://reviews.apache.org/r/50673/diff/


Testing
---


Thanks,

Alex Clemmer