Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

From: Jeff King <peff@peff.net>
Date: Nov 7, 2016, 21:10 UTC
Message-ID: <20161107211010.xo3243egggdgscou@sigill.intra.peff.net>
In-Reply-To: <CACsJy8AO2KtpxFu=wRjW1DoCA9bfpF1VoJUn__2ib-ML0XT66w@mail.gmail.com>

On Mon, Nov 07, 2016 at 05:03:42PM +0700, Duy Nguyen wrote:

Show 15 quoted lines

> On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:
> > The attributes system may sometimes read in-tree files from
> > the filesystem, and sometimes from the index. In the latter
> > case, we do not resolve symbolic links (and are not likely
> > to ever start doing so). Let's open filesystem links with
> > O_NOFOLLOW so that the two cases behave consistently.
> 
> This sounds backward to me. The major use case is reading
> .gitattributes on worktree, which follows symlinks so far. Only
> git-archive has a special need to read index-only versions. The
> worktree behavior should influence the in-index one, not the other way
> around. If we could die("BUG" when git-archive is used on symlinks
> (without --worktree-attributes). If people are annoyed by it, they can
> implement symlink folllowing (to another version in index, not on
> worktree).

I agree it feels a little backwards, as we are choosing the lowest-common denominator of the two (so it would be reasonable to have the in-index version follow symbolic links, or at least do so on platforms where core.symlinks is true).

And I'll admit my main motivation is not that index/filesystem parity, but rather just that:

  git clone git://host.com/malicious-repo.git
  git log

might create and read symlinks to arbitrary files on the cloner's box. I'm not sure to what degree to be worried about that. It's not like you can't make other arbitrary symlinks which are likely to be read if the user actually starts looking at checked-out files. It's just that we usually try to make a clone+log of a malicious repository safe.

That being said, I'm not convinced that reading the index version of .gitattributes and .gitignore is just an optimization. Don't we read the destination attributes when checking out a new tree? And doesn't merge need to use the in-index version when we see conflicts?

So I was hoping that this was a practice that is unlikely to be in wide use, and that we could simply ban in order to keep the attribute and ignore code simpler and safer, both now and if we change them to do more in-index lookups.

-Peff