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

Add UNC path prefix before adding to safe directory list #14368

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

niik
Copy link
Member

@niik niik commented Apr 14, 2022

Description

This matches the fix for Git CLI over at git-for-windows/git#3791. Whenever we encounter a UNC path (i.e path starting with //) from Git's unsafe error warning we'll prepend %(prefix)/ to get Git to ultimately resolve it to the same path as the repository.

So when we encounter an error message like this:

unsafe repository ('//wsl/something' is owned by someone else)
To add an exception for this directory, call:
    git config --global --add safe.directory %(prefix)//wsl/something

We'll extract //wsl/something from the first line and prepend it with %(prefix)/. The reason we're not just plucking the path from the last line (i.e. the "example") is that that path will be quoted for use in a shell whereas we'll rely on Node to do our quoting for us.

Note that this will not have any effect until we're able to upgrade to a version of Git which supports %(prefix) (2.34.0 or higher). We're working with the fine folks on the Git side to hopefully get our hands on a Git for Windows version we can sign and ship today.

Release notes

Notes: [Fixed] Support adding repositories located on network drives (such as NAS, WSL, SMB etc) to the list of safe directories in Git

cc @vdye @derrickstolee

@niik niik requested a review from tidy-dev April 14, 2022 12:38
Comment on lines +169 to +177
export async function addSafeDirectory(path: string) {
// UNC-paths on Windows need to be prefixed with `%(prefix)/`, see
// https://github.com/git-for-windows/git/commit/e394a16023cbb62784e380f70ad8a833fb960d68
if (__WIN32__ && path[0] === '/') {
path = `%(prefix)/${path}`
}

addGlobalConfigValueIfMissing('safe.directory', path)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chiming in from Git's perspective to say that this looks right to me. Thanks!

Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ From my understanding of following the threads, this makes sense. 🙂

@niik niik merged commit dac424d into development Apr 14, 2022
@niik niik deleted the unc-path-allow-list branch April 14, 2022 16:58
@mdell-seradex
Copy link

I was just looking at commit 862f427.
It looks to have a minor mistake in the comment to me.
if the path is owner by a different user than the current Specifically the is owner part.

@fabiofdsantos
Copy link

Thaaaanks 👍🏻

@mdell-seradex
Copy link

@raywhite714 I don't think you wanted to post that.

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.

5 participants