Re: [PATCH 0/6] open in-tree files with O_NOFOLLOW

[Jeff King <peff@peff.net>]

On Thu, Feb 25, 2021 at 11:25:19AM -0800, Junio C Hamano wrote:

> >   [1/6]: add open_nofollow() helper
> >   [2/6]: attr: convert "macro_ok" into a flags field
> >   [3/6]: exclude: add flags parameter to add_patterns()
> >   [4/6]: attr: do not respect symlinks for in-tree .gitattributes
> >   [5/6]: exclude: do not respect symlinks for in-tree .gitignore
> >   [6/6]: mailmap: do not respect symlinks for in-tree .mailmap
> [...]
> 
> So, I've read these changes and they all looked quite reasonable.
>
> Where do we want to go from here?
> 
> Merge it down and forget about the changes in verify_path() and fsck
> in the jk/symlinked-dotgitx-files topic?  Do we want to also cover
> the .gitmodules file with the same mechansim?

Thanks, I'm glad somebody looked at it. :)

Having pondered it, this really seems like a less risky approach than
forbidding symlinks for those paths. It is not impacting what is allowed
in Git, so nobody's repo will break. And we do not even have to worry
that our name-matching code is correct, since we are asking the OS to do
the right thing.

The biggest risk to me is that there is some hiccup with Windows: they
don't have any NOFOLLOW equivalent, and the fallback lstat() is somehow
slower than a real open(). But that seems unlikely (I could well believe
that lstat+open is slower for them, but that only matters if you
actually have a huge number of gitattributes files to open, in which
case you're probably spending your time reading and parsing them
anyway).

So I'd be happy to proceed with this and throw out
jk/symlinked-dotgitx-files. We can salvage the fsck checks from there,
leaving them as warnings, but it's not urgent (they are just
informational as "btw, this symlink won't work like you think it will").
So I'd probably do that as a separate series.

We could also cover .gitmodules, but I'm inclined not to. It's work and
risk to convert it to this form, for little gain. Nobody seems to have
been bothered by the symlink restriction. I guess it would take some
is_ntfs_gitmodules() checks out of the verify_path() code, which could
have some small performance benefit. But we'd definitely still need to
identify .gitmodules files in fsck, because we have to check over their
actual contents.

So I'd likewise be content to leave that to another series (or never if
nobody sees an upside to it).

-Peff