Jeff King [off-list ref] writes:
On Tue, Feb 16, 2021 at 07:48:23AM -0500, Jeff King wrote:
quoted
I am beginning to wonder if just opening them all with O_NOFOLLOW (and a
hacky 2-syscall fallback for portability) might be less ugly than all of
this.
So here's what that series might look like. It would replace all of this
verify_path() stuff entirely (and fsck, though we might want to add
detection to fsck just as an informational thing). It gives similar
protections, and would similarly force people using an in-tree symlink
to stop doing that. But it makes it much less of a pain to do so,
because they can still check out, etc; the symlinks just won't be
followed.
I think we could even use the same technique to roll back the
restrictions on .gitmodules being a symlink. That one makes me a bit
more nervous, just because we also write it. I _think_ that might be
safe, because we only do so using a temp file and rename(), which should
replace the symlink.
[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
attr.c | 60 +++++++++++++++++++++++++--------------
builtin/sparse-checkout.c | 8 +++---
dir.c | 21 ++++++++++----
dir.h | 3 +-
git-compat-util.h | 7 +++++
mailmap.c | 22 ++++++++++----
t/t0003-attributes.sh | 36 +++++++++++++++++++++--
t/t0008-ignores.sh | 34 ++++++++++++++++++++++
t/t4203-mailmap.sh | 31 ++++++++++++++++++++
wrapper.c | 16 +++++++++++
10 files changed, 197 insertions(+), 41 deletions(-)
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.
On Thu, Feb 25, 2021 at 11:25:19AM -0800, Junio C Hamano wrote:
quoted
[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