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

>> Observations:
>>
>> - since some widely used repositories have .gitignore symlinks, I
>>   think we can't forbid it in fsck, alas
>
> I am a little puzzled here. You said you had the fsck checks for the
> last year, so why did they just come up now? I guess nobody sets
> transfer.fsckObjects, and because you were testing only with clients,
> your server implementation didn't reject pushes?
>
> I agree it's annoying for them if they fail fsck, but it's not entirely
> a show-stopper. There are config options for fine-tuning what you're
> willing to enforce or ignore. But they of course are also annoying to
> use, because every receiver of a transfer needs to set them (on GitHub,
> for example, you have to email Support).

>> - 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
>
> I actually wrote a patch several years ago for checking symlinks (not
> just these ones, but _any_ symlinks in the repo, but of course it would
> be easy to limit it more). It's included at the end of this mail. It's
> been part of my daily build for many years, so I'm confident it doesn't
> crash or have other bad behavior. But it's possible the logic for what
> it catches is faulty.

>> - we could use a clearer error message than "invalid path".
>
> That part is tricky. The "invalid path" error comes from the caller of
> verify_path(), and we have no way to pass back an intelligent error
> there. We can call error() ourselves, of course. That adds an extra line
> of output, but it's rare enough for verify_path() to fail that it's
> likely OK. However, I would worry that some callers might be surprised
> by it producing output at all.
>
> An alternative is letting the caller pass in a strbuf that we fill out
> with an extra error string.

>> 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?).
>
> I suspect it's too late for "fail entirely". We may have already written
> to the filesystem, and rolling back is difficult and error-prone. In
> general I'd expect to checkout what we can, produce errors for the rest,
> and let the user work from there with "git status".
>
> But I may be wrong. The problem is loading the value into the index, not
> writing it to the filesystem. So perhaps the relevant code paths load
> the index fully before writing out anything to the filesystem, and it's
> easy to rollback. But I imagine they are using unpack-trees' flag to
> update the filesystem, and I assume that checks out as it loads entries
> (but I didn't confirm).