forked from git/git
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
path.c: translate WSL and Cygwin paths when resolving worktrees #6234
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
Open
johnnyshields
wants to merge
1
commit into
git-for-windows:main
Choose a base branch
from
johnnyshields:wsl-mnt-path-translation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+191
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| #!/bin/sh | ||
|
|
||
| test_description='translate WSL/Cygwin /mnt/<x>/ paths in worktree gitfiles | ||
|
|
||
| Verify that `git worktree add` artefacts written from inside WSL2 or | ||
| Cygwin/MSYS - which use POSIX-mounted paths like `/mnt/c/...` or | ||
| `/cygdrive/c/...` - are still resolvable when read back from native | ||
| Windows git. | ||
| ' | ||
|
|
||
| GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main | ||
| export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | ||
|
|
||
| . ./test-lib.sh | ||
|
|
||
| # Convert any drive-prefixed path Windows git might emit to the named | ||
| # POSIX-mount form. Handles MSYS form (/c/foo) and Windows forms | ||
| # (C:/foo and C:\foo). MINGW-only. | ||
| mount_form () { | ||
| prefix=$1 ;# /mnt or /cygdrive | ||
| path=$2 | ||
| case "$path" in | ||
| /[A-Za-z]/*) | ||
| echo "$path" | sed -E "s|^/([A-Za-z])/|$prefix/\\L\\1/|" | ||
| ;; | ||
| [A-Za-z]:/*) | ||
| echo "$path" | sed -E "s|^([A-Za-z]):/|$prefix/\\L\\1/|" | ||
| ;; | ||
| [A-Za-z]:'\'*) | ||
| echo "$path" | sed -E "s|^([A-Za-z]):.|$prefix/\\L\\1/|; s|\\\\|/|g" | ||
| ;; | ||
| *) | ||
| echo "$path" | ||
| ;; | ||
| esac | ||
| } | ||
|
|
||
| to_mnt () { | ||
| mount_form /mnt "$1" | ||
| } | ||
|
|
||
| to_cygdrive () { | ||
| mount_form /cygdrive "$1" | ||
| } | ||
|
|
||
| test_expect_success MINGW 'setup main repo' ' | ||
| git init repo && | ||
| test_commit -C repo init | ||
| ' | ||
|
|
||
| test_expect_success MINGW 'read_gitfile_gently translates /mnt/<x>/ gitdir' ' | ||
| test_when_finished "rm -rf wtlink actual" && | ||
| REAL=$(cd repo/.git && pwd) && | ||
| MNT=$(to_mnt "$REAL") && | ||
|
|
||
| # Sanity: the path must actually start with /mnt/ - if it does not, | ||
| # the host shell did not give us a path with a drive prefix and the | ||
| # rest of the test would be silently meaningless. | ||
| case "$MNT" in | ||
| /mnt/*) : ok ;; | ||
| *) BUG "to_mnt produced $MNT from $REAL" ;; | ||
| esac && | ||
|
|
||
| mkdir wtlink && | ||
| printf "gitdir: %s\n" "$MNT" >wtlink/.git && | ||
|
|
||
| (cd wtlink && git rev-parse --git-dir) >actual && | ||
| test_path_is_dir "$(cat actual)" | ||
| ' | ||
|
|
||
| test_expect_success MINGW 'read_gitfile_gently translates /cygdrive/<x>/ gitdir' ' | ||
| test_when_finished "rm -rf wtlink actual" && | ||
| REAL=$(cd repo/.git && pwd) && | ||
| CYG=$(to_cygdrive "$REAL") && | ||
|
|
||
| mkdir wtlink && | ||
| printf "gitdir: %s\n" "$CYG" >wtlink/.git && | ||
|
|
||
| (cd wtlink && git rev-parse --git-dir) >actual && | ||
| test_path_is_dir "$(cat actual)" | ||
| ' | ||
|
|
||
| test_expect_success MINGW 'read_gitfile_gently leaves /mnt/<multichar>/ alone' ' | ||
| test_when_finished "rm -rf wtlink" && | ||
| mkdir wtlink && | ||
| # "storage" is not a single drive letter, so this must not be | ||
| # translated. The path does not exist on Windows, so the open fails. | ||
| echo "gitdir: /mnt/storage/no/such/repo" >wtlink/.git && | ||
|
|
||
| test_must_fail git -C wtlink rev-parse --git-dir 2>err && | ||
| test_grep "not a git repository" err | ||
| ' | ||
|
|
||
| test_expect_success MINGW 'get_linked_worktree finds worktree recorded with /mnt/<x>/ path' ' | ||
| test_when_finished "rm -rf repo/wt repo/.git/worktrees/wt" && | ||
|
|
||
| git -C repo worktree add --detach wt && | ||
| WT_REAL=$(cd repo/wt && pwd) && | ||
| WT_MNT=$(to_mnt "$WT_REAL") && | ||
|
|
||
| # Overwrite the recorded worktree path with the WSL form, mimicking | ||
| # what `git worktree add` writes when run from inside WSL. | ||
| printf "%s/.git\n" "$WT_MNT" >repo/.git/worktrees/wt/gitdir && | ||
|
|
||
| # `git worktree list` reads that file via get_linked_worktree. | ||
| # After translation the worktree must still be reachable: it must | ||
| # NOT be flagged prunable, and a git operation inside the worktree | ||
| # directory must succeed. | ||
| git -C repo worktree list --porcelain >list && | ||
| ! grep -q "^prunable" list && | ||
| (cd "$WT_REAL" && git rev-parse --is-inside-work-tree) | ||
| ' | ||
|
|
||
| test_expect_success MINGW 'get_common_dir_noenv translates /mnt/<x>/ commondir' ' | ||
| test_when_finished "rm -rf wtdir wt actual" && | ||
|
|
||
| REAL=$(cd repo/.git && pwd) && | ||
| MNT=$(to_mnt "$REAL") && | ||
|
|
||
| # Build a synthetic linked-worktree gitdir that points at the main | ||
| # repo via a /mnt/<x>/ commondir record. | ||
| mkdir wtdir && | ||
| echo "$(cd repo && git rev-parse HEAD)" >wtdir/HEAD && | ||
| echo "$MNT" >wtdir/commondir && | ||
| printf "%s/.git\n" "$(pwd)" >wtdir/gitdir && | ||
|
|
||
| # rev-parse --git-common-dir on a checkout that points here should | ||
| # resolve through the translated commondir. | ||
| mkdir wt && | ||
| printf "gitdir: %s\n" "$(pwd)/wtdir" >wt/.git && | ||
| (cd wt && git rev-parse --git-common-dir) >actual && | ||
| test_path_is_dir "$(cat actual)" | ||
| ' | ||
|
|
||
| test_done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for Cygwin. But Git for Windows is based on MSYS2, where the drives are mapped as
/c/,/d/, etc. And by translating those always, you run now create ambiguities that cannot be overcome: In WSL,/c/is a valid path that you could now no longer access. Sure, you can argue that "nobody maps those", but here is a person speaking to you who did that in the past, and in general it is not a good sign when your code introduces ambiguities where there were none before the patch.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, WSL
/mnt/c/and Cygwin/cygwin/c/paths are unambiguous and are major wins--especially WSL--even if we can't agree what to do with MSYS2.Re: MSYS2, I can think of 4 possibilities:
/c/,/d/toC:\,D:\etc--I think this is arguably safe, see below.git config --global core.translateMsysPaths truegitdir = /c/...asC:\...and then read it back as/c/. This is defensible IMHO because MSYS2 is highly specialized to Windows, and storing paths in Windows format is not wrong, per se.Why I think MSYS2's
/c/,/d/, paths etc. are safe to alias tooRecall that two gates limit where the new logic runs:
#ifdef GIT_WINDOWS_NATIVE, so it's compiled in only on the mingw64 native build of Git for Windows. WSL git, Cygwin git, and the MSYS2-runtime git from pacman are separate builds and unaffected. Your "person who mkdir /c in WSL" continues to use a WSL git binary that has none of this code.For an ambiguity to actually bite a user, ALL of the following must coincide:
/c/within WSL Linux's root dir./c/foo, not visible to native Windows)./mnt/c/elsewhere/wt), i.e. not co-located with its parent repo, which would be exceedingly uncommon.C:/foowhose exact path mirrors/clashes with the WSL one/c/foo(otherwise its a no-op)Even with all six aligned, the failure mode is bounded: Git-for-Windows follows the translated
c:/foo, finds either nothing (and does a no-op, as it does today) or a real repo at that path that the user explicitly created. AFAIK Git has protection mechanisms that prevent worktrees from pointing to the wrong copy of their repo (based on bi-directional registration.)TLDR; I just don't see this as something that's seriously going to affect anyone, meanwhile the convenience of Windows-and-MSYS2 interop is a tangible quality-of-life win.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path
/mnt/on Linux can be anything, from a mount to a regular directory. Nothing, nothing at all guarantees that/mnt/crefers to the mountedC:drive iff this is a WSL instance. But as I had pointed out already (which you ignored), the same code runs also on regular Linux, where it definitely does not refer to a host'sC:drive because there is none.I understand that it is in fashion nowadays to double-down instead of fixing designs, but this is not going to work here. There are fundamental problems with the proposed design that won't stand a chance of being addressed merely by doubling down on a flawed idea.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on.
Yes,
/mnt/can technically be anything. However, 99% of Linux distros use/mnt/as part of the Linux FHS--even FHS-deviants like NixOS use/mnt/--and doubly-so for the subset of distros that are used on WSL.Again, this PR is for Git for Windows exclusively. It is simply saying, "If Git runs on Windows, and it sees a worktree with
gitdir = /mnt/c/foo/bar, it is reasonable to try to map it toC:\foo\bar", because in 99.99% of cases this arises from creating the worktree inside WSL, exactly as was reported in #6187.So we're talking cross-purposes here. Of course we can sit around all day and say "what ifs" i.e. "what if someone renames
/mnt/in Linux?" And in fact, those cases are fine, the behavior simply degrades to the no-op that is is today. So users who want Windows-WSL git interop need to live with not renaming/mnt/to something screwy (which by the way is going to break a whole lot more than git :)Perhaps a compromise here is to have feature flags:
git config --global core.enableWindowsWSLCompatibility truegit config --global core.enableWindowsCygwinCompatibility truegit config --global core.enableWindowsMSYSCompatibility trueThese would enable compatibility shims (
gitdirfor now, maybe other things in the future) that come with certain assumptions, i.e. that you haven't renamed/mnt/. For WSL specifically we can add the caveat about performance to the documentation.Would you be willing to accept a PR with those feature flags added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are elevating a fragile heuristic to a fundamental design decision. I find that highly unconvincing, and am unwilling to accept it into Git for Windows. Maybe you will have more luck getting it into Git for Windows via the Git project.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dscho that
/mnt/is used by 100% of WSL-available Git distros as the mountpoint dir is not a random "fragile heuristic", its a direct consequence of the FHS standard. Anyone who renames it must reasonably expect that various things will break.We are getting lost in the weeds here.
This PR is simply proposing to have Git on Windows interpret
gitdir = /mnt/c/<path>asgitdir = C:\gitdirpaths will not include/mnt//mnt/then this feature will gracefully degrade to a no-op--the exact same as it is today.I think we need to honestly ask who is Git for Windows intended for:
/mnt/?