Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

os: fix os.join_path('/', 'file.txt') and os.join_path_single('/', 'file.txt') (fix #20231) #21429

Closed
wants to merge 7 commits into from
Closed

os: fix os.join_path('/', 'file.txt') and os.join_path_single('/', 'file.txt') (fix #20231) #21429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0ac4ba7
os: fix `os.join_path(/, file.txt)` and `os.join_path_single(/, file.…
spytheman May 5, 2024
30c3491
os: extract join_path_test.v for easier testing of just os.join_path …
spytheman May 5, 2024
3fd85fb
fix js backend failures
spytheman May 5, 2024
faa21d5
fix off by 1 error in the loop, add more test cases
spytheman May 5, 2024
b5f9123
unify the behaviour of os.join_path and os.join_path_single for more …
spytheman May 5, 2024
c86eeb5
fix parser bug for `./v test-parser` for `import `
spytheman May 5, 2024
7fe84c5
fix dir_expansions_test.v on windows (USERPROFILE will contain \ norm…
spytheman May 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vlib/os/dir_expansions_test.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ fn test_ensure_folder_is_writable() {

fn test_expand_tilde_to_home() {
os.setenv('HOME', '/tmp/home/folder', true)
os.setenv('USERPROFILE', '/tmp/home/folder', true)
os.setenv('USERPROFILE', r'\tmp\home\folder', true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I did this I am fairly sure the word "regression" would come up. ;-)

Anyways, I took this verbatim into my PR. The extended suite of tests and refactoring is probably a good idea anyways, if we now are going forward with the path normalization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USERPROFILE is a variable that is only defined on windows, and our home_dir only uses it on windows.
On windows, it will contain \, not / .
The difference between the regression in your PR, and the change here, is that here, the change in the test, makes the test closer to the environment, that it tested.
In your PR, the change was done just to make the CI pass with the changes in the PR.

//
home_test := os.join_path(os.home_dir(), 'test', 'tilde', 'expansion')
home_expansion_test := os.expand_tilde_to_home(os.join_path('~', 'test', 'tilde',
Expand Down
62 changes: 62 additions & 0 deletions vlib/os/join_path_test.v
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import os

fn test_join_path() {
assert os.join_path('', '', '') == ''
assert os.join_path('', '') == ''
assert os.join_path('') == ''
assert os.join_path('b', '', '') == 'b'
assert os.join_path('b', '') == 'b'
assert os.join_path('b') == 'b'
assert os.join_path('', '', './b') == 'b'
assert os.join_path('', '', '/b') == 'b'
assert os.join_path('', '', 'b') == 'b'
assert os.join_path('', './b') == 'b'
assert os.join_path('', '/b') == 'b'
assert os.join_path('', 'b') == 'b'
assert os.join_path('b', '') == 'b'
$if windows {
assert os.join_path('./b', '') == r'.\b'
assert os.join_path('/b', '') == r'\b'
assert os.join_path('v', 'vlib', 'os') == r'v\vlib\os'
assert os.join_path('', 'f1', 'f2') == r'f1\f2'
assert os.join_path('v', '', 'dir') == r'v\dir'
assert os.join_path('foo\\bar', '.\\file.txt') == r'foo\bar\file.txt'
assert os.join_path('/opt/v', './x') == r'\opt\v\x'
} $else {
assert os.join_path('./b', '') == './b'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be just b imho

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

assert os.join_path('/b', '') == '/b'
assert os.join_path('v', 'vlib', 'os') == 'v/vlib/os'
assert os.join_path('/foo/bar', './file.txt') == '/foo/bar/file.txt'
assert os.join_path('', 'f1', 'f2') == 'f1/f2'
assert os.join_path('v', '', 'dir') == 'v/dir'
assert os.join_path('/', 'test') == '/test'
assert os.join_path('foo/bar', './file.txt') == 'foo/bar/file.txt'
assert os.join_path('/opt/v', './x') == '/opt/v/x'
assert os.join_path('./a', './b') == './a/b'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a/b here? I see no reason to keep the initial ./ prefix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It will be simpler without it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will have to be done in a separate focused PR, since it will break asserts like this one cmd/tools/vcomplete_test.v:229:

   > assert line == sorted_expected[i]
     Left value: sub0/
    Right value: ./sub0/

, and is not directly related to the join_path('/', 'x') problem in the issue.

Copy link
Member Author

@spytheman spytheman May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is implemented in https://github.com/spytheman/v/tree/cleanup_dot_slash_prefix_at_start_of_output_in_join_path for now.

}
}

fn test_join_path_single() {
assert os.join_path_single('', '') == ''
assert os.join_path_single('', './b') == 'b'
assert os.join_path_single('', '/b') == 'b'
assert os.join_path_single('', 'b') == 'b'
assert os.join_path_single('b', '') == 'b'
$if windows {
assert os.join_path_single('./b', '') == r'.\b'
assert os.join_path_single('/b', '') == r'\b'
assert os.join_path_single('/foo/bar', './file.txt') == r'\foo\bar\file.txt'
assert os.join_path_single('/', 'test') == r'\test'
assert os.join_path_single('foo\\bar', '.\\file.txt') == r'foo\bar\file.txt'
assert os.join_path_single('/opt/v', './x') == r'\opt\v\x'
} $else {
assert os.join_path_single('./b', '') == r'./b'
assert os.join_path_single('/b', '') == r'/b'
assert os.join_path_single('/foo/bar', './file.txt') == '/foo/bar/file.txt'
assert os.join_path_single('/', 'test') == '/test'
assert os.join_path_single('foo/bar', './file.txt') == 'foo/bar/file.txt'
assert os.join_path_single('/opt/v', './x') == '/opt/v/x'
assert os.join_path_single('./a', './b') == './a/b'
assert os.join_path_single('a', './b') == 'a/b'
}
}
48 changes: 39 additions & 9 deletions vlib/os/os.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,14 +587,11 @@ pub fn join_path(base string, dirs ...string) string {
sb.write_string(d)
}
}
normalize_path_in_builder(mut sb)
mut res := sb.str()
if sbase == '' {
if base == '' {
res = res.trim_left(path_separator)
}
if res.contains('/./') {
// Fix `join_path("/foo/bar", "./file.txt")` => `/foo/bar/./file.txt`
res = res.replace('/./', '/')
}
return res
}

Expand All @@ -612,12 +609,45 @@ pub fn join_path_single(base string, elem string) string {
defer {
unsafe { sbase.free() }
}
if base != '' {
sb.write_string(sbase)
sb.write_string(sbase)
if elem != '' {
sb.write_string(path_separator)
sb.write_string(elem)
}
normalize_path_in_builder(mut sb)
mut res := sb.str()
if base == '' {
res = res.trim_left(path_separator)
}
return res
}

@[direct_array_access]
fn normalize_path_in_builder(mut sb strings.Builder) {
mut fs := `\\`
mut rs := `/`
$if windows {
fs = `/`
rs = `\\`
}
for idx in 0 .. sb.len {
unsafe {
if sb[idx] == fs {
sb[idx] = rs
}
}
}
// Let `/foo/./bar.txt` be `/foo/bar.txt` in place:
for idx in 0 .. sb.len - 3 {
if sb[idx] == rs && sb[idx + 1] == `.` && sb[idx + 2] == rs {
unsafe {
for j := idx + 1; j < sb.len - 2; j++ {
sb[j] = sb[j + 2]
}
sb.len -= 2
}
}
}
sb.write_string(elem)
return sb.str()
}

// walk_ext returns a recursive list of all files in `path` ending with `ext`.
Expand Down
13 changes: 0 additions & 13 deletions vlib/os/os_test.c.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,19 +614,6 @@ fn test_file_ext() {
assert os.file_ext('\\.git\\') == ''
}

fn test_join() {
$if windows {
assert os.join_path('v', 'vlib', 'os') == 'v\\vlib\\os'
assert os.join_path('', 'f1', 'f2') == 'f1\\f2'
assert os.join_path('v', '', 'dir') == 'v\\dir'
} $else {
assert os.join_path('v', 'vlib', 'os') == 'v/vlib/os'
assert os.join_path('/foo/bar', './file.txt') == '/foo/bar/file.txt'
assert os.join_path('', 'f1', 'f2') == 'f1/f2'
assert os.join_path('v', '', 'dir') == 'v/dir'
}
}

fn test_rmdir_all() {
mut dirs := ['some/dir', 'some/.hidden/directory']
$if windows {
Expand Down
4 changes: 4 additions & 0 deletions vlib/v/parser/parser.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3688,6 +3688,10 @@ fn (mut p Parser) import_stmt() ast.Import {
return import_node
}
mut source_name := p.check_name()
if source_name == '' {
p.error_with_pos('import name can not be empty', pos)
return import_node
}
mut mod_name_arr := []string{}
mod_name_arr << source_name
if import_pos.line_nr != pos.line_nr {
Expand Down
Loading