vlib.os: fix join-path

[hholst80]

Closes #20231

[spytheman]

That is a regression.

[hholst80]

should os.join_path do normalization or not? That is a pretty big task. I think real_path can be used to cleanup it. Compare to os.path.join in Python they also do not normalize ./ it is important that the file will be interpreted correctly, not how it looks like. "foo/./bar" is perfectly valid path and is equivalent to "foo/bar"

Any kind of logic we expose here will be maintained forever. Less is more.

[spytheman]

It is better to have less edge cases in the output for this particular function.
The call sites can then be simpler.

[hholst80]

Neither C++ nor Python does this. I think this would be confusing and hard to maintain properly. If the caller needs proper normalization it should call normalize path after the call. Half baked normalization here helps nobody.

# cat t.cpp
#include <iostream>
#include <filesystem>

int main() {
  std::filesystem::path p1 = "/";
  std::filesystem::path p2 = "./test";
  std::filesystem::path p = p1 / p2;
  std::cout << p << std::endl;
}
[I] root@trump ~ 
# g++ -o t t.cpp
[I] root@trump ~ 
# ./t
"/./test"
[I] root@trump ~ 
# 

[spytheman]

It does help code that uses it, and it is not proper to make such a change, that breaks an existing test (which then needed changing), in a PR that fixes an unrelated problem (appending to /) .

[spytheman]

-		assert os.join_path('/foo/bar', './file.txt') == '/foo/bar/file.txt'
+		assert os.join_path('/foo/bar', './file.txt') == '/foo/bar/./file.txt'

That is a test, that passed on master, and you changed it here.

[spytheman]

Do not fud my PR with "breaks tests" when obviously no tests are failing.

Obviously, I am commenting under that very change.

However, I do not want to argue anymore, and justify my opinion, and I am not happy with your PR as it is. I also asked you something in the other PR, which you are ignoring.

[ttytm]

I agree with @spytheman about the regression.

Please also refer to how it's done in go:

package main

import (
	"path/filepath"
)

func main() {
	println(filepath.Join("/foo/bar", "./file.txt")) // -> /foo/bar/file.txt
}

[hholst80]

That is reasonable argument, only mentioned now, that the API should be compatible with Golang. Given how similar the API are to go in other areas like network that V should try and offer few surprises.

See, I like a good discussion, but I am not unreasonable. ;)

[ttytm]

Idk if they should be compatible. I wanted to put an example of more mature project that also has good engineers. If we find good reasons for it lets break the rules :D.

I think spy did a good work and also extends a lot of test cases. Personally, I like achieving a better product and finding solutions. So when another approaches arise that solve a problem in a more complete way it I usually have no problem giving into it. So we all get a better language :P

Things were just a little unlucky here that the approaches are so different and hardly compatible.

[ttytm]

As a simpler variant, could this work too?

Suggested change

	mut i := 0
	mut needs_sep := false
	if base != '' {
	sb.write_string(base)
	needs_sep = !base.ends_with(path_separator)
	} else {
	for i < dirs.len {
	d := dirs[i]
	i++
	if d != '' {
	sb.write_string(d)
	needs_sep = !base.ends_with(path_separator)
	break
	}
	}
	}
	sb.write_string(sbase)
	for d in dirs {
	for i < dirs.len {
	d := dirs[i]
	if d != '' {
	sb.write_string(path_separator)
	if needs_sep {
	sb.write_string(path_separator)
	}
	sb.write_string(d)
	needs_sep = !d.ends_with(path_separator)
	}
	i++
	}
	mut res := sb.str()
	if sbase == '' {
	res = res.trim_left(path_separator)
	}
	if res.contains('/./') {
	// Fix `join_path("/foo/bar", "./file.txt")` => `/foo/bar/./file.txt`
	res = res.replace('/./', '/')
	}
	res := sb.str()
	mut needs_sep := false
	if base != '' {
	sb.write_string(base)
	needs_sep = !base.ends_with(path_separator)
	}
	for d in dirs {
	if d != '' {
	if needs_sep {
	sb.write_string(path_separator)
	}
	sb.write_string(d)
	needs_sep = !d.ends_with(path_separator)
	}
	}
	res := sb.str()

[ttytm]

The suggestion looks buggy.

I'm referring to the part starting below the defer block:
Current:

	mut i := 0
	mut needs_sep := false
	if base != '' {
		sb.write_string(base)
		needs_sep = !base.ends_with(path_separator)
	} else {
		for i < dirs.len {
			d := dirs[i]
			i++
			if d != '' {
				sb.write_string(d)
				needs_sep = !base.ends_with(path_separator)
				break
			}
		}
	}
	for i < dirs.len {
		d := dirs[i]
		if d != '' {
			if needs_sep {
				sb.write_string(path_separator)
			}
			sb.write_string(d)
			needs_sep = !d.ends_with(path_separator)
		}
		i++
	}
	res := sb.str()

Suggested:

	mut needs_sep := false
	if base != '' {
		sb.write_string(base)
		needs_sep = !base.ends_with(path_separator)
	}
	for d in dirs {
		if d != '' {
			if needs_sep {
				sb.write_string(path_separator)
			}
			sb.write_string(d)
			needs_sep = !d.ends_with(path_separator)
		}
	}
	res := sb.str()

[hholst80]

Totally agree the default false value makes it possible to merge the loops and get rid of the index i.

[spytheman]

I think that the approach in #21429 is cleaner.

[spytheman]

@medvednikov what do you think?

[hholst80]

Sure, but it is still incomplete with respect to Golangs "Clean" method:

• https://pkg.go.dev/path#Clean
• https://9p.io/sys/doc/lexnames.html
• https://cs.opensource.google/go/go/+/master:src/internal/filepathlite/path.go

If everyone thinks that this is the gold standard, ok then. I have tried my best to argue for a smaller scope. :)

[ttytm]

Since I'm mentioned as a co-author in the full diff, because the parts I tried to help with during development got squashed, I have to say it's not signed off by me.

I think it's honorable that you leave me in there, I thank you for that, @hholst80, but the way it's done might also distort the situation.

Now, aiming to keep things technical and to choose the solution that best serves the project, why not go with the suggestion in Spy's PR and have this one updated? Or what issues are there when doing it like this?

[hholst80]

Since I'm mentioned as a co-author in the full diff, because the parts I tried to help with during development got squashed, I have to say it's not signed off by me.

I think it's honorable that you leave me in there, I thank you for that, @hholst80, but the way it's done might also distort the situation.

Now, aiming to keep things technical and to choose the solution that best serves the project, why not go with the suggestion in Spy's PR and have this one updated? Or what issues are there when doing it like this?

I don't think that matter. I should add Spy as a co-author as well because I stole code from his PR but he already maxed out on street cred.

I think from a newcomers perspective that it is important that my PR takes precedence because I took my time to report an issue and then to understand what was the problem. For me this was an investment of time, not only now, but in the future because I 1) believe in V's future and and 2) see myself as part of that future.

Ultimately it is a team effort to get it done and a PR process is just an exercise an proving ground if the communication works does the developers listen and respect each other.