[clang-format] convert path to Windows path if user is using a MSYS2 shell

[BLumia]

Currently, we simply rely on the result of git rev-parse --show-toplevel in cd_to_toplevel(), which when using MSYS2 shell under Windows, can result getting a UNIX path instead of Windows path, and os.chdir(toplevel) would simply fail.

This patch detects if user is using MSYS2 shell (by checking MSYSTEM environment variable and checking if cygpath exists) and will use cygpath to convert the path to Windows path.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Gary Wang (BLumia)

Changes

Currently, we simply rely on the result of git rev-parse --show-toplevel in cd_to_toplevel(), which when using MSYS2 shell under Windows, can result getting a UNIX path instead of Windows path, and os.chdir(toplevel) would simply fail.

This patch detects if user is using MSYS2 shell (by checking MSYSTEM environment variable and checking if cygpath exists) and will use cygpath to convert the path to Windows path.

---

Full diff: https://github.com/llvm/llvm-project/pull/111526.diff

1 Files Affected:

• (modified) clang/tools/clang-format/git-clang-format (+3)

diff --git a/clang/tools/clang-format/git-clang-format b/clang/tools/clang-format/git-clang-format
index bacbd8de245666..3424822ede3835 100755
--- a/clang/tools/clang-format/git-clang-format
+++ b/clang/tools/clang-format/git-clang-format
@@ -31,6 +31,7 @@ import os
 import re
 import subprocess
 import sys
+import shutil
 
 usage = ('git clang-format [OPTIONS] [<commit>] [<commit>|--staged] '
          '[--] [<file>...]')
@@ -413,6 +414,8 @@ def filter_ignored_files(dictionary, binary):
 def cd_to_toplevel():
   """Change to the top level of the git repository."""
   toplevel = run('git', 'rev-parse', '--show-toplevel')
+  if "MSYSTEM" in os.environ and shutil.which('cygpath') is not None:
+    toplevel = run('cygpath', '-w', toplevel)
   os.chdir(toplevel)
 
 

[mydeveloperday]

I personally use Cygwin and it's git understands unix paths is that not the case for msys2 or are you mixing msys2 with windows based git, won't this have the potential to break others?

[mydeveloperday]

Can you please log an issue for this to show what you see and a reproducer

[BLumia]

I personally use Cygwin and it's git understands unix paths is that not the case for msys2

Git can understand (and will return a) unix path, but it seems MSYS2's python won't accept unix path. It fails at os.chdir(toplevel), and toplevel is a unix path.

are you mixing msys2 with windows based git

I guess no:

Gary@legion-laptop ~/Source/elisa % which python
/ucrt64/bin/python
Gary@legion-laptop ~/Source/elisa % which git
/usr/bin/git

won't this have the potential to break others?

MSYSTEM and cygpath are checked to avoid breaking others but I'm not sure if there are other potential cases. MSYSTEM is a MSYS-only environment which cygwin doesn't use, btw.

Can you please log an issue for this to show what you see and a reproducer

See msys2/MINGW-packages#2457, simply starting a MSYS2 shell (if using Windows Terminal, you can use something like C:/msys64/msys2_shell.cmd -defterm -here -no-start -ucrt64 -shell=zsh, shell can also be bash), cd into a git repo with any valid clang-format configuration, and then execute git clang-format would be able to reproduce the issue. Or do I need to create a new issue in this repo?

[BLumia]

@mydeveloperday any update on this? Do I still need to create a new issue in this repo or provide any additional information?

[BLumia]

Ping

[MarcelHB]

I'd appreciate this fix, too.

[BLumia]

@HazardyKnusperkeks @owenca @cor3ntin sorry for the ping, kindly asking for a review. Thanks in advance!

[cor3ntin]

Would it not be be better to transform the result to a windows path, such as with pathlib.WindowsPath(toplevel).str() ?

[owenca]

I personally use Cygwin and it's git understands unix paths is that not the case for msys2

Git can understand (and will return a) unix path, but it seems MSYS2's python won't accept unix path. It fails at os.chdir(toplevel), and toplevel is a unix path.

Isn't this a Python/MSYS2 bug? Is it related to #114078?

[BLumia]

Isn't this a Python/MSYS2 bug?

I'm not sure if it should be considered as a MSYS2 bug.

IMO it should be expected that run('git', 'rev-parse', '--show-toplevel') will return an UNIX path since it will call MSYS2's git. And according to their documentation, it also seems it's intended to let python accept Windows-style path but with MSYS2's separator (see python -m site's output under MSYS2 for example).

Is it related to #114078?

Sort of, but #114078 won't fix this issue since MSYS2's shell will invoke git-clang-format instead of git-clang-format.bat

[BLumia]

Isn't this a Python/MSYS2 bug?

I'm not sure if it should be considered as a MSYS2 bug.

I also created a discussion thread, see: msys2/MSYS2-packages#4982