untracked symlinks are less precious than untracked files?

untracked symlinks are less precious than untracked files?

[Johannes Sixt]

While I was poking around in t6035-merge-dir-to-symlink.sh, I noticed this:

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index 92e02d5..46b401b 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -5,21 +5,22 @@ test_description='merging when a directory was replaced with 
a symlink'
 
 test_expect_success SYMLINKS '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 &&
+	# ln -s b-2 a/b &&
+	>a/b &&
 	git add -A &&
 	git commit -m "dir to symlink"
 '
 
 test_expect_success SYMLINKS 'keep a/b-2/c/d across checkout' '
 	git checkout HEAD^0 &&
 	git reset --hard master &&
 	git rm --cached a/b &&
 	git commit -m "untracked symlink remains" &&
 	 git checkout start^0 &&

With this change, where a symlink is replaced by a regular file, the 'git 
checkout start^0' fails. At this time, a/b is untracked. When it is a 
symlink, it is replaced by a directory. When it is a file, the test fails:

error: The following untracked working tree files would be overwritten by 
checkout:
        a/b

Is it by design that symlinks are less precious than files, or is it an 
oversight?

-- Hannes

Re: untracked symlinks are less precious than untracked files?

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Is it by design that symlinks are less precious than files, or is it an 
> oversight?

I don't recall making conscious distinction between symmlinks and regular
files, so it is likely to be just a bug. Perhaps using stat() where
lstat() should be used and mistaking an error return as missing, or
something silly like that?

Re: untracked symlinks are less precious than untracked files?

[Johannes Sixt]

On Mittwoch, 2. Februar 2011, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> > Is it by design that symlinks are less precious than files, or is it an
> > oversight?
>
> I don't recall making conscious distinction between symmlinks and regular
> files, so it is likely to be just a bug. Perhaps using stat() where
> lstat() should be used and mistaking an error return as missing, or
> something silly like that?

Hm, I don't think so. It seems to interact with the lstat_cache. When lstat 
reports a symlink, this result is cached; but if it is a regular file, it is 
not cached. I don't know, what the consequences are, though... I have to stop 
my investigations for tonight.

-- Hannes

[PATCH] Demonstrate breakage: checkout overwrites untracked symlink with directory

[Johannes Sixt]

This adds tests where an untracked file and an untracked symlink are in the
way where a directory should be created by 'git checkout'. Commit b1735b1a
(do not overwrite files in leading path, 2010-12-14) fixed the case where
a file is in the way, but the untracked symlink is still removed silently.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
On Mittwoch, 2. Februar 2011, Johannes Sixt wrote:
> It seems to interact with the lstat_cache. When lstat
> reports a symlink, this result is cached; but if it is a regular file, it
> is not cached.

The case where a file is in the way was fixed only in v1.7.3.4, but symlinks
are still affected. Clemens, can you help?

-- Hannes

PS: When a date is given for commit reference in a commit message as above,
do you prefer the author date or the committer date? Above, I took the
committer date, which is 2 months behind the author date.

 t/t2019-checkout-overwrite.sh |   50 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100755 t/t2019-checkout-overwrite.sh

diff --git a/t/t2019-checkout-overwrite.sh b/t/t2019-checkout-overwrite.sh
new file mode 100755
index 0000000..e4e529d
--- /dev/null
+++ b/t/t2019-checkout-overwrite.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='checkout must not overwrite an untracked objects'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	mkdir -p a/b/c &&
+	>a/b/c/d &&
+	git add -A &&
+	git commit -m base &&
+	git tag start
+'
+
+test_expect_success 'create a commit where dir a/b changed to file' '
+
+	git checkout -b file &&
+	rm -rf a/b &&
+	>a/b &&
+	git add -A &&
+	git commit -m "dir to file"
+'
+
+test_expect_success 'checkout commit with dir must not remove untracked a/b' '
+
+	git rm --cached a/b &&
+	git commit -m "un-track the file" &&
+	test_must_fail git checkout start &&
+	test -f a/b
+'
+
+test_expect_success 'create a commit where dir a/b changed to symlink' '
+
+	rm -rf a/b &&	# cleanup if previous test failed
+	git checkout -f -b symlink start &&
+	rm -rf a/b &&
+	ln -s foo a/b &&
+	git add -A &&
+	git commit -m "dir to symlink"
+'
+
+test_expect_failure 'checkout commit with dir must not remove untracked a/b' '
+
+	git rm --cached a/b &&
+	git commit -m "un-track the symlink" &&
+	test_must_fail git checkout start &&
+	test -h a/b
+'
+
+test_done
-- 
1.7.4.80.g89060

