Better handle file names starting with ~ in Path#expand

[byroot]

Currently p Path.new("~foo.txt").expand gives: Path["/home/crystal/oo.txt"].

The expected return value could either be Path["/home/crystal/~foo.txt"] or Path["/home/foo.txt"] depending on wether or not you'd like to support the ~username notation like Ruby's File.expand_path does.

I implemented the former as it's much simpler.

[ysbaddaden]

Pull request description is a little misleading (seems like ~foo is expanded to home directory), but implementation only takes care of ~ and ~/ is fine.

[straight-shoota]

This is clearly a bug, yes. IIRC I had considered implementing ~username to expand to /home/username (or whatever the home of username is), but this is not yet supported because of the additional effort of a cross-platform implementation. But it should be possible.

I guess we could fix this for now. However, that would lead to a breaking change when home directory resolution is implemented.
So maybe it would be better to already cover that case and either return a value that is only a good guess or raise an error that home path expansion cannot be resolved. Or provide a basic implementation right away.

[straight-shoota]

We could also decide we don't ever want username-based home resolution. Then it's fine to merge this anyway.

But I think it's a neat feature and typically provided by shells, so it would be great to have it available. Even if it's probably not very commonly used.

[byroot]

I guess we could fix this for now. However, that would lead to a breaking change when home directory resolution is implemented.

Regardless of wether or not this fix is merged, implementing home directory resolution will be a breaking change.

If people are sure they want to implement it, I can try to take a stab at it, but it's far from trivial, there's a lot of subtilities to it. For instance, what if the user doesn't exist? Ruby will raise an error:

>> File.expand_path('~foo/bar')
ArgumentError (user foo doesn't exist)

This is to say that there's quite a lot of API design consideration in such feature, and beside mimicking Ruby I don't really feel I can make these decisions myself.

[straight-shoota]

Regardless of wether or not this fix is merged, implementing home directory resolution will be a breaking change.

Currently, the ~username format is broken anyway, so this is not an issue.

For instance, what if the user doesn't exist?

This should result in a No such file or directory error.

Implementation of this feature requires #7738 and then it shouldn't be hard to do:

elsif name.starts_with?("~")
  username = parts.first.lchop('~')
  user = System::User.from_name?(username)
  raise "No such file or directory" unless user
  name = new_instance(user.home_dir).join(*parts[1..])
end

[byroot]

Currently, the ~username format is broken anyway, so this is not an issue.

Well, based on the method source, I think it's fair to say that it's not broken but simply not implemented. I get that this might seem like splitting hairs, but the distinction is important.

To give more context to this PR, here's what led me to it: https://old.reddit.com/r/crystal_programming/comments/bn9veq/fileexpand_path_with_filename_that_starts_with/

The user on this post didn't expect File.expand_path("~foo") to try to resolve a foo user home.

[straight-shoota]

No, it's broken. The first character after the tilde is chopped off. Path.new("~foo").expand_path returns the equivalent to Path.new("~/oo").expand_path.

"not implemented" would not change the path at all.

When we repair it we can repair it to "not implemented" which would result in a breaking change later, or directly implement it (at least recognize the syntax), skipping the breaking change.

[byroot]

@straight-shoota yes it's broken, but the source code doesn't show any kind of attempt at supporting ~username.

It returns ~/oo because it check for start_with?("~") and then assume the second character is /.

So it's I'm just trying to fix an obvious bug. Sure we could think wether we'd like to also implement a feature at the same time, but IMO they are two different things, and this simple fix should make it now, and eventually the feature be added later on it someone feels like it.

[straight-shoota]

The current implementation already handles path starting with ~ specially. It does so in a weird way, that needs fixing. But when we fix that, I'd like to fix it with further feature expansion in mind to avoid a breaking change later.

[ysbaddaden]

The current implementation is broken: it chops the tilde and the following character in cases where it should not, since only ~ and ~/path are supported.

Choosing to handle ~username is another matter. It involves cross platform discrepancies and additional syscalls to get the home of a given user, potentially failing if the user doesn't exist.

If someone is willing to implement the latter with support for all platforms, please do so, it would be a nice to have feature.

In the meantime and otherwise, let's just merge this fix.