More symlink/directory troubles

More symlink/directory troubles

[James Pickens]

This is a follow up to the original thread and patch at
http://article.gmane.org/gmane.comp.version-control.git/122297.  There was
a bug report about problems when a directory is replaced with a symlink.  I
said that the patch fixed the bug for me, but I didn't test thoroughly
enough, because it turns out there are 3 bugs, and the patch only fixed one
of them.

I am sending some test scripts to demonstrate the bugs in hopes of spurring
some activity here.  I think the convention is to use test_expect_failure
for this sort of test, so that's what I did.

Unfortunately I have very little time in the next 2 weeks, so I probably
won't be able to do much more than send the tests, and fix them up if
necessary.  In 2 weeks I can take a look at the C code, if it isn't already
fixed by then.

James

[PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.

[James Pickens]

This test creates two directories, a/b and a/b-2, then replaces a/b with
a symlink to a/b-2, then merges that change into another branch that
contains an unrelated change.

There are two bugs:
1. 'git checkout' wrongly deletes work tree file a/b-2/d.
2. 'git merge' wrongly deletes work tree file a/b-2/d.

Signed-off-by: James Pickens <james.e.pickens@intel.com>
---
 t/t6035-merge-dir-to-symlink.sh |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100755 t/t6035-merge-dir-to-symlink.sh

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
new file mode 100755
index 0000000..926c8ed
--- /dev/null
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='merging when a directory was replaced with a symlink'
+. ./test-lib.sh
+
+test_expect_success setup '
+	mkdir -p a/b/c a/b-2/c &&
+	> a/b/c/d &&
+	> a/b-2/c/d &&
+	> a/x &&
+	git add -A &&
+	git commit -m base &&
+	rm -rf a/b &&
+	ln -s b-2 a/b &&
+	git add -A &&
+	git commit -m "dir to symlink"
+'
+
+test_expect_failure 'checkout should not delete a/b-2/c/d' '
+	git checkout -b temp HEAD^ &&
+	test -f a/b-2/c/d
+'
+
+test_expect_failure 'merge should not delete a/b-2/c/d' '
+	echo x > a/x &&
+	git add a/x &&
+	git commit -m x &&
+	git merge master &&
+	test -f a/b-2/c/d
+'
+
+test_done
-- 
1.6.2.5.1

[PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.

[James Pickens]

This test creates two directories, a/b and a/b-2, replaces a/b-2 with a
symlink to a/b, then merges that change into another branch that
contains unrelated changes.  Since the changes are unrelated, the merge
should be free of conflicts, but 'git merge' gives a file/directory
conflict.

Note that this test is almost identical to t6035, except that instead of
replacing a/b with a symlink, it replaces a/b-2 with a symlink.  This
test results in a merge conflict, whereas t6035 does not.

Signed-off-by: James Pickens <james.e.pickens@intel.com>
---
 t/t6036-merge-dir-to-symlink.sh |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100755 t/t6036-merge-dir-to-symlink.sh

diff --git a/t/t6036-merge-dir-to-symlink.sh b/t/t6036-merge-dir-to-symlink.sh
new file mode 100755
index 0000000..020db7c
--- /dev/null
+++ b/t/t6036-merge-dir-to-symlink.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='merging when a directory was replaced with a symlink'
+. ./test-lib.sh
+
+test_expect_success setup '
+	mkdir -p a/b/c a/b-2/c &&
+	> a/b/c/d &&
+	> a/b-2/c/d &&
+	> a/x &&
+	git add -A &&
+	git commit -m base &&
+	rm -rf a/b-2 &&
+	ln -s b a/b-2 &&
+	git add -A &&
+	git commit -m "dir to symlink"
+'
+
+test_expect_failure 'checkout should not delete a/b/c/d' '
+	git checkout -b temp HEAD^ &&
+	test -f a/b/c/d
+'
+
+test_expect_failure 'merge should not have conflicts' '
+	echo x > a/x &&
+	git add a/x &&
+	git commit -m x &&
+	git merge master'
+
+test_done
-- 
1.6.2.5.1

Re: [PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.

[Michael J Gruber]

James Pickens venit, vidit, dixit 29.07.2009 00:13:
> This test creates two directories, a/b and a/b-2, replaces a/b-2 with a
> symlink to a/b, then merges that change into another branch that
> contains unrelated changes.  Since the changes are unrelated, the merge
> should be free of conflicts, but 'git merge' gives a file/directory
> conflict.
> 
> Note that this test is almost identical to t6035, except that instead of
> replacing a/b with a symlink, it replaces a/b-2 with a symlink.  This
> test results in a merge conflict, whereas t6035 does not.

In fact they are identical: Exchange b for b-2 and vice versa everywhere
and you get the same test, except for the fact that in 1/2 you "test -f"
in the last step. But I'm sure that test fails at the merge step already
(because of a dirty worktree), doesn't it? You should see this when
running the test with -d/-v. (I'm guessing, I haven't run your test.)

> 
> Signed-off-by: James Pickens <james.e.pickens@intel.com>
> ---
>  t/t6036-merge-dir-to-symlink.sh |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
>  create mode 100755 t/t6036-merge-dir-to-symlink.sh
> 
> diff --git a/t/t6036-merge-dir-to-symlink.sh b/t/t6036-merge-dir-to-symlink.sh
> new file mode 100755
> index 0000000..020db7c
> --- /dev/null
> +++ b/t/t6036-merge-dir-to-symlink.sh
> @@ -0,0 +1,30 @@
> +#!/bin/sh
> +
> +test_description='merging when a directory was replaced with a symlink'
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	mkdir -p a/b/c a/b-2/c &&
> +	> a/b/c/d &&
> +	> a/b-2/c/d &&
> +	> a/x &&
> +	git add -A &&
> +	git commit -m base &&
> +	rm -rf a/b-2 &&
> +	ln -s b a/b-2 &&
> +	git add -A &&
> +	git commit -m "dir to symlink"
> +'
> +
> +test_expect_failure 'checkout should not delete a/b/c/d' '
> +	git checkout -b temp HEAD^ &&
> +	test -f a/b/c/d
> +'
> +
> +test_expect_failure 'merge should not have conflicts' '
> +	echo x > a/x &&
> +	git add a/x &&
> +	git commit -m x &&
> +	git merge master'
> +
> +test_done

As in 1/2, I think the first expect_failure leaves a dirty/unexpected
worktree (d missing) which causes the merge failure in the last step.

Michael

RE: [PATCH 2/2] Demonstrate merge failure when a directory is replaced with a symlink.

[Pickens, James E]

On Wed, Jul 29, 2009, Michael J Gruber<git@drmicha.warpmail.net> wrote:
> In fact they are identical: Exchange b for b-2 and vice versa everywhere
> and you get the same test, except for the fact that in 1/2 you "test -f"
> in the last step. But I'm sure that test fails at the merge step already
> (because of a dirty worktree), doesn't it? You should see this when
> running the test with -d/-v. (I'm guessing, I haven't run your test.)

<snip>

> As in 1/2, I think the first expect_failure leaves a dirty/unexpected
> worktree (d missing) which causes the merge failure in the last step.

That's true, but this test still fails even after applying Kjetil's patch
that fixes the first problem (so the work tree is clean before 'git merge'
is run).

James

Re: [PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.

[Michael J Gruber]

James Pickens venit, vidit, dixit 29.07.2009 00:13:
> This test creates two directories, a/b and a/b-2, then replaces a/b with
> a symlink to a/b-2, then merges that change into another branch that
> contains an unrelated change.
> 
> There are two bugs:
> 1. 'git checkout' wrongly deletes work tree file a/b-2/d.
> 2. 'git merge' wrongly deletes work tree file a/b-2/d.
> 
> Signed-off-by: James Pickens <james.e.pickens@intel.com>
> ---
>  t/t6035-merge-dir-to-symlink.sh |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
>  create mode 100755 t/t6035-merge-dir-to-symlink.sh
> 
> diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
> new file mode 100755
> index 0000000..926c8ed
> --- /dev/null
> +++ b/t/t6035-merge-dir-to-symlink.sh
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='merging when a directory was replaced with a symlink'
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	mkdir -p a/b/c a/b-2/c &&
> +	> a/b/c/d &&
> +	> a/b-2/c/d &&
> +	> a/x &&
> +	git add -A &&
> +	git commit -m base &&
> +	rm -rf a/b &&
> +	ln -s b-2 a/b &&
> +	git add -A &&
> +	git commit -m "dir to symlink"
> +'
> +
> +test_expect_failure 'checkout should not delete a/b-2/c/d' '
> +	git checkout -b temp HEAD^ &&
> +	test -f a/b-2/c/d
> +'
> +
> +test_expect_failure 'merge should not delete a/b-2/c/d' '
> +	echo x > a/x &&
> +	git add a/x &&
> +	git commit -m x &&
> +	git merge master &&
> +	test -f a/b-2/c/d
> +'
> +
> +test_done

Isn't the failure of the second test caused by that of the first one?
a/b-2/c/d is gone from the worktree, and master does not touch it, so
the merge leaves the worktree version (non-existent) as is.

Michael

Re: [PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> +test_expect_failure 'checkout should not delete a/b-2/c/d' '
>> +	git checkout -b temp HEAD^ &&
>> +	test -f a/b-2/c/d
>> +'
>> +
>> +test_expect_failure 'merge should not delete a/b-2/c/d' '
>> +	echo x > a/x &&
>> +	git add a/x &&
>> +	git commit -m x &&
>> +	git merge master &&
>> +	test -f a/b-2/c/d
>> +'
>> +
>> +test_done
>
> Isn't the failure of the second test caused by that of the first one?
> a/b-2/c/d is gone from the worktree, and master does not touch it, so
> the merge leaves the worktree version (non-existent) as is.

To avoid that impression the second test should probably have been written
to start from a clean slate, using "reset --hard" or something.

Kjetil's patch actually fixes the first one, but the second one will still
show breakage.

I wonder if the breakage is in recursive merge or in the generic read-tree
three-way merge code.  I highly suspect that using "git merge -s resolve"
would make the test pass.  Historically recursive merge is known to be
careless in many corner cases.

RE: [PATCH 1/2] Demonstrate bugs when a directory is replaced with a symlink.

[Pickens, James E]

On Wed, Jul 29, 2009, Junio C Hamano<gitster@pobox.com> wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> Isn't the failure of the second test caused by that of the first one?
>> a/b-2/c/d is gone from the worktree, and master does not touch it, so
>> the merge leaves the worktree version (non-existent) as is.
>
> To avoid that impression the second test should probably have been written
> to start from a clean slate, using "reset --hard" or something.

I'll send a new patch shortly that combines the two tests into one and
includes the "reset --hard".

> Kjetil's patch actually fixes the first one, but the second one will still
> show breakage.
>
> I wonder if the breakage is in recursive merge or in the generic read-tree
> three-way merge code.  I highly suspect that using "git merge -s resolve"
> would make the test pass.  Historically recursive merge is known to be
> careless in many corner cases.

You're right, using the resolve strategy does make the test pass.

James

[PATCH v2] Demonstrate bugs when a directory is replaced with a symlink

[Pickens, James E]

This test creates two directories, a/b and a/b-2, then replaces a/b with
a symlink to a/b-2, then merges that change into another branch that
contains an unrelated change.

There are two bugs:
1. 'git checkout' wrongly deletes work tree file a/b-2/d.
2. 'git merge' wrongly deletes work tree file a/b-2/d.

The test goes on to create another branch in which a/b-2 is replaced
with a symlink to a/b (i.e., the reverse of what was done the first
time), and merge it with the "unrelated changes" branch.

There's another bug:
3. Since the changes are unrelated, the merge should be clean, but git
   reports a conflict.

Note that using the resolve strategy instead of recursive makes the
second bug go away, but not the third one.

Signed-off-by: James Pickens <james.e.pickens@intel.com>
---
This version combines the two tests into one, and cleans up between steps
so that the early failures don't affect the later tests.

This time I include the bare minimum commands inside the
test_expect_failure calls, which seems like the right thing to do, since
the other commands are expected to "succeed" (exit code of 0).

BTW I'm sending this patch using 'git format-patch' + Outlook instead of
'git send-email'; apologies if it gets botched.

 t/t6035-merge-dir-to-symlink.sh |   49 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100755 t/t6035-merge-dir-to-symlink.sh

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
new file mode 100755
index 0000000..94a9f32
--- /dev/null
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='merging when a directory was replaced with a symlink'
+. ./test-lib.sh
+
+test_expect_success 'setup a merge where dir a/b changed to symlink' '
+       mkdir -p a/b/c a/b-2/c &&
+       > a/b/c/d &&
+       > a/b-2/c/d &&
+       > a/x &&
+       git add -A &&
+       git commit -m base &&
+       rm -rf a/b &&
+       ln -s b-2 a/b &&
+       git add -A &&
+       git commit -m "dir to symlink" &&
+       git checkout -b temp HEAD^
+'
+
+test_expect_failure 'checkout should not have deleted a/b-2/c/d' '
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'clean the work tree and do the merge' '
+       git reset --hard &&
+       test -f a/b-2/c/d &&
+       echo x > a/x &&
+       git add a/x &&
+       git commit -m x &&
+       git merge master
+'
+
+test_expect_failure 'merge should not have deleted a/b-2/c/d' '
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
+       git checkout -f -b temp2 master^ &&
+       rm -rf a/b-2 &&
+       ln -s b a/b-2 &&
+       git add -A &&
+       git commit -m "dir a/b-2 to symlink"
+'
+
+test_expect_failure 'merge should not have conflicts' '
+       git merge temp
+'
+
+test_done
--
1.6.2.5

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

[Junio C Hamano]

"Pickens, James E" <james.e.pickens@intel.com> writes:

> This test creates two directories, a/b and a/b-2, then replaces a/b with
> a symlink to a/b-2, then merges that change into another branch that
> contains an unrelated change.

Thanks.

> Note that using the resolve strategy instead of recursive makes the
> second bug go away, but not the third one.

It is better to have separate tests for documentation purposes to help
people who track down the breakage in such a case.

> +test_expect_failure 'checkout should not have deleted a/b-2/c/d' '
> +       test -f a/b-2/c/d
> +'
> +
> +test_expect_success 'clean the work tree and do the merge' '
> +       git reset --hard &&
> +       test -f a/b-2/c/d &&
> +       echo x > a/x &&
> +       git add a/x &&
> +       git commit -m x &&
> +       git merge master
> +'
> +
> +test_expect_failure 'merge should not have deleted a/b-2/c/d' '
> +       test -f a/b-2/c/d
> +'

So...

	test_expect_success 'setup for merge test' '
        	...
                git commit -m x &&
                git tag baseline
	'

	test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
		git reset --hard &&
        	git checkout baseline^0 &&
                git merge -s resolve master
	'

	test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
		git reset --hard &&
        	git checkout baseline^0 &&
                git merge -s recursive master
	'

Likewise for the other one.

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

[Pickens, James E]

This test creates two directories, a/b and a/b-2, then replaces a/b with
a symlink to a/b-2, then merges that change into the 'baseline' commit,
which contains an unrelated change.

There are two bugs:
1. 'git checkout' incorrectly deletes work tree file a/b-2/d.
2. 'git merge' incorrectly deletes work tree file a/b-2/d.

The test goes on to create another branch in which a/b-2 is replaced
with a symlink to a/b (i.e., the reverse of what was done the first
time), and merge it into the 'baseline' commit.

There is a different bug:
3. The merge should be clean, but git reports a conflict.

Signed-off-by: James Pickens <james.e.pickens@intel.com>
---

Ok, one more try.  I added Junio's latest feedback, and also added more checks
for correct merge results after each merge.  For the merges that incorrectly
report conflicts, those checks won't be executed since the conflict stops the
test.  If/when the bug causing the merge conflict is fixed, it will become
important to check the merge results, so those checks might as well be there
from the beginning.

 t/t6035-merge-dir-to-symlink.sh |   76 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100755 t/t6035-merge-dir-to-symlink.sh

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
new file mode 100755
index 0000000..89e8e6a
--- /dev/null
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+
+test_description='merging when a directory was replaced with a symlink'
+. ./test-lib.sh
+
+test_expect_success 'create a commit where dir a/b changed to symlink' '
+       mkdir -p a/b/c a/b-2/c &&
+       > a/b/c/d &&
+       > a/b-2/c/d &&
+       > a/x &&
+       git add -A &&
+       git commit -m base &&
+       git tag start &&
+       rm -rf a/b &&
+       ln -s b-2 a/b &&
+       git add -A &&
+       git commit -m "dir to symlink" &&
+       git checkout start^0
+'
+
+test_expect_failure 'checkout should not have deleted a/b-2/c/d' '
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'setup for merge test' '
+       git reset --hard &&
+       test -f a/b-2/c/d &&
+       echo x > a/x &&
+       git add a/x &&
+       git commit -m x &&
+       git tag baseline
+'
+
+test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       git merge -s resolve master &&
+       test -h a/b &&
+       test -f a/b-2/c/d
+'
+
+test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       git merge -s recursive master &&
+       test -h a/b &&
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
+       git reset --hard &&
+       git checkout start^0 &&
+       rm -rf a/b-2 &&
+       ln -s b a/b-2 &&
+       git add -A &&
+       git commit -m "dir a/b-2 to symlink" &&
+       git tag test2
+'
+
+test_expect_failure 'merge should not have conflicts (resolve)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       git merge -s resolve test2 &&
+       test -h a/b-2 &&
+       test -f a/b/c/d
+'
+
+test_expect_failure 'merge should not have conflicts (recursive)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       git merge -s recursive test2 &&
+       test -h a/b-2 &&
+       test -f a/b/c/d
+'
+
+test_done
--
1.6.2.5

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

[Linus Torvalds]

On Wed, 29 Jul 2009, Pickens, James E wrote:
>
> This test creates two directories, a/b and a/b-2, then replaces a/b with
> a symlink to a/b-2, then merges that change into the 'baseline' commit,
> which contains an unrelated change.

Great tests.

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.

Kjetil, can you look at that? 

		Linus

---
 cache.h |    3 +++
 entry.c |   15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index e6c7f33..9222774 100644
--- a/cache.h
+++ b/cache.h
@@ -468,6 +468,9 @@ extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_obje
 extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
 extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 
+/* "careful lstat()" */
+extern int check_path(const char *path, int len, struct stat *st);
+
 #define REFRESH_REALLY		0x0001	/* ignore_valid */
 #define REFRESH_UNMERGED	0x0002	/* allow unmerged */
 #define REFRESH_QUIET		0x0004	/* be quiet about it */
diff --git a/entry.c b/entry.c
index d3e86c7..f276cf3 100644
--- a/entry.c
+++ b/entry.c
@@ -175,6 +175,19 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 	return 0;
 }
 
+/*
+ * This is like 'lstat()', except it refuses to follow symlinks
+ * in the path.
+ */
+int check_path(const char *path, int len, struct stat *st)
+{
+	if (has_symlink_leading_path(path, len)) {
+		errno = ENOENT;
+		return -1;
+	}
+	return lstat(path, st);
+}
+
 int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath)
 {
 	static char path[PATH_MAX + 1];
@@ -188,7 +201,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 	strcpy(path + len, ce->name);
 	len += ce_namelen(ce);
 
-	if (!lstat(path, &st)) {
+	if (!check_path(path, len, &st)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);
 		if (!changed)
 			return 0;

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 29 Jul 2009, Pickens, James E wrote:
>>
>> This test creates two directories, a/b and a/b-2, then replaces a/b with
>> a symlink to a/b-2, then merges that change into the 'baseline' commit,
>> which contains an unrelated change.
>
> Great tests.
>
> This patch should fix the 'checkout' issue.

Thanks.

I've queued v2 with local fixes and Kjetil's earlier fix in 'pu' that
updates has_symlink_leading_path() breakage.  Will take a look at your
patch (and v3 test) later.

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

[Kjetil Barvik]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 29 Jul 2009, Pickens, James E wrote:
>>
>> This test creates two directories, a/b and a/b-2, then replaces a/b with
>> a symlink to a/b-2, then merges that change into the 'baseline' commit,
>> which contains an unrelated change.
>
> Great tests.
>
> 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.
>
> Kjetil, can you look at that? 

  Yes, I will take a look.  Also, on all the other mails CC'ed to me
  today. Give me a cople of days.

  Sorry, I do not work at "full normal speed" for the moment.  But, I
  will try to my best.

  -- kjetil

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

[Linus Torvalds]

On Wed, 29 Jul 2009, Linus Torvalds wrote:
>
> 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.
> 
> Kjetil, can you look at that? 

Hmm... This looks like it should do it.

It doesn't make the test _pass_ (we don't seem to be creating a/b-2/c/d 
properly - I haven't checked why yet, but I suspect it is becasue we think 
it already exists due to the symlinked version lstat'ing fine), but it 
seems to do the right thing.

		Linus

---
 dir.c      |   20 --------------------
 symlinks.c |   26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/dir.c b/dir.c
index e05b850..2204826 100644
--- a/dir.c
+++ b/dir.c
@@ -911,23 +911,3 @@ void setup_standard_excludes(struct dir_struct *dir)
 	if (excludes_file && !access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
-
-int remove_path(const char *name)
-{
-	char *slash;
-
-	if (unlink(name) && errno != ENOENT)
-		return -1;
-
-	slash = strrchr(name, '/');
-	if (slash) {
-		char *dirs = xstrdup(name);
-		slash = dirs + (slash - name);
-		do {
-			*slash = '\0';
-		} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));
-		free(dirs);
-	}
-	return 0;
-}
-
diff --git a/symlinks.c b/symlinks.c
index 4bdded3..349c8d5 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -306,3 +306,29 @@ void remove_scheduled_dirs(void)
 {
 	do_remove_scheduled_dirs(0);
 }
+
+int remove_path(const char *name)
+{
+	char *slash;
+
+	/*
+	 * If we have a leading symlink, we remove
+	 * just the symlink!
+	 */
+	if (has_symlink_leading_path(name, strlen(name)))
+		name = default_cache.path;
+
+	if (unlink(name) && errno != ENOENT)
+		return -1;
+
+	slash = strrchr(name, '/');
+	if (slash) {
+		char *dirs = xstrdup(name);
+		slash = dirs + (slash - name);
+		do {
+			*slash = '\0';
+		} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));
+		free(dirs);
+	}
+	return 0;
+}

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

[Linus Torvalds]

On Wed, 29 Jul 2009, Linus Torvalds wrote:
> 
> Hmm... This looks like it should do it.
> 
> It doesn't make the test _pass_ (we don't seem to be creating a/b-2/c/d 
> properly - I haven't checked why yet, but I suspect it is becasue we think 
> it already exists due to the symlinked version lstat'ing fine), but it 
> seems to do the right thing.

Never mind. The patch does what I set out to do, but it's not relevant for 
the problem.

What happens is:

 - we remove a/b/c/d to make room for the a/b symlink:

	merge_trees ->
	  git_merge_trees ->
	    check_updates ->
	      checkout_entry ->
	        remove_subtree("a/b") ->
	          recursive rm

   This is correct

 - then we create the a/b symlink to a/b2

	merge_trees ->
	  git_merge_trees ->
	    check_updates ->
	      checkout_entry ->
	        write_entry ->
	          symlink

   This is correct

 - Then we remove 'a/b/c/d' again for the 'unmerged_cache()' case:

	merge_trees ->
	  process_entry ->
	    remove_file

   because th eprocess_entry() will decide that the original tree had that 
   "a/b/c/d" file (true) that needs to be removed (false - we already 
   did that as part of creating "a/b").

Annoying.

		Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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.

This is a good thing to do, 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 to do so.

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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.