Re: [PATCH] Demonstrate breakage: checkout overwrites untracked symlink with directory

[Clemens Buchacher]

Hi Hannes,

On Sat, Feb 05, 2011 at 07:18:44PM +0100, Johannes Sixt wrote:
>
> This adds tests where an untracked file and an untracked symlink are in the
> way where a directory should be created by 'git checkout'. Commit b1735b1a
> (do not overwrite files in leading path, 2010-12-14) fixed the case where
> a file is in the way, but the untracked symlink is still removed silently.

Indeed. It was my impression from reading the code that this
behavior is intentional. To protect symlinks from being overwritten
as well, I believe we simply have to remove FL_SYMLINK from the
following line in check_leading_path().

diff --git a/symlinks.c b/symlinks.c
index 3cacebd..034943b 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -223,7 +223,7 @@ int check_leading_path(const char *name, int len)
        int flags;
        int match_len = lstat_cache_matchlen(cache, name, len, &flags,
                           FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
-       if (flags & (FL_SYMLINK|FL_NOENT))
+       if (flags & FL_NOENT)
                return 0;
        else if (flags & FL_DIR)
                return -1;

It does fix your testcase, but it may break others and I will have
to review the code to be sure.

Clemens

Re: [PATCH] Demonstrate breakage: checkout overwrites untracked symlink with directory

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/symlinks.c b/symlinks.c
> index 3cacebd..034943b 100644
> --- a/symlinks.c
> +++ b/symlinks.c
> @@ -223,7 +223,7 @@ int check_leading_path(const char *name, int len)
>         int flags;
>         int match_len = lstat_cache_matchlen(cache, name, len, &flags,
>                            FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
> -       if (flags & (FL_SYMLINK|FL_NOENT))
> +       if (flags & FL_NOENT)
>                 return 0;
>         else if (flags & FL_DIR)
>                 return -1;

This function used to be named has-symlink-or-noent-leading-path before
f66caaf (do not overwrite files in leading path, 2010-10-09) and was used
to check for exactly that condition.  For example, verify_absent_1() used
it to verify that a path A/B/C/D is not on the filesystem (e.g. in
preparation for checking it out) by making sure that none of A, A/B, or
A/B/C exists -- or is an untracked symlink.  If one of them is a symlink
leading elsewhere, even if lstat("A/B/C/D") said the path exists, A/B/C/D
is not something we have in the work tree, and we decide that we can check
out the path by possibly removing intermediate symbolic link and running
mkdir.

Now you are changing the semantics of the function, so that we cannot
clobber intermediate symbolic links when we check out a path.  It may
probably be a good change.

Can we rename this function to fix the naming regression introduced in
f66caaf, by the way?  "check_leading_path()" is a horrible name for a
function that takes some parameters and returns a boolean, as the boolness
of the function already says enough that it is about "check", giving the
first part of the name 0-bit of information, and the remainder of the name
doesn't say much either: what aspect of leading-path is the function
about?  Should the pathcomponents exist, should they not exist, why should
the caller care?

A name that explains for what purpose the caller is expected to call it is
probably the best kind of name.  As long as the purpose does not change,
even though the implementation and the semantics are changed later, the
name can stay the same without losing its meaning.  The second best kind
is a name that explains what it does.  The old name of this function was
of this kind, until f66caaf renamed it to a meaningless name.

Perhaps can-clobber-to-checkout would be a good candidate.

Re: [PATCH] Demonstrate breakage: checkout overwrites untracked symlink with directory

[Clemens Buchacher]

On Wed, Feb 09, 2011 at 03:48:12PM -0800, Junio C Hamano wrote:
> 
> Perhaps can-clobber-to-checkout would be a good candidate.

How about leading_path_in_use() instead? Whether files in the path
can be clobbered or not is checked in a separate step, which is
currently called ok_to_remove().

Clemens

Re: [PATCH] Demonstrate breakage: checkout overwrites untracked symlink with directory

[Johannes Sixt]

I forgot to test the patch on Windows.. Would you please squash this
into js/checkout-untracked-symlink~1 ?

Thanks a lot!

--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] fixup! Make test case number unique, mark tests with SYMLINKS prerequisite

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 ...ut-overwrite.sh => t2021-checkout-overwrite.sh} |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
 rename t/{t2019-checkout-overwrite.sh => t2021-checkout-overwrite.sh} (83%)

