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

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:
> On Mon, Oct 26, 2020 at 08:35:18PM -0700, Jonathan Nieder wrote:

>> 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).

Yes.  And to reiterate the point a little: the reason nobody sets
transfer.fsckObjects is that we haven't made it easy to distinguish
between "hard error, should never be overridden" checks (like
BAD_PARENT_SHA1), "new tools shouldn't write these but they exist in
important repos like perl.git and anything consuming Git repositories
needs to cope with them" (like MISSING_SPACE_BEFORE_DATE from some
commits' concatenated authors), and so on.

> So I won't be too devastated to remove the symlink checks, or possibly
> downgrade them to purely warnings (or "info"; the naming in fsck.c is
> confusing, because the transfer operations take even warnings as fatal.
> I suspect we could do with some cleanup there).

Downgrading the .gitignore check to warning sounds okay.  .gitmodules
would still want to be an error, of course.

>> - 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.
[...]
> Here's the patch.

Nice!  I'll try to find time to experiment with this.

[...]
>> - 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.

I think passing in a struct to govern error handling sounds nice.
Alternatively, this might suggest that verify_path is not the right
place for the user-facing check: it's useful because it's exhaustive,
but it may be that we can also catch the same issue earlier and
produce a nicer diagnostic.

>> 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).

Right, this would require taking two passes.  I think we do already
perform two passes (first deletions, then additions); I'll have to
look a little more closely to see whether this is straightforward to
do.

I do still like the idea of not respecting .gitignore and
.gitattributes symlinks, by the way.  What the experiences upthread
tell me is just that users need better facilities for repairing
existing repositories.

Thanks,
Jonathan