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

Pre-commit fails for git >=2.25 if repo is on a Windows subst drive #1610

Closed
jcameron73 opened this issue Sep 22, 2020 · 5 comments
Closed

Pre-commit fails for git >=2.25 if repo is on a Windows subst drive #1610

jcameron73 opened this issue Sep 22, 2020 · 5 comments
Labels

Comments

@jcameron73
Copy link

@jcameron73 jcameron73 commented Sep 22, 2020

Cross reference for another issue with same apparent root cause: microsoft/vscode#100274 (comment)

Issue observed with pre-commit==2.7.1 and git 2.27.
Issue resolved with downgrading git to 2.21 (I only have access to certain versions on my work machine).

Steps to recreate for pre-commit (some taken from the above cross-reference):

  • Install git >= 2.25 on Windows

  • Create a subst drive (mkdir C:\subst_dir && subst Z: C:\subst_dir)

  • Create a git repo in there (mkdir Z:\repo && cd /d Z:\repo && git init)

  • Add some python code, configure pre-commit, and run pre-commit.

Failure observed: An unexpected error has occurred: ValueError: path is on mount 'Z:', start on mount 'C:'

Diagnosis - it appears that the use of git rev-parse --show-toplevel in pre_commit.main.get_root() is suffering the same issue as seen in cross-referenced ticket; git will "see through" the subst command and rather than return a path on the subst-defined Z: drive, it will return the path from the C: drive. With this, after pre_commit.main._adjust_args_and_chdir() calls pre_commit.main.get_root() and does a chdir to the returned location, the following call to os.path.relpath(args.config) then fails with the ValueError as above, because it sees the path to the config file being on Z: but the current location being on C:.

Afraid I don't have a suggested resolution but wanted to flag this up. I'm not too familiar with Windows systems and I'm a long way from Admin access on my work machine so opportunities for testing are limited; this was discovered as my scratch space for repos is a subst drive.

@christopherdoyle
Copy link

@christopherdoyle christopherdoyle commented Oct 30, 2020

I've come across this because I'm having a similar issue with mapped network drives (e.g. net use). I've replicated your issue with pre-commit 2.8.1 and git 2.29.1.

My fist stab at a solution that works in both git 2.17 and 2.29 is the following change to pre_commit.main._adjust_args_and_chdir:

from pathlib import Path
toplevel = Path(git.get_root()).resolve()
os.chdir(toplevel)

args.config = os.path.relpath(Path(args.config).resolve())

Probably should have if sys.platform == "win32" in there.

In the case of subst, Path.resolve will give the underlying location C:\subst_dir. For mapped drives it will return the network path, e.g. \\MyServer\Directory.

@mrogaski
Copy link
Contributor

@mrogaski mrogaski commented Dec 6, 2020

Using --show-cdup instead of --show-toplevel, as was suggested in the VSCode issue listed in the OP, is worth considering.

def get_root() -> str:
    try:
        root = cmd_output('git', 'rev-parse', '--show-cdup')[1].strip()
    except CalledProcessError:
        raise FatalError(
            'git failed. Is it installed, and are you in a Git repository '
            'directory?',
        )
    if not root:
        return os.getcwd()
    return root

The original failure mode (ie. issuing the command from in .git) will fail, but will be a non-zero return code from Git and be caught by CalledProcessError.

@asottile
Copy link
Member

@asottile asottile commented Dec 6, 2020

can probably be return os.path.abspath(root) to retain the original behaviour -- one thing to check would be the version in which --show-cdup was added in case that limits compatibility (but as long as it's sufficiently old it should be fine without needing a fallback)

@asottile
Copy link
Member

@asottile asottile commented Dec 6, 2020

yeah it's super old so that should be a fine patch!

$ git describe --contains 5f94c730f31862c7f500173ee3a9d141c4730f0b
v1.1.0~31
@jcameron73
Copy link
Author

@jcameron73 jcameron73 commented Dec 12, 2020

I've pulled this into the project where the issue first arose and it's working beautifully - thank you!

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

Successfully merging a pull request may close this issue.

4 participants