> On Oct. 16, 2014, 5:01 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 I'm coming from this is a shared function that is applicable for all
> > paths in Mesos, and I won't really expect that "" == "/" in the path split,
> > and assume I just get the directory names in the vector.
> >
> > Do you need to keep this info for your files.cpp?
Then all paths which are vectors __must__ either be absolute paths or the all
__must__ be relative paths. There is no ability to represent both. I can't
distinguish "/a/b" -> {"a","b"} from {"a/b"} -> {"a","b"}. I think there is
considerable value in being able to represent both, and definitively know which
is being represented. This parallels the variadic join in it's ability to
represent both absolute paths and relative paths.
If I use a '/' to indicate the root rather than empty, then appending an
absolute vector to non-absolute doesn't work. Empty string from what I can see
makes the simplest/cleanest base case which needs the least special logic to
allow both relative and absolute paths to be specified. It "just works" so long
as people always use path::join() to join together their vector into a path
string.
> On Oct. 16, 2014, 5:01 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>
> >
> > I see, but I'm still not clear why we're fixing up /a/b///c/d into
> > /a/b/c/d for the user though?
Because real code in the wild will call the API with /a/b///c/d at some point.
There is a lot of string joining code out there that does that.
Practically it is something we get for free / I'm not explicitly cleaning it up
here. `strings::join(vector<>)` needs to do it to function properly when
someone appends a absolute path to a relative path {"a"} + {"","b"} ->
{"a","","b"} -> "a//b" isn't expected behavior from an end user (They really
don't need to know about the empty chunk unless they are printing, in which
case they should call path::join()). We don't lose anything by accepting this,
it is the same behavior that people get from the open() syscall.
If someone passes in "/a/b//c/d" they are going to expect that the filesystem
inside of /files/browse.json acts like it just passed that to opendir() and
found the directory. That we have built a custom virtual filesystem is
something they shouldn't be even remotely aware of.
We also have no good way of propogating that sort of an error out. If I don't
clean / sanitize it, the developer gets a '404 FileNotFound'. When they go to
the box, and 'cd' to that directory, it works. Just mesos looks/feels broken,
and is hard to debug / figure out what is broken.
Making it so that it just works reduces user frustration, without costing us
any additional code.
> On Oct. 16, 2014, 5:01 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 136
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line136>
> >
> > Why handle it specially here instead of just using the while below?
If you can make the while clean and handle all the cases, then definitely yes.
This is checking that the first token is full, and the second token is empty,
{"a", ""}, which doesn't logically make sense. We can't start the combining at
index 1 always because of the {"", ""} case. Just informing the loop to start
at 1 here seems easiest (The code could have the explicit erase removed as the
loop will do it, but I think it makes the code more explicit what is happening).
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56885
-----------------------------------------------------------
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
>
>
