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

From: Jeff King <peff@peff.net>
Date: Feb 26, 2021, 06:35 UTC
Message-ID: <YDiWs6yyv3U9YvC2@coredump.intra.peff.net>
In-Reply-To: <xmqqh7m0uezk.fsf@gitster.g>

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

Show 15 quoted lines

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