Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jojy Varghese]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.
> 
> Jojy Varghese wrote:
> @jie. I tested it already and verified that FTS_SLNONE  is not hit (using 
> logs).
> 
> Neil Conway wrote:
> Can we add a unit test anyway, please? This is corner-case behavior that 
> we should be validating we handle correctly, regardless of whether 
> `FTS_SLNONE` is actually set.
> 
> Jojy Varghese wrote:
> I have added unit tests in patch https://reviews.apache.org/r/45002/. 
> Would love to hear more test ideas.
> 
> Neil Conway wrote:
> I'd like a test case that covers `rmdir` of a symbolic link that points 
> to a non-existent file/directory; as far as I can see, the existing tests do 
> not cover this case.

What do you think about the test case `RemoveDirectoryWithNoTargetSymbolicLink` 
in the test file?


- Jojy


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


On March 24, 2016, 1:10 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Neil Conway]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.
> 
> Jojy Varghese wrote:
> @jie. I tested it already and verified that FTS_SLNONE  is not hit (using 
> logs).
> 
> Neil Conway wrote:
> Can we add a unit test anyway, please? This is corner-case behavior that 
> we should be validating we handle correctly, regardless of whether 
> `FTS_SLNONE` is actually set.
> 
> Jojy Varghese wrote:
> I have added unit tests in patch https://reviews.apache.org/r/45002/. 
> Would love to hear more test ideas.

I'd like a test case that covers `rmdir` of a symbolic link that points to a 
non-existent file/directory; as far as I can see, the existing tests do not 
cover this case.


- Neil


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


On March 24, 2016, 1:10 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jojy Varghese]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.
> 
> Jojy Varghese wrote:
> @jie. I tested it already and verified that FTS_SLNONE  is not hit (using 
> logs).
> 
> Neil Conway wrote:
> Can we add a unit test anyway, please? This is corner-case behavior that 
> we should be validating we handle correctly, regardless of whether 
> `FTS_SLNONE` is actually set.

I have added unit tests in patch https://reviews.apache.org/r/45002/. Would 
love to hear more test ideas.


- Jojy


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


On March 24, 2016, 1:10 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jojy Varghese]

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

(Updated March 24, 2016, 1:10 a.m.)


Review request for mesos, Jie Yu and Neil Conway.


Changes
---

review addressed.


Repository: mesos


Description
---

Fixed rmdir comment for FTS_SLNONE as per coding guidelines.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Neil Conway]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.
> 
> Jojy Varghese wrote:
> @jie. I tested it already and verified that FTS_SLNONE  is not hit (using 
> logs).

Can we add a unit test anyway, please? This is corner-case behavior that we 
should be validating we handle correctly, regardless of whether `FTS_SLNONE` is 
actually set.


- Neil


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


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jie Yu]

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


Fix it, then Ship it!





3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 71)


as we don't set `FTS_COMFOLLOW` or `FTS_LOGICAL`.


- Jie Yu


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jojy Varghese]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.

@jie. I tested it already and verified that FTS_SLNONE  is not hit (using logs).


- Jojy


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


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Cong Wang]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.

Looking at 
https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
 I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
FTS_LOGICAL or FTS_COMFOLLOW.


- Cong


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


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jojy Varghese]

> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > 
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?

I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for FTS_SLNONE 
to be returned.


- Jojy


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


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Neil Conway]

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




3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 71)


This comment seems incorrect:

(1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as well 
as `FTS_COMFOLLOW`.

(2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for `FTS_SLNONE` to 
be returned?


- Neil Conway


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

[Jojy Varghese]

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

Fixed rmdir comment for FTS_SLNONE as per coding guidelines.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 

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


Testing
---

make check.


Thanks,

Jojy Varghese