Re: untracked symlinks are less precious than untracked files?

Johannes Sixt [off-list ref] 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?

On Mittwoch, 2. Februar 2011, Junio C Hamano wrote:

Johannes Sixt [off-list ref] writes:

quoted

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

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 <redacted>
---
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

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

Clemens Buchacher [off-list ref] writes:

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.

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

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 <redacted>
Subject: [PATCH] fixup! Make test case number unique, mark tests with SYMLINKS prerequisite

Signed-off-by: Johannes Sixt <redacted>
---
 ...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

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 <redacted>
---

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