Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore

[Jonathan Nieder <jrnieder@gmail.com>]

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