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

From: Jeff King <peff@peff.net>
Date: Nov 9, 2016, 16:45 UTC
Message-ID: <20161109164539.fn4c6fj3dyvceb7m@sigill.intra.peff.net>
In-Reply-To: <CACsJy8ASMBk+Yak7LyybANFYkoU_Poi1ZGY=ufKtq1vSkoYCXQ@mail.gmail.com>

On Wed, Nov 09, 2016 at 04:22:12PM +0700, Duy Nguyen wrote:

Show 11 quoted lines

> > Symlinks are likewise tricky.  If we see that a symlink points to
> > "foo/../bar", then we don't know if it leaves the repository unless we
> > also look at "foo" to see if it is also a symlink. So you really end up
> > having to resolve the symlink yourself (and when checking out multiple
> > files, there's an ordering dependency).
> 
> We do have this dependency problem right now (e.g. files A and
> .gitattributes are checked out at the same time and .gitattributes has
> some attribute on A). It looks like we resolve it by reading the index
> version at checkout time. We probably can do the same for gitattribute
> symlinks.

Right, but then we can't use filesystem functions like realpath() to do the lookup. I guess we could do a pass after the checkout is done to "fix" any out-of-tree symlinks we just created.

This is exactly the sort of complexity I was trying to avoid with my original series. :)

If that isn't an option, I think I prefer something like the core.saneSymlinks approach I mentioned. It has the additional bonus of protecting not just git commands, but other commands that might inspect the filesystem.

Show 10 quoted lines

> > So one reasonable fix might be to have a config option like
> > "core.saneSymlinks" that enforces both of those rules for _all_ symlinks
> > that we checkout to the working tree. And it could either refuse to
> > check them out, or replace them with a file containing the symlink
> > content (as we do on systems that don't support symlinks, IIRC).
> 
> I wonder if anyone want core.saneSymlinks on, but they have some links
> that do not meet the above checks and still want to follow them
> anyway. One way to add such an exception is mark the path with an
> attribute "follow". Yeah I have a dependency loop :(

That could come later if somebody wants it, I think (especially if the config option is not on by default). I have a feeling that callers will either care about out-of-tree symlinks or not. Trusting the repository to say "but these ones are OK" doesn't work for the paranoid ones, and everybody else just assumes the repository is sane.

-Peff