From 9a6728d4d14a08791861a63b7567ee0b50e59f70 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 4 Apr 2013 15:03:35 -0400 Subject: rm: do not complain about d/f conflicts during deletion If we used to have an index entry "d/f", but "d" has been replaced by a non-directory entry, the user may still want to run "git rm" to delete the stale index entry. They could use "git rm --cached" to just touch the index, but "git rm" should also work: we explicitly try to handle the case that the file has already been removed from the working tree. However, because unlinking "d/f" in this case will not yield ENOENT, but rather ENOTDIR, we do not notice that the file is already gone. Instead, we report it as an error. The simple solution is to treat ENOTDIR in this case exactly like ENOENT; all we want to know is whether the file is already gone, and if a leading path is no longer a directory, then by definition the sub-path is gone. Reported-by: jpinheiro <7jpinheiro@gmail.com> Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rm.c | 2 +- dir.c | 2 +- t/t3600-rm.sh | 25 +++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/builtin/rm.c b/builtin/rm.c index dabfcf6890..7b91d52f39 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -110,7 +110,7 @@ static int check_local_mod(unsigned char *head, int index_only) ce = active_cache[pos]; if (lstat(ce->name, &st) < 0) { - if (errno != ENOENT) + if (errno != ENOENT && errno != ENOTDIR) warning("'%s': %s", ce->name, strerror(errno)); /* It already vanished from the working tree */ continue; diff --git a/dir.c b/dir.c index 57394e452e..f9e7355c69 100644 --- a/dir.c +++ b/dir.c @@ -1603,7 +1603,7 @@ int remove_path(const char *name) { char *slash; - if (unlink(name) && errno != ENOENT) + if (unlink(name) && errno != ENOENT && errno != ENOTDIR) return -1; slash = strrchr(name, '/'); diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 37bf5f13b0..73772b2790 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -622,4 +622,29 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc rm -rf submod ' +test_expect_success 'rm of d/f when d has become a non-directory' ' + rm -rf d && + mkdir d && + >d/f && + git add d && + rm -rf d && + >d && + git rm d/f && + test_must_fail git rev-parse --verify :d/f && + test_path_is_file d +' + +test_expect_success SYMLINKS 'rm of d/f when d has become a dangling symlink' ' + rm -rf d && + mkdir d && + >d/f && + git add d && + rm -rf d && + ln -s nonexistent d && + git rm d/f && + test_must_fail git rev-parse --verify :d/f && + test -h d && + test_path_is_missing d +' + test_done -- cgit v1.2.3 From 96ec8ee92aad52b6556e87d0166a7b9c4cd7c324 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 4 Apr 2013 15:03:58 -0400 Subject: t3600: test behavior of reverse-d/f conflict The previous commit taught "rm" that it is safe to consider "d/f" removed when "d" has become a non-directory. This patch adds a test for the opposite: a file "d" that becomes a directory. In this case, "git rm" does need to complain, because we should not be removing arbitrary content under "d". Git already behaves correctly, but let's make sure that remains the case by protecting the behavior with a test. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3600-rm.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 73772b2790..a2e1a03965 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -647,4 +647,16 @@ test_expect_success SYMLINKS 'rm of d/f when d has become a dangling symlink' ' test_path_is_missing d ' +test_expect_success 'rm of file when it has become a directory' ' + rm -rf d && + >d && + git add d && + rm -f d && + mkdir d && + >d/f && + test_must_fail git rm d && + git rev-parse --verify :d && + test_path_is_file d/f +' + test_done -- cgit v1.2.3 From 03415ca8db28e67ca04e04126b42f38a90c825de Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 4 Apr 2013 20:00:09 -0400 Subject: t3600: document failure of rm across symbolic links If we have a symlink "d" that points to a directory, we should not be able to remove "d/f". In the normal case, where "d/f" does not exist in the index, we already disallow this, as we only remove things that git knows about in the index. So for something like: ln -s /outside/repo foo git add foo git rm foo/bar we will properly produce an error (as there is no index entry for foo/bar). However, if there is an index entry for the path (e.g., because the movement is due to working tree changes that have not yet been reflected in the index), we will happily delete it, even though the path we delete from the filesystem is not the same as the path in the index. This patch documents that failure with a test. While this is a bug, it should not be possible to cause serious data loss with it. For any path that does not have an index entry, we will complain and bail. For a path which does have an index entry, we will do the usual up-to-date content check. So even if the deleted path in the filesystem is not the same as the one we are removing from the index, we do know that they at least have the same content, and that the content is included in HEAD. That means the worst case is not the accidental loss of content, but rather confusion by the user when a copy of a file another part of the tree is removed. Which makes this bug a minor and hard-to-trigger annoyance rather than a data-loss bug (and hence the fix can be saved for a rainy day when somebody feels like working on it). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3600-rm.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index a2e1a03965..0c44e9f5d0 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -659,4 +659,32 @@ test_expect_success 'rm of file when it has become a directory' ' test_path_is_file d/f ' +test_expect_success SYMLINKS 'rm across a symlinked leading path (no index)' ' + rm -rf d e && + mkdir e && + echo content >e/f && + ln -s e d && + git add -A e d && + git commit -m "symlink d to e, e/f exists" && + test_must_fail git rm d/f && + git rev-parse --verify :d && + git rev-parse --verify :e/f && + test -h d && + test_path_is_file e/f +' + +test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' ' + rm -rf d e && + mkdir d && + echo content >d/f && + git add -A e d && + git commit -m "d/f exists" && + mv d e && + ln -s e d && + test_must_fail git rm d/f && + git rev-parse --verify :d/f && + test -h d && + test_path_is_file e/f +' + test_done -- cgit v1.2.3