Skip to content

Conversation

ethomson
Copy link
Member

@ethomson ethomson commented Feb 18, 2024

The gift that keeps on giving — it will rival core.autocrlf for user disappointment.

A number of changes here:

  1. We ensure that we cope correctly with %(prefix) in front of paths.
  2. We stop requiring that UNC paths start with %(prefix)
  3. Report the directory path without a trailing slash
  4. Ensure that root paths are correctly handled
  5. Ensure that the repository path is canonicalized on Windows — we were already doing this on POSIX, and this aligns with Git itself
  6. To cope with this, we need to start canonicalizing the clar sandbox paths so that the repository path is deterministic and we can continue joining paths simply

@ethomson ethomson force-pushed the ethomson/safedirectory branch 4 times, most recently from a7f3a5c to 028b6dd Compare February 19, 2024 15:41
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.
@ethomson ethomson force-pushed the ethomson/safedirectory branch from 028b6dd to 9e53232 Compare February 19, 2024 16:41
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.
@ethomson ethomson force-pushed the ethomson/safedirectory branch from 9e53232 to 9c31bd0 Compare February 19, 2024 17:09
@ethomson
Copy link
Member Author

Fixes #6737

@csware
Copy link
Contributor

csware commented Feb 20, 2024

As another improvement, the error message should provide the %(prefix)/ as vanilla Git does.

A fix would also be nice to have for 1.7.3

@ethomson
Copy link
Member Author

As another improvement, the error message should provide the %(prefix)/ as vanilla Git does.

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.

@ethomson ethomson merged commit caf1747 into main Feb 20, 2024
@ethomson ethomson deleted the ethomson/safedirectory branch February 20, 2024 16:34
@ethomson
Copy link
Member Author

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.

@csware
Copy link
Contributor

csware commented Feb 20, 2024

I'm testing the main branch right now...

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

Successfully merging this pull request may close these issues.

2 participants