Fix pipx reinstall crash with absolute path

[chrysle]

• I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Closes #1324. This checks whether the argument given as package name inside VenvContainer.get_venv_dir() is an absolute path or existing path, raising PipxError if so. This is due to the behaviour that an absolute path passed into the pathlib.PurePath.joinpath() function to join with another path will return the same.

Test plan

Tested by running

pipx reinstall /some/absolute/path
mkdir existing_path ; pipx reinstall existing_path

[Gitznik]

What's the behavior if you pass a relative path? I assume it will also lead to unexpected results?

[chrysle]

It just gets "normalized" and then appended to pipx's venv path, due to the specialty noted above, therefore fails. I thought it was not worth the while catching this mistake, too, since few users should attempt something like that and be able to solve it easily by the output given. We can not disallow passing in existing paths by default, since installing from a local archive for example is also a case.

[jayqi]

Ah, classic two-absolute-paths-where-one-clobbers-the-other.

I'm also generally kind of suspicious of allowing paths being passed in to reinstall at all. I get that relative paths will likely result in a no-op, and actually I think that is bad. One of the reasons I ended up trying an absolute path was because I tried a relative path and got a "nothing to reinstall" message instead of something telling me that I was wildly misusing the argument.

We can not disallow passing in existing paths by default, since installing from a local archive for example is also a case.

This doesn't sound right to me for reinstall? If reinstall is supposed to expect the name of a package (which is also the name of pipx's venv for it), even if someone had originally installed from a local source path, shouldn't the name just be the name of the package?

If this is the case, then it feels like we should be stricter about reinstall only accepting valid package names and not accepting paths at all. PyPA's specification has a regex for valid package names: https://packaging.python.org/en/latest/specifications/name-normalization/#name-format

I also think the error messages here could be clearer. If there's no venv named, e.g., foobar, then currently it says:

Nothing to reinstall for foobar 😴

which is pretty ambiguous. I did have a package installed (that I was trying to pass in a path for) and this message made it seem like pipx just didn't think reinstallation was necessary.

This would be much more clear to the user if it says something like

No package named foobar installed

[chrysle]

This doesn't sound right to me for reinstall? If reinstall is supposed to expect the name of a package (which is also the name of pipx's venv for it), even if someone had originally installed from a local source path, shouldn't the name just be the name of the package?

That's because pipx install also makes use of this function. Probably we should handle this case separately for the reinstall command, as you suggested. Thinking of it, this seems to be much more reasonable. I could rework the PR towards that.

This would be much more clear to the user if it says something like

No package named foobar installed

I agree with that. Would you like to improve it in a separate PR?

[chrysle]

Could you review again?

[Gitznik]

I'm on vacation for the next few weeks, so someone else needs to take over the reviews for this.

[Gitznik]

Nice, lgtm