diff --git a/t/t2019-checkout-overwrite.sh b/t/t2021-checkout-overwrite.sh
similarity index 83%
rename from t/t2019-checkout-overwrite.sh
rename to t/t2021-checkout-overwrite.sh
index e4e529d..27db2ad 100755
--- a/t/t2019-checkout-overwrite.sh
+++ b/t/t2021-checkout-overwrite.sh
@@ -29,7 +29,7 @@ test_expect_success 'checkout commit with dir must not remove untracked a/b' '
 	test -f a/b
 '
 
-test_expect_success 'create a commit where dir a/b changed to symlink' '
+test_expect_success SYMLINKS 'create a commit where dir a/b changed to symlink' '
 
 	rm -rf a/b &&	# cleanup if previous test failed
 	git checkout -f -b symlink start &&
@@ -39,7 +39,7 @@ test_expect_success 'create a commit where dir a/b changed to symlink' '
 	git commit -m "dir to symlink"
 '
 
-test_expect_failure 'checkout commit with dir must not remove untracked a/b' '
+test_expect_failure SYMLINKS 'checkout commit with dir must not remove untracked a/b' '
 
 	git rm --cached a/b &&
 	git commit -m "un-track the symlink" &&
-- 
1.7.4.2.gb816c.dirty

[PATCH] do not overwrite untracked symlinks

[Clemens Buchacher]

Git traditionally overwrites untracked symlinks silently. This will
generally not cause massive data loss, but it is inconsistent with
the behavior for regular files, which are not silently overwritten.

With this change, git refuses to overwrite untracked symlinks by
default. If the user really wants to overwrite the untracked
symlink, he has git-clean and git-checkout -f at his disposal.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I checked and there are no undesireable side-effects. One test had
to be modified slightly because it does overwrite an untracked
symlink.

 symlinks.c                      |    2 +-
 t/t6035-merge-dir-to-symlink.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 3cacebd..034943b 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -223,7 +223,7 @@ int check_leading_path(const char *name, int len)
 	int flags;
 	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
 			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
-	if (flags & (FL_SYMLINK|FL_NOENT))
+	if (flags & FL_NOENT)
 		return 0;
 	else if (flags & FL_DIR)
 		return -1;
diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index 92e02d5..1de285b 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -22,7 +22,7 @@ test_expect_success SYMLINKS 'keep a/b-2/c/d across checkout' '
 	git reset --hard master &&
 	git rm --cached a/b &&
 	git commit -m "untracked symlink remains" &&
-	 git checkout start^0 &&
+	 git checkout -f start^0 &&
 	 test -f a/b-2/c/d
 '
 
-- 
1.7.3.1.105.g84915

