Re: [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning

From: Jeff King <peff@peff.net>
Date: Feb 16, 2021, 01:16 UTC
Message-ID: <YCsc0OePtrotjeg5@coredump.intra.peff.net>
In-Reply-To: <87y2foaltl.fsf@evledraar.gmail.com>

On Tue, Feb 16, 2021 at 01:38:30AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 18 quoted lines

> On Tue, Feb 16 2021, Jeff King wrote:
> 
> > While there are some minor security implications to having these files
> > be symlinks, this is out-weighed by the inconvenience of blocking
> > historical commits in some projects that might include them.
> 
> Digging up the relevant thread that's the projects noted at
> https://lore.kernel.org/git/20201027033518.GH2645313@google.com/ ?
> 
> I cloned the openmrn.git repository noted there, and checkout dies with:
> 
>     error: invalid path 'applications/clinic_app/targets/linux.x86/.gitignore'
>     fatal: Could not reset index file to revision 'HEAD'.
> 
> I'm running a recent-ish snapshot of next at d98b1dd5eaa7, so with your
> verify_path() change in current "seen".
> 
> So this series changes nothing about the checkout, just the fsck check?

Right.

> I see there's your
> https://lore.kernel.org/git/20201027075853.GH3005508@coredump.intra.peff.net/#t
> to improve the !!symlink() codepath in apply.c

That's a somewhat orthogonal approach, that tries to look at out-of-tree symlinks (rather than banning all symlinks for these files).

I think it's worth banning them all, though; they don't actually work as you'd expect (i.e., whenever we read them in-repo the symlinks do nothing).

> Still, it seems like a rather jarring gap in implementation to just warn
> about this in fsck for the benefit of e.g. server operations, but then
> hard die on the current client.

It's not for the benefits of servers. It's because the solution for them is to stop symlinking, which fixes clone/checkout of new commits. But they'll still have those old trees hanging around in their history. If everybody rejects them, then it becomes difficult to push/fetch at all.

That said, they'd probably want to checkout those old commits, too. So we probably do need a config override, even if it's a broad one ("trust me, this repo is OK, just allow symlinks for these special files").

Show 7 quoted lines

> There seems to be no way around that hard die, and both repos in that
> report are ones that are just symlinking .gitignore to a
> ../somedir/.gitignore deep in their own tree.
> 
> So aren't we both making the fsck check too loose and the client too
> strict? Would anyone care if this was an error on fsck if we did the "is
> outside repo?" check?

An outside-the-repo check would probably be less invasive, but:

  - it still allows broken setups

  - it's significantly more complex. I know that the implementation I
    showed errs on the side of complaining in at least some cases
    (because it doesn't know if intermediate paths are themselves
    symlinks). But I'd worry there are also cases where it covers too
    little, nullifying the protection.

-Peff