> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 35 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line35> > > > > IMO calling asAbsolute("") sounds more like a bug then just returning > > "/".
Practically there isn't a good place to return such an error. This code is generally acting on paths given by a user of mesos / caller of a mesos API, so doing a assert() or CHECK() definitely is not the right thing. The given behavior is a simple, reasonable way to continue without erroring. In debugging code which does call this wrong, you get exactly the same thing every time, which doesn't make it particularly hard to track down. > On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 46 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line46> > > > > It's quite unclear what path::clean means, and have to read the tests > > to see what you're trying to do. When is this useful? > > If we're trying to clean up user configuration, I rather we expose the > > error instead of trying to clean up mistakes. I can add a little documentation about the sorts of things it cleans up. It's not user configuration, it is runtime parameters to API calls where this comes in the most. Someone calls `/files/browse.json?path=foo` `/files/browse.json?path=foo/` `/files/browse.json?path=/foo` `/files/browse.json?path=/foo/`. All of those are reasonable ways to get to the same place in the filesystem. They follow what people expect from the filesystem, and so we need to handle them all. I can document some of the exact behavior here, but really it is the combined behavior of split and join, as those do the cleanup implicitly in their operation. All the normalization of the paths used to live just in files/files.cpp and was applied inconsistenly (even within the one file). The path::clean() makes one consistent place for cleaning up a path (which may be virtual, so we can't use the OS calls) to live. > On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 127 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line127> > > > > Illegal based on? It doesn't make any logical sense. You can't have an empty directory name inside a directory. Joining it would result in a/ which isn't what is desired. > On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 84 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line84> > > > > I don't think the result of path::join is always expected to be > > absolute paths? I totally see where people want to join two relative paths > > together. It isn't expected to be an absolute path. The code doesn't force that. It simply states that if the first portion of a path is an empty string (You pass the path {"", "foo", "bar"}, that path is absolute /foo/bar. If the first part isn't empty, then the path is relative {"foo","bar"} -> 'foo/bar'. With vectors just appending the vectors works just as you expect when you then join() the resulting vector. If you join an absolute path after another path, that will also come out as a developer expects (because join() is accepting of extra spaces in the middle of the path). The unit test has lots of examples that both relative and absolute paths are accepted. They didn't particularly use to be before the rewrite in MESOS-1733. That used to assume they are always relative. > On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 121 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line121> > > > > Why does a absolute path give you two empty splits? It allows us to tell "/" from just having an empty path "". Just an empty path is relative. Empty preceeding anything is a specification of an absolute path, starting at the root of a filesystem. Non-empty means that the path is relative. Admittedly this case could be reduced to {""}, which would have the same round trip effect. But the code is fairly thoroughly tested with the two parts, and that logically makes more sense to me. An empty directory name at the start means the root directory. > On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 123 > > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line123> > > > > So an empty string means slash then? if you want to keep the beginning > > slash(s) why not just use slash? > > However, I think you have very specific use cases in mind implementing > > split and clean, and from the comments I can't really understand where > > you're coming from. > > Can you comment on all these methods explaining what path::clean and > > path::split means and does? Because joining if we use '/' is difficult. Esp. if you append an absolute path to the end of an absolute path {"a","b"} + {"/","a","b"}. Empty is what we should sanitize out anways ('a//b' people expect to equal 'a/b'). I can document a little more what clean does, although I don't think there is anything particularly useful to be said other than "It makes paths into a more normalized form, maintinaing meaning". (There is a reason it doesn't remove/sanitize out '..' or '.'). Split has name that matches content. I could write out every single base case it can explode to, but the behavior is writtent o roughly mirror join(). Inside the code of the function itself it comments why / what it is trying to clean at each step. > On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 25 > > <https://reviews.apache.org/r/26766/diff/1/?file=722432#file722432line25> > > > > Can we rename RoundTrip and RoundTripReduce to with names that > > resemable more with string functions? Like VerifySplitJoin? Also just two > > overloads. They are specifically testing that the join and split round trip properly. That if you split a string you get to the expected intermediate. And if you join it you get back to where you started. They are much more about can we start with a string, parse it, and put it back together and get what we gave it (do a round trip), than verifying the operation of the individual components (although they also do that). Can you give any specific logic as to why 'VerifySplitJoin' is going to be easier for later developers to grok what is being tested by it? I explicitly want them to be different names, because they have different testing behavior. RoundTrip() tests I get back exactly what I started with. If I don't it really is an error, not just "oh, I should add another parameter and the test case will pass". It has a very different meaning and test guarantee when the result ends up different than the source string, and I want it to be obvious that "For the string being tested, it is the case that we expect it to be simplified" - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26766/#review56865 ----------------------------------------------------------- On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26766/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2014, 6:46 p.m.) > > > Review request for mesos and Timothy Chen. > > > Bugs: MESOS-1878 > https://issues.apache.org/jira/browse/MESOS-1878 > > > Repository: mesos-git > > > Description > ------- > > Adds 3 new functions: asAbsolute, clean, and split(). All three were > hand-coded inside of mesos files (files/files.cpp). This puts them in a > common place, and adds unit tests for their behavior. > > The functions depend on eachother somewhat, so I pulled out the declarations > to make them all forward declared. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp > 357a75a8bac497465671456aa9cd9181123cc635 > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp > aedf93573ea89e46bf7b7b91f2258049af2fd79f > > Diff: https://reviews.apache.org/r/26766/diff/ > > > Testing > ------- > > make distcheck > > > Thanks, > > Cody Maloney > >
