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.

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

get_vcs: improve Cygwin git support under Windows #704

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

lazka
Copy link
Contributor

@lazka lazka commented Feb 26, 2024

(I haven't created an issue since the issue template suggested to skip it, but I can create one if wanted)

In MSYS2 we currently use native Python in combination with cygwin git, the former working with Windows paths, while the latter returns Unix paths, which means get_vcs() will find a git repo, but with a bogus git directory.

It's quite easy to fix by using git in a way so it returns relative paths, without adding any special cases.

  1. Improve the test coverage of get_vcs() to make sure we don't break things
  2. Switch git to use relative paths instead of absolute paths
  3. Some further cleanup to not change the working dir, while at it.

See the separate commits for details.

This makes the test suite 100% pass under MSYS2.

Cover more cases:

* that get_vcs() actually returns the git root path
* finding the git dir from a subdirectory
* git not finding a git repo
* a gitignored directory not being found
In MSYS2 we currently use cygwin git which returns/prints unix paths. For
example "rev-parse --show-toplevel" returns absolute paths which Windows
CPython doesn't understand, and thus get_vcs() fails to find the git repo
root.

Instead of using "--show-toplevel" which returns the absolute path to the git
root, we can just use "--show-cdup" which returns the relative path from the
current working directory to the git root, for example "../" in a repo
subdirectory. Since this is a relative path it's also valid as a Windows path,
and combined with the working directory we get the git root, the same as
before.
@lazka lazka force-pushed the get-vcs-cleanups branch 2 times, most recently from 5e14403 to b2d7575 Compare February 26, 2024 07:09
It's not needed to change global state here, just change the
working directory of the child processes instead.

Since os.chdir() raised for non-existing paths, keep
that behaviour by passing strict to resolve().
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@lazka lazka changed the title get_vcs: support Cygwin git under Windows get_vcs: improve Cygwin git support under Windows Feb 26, 2024
@abn abn merged commit e50e6f0 into python-poetry:main Feb 26, 2024
19 checks passed
@lazka
Copy link
Contributor Author

lazka commented Feb 27, 2024

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants