Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Blake Burkhart" <bburky@bburky.com>, git <git@vger.kernel.org>
Subject: Re: [PATCH 0/6] open in-tree files with O_NOFOLLOW
Date: Fri, 26 Feb 2021 01:35:31 -0500
Message-ID: <YDiWs6yyv3U9YvC2@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqh7m0uezk.fsf@gitster.g>

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

  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-13 17:49 Limited local file inclusion with .mailmap symlinks and git-archive Blake Burkhart
2021-02-15 23:17 ` Jeff King
2021-02-15 23:18   ` [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning Jeff King
2021-02-16  0:38     ` Ævar Arnfjörð Bjarmason
2021-02-16  1:16       ` Jeff King
2021-02-16  1:56         ` Junio C Hamano
2021-02-16 12:54           ` Jeff King
2021-02-16 12:48         ` Jeff King
2021-02-16 14:43           ` [PATCH 0/6] open in-tree files with O_NOFOLLOW Jeff King
2021-02-16 14:44             ` [PATCH 1/6] add open_nofollow() helper Jeff King
2021-02-16 14:54               ` Jeff King
2021-02-16 15:44                 ` Taylor Blau
2021-02-16 16:02                   ` Jeff King
2021-02-16 16:07                     ` Taylor Blau
2021-02-16 16:11                       ` Taylor Blau
2021-02-16 16:19                         ` Jeff King
2021-02-16 14:44             ` [PATCH 2/6] attr: convert "macro_ok" into a flags field Jeff King
2021-02-16 14:44             ` [PATCH 3/6] exclude: add flags parameter to add_patterns() Jeff King
2021-02-16 14:44             ` [PATCH 4/6] attr: do not respect symlinks for in-tree .gitattributes Jeff King
2021-02-16 14:44             ` [PATCH 5/6] exclude: do not respect symlinks for in-tree .gitignore Jeff King
2021-02-16 14:44             ` [PATCH 6/6] mailmap: do not respect symlinks for in-tree .mailmap Jeff King
2021-02-16 14:57               ` Jeff King
2021-02-25 19:25             ` [PATCH 0/6] open in-tree files with O_NOFOLLOW Junio C Hamano
2021-02-26  6:35               ` Jeff King [this message]
2021-02-15 23:19   ` [PATCH 2/2] disallow symlinked .mailmap files Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YDiWs6yyv3U9YvC2@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=bburky@bburky.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git