Re: [PATCH] do not overwrite untracked symlinks

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Git traditionally overwrites untracked symlinks silently. This will
> generally not cause massive data loss, but it is inconsistent with
> the behavior for regular files, which are not silently overwritten.
>
> With this change, git refuses to overwrite untracked symlinks by
> default. If the user really wants to overwrite the untracked
> symlink, he has git-clean and git-checkout -f at his disposal.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
> I checked and there are no undesireable side-effects. One test had
> to be modified slightly because it does overwrite an untracked
> symlink.
>
>  symlinks.c                      |    2 +-
>  t/t6035-merge-dir-to-symlink.sh |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/symlinks.c b/symlinks.c
> index 3cacebd..034943b 100644
> --- a/symlinks.c
> +++ b/symlinks.c
> @@ -223,7 +223,7 @@ int check_leading_path(const char *name, int len)
>  	int flags;
>  	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
>  			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
> -	if (flags & (FL_SYMLINK|FL_NOENT))
> +	if (flags & FL_NOENT)
>  		return 0;
>  	else if (flags & FL_DIR)
>  		return -1;

> diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
> index 92e02d5..1de285b 100755
> --- a/t/t6035-merge-dir-to-symlink.sh
> +++ b/t/t6035-merge-dir-to-symlink.sh
> @@ -22,7 +22,7 @@ test_expect_success SYMLINKS 'keep a/b-2/c/d across checkout' '
>  	git reset --hard master &&
>  	git rm --cached a/b &&
>  	git commit -m "untracked symlink remains" &&
> -	 git checkout start^0 &&
> +	 git checkout -f start^0 &&
>  	 test -f a/b-2/c/d
>  '

The title of the test says that checkout must keep a/b-2/c/d; if "git
checkout" without "-f" doesn't do so and you had to change it to "git
checkout -f", it would mean one of two things: (1) you broke "checkout",
or (2) the behaviour the test wanted to keep working turned out to be
unwanted (iow, "git checkout" without "-f" should fail under the initial
condition this test sets up).

I suspect the situation is the latter.

What this test in 6035 does before your patch is:

 0. Prepare a tree with the following path (no intermediate symlinks)

	a/b/c/d
        a/b-2/c/d
        a/x

    and call that "start";

    prepare another tree with the following path

	a/b -> b-2 (symlink)
        a/b-2/c/d
        a/x

    make it a child of the "start".

 1. Detach the head at the second commit (i.e. a/b symlink pointing at
    b-2), and then remove a/b symlink from the index.  Make a commit with
    that index.

    The index at that point has:

	a/b-2/c/d
        a/x

    and in the working tree there is an untracked a/b symlink that point
    at b-2.

 2. Try to switch to "start"'s tree.  To be able to check it out, a
    directory needs to be created at a/b (as we need c/d underneath it),
    but untracked symbolic link in the working tree a/b interferes with
    it, and it _should_ fail.


So instead of testing "with -f, the untracked path is nuked" and still
calling the test to "preserve a/b-2/c/d", shouldn't you rename the test to
make sure the untracked symlink is preserved, and make sure checkout
without "-f" fails?

Of course, in addition to the above, it would be a good idea to add
another test that makes sure that with "checkout -f" the user can get rid
of the untracked symlink and check out what he wanted to check out.

Hmm?

Re: [PATCH] do not overwrite untracked symlinks

[Clemens Buchacher]

Hi Junio,

On Sun, Feb 20, 2011 at 11:15:26PM -0800, Junio C Hamano wrote:
> 
> The title of the test says that checkout must keep a/b-2/c/d; if "git
> checkout" without "-f" doesn't do so and you had to change it to "git
> checkout -f", it would mean one of two things: (1) you broke "checkout",
> or (2) the behaviour the test wanted to keep working turned out to be
> unwanted (iow, "git checkout" without "-f" should fail under the initial
> condition this test sets up).

I didn't write the test, but to me it looks like the test wants to
make sure that while the symlink is removed, the tree it's pointing
to is not removed. I am not sure why that was ever a concern. But
by adding -f the test stays the same, except for the fact that it
is now forcefully overwriting a symlink, which could be done
silently before.

But I am fine with removing the test if you think it's meaningless.
And Hannes posted a number of tests for "symlink preservation"
earlier in this thread:

 http://mid.gmane.org/201102051918.44848.j6t@kdbg.org

