fsutil: Add SlashedPathDir() and SlashedPathClean() functions #75
Conversation
woky
force-pushed
the
pub/fsutil-dir-clean
branch
from
8d9b05b
to
4186c05
Compare
internal/slicer/slicer.go
Outdated
| cleanPath := filepath.Clean(path) | ||
| slashPath := cleanPath | ||
| if path[len(path)-1] == '/' && cleanPath != "/" { | ||
| slashPath += "/" | ||
| } | ||
| path = fsutil.Clean(path) |
There was a problem hiding this comment.
The output of these two snippets are not identical I believe?
slashpath will always end with a Separator.
path uses fsutil.Clean() only, which could end without a Separator.
There was a problem hiding this comment.
Can you elaborate on why you think slashpath will always end with separator? From diff snippet it looks like it'll only end with separator when path ends with /'.
There was a problem hiding this comment.
I just did a test because my brain failed to produce an explanation:
old addKnownPath("/"):
output: "/"
new addKnownPath("/"):
output: "/"
output: "//"
Check your new Clean() unit test does not try the "/" root case. It produces "//". Did I miss something ?
There was a problem hiding this comment.
Thanks for the find. I didn't notice the bug in Clean() (now SlashedPathClean()). I've fixed the bug there and added test cases for root directory. Please resolve this thread if appropriate.
woky
force-pushed
the
pub/fsutil-dir-clean
branch
2 times, most recently
from
c9cff06
to
7c281e9
Compare
cjdcordeiro
added
Priority
internal/fsutil/path_test.go
Outdated
| input string | ||
| output string | ||
| }{ | ||
| {"/a/b/c", "/a/b/c"}, |
There was a problem hiding this comment.
I think you should add "/" root here :)
There was a problem hiding this comment.
Thanks! Indeed it didn't handle / correctly. It should be fixed now and there test cases testing various forms of the root directory. Please check it out and resolve if appropriate.
woky
force-pushed
the
pub/fsutil-dir-clean
branch
2 times, most recently
from
a4c15cb
to
51ac20a
Compare
woky
added
Blocked
woky
force-pushed
the
pub/fsutil-dir-clean
branch
4 times, most recently
from
af0c12e
to
bee9816
Compare
woky
force-pushed
the
pub/fsutil-dir-clean
branch
3 times, most recently
from
f0e4b5e
to
b003238
Compare
woky
changed the title
|
This PR depends on #69. |
Currently we only test with the embedded base-files files package. This quite limits what we can test. But since commit a3315e3 ("testutil/pkgdata: Create deb programatically") we have the ability to define our own custom package content. Add support for testing with custom packages. These can be defined in each test case per archive name. The content can be defined directly in test case definition with the help of testutil.MustMakeDeb(). The package content is wrapped in the testPackage structure. This introduces one level of nesting in each test case package definition. In future, we will add package metadata to the package definition, namely version. So wrapping the package content in the structure now and later just adding a new field to it the structure later will make the future diff much smaller.
At some point we will need to get information about (deb) packages from (Go) packages other than archive, namely from the slicer package. For instance: - Package entries in Chisel DB will include version, digest and architecture. - Multi-archive package selection will need to know versions of a package in all configured archives. Add Info() method to the Archive interface that returns a new PackageInfo interface that contains accessors for the underlying control.Section of the package. Currently these accessors are Name(), Version(), Arch() and SHA256() methods. The reason to introduce PackageInfo interface instead of returing control.Section directly is keep the archive abstraction and not leak implementation details.
woky
force-pushed
the
pub/fsutil-dir-clean
branch
from
b003238
to
f83df40
Compare
From the code: These functions exists because we work with slash terminated directory paths that come from deb package tarballs but standard library path functions treat slash terminated paths as unclean. We use ad-hoc code to make filepath.Dir() slash terminated in two places right now. For example this code targetDir := filepath.Dir(strings.TrimRight(targetPath, "/")) + "/" is not strictly correct as "/a/b/.///" will be turned into "/a/b/./" which is still "/a/b". Since we deal with slash terminated directory paths everywhere, create and use dedicated helper functions to process them.
woky
force-pushed
the
pub/fsutil-dir-clean
branch
from
f83df40
to
7febfeb
Compare
cjdcordeiro
added
Priority
|
Per our offline discussion, I think our best way forward is to close this PR and discuss with @rebornplusplus to see if it is a priority right now, and how to amend the code here to make it compatible with the latest changes. We will take that discussion offline and we can always re-open the PRs or create new ones as we see fit. |
From the code:
These functions exists because we work with slash terminated
directory paths that come from deb package tarballs but standard
library path functions treat slash terminated paths as unclean.
We use ad-hoc code to make filepath.Dir() slash terminated in two places
right now. For example this code
targetDir := filepath.Dir(strings.TrimRight(targetPath, "/")) + "/"
is not strictly correct as "/a/b/.///" will be turned into "/a/b/./"
which is still "/a/b".
Since we deal with slash terminated directory paths everywhere, create
and use dedicated helper functions to process them.