-
Notifications
You must be signed in to change notification settings - Fork 2.5k
More safe.directory
improvements
#6739
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
Conversation
a7f3a5c
to
028b6dd
Compare
We currently only canonicalize the temp sandbox directory on macOS, which is critical since `/tmp` is really `/private/tmp`. However, we should do it everywhere, so that tests can actually expect a consistent outcome by looking at `clar_sandbox_path()`.
We may need to strip multiple slashes at the end of a path; provide a method to do so.
We may quickly want to determine if the given path is the root path ('/') on POSIX, or the root of a drive letter (eg, 'A:/', 'C:\') on Windows.
Clar handles multiple levels of hierarchy in a test name _but_ it does so assuming that there are not tests at a parent folder level. In other words, if you have some tests at path/core.c and path/win32.c, then you cannot have tests in path.c. If you have tests in path.c, then the things in path/*.c will be ignored. Move the tests in path.c into path/core.c.
028b6dd
to
9e53232
Compare
The POSIX `realpath` function canonicalizes relative paths, symbolic links, and case (on case-insensitive filesystems). For example, on macOS, if you create some file `/private/tmp/FOO`, and you call `realpath("/tmp/foo")`, you get _the real path_ returned of `/private/tmp/FOO`. To emulate this behavior on win32, we call `GetFullPathName` to handle the relative to absolute path conversion, then call `GetLongPathName` to handle the case canonicalization.
Ensure that we clean up cruft that we create for testing, so that future tests don't have troubles.
Our error messages should provide the literal path that users should add to their safe directory allowlist, which means it should not have a trailing slash.
Ensure that we can support safe.directory entries that are prefixed with '%(prefix)/'.
'%(prefix)/' as a leading path name is safe to strip on all platforms. It doesn't make sense to use on non-Windows, but supporting it there does allow us to easily dev/test.
Provide a helper method in the tests to set up the safe.directory configuration for a normal and bare repository to avoid some copy/pasta. (Some copypasta will remain, since there's customizations to the file that are not trivially abstractable.)
On Windows, you may open a repo with UNC path format -- for example \\share\foo\bar, or as a git-style path, //share/foo/bar. In this world, Git for Windows expects you to translate this to $(prefix)//share/foo/bar. We can test for that by using the fact that local drives are exposed as hidden shares. For example, C:\Foo is //localhost/C$/Foo/.
When doing directory name munging to remove trailing slashes, ensure that we do not remove the trailing slash from the path root, whether that's '/' or 'C:\'.
Recent versions of Git for Windows allow a sane UNC style path to be used directly in `safe.directory` (without needing the `%(prefix)` silliness). Match that behavior.
9e53232
to
9c31bd0
Compare
Fixes #6737 |
As another improvement, the error message should provide the A fix would also be nice to have for 1.7.3 |
Right - sorry - I went back to your PR and realized that I never actually finished typing up the comments. We need to avoid multi-line error messages since some people use them in odd places. I don't have a great idea for how to plumb this through (the directory that's problematic vs the configuration command that you would run). This feels like a good use for the "warnings API" that we keep talking about and never shipping, so let me give it some thought. |
I'm willing to back port to v1.7, but I'm also preparing to cut v1.8 if that makes more sense for you. |
I'm testing the main branch right now... |
The gift that keeps on giving — it will rival
core.autocrlf
for user disappointment.A number of changes here:
%(prefix)
in front of paths.%(prefix)