Hi, Jeff King wrote: > However, it's still a reasonable idea to forbid symlinks for these > files: > > - As noted, they can still be used to read out-of-repo files (which is > fairly restricted, but in some circumstances you can probe file > content by speculatively creating files and seeing if they get > ignored) > > - They don't currently behave well in all cases. We sometimes read > these files from the index, where we _don't_ follow symlinks (we'd > just treat the symlink target as the .gitignore or .gitattributes > content, which is actively wrong). > > This patch forbids symlinked versions of these files from entering the > index. We already have helpers for obscured forms of the names from > e7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11) and > 0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which > were done as part of the series touching .gitmodules. Thanks again for this. Since this patch has been in "next", we've gotten a little experience of it at Google. We've been running with the fsck check for a while (more than a year), but not the verify_dotfile check. The verify_dotfile check didn't trigger for anyone with .gitattributes, but it hit several people for .gitignore. Some examples that users have mentioned: Before https://android-review.googlesource.com/c/platform/tools/test/connectivity/+/1462771/ Android used a .gitignore symlink for two directories that had similar gitignore requirements. Diagnosing the error was confusing for them, especially because the "repo" wrapper tool produced messages like error.GitError: Cannot initialize work tree for platform/tools/test/connectivity Eventually someone manually ran $ git add --a error: invalid path 'acts_tests/.gitignore' error: unable to add 'acts_tests/.gitignore' to index fatal: adding files failed which helped them realize it was a git issue and helped me point them to the need to replace the symlink with a plain file. As another example, a user working with the https://github.com/bakerstu/openmrn.git repository noticed "git checkout" commands failing. In this user's case, the checkout failed part-way through, producing confusing behavior ("git status" showing entries missing from the index). When I tried to reproduce this, I wasn't able to clone the repository at all because it failed fsck; after disabling transfer.fsckObjects, I still wasn't able to check out HEAD. Observations: - since some widely used repositories have .gitignore symlinks, I think we can't forbid it in fsck, alas - it would be useful to be able to check whether these symlinks would not escape the worktree, for a more targeted check. It might be nice to even respect these settings when they would not escape the worktree, but not necessarily - we could use a clearer error message than "invalid path". There's some room for improvement in "git checkout"'s error handling, too --- I think my ideal would be if the operation would fail entirely, with an advice message describing a checkout command that would succeed (But how do I checkout another commit while excluding some files? Should it suggest a sparse checkout?). That's all I have time for today --- will revisit again tomorrow. Thoughts? Thanks, Jonathan
