Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated June 24, 2015, 2:44 a.m.)


Review request for mesos and Cody Maloney.


Changes
---

Addressed comments.


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


Repository: mesos-incubating


Description
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
59595c9d0372cb7fc8ec5acbab91ee298b673167 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
7dd266421377e3f531c15cdf17377141f26c2715 

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


Testing
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

> On June 23, 2015, 5:29 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 76
> > 
> >
> > don't you want to do the same short circuit as in basename() for path 
> > containing only slashes?

In `basename` that short-circuit is needed. For `basename` consider `"///"` as 
input, the expected (= `::basename`) output would be `"/"` but without that 
clause the resulting output would be `""`. In `dirname` however that is not 
needed. Same example as before, `"///"` is expected to return `"/"` and that is 
exactly what we get. Feel free to reopen this issue in case I missed something.


- Till


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


On June 24, 2015, 2:44 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated June 24, 2015, 2:44 a.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> 59595c9d0372cb7fc8ec5acbab91ee298b673167 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> 7dd266421377e3f531c15cdf17377141f26c2715 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

> On June 23, 2015, 2:37 p.m., Kapil Arya wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, lines 40-52
> > 
> >
> > It probably applies to the Class itself, but should we also be testing 
> > for repeated middle slashes like "/a//b/c//d/"? Or is it beyond the scope 
> > of Path class?

IMHO whatever ::basename / ::dirname produce on such input, we should match -- 
hence I am adding your suggested patterns (while also trying them on the 
standard variants).


- Till


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


On June 23, 2015, 8:26 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated June 23, 2015, 8:26 a.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> 59595c9d0372cb7fc8ec5acbab91ee298b673167 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> 7dd266421377e3f531c15cdf17377141f26c2715 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Vinod Kone]

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

Ship it!


much cleaner. thank you!


3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 41)


extraneous "/" in slashes.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 45)


ditto.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 73)


ditto.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 76)


don't you want to do the same short circuit as in basename() for path 
containing only slashes?



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 81)


s/non trailing// ?


- Vinod Kone


On June 23, 2015, 8:26 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated June 23, 2015, 8:26 a.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> 59595c9d0372cb7fc8ec5acbab91ee298b673167 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> 7dd266421377e3f531c15cdf17377141f26c2715 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Kapil Arya]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp (lines 40 - 52)


It probably applies to the Class itself, but should we also be testing for 
repeated middle slashes like "/a//b/c//d/"? Or is it beyond the scope of Path 
class?


- Kapil Arya


On June 23, 2015, 4:26 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated June 23, 2015, 4:26 a.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> 59595c9d0372cb7fc8ec5acbab91ee298b673167 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> 7dd266421377e3f531c15cdf17377141f26c2715 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated June 23, 2015, 8:26 a.m.)


Review request for mesos and Cody Maloney.


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


Repository: mesos-incubating


Description
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
59595c9d0372cb7fc8ec5acbab91ee298b673167 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
7dd266421377e3f531c15cdf17377141f26c2715 

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


Testing
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated June 5, 2015, 1:31 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

Removed some include artefact.


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


Repository: mesos-incubating


Description
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 

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


Testing
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated June 5, 2015, 1:03 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

Some simplification, added more comments and did a rebase.


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


Repository: mesos-incubating


Description
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 

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


Testing
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

> On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 29
> > 
> >
> > nice tests.

Can I drop this issue :)?


