{"thread":{"id":"33367","subject":"Behavior of git rm","startedAt":"2013-04-03T14:50:24Z","lastAt":"2013-04-05T05:04:24Z","messageCount":18,"participants":["jpinheiro","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"213038","messageId":"1365000624535-7581485.post@n2.nabble.com","threadId":"33367","inReplyTo":null,"subject":"Behavior of git rm","fromName":"jpinheiro","fromEmail":"7jpinheiro@gmail.com","sentAt":"2013-04-03T14:50:24Z","receivedAt":"2013-04-03T14:50:24Z","isPatch":false,"sender":{"key":"7jpinheiro@gmail.com","avatar":null},"body":"Hi all,\n\nWe are students from Universidade do Minho in Portugal, and we are using git\nin project as a case study.\nWhile experimenting with git we found an unexpected behavior with git rm.\nHere is a trace of the unexpected behavior:\n\n$ git init\n$ mkdir D\n$ echo \"Hi\" > D/F\n$ git add D/F\n$ rm -r D\n$ echo \"Hey\" > D\n$ git rm D/F\nwarning: 'D/F': Not a directory\nrm 'D/F'\nfatal: git rm: 'D/F': Not a directory\n\n\nIf the file D created with last echo did not exist or was named differently\nthen no error would occur as expected. For example:\n\n$ git init\n$ mkdir D\n$ echo \"Hi\" > D/F\n$ git add D/F\n$ rm -r D\n$ echo \"Hey\" > F\n$ git rm D/F\n\nThis works as expected, and the only difference is the name of the file of\nthe last echo.\nIs this the expected behavior of git rm?\n\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/Behavior-of-git-rm-tp7581485.html\nSent from the git mailing list archive at Nabble.com.\n"},{"id":"213049","messageId":"20130403155841.GA16885@sigill.intra.peff.net","threadId":"33367","inReplyTo":"1365000624535-7581485.post@n2.nabble.com","subject":"Re: Behavior of git rm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-03T15:58:41Z","receivedAt":"2013-04-03T15:58:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2013 at 07:50:24AM -0700, jpinheiro wrote:\n\n> While experimenting with git we found an unexpected behavior with git rm.\n> Here is a trace of the unexpected behavior:\n> \n> $ git init\n> $ mkdir D\n> $ echo \"Hi\" > D/F\n> $ git add D/F\n> $ rm -r D\n> $ echo \"Hey\" > D\n> $ git rm D/F\n> warning: 'D/F': Not a directory\n> rm 'D/F'\n> fatal: git rm: 'D/F': Not a directory\n\nWe drop the D/F entry from the index, but then fail to actually remove\nit from the filesystem, because it has already been replaced. It is\nimpossible to tell from this toy example what the true intent was, but\nin such a situation, there is a reasonable chance that the user should\nhave invoked \"rm --cached\" in the first place.\n\nThat being said, we do try to handle files which have already gone\nmissing; when unlink() fails, we do not consider it an error if we got\nENOENT. We could perhaps add ENOTDIR to that list, as it also indicates\nthat the file is gone (it just happens that one of its prefix\ndirectories was replaced with something else).\n\nThe opposite case is also interesting:\n\n  $ git init\n  $ echo 1 >D\n  $ git add D\n  $ rm D\n  $ mkdir D\n  $ echo 2 >D/F\n  $ git rm D\n  rm 'D'\n  fatal: git rm: 'D': Is a directory\n\nWe expect to see 'D' as a file, but it is now a directory. We _could_\nrecursively remove the directory, but that has the potential to delete\nfiles that the user does not expect.\n\nSo in both cases, \"git rm\" could certainly detect the situation and\nproceed with the destructive operation. But when there is such a\nconflict between what's in the working tree and what's in the index, I\nthink we may be better off erring on the conservative side and bailing,\nand letting the user reconcile the differences themselves (using either\n\"git add\" or \"git rm --cached\" to update the index, or deciding how to\nhandle the working tree contents themselves with regular \"rm\").\n\nOf the two situations, I think the first one is less likely to be\ndestructive (noticing that a file is already gone via ENOTDIR), as we\nare only proceeding with the index deletion, and we end up not touching\nthe filesystem at all. That patch would look something like:\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex dabfcf6..7b91d52 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -110,7 +110,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\tce = active_cache[pos];\n \n \t\tif (lstat(ce->name, &st) < 0) {\n-\t\t\tif (errno != ENOENT)\n+\t\t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\t\twarning(\"'%s': %s\", ce->name, strerror(errno));\n \t\t\t/* It already vanished from the working tree */\n \t\t\tcontinue;\ndiff --git a/dir.c b/dir.c\nindex 57394e4..f9e7355 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1603,7 +1603,7 @@ int remove_path(const char *name)\n {\n \tchar *slash;\n \n-\tif (unlink(name) && errno != ENOENT)\n+\tif (unlink(name) && errno != ENOENT && errno != ENOTDIR)\n \t\treturn -1;\n \n \tslash = strrchr(name, '/');\n"},{"id":"213053","messageId":"7vli8z5xfr.fsf@alter.siamese.dyndns.org","threadId":"33367","inReplyTo":"20130403155841.GA16885@sigill.intra.peff.net","subject":"Re: Behavior of git rm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-03T17:35:52Z","receivedAt":"2013-04-03T17:35:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Of the two situations, I think the first one is less likely to be\n> destructive (noticing that a file is already gone via ENOTDIR), as we\n> are only proceeding with the index deletion, and we end up not touching\n> the filesystem at all.\n\nNice to see sound reasoning.\n\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index dabfcf6..7b91d52 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -110,7 +110,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n>  \t\tce = active_cache[pos];\n>  \n>  \t\tif (lstat(ce->name, &st) < 0) {\n> -\t\t\tif (errno != ENOENT)\n> +\t\t\tif (errno != ENOENT && errno != ENOTDIR)\n\nOK.  We may be running lstat() on D/F but there may be D that is not\na directory.  If it is a file, we get ENOTDIR.\n\nBy the way, if D is a dangling symlink, we get ENOENT; in such a\ncase, we report \"rm 'D/F'\" on the output and remove the index entry.\n\n\t$ rm -f .git/index && rm -fr D E\n\t$ mkdir D && >D/F && git add D && rm -fr D\n        $ ln -s erewhon D && git rm D/F && git ls-files\n        rm 'D/F'\n\nAlso if D is a symlink that point at a directory E, \"git rm\" does\nsomething interesting.\n\n(1) Perhaps we want a complaint in this case.\n\n\t$ rm -f .git/index && rm -fr D E\n\t$ mkdir D && >D/F && git add D && rm -fr D\n\t$ mkdir E && ln -s E D && git rm D/F\n\n(2) Perhaps we want to make sure D/F is not beyond a symlink in this\n    case.\n\n\t$ rm -f .git/index && rm -fr D E\n\t$ mkdir D && >D/F && git add D && rm -fr D\n\t$ mkdir E && ln -s E D && date >E/F && git rm D/F\n\n\n\t$ git rm -f D/F\n\n> diff --git a/dir.c b/dir.c\n> index 57394e4..f9e7355 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1603,7 +1603,7 @@ int remove_path(const char *name)\n>  {\n>  \tchar *slash;\n>  \n> -\tif (unlink(name) && errno != ENOENT)\n> +\tif (unlink(name) && errno != ENOENT && errno != ENOTDIR)\n>  \t\treturn -1;\n\nDitto.\n\n>  \n>  \tslash = strrchr(name, '/');\n"},{"id":"213069","messageId":"20130403203612.GB3982@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7vli8z5xfr.fsf@alter.siamese.dyndns.org","subject":"Re: Behavior of git rm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-03T20:36:12Z","receivedAt":"2013-04-03T20:36:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2013 at 10:35:52AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/builtin/rm.c b/builtin/rm.c\n> > index dabfcf6..7b91d52 100644\n> > --- a/builtin/rm.c\n> > +++ b/builtin/rm.c\n> > @@ -110,7 +110,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n> >  \t\tce = active_cache[pos];\n> >  \n> >  \t\tif (lstat(ce->name, &st) < 0) {\n> > -\t\t\tif (errno != ENOENT)\n> > +\t\t\tif (errno != ENOENT && errno != ENOTDIR)\n> \n> OK.  We may be running lstat() on D/F but there may be D that is not\n> a directory.  If it is a file, we get ENOTDIR.\n> \n> By the way, if D is a dangling symlink, we get ENOENT; in such a\n> case, we report \"rm 'D/F'\" on the output and remove the index entry.\n>\n> \t$ rm -f .git/index && rm -fr D E\n> \t$ mkdir D && >D/F && git add D && rm -fr D\n>         $ ln -s erewhon D && git rm D/F && git ls-files\n>         rm 'D/F'\n\nThat seems sane to me, and makes me feel like handling ENOTDIR here is\nthe right direction.  What that conditional is trying to say is \"if it\nis because the file is not there...\", and so far we know of three\nconditions where it is not there:\n\n  1. There is no entry at that path.\n\n  2. There is a non-directory in the prefix of that path.\n\n  3. There is a dangling symlink in the prefix of that path.\n\n(1) and (3) we already handle via ENOENT. I think it is sane to handle\n(2) the same as (3), but we do not do so currently.\n\n> Also if D is a symlink that point at a directory E, \"git rm\" does\n> something interesting.\n> \n> (1) Perhaps we want a complaint in this case.\n> \n> \t$ rm -f .git/index && rm -fr D E\n> \t$ mkdir D && >D/F && git add D && rm -fr D\n> \t$ mkdir E && ln -s E D && git rm D/F\n\nI think that is OK without complaint; the user asked to get rid of D/F,\nand it is indeed gone (as well as its index entry) after the call\nfinishes. And we did not even need to delete anything, so we cannot be\nlosing data. I am much more concerned about this case:\n\n> (2) Perhaps we want to make sure D/F is not beyond a symlink in this\n>     case.\n> \n> \t$ rm -f .git/index && rm -fr D E\n> \t$ mkdir D && >D/F && git add D && rm -fr D\n> \t$ mkdir E && ln -s E D && date >E/F && git rm D/F\n\nwhere the user is deleting something that may or may not be related to\nthe original D/F. On the other hand, I don't have that much sympathy;\n\"rm\" would make the same deletion. But hmm...shouldn't we be doing an\nup-to-date check? Indeed:\n\n  $ git rm D/F\n  error: 'D/F' has staged content different from both the file and the HEAD\n  (use -f to force removal)\n  $ git commit -m foo && git rm D/F\n  $ git rm D/F\n  error: 'D/F' has local modifications\n  (use --cached to keep the file, or -f to force removal)\n\nSo I do not think we need any extra safety; the content-level checks\nshould be enough to make sure we are not losing anything.\n\n-Peff\n"},{"id":"213163","messageId":"20130404190211.GA15912@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7vli8z5xfr.fsf@alter.siamese.dyndns.org","subject":"Re: Behavior of git rm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T19:02:11Z","receivedAt":"2013-04-04T19:02:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2013 at 10:35:52AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Of the two situations, I think the first one is less likely to be\n> > destructive (noticing that a file is already gone via ENOTDIR), as we\n> > are only proceeding with the index deletion, and we end up not touching\n> > the filesystem at all.\n> \n> Nice to see sound reasoning.\n\nHere's a patch series which I think covers what we've discussed.\n\n  [1/3]: rm: do not complain about d/f conflicts during deletion\n  [2/3]: t3600: test behavior of reverse-d/f conflict\n  [3/3]: t3600: test rm of path with changed leading symlinks\n\nThe first one is the code change, and the rest just documents the cases\nwe discussed.\n\nThe third one is a little subtle. For the most part is it just testing\nthe normal \"changed content requires --force\" behavior of rm. But I\nthink it is worth having because it also makes sure that after deleting\n\"d/f\" when \"d\" is a symlink to \"e\", that we do not remove the new\ndirectory \"e\" nor the symlink \"d\". I do not think this case was\nexplicitly planned for, but it does do the right thing now, and given\nthe subtlety, I'd rather somebody who changes it notice the breakage in\nthe test suite.\n\n-Peff\n"},{"id":"213164","messageId":"20130404190335.GA4063@sigill.intra.peff.net","threadId":"33367","inReplyTo":"20130404190211.GA15912@sigill.intra.peff.net","subject":"[PATCH 1/3] rm: do not complain about d/f conflicts during deletion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T19:03:35Z","receivedAt":"2013-04-04T19:03:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we used to have an index entry \"d/f\", but \"d\" has been\nreplaced by a non-directory entry, the user may still want\nto run \"git rm\" to delete the stale index entry. They could\nuse \"git rm --cached\" to just touch the index, but \"git rm\"\nshould also work: we explicitly try to handle the case that\nthe file has already been removed from the working tree.\n\nHowever, because unlinking \"d/f\" in this case will not yield\nENOENT, but rather ENOTDIR, we do not notice that the file\nis already gone. Instead, we report it as an error.\n\nThe simple solution is to treat ENOTDIR in this case exactly\nlike ENOENT; all we want to know is whether the file is\nalready gone, and if a leading path is no longer a\ndirectory, then by definition the sub-path is gone.\n\nReported-by: jpinheiro <7jpinheiro@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rm.c  |  2 +-\n dir.c         |  2 +-\n t/t3600-rm.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex dabfcf6..7b91d52 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -110,7 +110,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\tce = active_cache[pos];\n \n \t\tif (lstat(ce->name, &st) < 0) {\n-\t\t\tif (errno != ENOENT)\n+\t\t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\t\twarning(\"'%s': %s\", ce->name, strerror(errno));\n \t\t\t/* It already vanished from the working tree */\n \t\t\tcontinue;\ndiff --git a/dir.c b/dir.c\nindex 57394e4..f9e7355 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1603,7 +1603,7 @@ int remove_path(const char *name)\n {\n \tchar *slash;\n \n-\tif (unlink(name) && errno != ENOENT)\n+\tif (unlink(name) && errno != ENOENT && errno != ENOTDIR)\n \t\treturn -1;\n \n \tslash = strrchr(name, '/');\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 37bf5f1..73772b2 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -622,4 +622,29 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc\n \trm -rf submod\n '\n \n+test_expect_success 'rm of d/f when d has become a non-directory' '\n+\trm -rf d &&\n+\tmkdir d &&\n+\t>d/f &&\n+\tgit add d &&\n+\trm -rf d &&\n+\t>d &&\n+\tgit rm d/f &&\n+\ttest_must_fail git rev-parse --verify :d/f &&\n+\ttest_path_is_file d\n+'\n+\n+test_expect_success SYMLINKS 'rm of d/f when d has become a dangling symlink' '\n+\trm -rf d &&\n+\tmkdir d &&\n+\t>d/f &&\n+\tgit add d &&\n+\trm -rf d &&\n+\tln -s nonexistent d &&\n+\tgit rm d/f &&\n+\ttest_must_fail git rev-parse --verify :d/f &&\n+\ttest -h d &&\n+\ttest_path_is_missing d\n+'\n+\n test_done\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213165","messageId":"20130404190358.GB4063@sigill.intra.peff.net","threadId":"33367","inReplyTo":"20130404190211.GA15912@sigill.intra.peff.net","subject":"[PATCH 2/3] t3600: test behavior of reverse-d/f conflict","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T19:03:58Z","receivedAt":"2013-04-04T19:03:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit taught \"rm\" that it is safe to consider\n\"d/f\" removed when \"d\" has become a non-directory. This\npatch adds a test for the opposite: a file \"d\" that becomes\na directory.\n\nIn this case, \"git rm\" does need to complain, because we\nshould not be removing arbitrary content under \"d\". Git\nalready behaves correctly, but let's make sure that remains\nthe case by protecting the behavior with a test.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3600-rm.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 73772b2..a2e1a03 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -647,4 +647,16 @@ test_expect_success SYMLINKS 'rm of d/f when d has become a dangling symlink' '\n \ttest_path_is_missing d\n '\n \n+test_expect_success 'rm of file when it has become a directory' '\n+\trm -rf d &&\n+\t>d &&\n+\tgit add d &&\n+\trm -f d &&\n+\tmkdir d &&\n+\t>d/f &&\n+\ttest_must_fail git rm d &&\n+\tgit rev-parse --verify :d &&\n+\ttest_path_is_file d/f\n+'\n+\n test_done\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213167","messageId":"20130404190621.GA7484@sigill.intra.peff.net","threadId":"33367","inReplyTo":"20130404190211.GA15912@sigill.intra.peff.net","subject":"[PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T19:06:21Z","receivedAt":"2013-04-04T19:06:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we have a path \"d/f\" but replace \"d\" with a symlink to a\nnew directory \"e\", how should we handle \"git rm d/f\"?\n\nIt may seem at first like we need new protections to make\nsure that we do not delete random content from \"e/f\".\nHowever, we are already covered by git-rm's existing\nprotections: it is happy if the working tree file is either\nalready deleted, or if its content matches that of the index\nand HEAD (and otherwise requires \"-f\").\n\nLet's add some tests to make sure that these protections\nremain in place when used across symlinks. We also want to\nmake sure that neither the symlink nor the pointed-to\ndirectory is accidentally removed in an attempt to clean up\nempty elements of the leading path.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3600-rm.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a2e1a03..9eaec08 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,4 +659,47 @@ test_expect_success 'rm of file when it has become a directory' '\n \ttest_path_is_file d/f\n '\n \n+test_expect_success 'set up commit with d/f' '\n+\trm -rf d e &&\n+\tmkdir d &&\n+\techo content >d/f &&\n+\tgit add d &&\n+\tgit commit -m d\n+'\n+\n+test_expect_success SYMLINKS 'replace dir with symlink to dir (file missing)' '\n+\tgit reset --hard &&\n+\trm -rf d e &&\n+\tmkdir e &&\n+\tln -s e d &&\n+\tgit rm d/f &&\n+\ttest_must_fail git rev-parse --verify :d/f &&\n+\ttest -h d &&\n+\ttest_path_is_dir e\n+'\n+\n+test_expect_success SYMLINKS 'replace dir with symlink to dir (same content)' '\n+\tgit reset --hard &&\n+\trm -rf d e &&\n+\tmkdir e &&\n+\techo content >e/f &&\n+\tln -s e d &&\n+\tgit rm d/f &&\n+\ttest_must_fail git rev-parse --verify :d/f &&\n+\ttest -h d &&\n+\ttest_path_is_dir e\n+'\n+\n+test_expect_success SYMLINKS 'replace dir with symlink to dir (new content)' '\n+\tgit reset --hard &&\n+\trm -rf d e &&\n+\tmkdir e &&\n+\techo changed >e/f &&\n+\tln -s e d &&\n+\ttest_must_fail git rm d/f &&\n+\tgit rev-parse --verify :d/f &&\n+\ttest -h d &&\n+\ttest_path_is_file e/f\n+'\n+\n test_done\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213174","messageId":"7v6202hykh.fsf@alter.siamese.dyndns.org","threadId":"33367","inReplyTo":"20130404190621.GA7484@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T19:42:54Z","receivedAt":"2013-04-04T19:42:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +test_expect_success SYMLINKS 'replace dir with symlink to dir (same content)' '\n> +\tgit reset --hard &&\n> +\trm -rf d e &&\n> +\tmkdir e &&\n> +\techo content >e/f &&\n> +\tln -s e d &&\n> +\tgit rm d/f &&\n> +\ttest_must_fail git rev-parse --verify :d/f &&\n> +\ttest -h d &&\n> +\ttest_path_is_dir e\n> +'\n\nThis does not check if e/f still exists in the working tree, and I\nsuspect \"git rm d/f\" removes it.\n\nIf you do this:\n\n\trm -fr d e\n        mkdir e\n        >e/f\n        ln -s e d\n        git add d/f\n\nwe do complain that d/f is beyond a symlink (meaning that all you\ncan add is the symlink d that may happen to point at something).\n\nSilent removal of e/f that is unrelated to the current project's\ntracked contents feels very wrong, and at the same time it looks to\nme that it is inconsistent with what we do when adding.\n\nI need a bit more persuading to understand why it is not a bug, I\nthink.\n\n> +test_expect_success SYMLINKS 'replace dir with symlink to dir (new content)' '\n> +\tgit reset --hard &&\n> +\trm -rf d e &&\n> +\tmkdir e &&\n> +\techo changed >e/f &&\n> +\tln -s e d &&\n> +\ttest_must_fail git rm d/f &&\n> +\tgit rev-parse --verify :d/f &&\n> +\ttest -h d &&\n> +\ttest_path_is_file e/f\n> +'\n> +\n>  test_done\n"},{"id":"213178","messageId":"20130404195554.GA20823@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7v6202hykh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T19:55:54Z","receivedAt":"2013-04-04T19:55:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 12:42:54PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +test_expect_success SYMLINKS 'replace dir with symlink to dir (same content)' '\n> > +\tgit reset --hard &&\n> > +\trm -rf d e &&\n> > +\tmkdir e &&\n> > +\techo content >e/f &&\n> > +\tln -s e d &&\n> > +\tgit rm d/f &&\n> > +\ttest_must_fail git rev-parse --verify :d/f &&\n> > +\ttest -h d &&\n> > +\ttest_path_is_dir e\n> > +'\n> \n> This does not check if e/f still exists in the working tree, and I\n> suspect \"git rm d/f\" removes it.\n\nI guess I should have been more clear in my test; I think it _should_ be\nremoved (and it is). You do not necessarily care that \"d\" is now the\nsymlink and not the actual path; it is safe to remove d/f even though it\nis behind a symlink now, because it has the exact same content that it\nhad before (it is of course important that we still remove the actual\nd/f index entry, but as far as the working tree goes, we only care that\nit is safe to remove, and that we remove it).\n\nIOW, I should have been more explicit like this:\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 9eaec08..3b51a63 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -687,7 +687,8 @@ test_expect_success SYMLINKS 'replace dir with symlink to dir (same content)' '\n \tgit rm d/f &&\n \ttest_must_fail git rev-parse --verify :d/f &&\n \ttest -h d &&\n-\ttest_path_is_dir e\n+\ttest_path_is_dir e &&\n+\ttest_path_is_missing e/f\n '\n \n test_expect_success SYMLINKS 'replace dir with symlink to dir (new content)' '\n\n> If you do this:\n> \n> \trm -fr d e\n>         mkdir e\n>         >e/f\n>         ln -s e d\n>         git add d/f\n> \n> we do complain that d/f is beyond a symlink (meaning that all you\n> can add is the symlink d that may happen to point at something).\n\nRight, but that is because you are adding a bogus entry to the index; we\ncannot have both 'd' as a symlink and 'd/f' as a path in our git tree.\nBut in the removal case, the index manipulation is perfectly reasonable.\nYou are deleting the existing \"d/f\" entry. The only confusion comes from\nthe fact that the working tree does not match that anymore.\n\n> Silent removal of e/f that is unrelated to the current project's\n> tracked contents feels very wrong, and at the same time it looks to\n> me that it is inconsistent with what we do when adding.\n> \n> I need a bit more persuading to understand why it is not a bug, I\n> think.\n\nBut that's the point of the two content tests. It _isn't_ unrelated to\nthe current project's tracked contents; it's the exact same content at\nthe same path (albeit accessed via symlinks now). The likely case for\nthis is something like:\n\n  mv dir somewhere/else\n  ln -s somewhere/else/dir dir\n\nI do not mind if you want to insert extra protection to not cross\nsymlink boundaries (which would obviously invalidate my test).  But I\ndon't think it is necessary because of the existing content-level\nprotections.  Adding extra protections would disallow \"git rm dir/file\" in\nthe above case, but I don't think it's that inconvenient; the user just\nhas to make the index aware of the typechange first via \"git add\".\n\n-Peff\n"},{"id":"213188","messageId":"7v1uaqhwb4.fsf@alter.siamese.dyndns.org","threadId":"33367","inReplyTo":"20130404195554.GA20823@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T20:31:43Z","receivedAt":"2013-04-04T20:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> If you do this:\n>> \n>> \trm -fr d e\n>>         mkdir e\n>>         >e/f\n>>         ln -s e d\n>>         git add d/f\n>> \n>> we do complain that d/f is beyond a symlink (meaning that all you\n>> can add is the symlink d that may happen to point at something).\n>\n> Right, but that is because you are adding a bogus entry to the index; we\n> cannot have both 'd' as a symlink and 'd/f' as a path in our git tree.\n> But in the removal case, the index manipulation is perfectly reasonable.\n\nI think you misread me.  I am not adding 'd' as a symlink at all.\nIIRC, ancient versions of Git got this case wrong and added d/f to\nthe index, which we later fixed.\n\nI have been hinting that we should do the same safety not to touch\n(even compare the contents of) e/f, because the only reason we even\nlook at it is because it appears beyond a symbolic link 'd'.\n"},{"id":"213198","messageId":"20130404210304.GA25811@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7v1uaqhwb4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T21:03:04Z","receivedAt":"2013-04-04T21:03:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 01:31:43PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> If you do this:\n> >> \n> >> \trm -fr d e\n> >>         mkdir e\n> >>         >e/f\n> >>         ln -s e d\n> >>         git add d/f\n> >> \n> >> we do complain that d/f is beyond a symlink (meaning that all you\n> >> can add is the symlink d that may happen to point at something).\n> >\n> > Right, but that is because you are adding a bogus entry to the index; we\n> > cannot have both 'd' as a symlink and 'd/f' as a path in our git tree.\n> > But in the removal case, the index manipulation is perfectly reasonable.\n> \n> I think you misread me.  I am not adding 'd' as a symlink at all.\n> IIRC, ancient versions of Git got this case wrong and added d/f to\n> the index, which we later fixed.\n\nI think I just spoke sloppily. What is bogus about \"d/f\" is not that \"d\"\nis necessarily in the index right now, but that adding \"d/f\" implies\nthat \"d\" is a tree, which it clearly is not. Git maps filesystem\nsymlinks into the index and into its trees without dereferencing them.\nSo whether we have \"d\" in the index right now or not, \"d/f\" is wrong\nconceptually.\n\nBut I do not think the \"we are mis-representing the filesystem\" problem\napplies to this \"rm\" case. We are not adding anything bogus into the\nindex; on the contrary, we are deleting something that no longer matches\nthe filesystem representation (and is actually the same bogosity that we\navoid adding under the rule above).\n\nI do agree that it violates git's general behavior with symlinks (i.e.,\nthat they are not dereferenced).\n\n> I have been hinting that we should do the same safety not to touch\n> (even compare the contents of) e/f, because the only reason we even\n> look at it is because it appears beyond a symbolic link 'd'.\n\nI can certainly see the safety argument that crossing a symlink at \"d\"\nmeans the resulting \"d/f\" is not necessarily related to the original\n\"d/f\" that is in the index. As I said, I do not mind having the extra\nprotection; my argument was only that the content-check already protects\nus, so the extra protection is not necessary. And the implication is\nthat I do not feel like working on it. :) I do not mind at all if you\ndrop my third patch (and that is part of the reason I split it out from\npatch 2, which I do think is a no-brainer), and I am happy to review\npatches to do the symlink check if you want to work on it.\n\nHaving made the argument that the content-check is enough, though, I\nthink there is an interesting corner case where it might not be. I don't\nmind \"git rm d/f\" deleting \"e/f\" inside the repository when \"d\" is a\nsymlink to \"e\". But what would happen with:\n\n  rm -rf d\n  ln -s /path/outside/repo d\n  git rm d/f\n\nDeleting across symlinks inside the repo can be brushed aside with \"eh,\nwell, it is just another way to mention the same name in the\nfilesystem\". But deleting anything outside of the repo seems actively\nwrong.\n\nAnd more on that \"brushed aside\". I think it is easy in the cases we\nhave been talking about, namely where \"d/f\" still exists in the index,\nto think that \"git rm d/f\" is useful and the question is one of safety:\nshould we delete e/f if it is pointed to? But let us imagine that d/f is\n_not_ in the index, but \"d\" is a symlink pointing to some/long/path\".\nThe user wants to be lazy and say \"git rm d/f\", because typing\n\"some/long/path\" is too much work. But what happens to the index? We\nshould probably not be removing \"some/long/path\".\n\nHmm. I think you have convinced me (or perhaps I have convinced myself)\nthat we should generally not be crossing symlink boundaries in\ntranslating names between the filesystem and index. I still don't want\nto work on it, though. :)\n\n-Peff\n"},{"id":"213219","messageId":"7vhajlgabi.fsf@alter.siamese.dyndns.org","threadId":"33367","inReplyTo":"20130404210304.GA25811@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T23:12:01Z","receivedAt":"2013-04-04T23:12:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Deleting across symlinks inside the repo can be brushed aside with \"eh,\n> well, it is just another way to mention the same name in the\n> filesystem\". But deleting anything outside of the repo seems actively\n> wrong.\n\nYup, you finally caught up ;-) IIRC, such an outside repository\ntarget was the case people realized that \"git add\" shouldn't see\nacross symlinks.\n\n> Hmm. I think you have convinced me (or perhaps I have convinced myself)\n> that we should generally not be crossing symlink boundaries in\n> translating names between the filesystem and index. I still don't want\n> to work on it, though. :)\n\nThat is OK.  Just let's not etch a wrong behaviour in stone with\nthat test.\n"},{"id":"213221","messageId":"20130404232903.GA27128@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7vhajlgabi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-04T23:29:04Z","receivedAt":"2013-04-04T23:29:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 04:12:01PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Deleting across symlinks inside the repo can be brushed aside with \"eh,\n> > well, it is just another way to mention the same name in the\n> > filesystem\". But deleting anything outside of the repo seems actively\n> > wrong.\n> \n> Yup, you finally caught up ;-) IIRC, such an outside repository\n> target was the case people realized that \"git add\" shouldn't see\n> across symlinks.\n\nIt would help if you spelled it out rather than making me come to it\nwhile arguing against you. ;P\n\n> > Hmm. I think you have convinced me (or perhaps I have convinced myself)\n> > that we should generally not be crossing symlink boundaries in\n> > translating names between the filesystem and index. I still don't want\n> > to work on it, though. :)\n> \n> That is OK.  Just let's not etch a wrong behaviour in stone with\n> that test.\n\nSo let's drop patch 3. Do we want instead to have an expect_failure\ndocumenting the correct behavior?\n\n-Peff\n"},{"id":"213222","messageId":"7vd2u9g9bg.fsf@alter.siamese.dyndns.org","threadId":"33367","inReplyTo":"20130404232903.GA27128@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-04T23:33:39Z","receivedAt":"2013-04-04T23:33:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So let's drop patch 3. Do we want instead to have an expect_failure\n> documenting the correct behavior?\n\nI think that is very much preferred.\n"},{"id":"213223","messageId":"20130405000009.GA27775@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7vd2u9g9bg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T00:00:09Z","receivedAt":"2013-04-05T00:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 04:33:39PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So let's drop patch 3. Do we want instead to have an expect_failure\n> > documenting the correct behavior?\n> \n> I think that is very much preferred.\n\nHere's a replacement for patch 3, then. I wasn't sure if the\neditorializing in the last 2 paragraphs should go in the commit message\nor the cover letter; feel free to tweak as you see fit.\n\n-- >8 --\nSubject: [PATCH] t3600: document failure of rm across symbolic links\n\nIf we have a symlink \"d\" that points to a directory, we\nshould not be able to remove \"d/f\". In the normal case,\nwhere \"d/f\" does not exist in the index, we already disallow\nthis, as we only remove things that git knows about in the\nindex. So for something like:\n\n  ln -s /outside/repo foo\n  git add foo\n  git rm foo/bar\n\nwe will properly produce an error (as there is no index\nentry for foo/bar). However, if there is an index entry for\nthe path (e.g., because the movement is due to working tree\nchanges that have not yet been reflected in the index), we\nwill happily delete it, even though the path we delete from the\nfilesystem is not the same as the path in the index.\n\nThis patch documents that failure with a test.\n\nWhile this is a bug, it should not be possible to cause\nserious data loss with it. For any path that does not have\nan index entry, we will complain and bail. For a path which\ndoes have an index entry, we will do the usual up-to-date\ncontent check. So even if the deleted path in the filesystem\nis not the same as the one we are removing from the index,\nwe do know that they at least have the same content, and\nthat the content is included in HEAD.\n\nThat means the worst case is not the accidental loss of\ncontent, but rather confusion by the user when a copy of a\nfile another part of the tree is removed. Which makes this\nbug a minor and hard-to-trigger annoyance rather than a\ndata-loss bug (and hence the fix can be saved for a rainy\nday when somebody feels like working on it).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3600-rm.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex a2e1a03..0c44e9f 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -659,4 +659,32 @@ test_expect_success 'rm of file when it has become a directory' '\n \ttest_path_is_file d/f\n '\n \n+test_expect_success SYMLINKS 'rm across a symlinked leading path (no index)' '\n+\trm -rf d e &&\n+\tmkdir e &&\n+\techo content >e/f &&\n+\tln -s e d &&\n+\tgit add -A e d &&\n+\tgit commit -m \"symlink d to e, e/f exists\" &&\n+\ttest_must_fail git rm d/f &&\n+\tgit rev-parse --verify :d &&\n+\tgit rev-parse --verify :e/f &&\n+\ttest -h d &&\n+\ttest_path_is_file e/f\n+'\n+\n+test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n+\trm -rf d e &&\n+\tmkdir d &&\n+\techo content >d/f &&\n+\tgit add -A e d &&\n+\tgit commit -m \"d/f exists\" &&\n+\tmv d e &&\n+\tln -s e d &&\n+\ttest_must_fail git rm d/f &&\n+\tgit rev-parse --verify :d/f &&\n+\ttest -h d &&\n+\ttest_path_is_file e/f\n+'\n+\n test_done\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213237","messageId":"7vsj35efne.fsf@alter.siamese.dyndns.org","threadId":"33367","inReplyTo":"20130405000009.GA27775@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-05T04:59:49Z","receivedAt":"2013-04-05T04:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's a replacement for patch 3, then. I wasn't sure if the\n> editorializing in the last 2 paragraphs should go in the commit message\n> or the cover letter; feel free to tweak as you see fit.\n\nThey look fine as they are.\n\n> That means the worst case is not the accidental loss of content,\n> but rather confusion by the user when a copy of a file another\n> part of the tree is removed.\n\nA copy of a file that is on the filesystem that may not be related\nto the project at all may be lost, and the user may not notice the\nlossage for quite a while.  A symlink that points at /etc/passwd may\ncause the file to be removed and the user will hopefully notice, but\nif the pointed-at file is something in $HOME/tmp/ that you occasionally\nuse, you may not notice the lossage immediately, and when you notice\nthe loss, the only assurance you have is that there is a blob that\nrecords what was lost _somewhere_ in _some_ of your project that had\na symlink that points at $HOME/tmp/ at some point in the past.\n\n\"Exists somewhere, not lost\" is not a very useful assurance, if you\nask me ;-)\n"},{"id":"213238","messageId":"20130405050424.GA30533@sigill.intra.peff.net","threadId":"33367","inReplyTo":"7vsj35efne.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T05:04:24Z","receivedAt":"2013-04-05T05:04:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 04, 2013 at 09:59:49PM -0700, Junio C Hamano wrote:\n\n> > That means the worst case is not the accidental loss of content,\n> > but rather confusion by the user when a copy of a file another\n> > part of the tree is removed.\n> \n> A copy of a file that is on the filesystem that may not be related\n> to the project at all may be lost, and the user may not notice the\n> lossage for quite a while.  A symlink that points at /etc/passwd may\n> cause the file to be removed and the user will hopefully notice, but\n> if the pointed-at file is something in $HOME/tmp/ that you occasionally\n> use, you may not notice the lossage immediately, and when you notice\n> the loss, the only assurance you have is that there is a blob that\n> records what was lost _somewhere_ in _some_ of your project that had\n> a symlink that points at $HOME/tmp/ at some point in the past.\n\nIt's actually quite hard to lose those files. We will only remove the\nfile if it has a matching index entry. So you cannot do:\n\n  ln -s /etc foo\n  git rm foo/passwd\n\nbecause there is no index entry for foo/passwd. You would have to do:\n\n  mkdir foo\n  echo content >foo/passwd\n  git add foo/passwd\n  rm -rf foo\n  ln -s /etc foo\n  git rm foo/passwd\n\nand then you only lose it if it matches exactly \"content\". And\nrecovering it, you know that the original path that held the content was\ncalled \"passwd\". So yes, technically you could lose a file outside of\nthe repo and have trouble finding which path it came from later. But in\npractice, not really.\n\nAnyway, it is academic at this point. :)\n\n-Peff\n"}]}