We should rename the test to 2020, since 2019 is by now already
taken.  But otherwise I think the tests are fine as-is. I also used
them to test my patch.

Clemens

Re: [PATCH] do not overwrite untracked symlinks

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I didn't write the test, but to me it looks like the test wants to
> make sure that while the symlink is removed, the tree it's pointing
> to is not removed.

Yes, I agree with that reading.  I'd squash in this on top, first making
sure that "do not overwrite untracked symlinks" (which is the title of
this patch) won't be broken, and then the original test that wanted to
make sure that checking out a/b/c/d when a/b is pointing to an unrelated
part of the tree does not nuke whatever is pointed at that symbolic link.

Thanks.

My unhappiness with the undescriptive "check_leading_path()" still
remains, though...

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index 1de285b..2599ae5 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -17,13 +17,21 @@ test_expect_success SYMLINKS 'create a commit where dir a/b changed to symlink'
 	git commit -m "dir to symlink"
 '
 
-test_expect_success SYMLINKS 'keep a/b-2/c/d across checkout' '
+test_expect_success SYMLINKS 'checkout does not clobber untracked symlink' '
 	git checkout HEAD^0 &&
 	git reset --hard master &&
 	git rm --cached a/b &&
 	git commit -m "untracked symlink remains" &&
-	 git checkout -f start^0 &&
-	 test -f a/b-2/c/d
+	test_must_fail git checkout start^0
+'
+
+test_expect_success SYMLINKS 'a/b-2/c/d is kept when clobbering symlink b' '
+	git checkout HEAD^0 &&
+	git reset --hard master &&
+	git rm --cached a/b &&
+	git commit -m "untracked symlink remains" &&
+	git checkout -f start^0 &&
+	test -f a/b-2/c/d
 '
 
 test_expect_success SYMLINKS 'checkout should not have deleted a/b-2/c/d' '

Re: [PATCH] do not overwrite untracked symlinks

[Clemens Buchacher]

On Mon, Feb 21, 2011 at 10:54:38PM -0800, Junio C Hamano wrote:
> 
> Yes, I agree with that reading.  I'd squash in this on top, first making
> sure that "do not overwrite untracked symlinks" (which is the title of
> this patch) won't be broken, and then the original test that wanted to
> make sure that checking out a/b/c/d when a/b is pointing to an unrelated
> part of the tree does not nuke whatever is pointed at that symbolic link.

Ok, perfect.

> My unhappiness with the undescriptive "check_leading_path()" still
> remains, though...

Ok. If I can't figure anything out this weekend, I'll rename it.
Promise.

It's just that I'm even more unhappy about the actual code rather
than the function name.  And each time I look at it I end up trying
to fix it, only to find out that it affects so many parts of the
code that don't make any sense. 

For example, we already have a function check_path() in entry.c,
which does something very similar and is called by
checkout_entry(), which in turn does something similar to
verify_absent(). In fact, I suspect that both functions are called
twice by the same code path around unpack_trees, for the same paths
and for the same reason.

I strongly feel that we should separate the merge process into two
steps.

 - First, do everything in the index, ignoring the work tree.

 - Second, checkout the index to the work tree while making sure no
   changes or untracked files get overwritten.

I know I have talked about this before, and I don't know if I will
ever find the time to implement such a major change. But I can't
get myself to feel good about fixing the function name, but not the
function.

Clemens

Re: [PATCH] do not overwrite untracked symlinks

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I strongly feel that we should separate the merge process into two
> steps.
>
>  - First, do everything in the index, ignoring the work tree.
>
>  - Second, checkout the index to the work tree while making sure no
>    changes or untracked files get overwritten.

Yes, I think most people who are familiar with the internals agreed on
this long time ago.

> ... But I can't
> get myself to feel good about fixing the function name, but not the
> function.

I think I said what I wanted to say badly.

I agree that the code structure is suboptimal; the badly named function is
just a manifestation of the fact that the abstraction used in the
codepaths involved is not a good one, I think.