> On May 19, 2015, 8:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 32-90
> > 
> >
> > Can you add comments to the code here? It is really hard to follow 
> > what's happening.
> > 
> > Alternatively, can you simplify these using "strings" functions?
> > 
> > For example, looking at http://linux.die.net/man/3/basename 
> > ```
> > The functions dirname() and basename() break a null-terminated pathname 
> > string into directory and filename components. In the usual case, dirname() 
> > returns the string up to, but not including, the final '/', and basename() 
> > returns the component following the final '/'. Trailing '/' characters are 
> > not counted as part of the pathname.
> > 
> > If path does not contain a slash, dirname() returns the string "." 
> > while basename() returns a copy of path. If path is the string "/", then 
> > both dirname() and basename() return the string "/". If path is a NULL 
> > pointer or points to an empty string, then both dirname() and basename() 
> > return the string ".".
> > ```
> > 
> > this is how I would implement basename
> > 
> > ```
> > string basename() 
> > {
> >// Remove trailing "/", if exists.
> >string result = strings::remove(value, "/", strings::SUFFIX);
> >
> >if (result.empty()) {
> >  return string(".");
> >}
> >
> >if (!strings::contains(result, "/")) {
> >  return result;
> >}
> >
> >if (result == "/") {
> >  return result;
> >}
> >
> >vector tokens = strings::tokenize(result, "/");
> >return tokens[tokens.size() - 1];
> > }
> > ```
> > 
> > I haven't checked all the edge cases, but you get the idea.

I thought a while about how we could use strings::remove but unfortunately I 
did not see a good way to make sure that e.g. multiple trailing slashes are 
properly cut off. In the end I decided that using an indexed based parsing 
would be more efficient and by the added comments also readable fine - at least 
that is what I think :)


- Till


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


On May 17, 2015, 7:46 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated May 17, 2015, 7:46 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Alexander Rojas]

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



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


+1


- Alexander Rojas


On May 17, 2015, 9:46 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated May 17, 2015, 9:46 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Vinod Kone]

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



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Can you add comments to the code here? It is really hard to follow what's 
happening.

Alternatively, can you simplify these using "strings" functions?

For example, looking at http://linux.die.net/man/3/basename 
```
The functions dirname() and basename() break a null-terminated pathname 
string into directory and filename components. In the usual case, dirname() 
returns the string up to, but not including, the final '/', and basename() 
returns the component following the final '/'. Trailing '/' characters are not 
counted as part of the pathname.

If path does not contain a slash, dirname() returns the string "." while 
basename() returns a copy of path. If path is the string "/", then both 
dirname() and basename() return the string "/". If path is a NULL pointer or 
points to an empty string, then both dirname() and basename() return the string 
".".
```

this is how I would implement basename

```
string basename() 
{
   // Remove trailing "/", if exists.
   string result = strings::remove(value, "/", strings::SUFFIX);
   
   if (result.empty()) {
 return string(".");
   }
   
   if (!strings::contains(result, "/")) {
 return result;
   }
   
   if (result == "/") {
 return result;
   }
   
   vector tokens = strings::tokenize(result, "/");
   return tokens[tokens.size() - 1];
}
```

I haven't checked all the edge cases, but you get the idea.



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp


nice tests.


- Vinod Kone


On May 17, 2015, 7:46 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34256/
> ---
> 
> (Updated May 17, 2015, 7:46 p.m.)
> 
> 
> Review request for mesos and Cody Maloney.
> 
> 
> Bugs: MESOS-1303
> https://issues.apache.org/jira/browse/MESOS-1303
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Introducing Path::dirname() and Path::basename() as a thread safe replacement 
> of the respective system functions. Also contains new tests covering corner 
> cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 
> 
> Diff: https://reviews.apache.org/r/34256/diff/
> 
> 
> Testing
> ---
> 
> make check (including new tests).
> 
> Result comparison to match ::dirname and ::basename on interesting cases.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 34256: Added Path::dirname() and Path::basename().

[Till Toenshoff]

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

(Updated May 17, 2015, 7:46 p.m.)


Review request for mesos and Cody Maloney.


Changes
---

Moved implementations into Path.


Summary (updated)
-

Added Path::dirname() and Path::basename().


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


Repository: mesos-incubating


Description (updated)
---

Introducing Path::dirname() and Path::basename() as a thread safe replacement 
of the respective system functions. Also contains new tests covering corner 
cases.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df650 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp cf35412 

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


Testing (updated)
---

make check (including new tests).

Result comparison to match ::dirname and ::basename on interesting cases.


Thanks,

Till Toenshoff