Re: [PATCH v3] Demonstrate bugs when a directory is replaced with a symlink

Linus Torvalds [off-list ref] writes:

This patch should fix the 'checkout' issue.

I made it use a new generic helper function ("check_path()"), since there 
are other cases like this that use just 'lstat()', and I bet we want to 
change that.

The 'merge' issue is different, though: it's not due to a blind 'lstat()', 
but due to a blind 'unlink()' done by 'remove_path()'. I think 
'remove_path()' should be taught to look for symlinks, and remove just the 
symlink - but that's a bit more work, especially since the symlink cache 
doesn't seem to expose any way to get the "what is the first symlink path" 
information.

I think this is a good thing to do regardless, but the James's "checkout"
test fails for an unrelated reason.

The tree has

        120000 blob a36b773	a/b		-> b-2
        100644 blob e69de29	a/b-2/c/d
        100644 blob e69de29	a/x

checked out, and wants to switch to 

        100644 blob e69de29	a/b-2/c/d
        100644 blob e69de29	a/b/c/d
        100644 blob e69de29	a/x

checkout_entry() is called to check out "a/b/c/d".  If "a/b" symlink
were still there, the lstat() you fixed will be fooled.

But in James's test, because the symlink "a/b" is tracked in the
switched-from commit and is being obliterated by switching to a tree that
has a directory there, we (should) have called deleted_entry() on a/b to
mark it for removal, and inside check_updates() before going into the loop
to call checkout_entry(), we would have already removed the symlink "a/b"
that is going away inside unlink_entry().

The problem is that has_symlink_or_noent_leading_path() called from there
lies, without Kjetil's fix c52dc70 (lstat_cache: guard against full match
of length of 'name' parameter, 2009-06-14) that is in 'pu'.

If the original tree in the test did not have "a/b" tracked, but has an
untracked symlink "a/b" that points at b-2, then "a/b" will stay in the
work tree when the codepath your patch touches is reached, and the problem
will be demonstrated. Your patch will fix that issue.

So both fixes are necessary, and we need a separate test to illustrate
what your patch fixes.

I'll push out some updates.