{"thread":{"id":"22707","subject":"Bug Report ( including test script ): Non-Fastforward merges misses directory deletion","startedAt":"2010-02-18T10:00:35Z","lastAt":"2010-02-19T07:40:47Z","messageCount":5,"participants":["Sebastian Thiel","Jeff King","Junio C Hamano","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"134942","messageId":"loom.20100218T104300-858@post.gmane.org","threadId":"22707","inReplyTo":null,"subject":"Bug Report ( including test script ): Non-Fastforward merges misses directory deletion","fromName":"Sebastian Thiel","fromEmail":"byronimo@gmail.com","sentAt":"2010-02-18T10:00:35Z","receivedAt":"2010-02-18T10:00:35Z","isPatch":false,"sender":{"key":"byronimo@gmail.com","avatar":"https://gravatar.com/avatar/84cc94f1c3a848e9e0e11bb1545f5db3245280ffd354687ea8116546509c8429?d=mp&s=160"},"body":"Hello, \n\nI recently recognized a bug that is related to the merge of deletions. \nIf there is a single file at path 'dir/subdir/file', and the file is deleted in\none branch called 'del', git merge fails to delete 'dir' if 'del' is merged into\nanother branch where the path still existed if --no-ff is given ( or if a\nfast-forward is not possible ). Apparently, it will only delete the immediate\nparent directory, but cannot work its way up to the remaining empty directories.\nIf a fast-forward is possible, 'dir' will be deleted as one would expect it -\nperhaps git will internally just do a checkout which is implemented differently.\n\nThe issue could be reproduced on git 1.7.0 and 1.6.5, I have not tested other\nversions though.\n\nTo reproduce the issue, execute the following script. It will exit with status 5\nto indicate the base top-level directory still exists.\n\nRegards, \nSebastian\n\n--------------------------------------------------------------------------\n\n#!/bin/bash\nreponame=testrepo\nbasedir=dir\ndirpath=$basedir/subdir\nfilepath=$dirpath/file\n\n# setup git repo\nmkdir $reponame\ncd $reponame\ngit init\n\n# make dir and file\nmkdir -p $dirpath\necho data > $filepath\n\n# initial commit\ngit add $dirpath\ngit commit -m \"initial commit\"\n\n# create branch with deletion\ngit co -b del\ngit rm -r $dirpath\ngit commit -m \"deleted folder\"\n\n# merge fast forward - it works\ngit co master\ngit merge del\n\n# assertion - directory must not exist\n[[ ! -d $dirpath ]] || exit 1\n[[ ! -d $basedir ]] || exit 2\n\n# undo merge, again with non-fastforward\ngit reset --hard master~1\n\n# as a test, one can make a fast-forward impossible - the issue still shows up\n#echo \"some data\" > new_file\n#git add new_file\n#git commit -m \"new file\"\n#git merge del\n\ngit merge --no-ff del\n\n# the directory should be gone, but effectively only the file is AND the files\n# empty parent directory\n[[ ! -f $filepath ]] || exit 3\n[[ ! -d $dirpath ]] || exit 4\n[[ ! -d $basedir ]] || exit 5\n\necho \"It worked actually !\"\n"},{"id":"134952","messageId":"loom.20100218T113103-602@post.gmane.org","threadId":"22707","inReplyTo":"loom.20100218T104300-858@post.gmane.org","subject":"Re: Bug Report ( including test script ): Non-Fastforward merges misses directory deletion","fromName":"Sebastian Thiel","fromEmail":"byronimo@gmail.com","sentAt":"2010-02-18T11:43:23Z","receivedAt":"2010-02-18T11:43:23Z","isPatch":false,"sender":{"key":"byronimo@gmail.com","avatar":"https://gravatar.com/avatar/84cc94f1c3a848e9e0e11bb1545f5db3245280ffd354687ea8116546509c8429?d=mp&s=160"},"body":"I did some additional testing and now this issue makes more sense to me. \n\nTo me it appears as if merge, once it detects a file deletion, \ninternally uses git-rm to delete the affected files from the working tree. \nGit-rm will only delete the file's immediate parent directory, but does not \nconsider other empty parent directories. \nGiven the working tree ...\n\ndir/subdir/subsubdir/file\n\n... if git-rm receives only one file for deletion, i.e. \n\ngit rm dir/subdir/subsubdir/file\n\nit will also delete subsubdir if it turns out to be empty after the deletion of\nfile. This might already be too much as the user might have had a reason not to\nspecify dir/subdir/subsubdir ( perhaps he wants to copy another file into it\nwhich sadly doesn't exist anymore ).\n\n\nConversely, if the user wants to delete a whole directory tree recursively, rm\nseems to resolve commands like  ...\n\ngit rm -r dir\n\n... to a list of file paths in the index, and applies the same logic as\npreviously mentioned. This results in unexpected behaviour regarding the working\ntree state, as it will leave 'dir/subdir' untouched although it was supposed to\nbe deleted recursively ( /bin/rm -R would have done it )\n\nTo my mind, this behaviour of git-rm is incorrect, when reading the docs I would\ncome to the conclusion that it will in fact delete subdirectories recursively,\nalthough I could expect that 'dir' should stay as it only cares about\nsubdirectories:\n\n\"A leading directory name (e.g. dir to remove dir/file1 and dir/file2) can be\ngiven to remove all files in the directory, and recursively all sub-directories,\nbut this requires the -r option to be explicitly given.\"\n\nPerhaps this behaviour is desired here, but it might be good to update the\ngit-rm docs to clearly reflect that.\n\nConsidering my previous findings about git-rm, the behaviour of git-merge is\nunderstandable. As git only tracks files, it would even be okay to keep possibly\nempty directories after a merge. The problem here is that git-merge in fact\ndeletes empty parent directories after file deletions which implies it cares,\nbut it does not do so recursively.\nI would suggest that it either does not touch the empty parent directories of\ndeleted files at all or that it removes empty parent directories to more closely\nmatch the actual index.\n\nTo increase the understanding for the severity of the working tree\ninconsistency, let me present the case I am working on. There is a file server\nwith a git repository. It keeps its working tree up-to-date with the tree of the\nhead commit at all times, hence empty folders may not exist as there are no\nempty trees. Clients push their changes into separate branches. A git-update\nhook checks it and will at some point allow the change to be merged into the\nchecked-out main branch. If this merge involves a deletion that effectively\nremoves directories, these would remain in the working tree if the merge ends up\nnot to be fast-forwarded. This confuses the users as they see the server's\nworking tree.\n\nI can workaround this issue by verifying that the merge was a fast-forward one.\nIf this was not the case, I use git-clean to remove everything not in the index.\n\nTo illustrate the git-rm recursive deletion issue, I appended the\n'test_git_rm_recursive' script.\n\nPlease see this post as an amendment to my previous post.\n\nThank you, \nSebastian\n\nThis test exits with 3 and a comment.\n--------------- test_git_rm -------------------\n#!/bin/bash\nreponame=testrepo_rm\nbasedir=dir\nsubdir=$basedir/subdir\nfileparentdir=$subdir/subsubdir\nfilepath=$fileparentdir/file\n\n# setup git repo\nmkdir $reponame\ncd $reponame\ngit init\n\n# make dir and file\nmkdir -p $fileparentdir\necho data > $filepath\n\n# initial commit\ngit add $filepath\ngit commit -m \"initial commit\" \n\n# delete the top-level dir - we expect recursive deletion as stated in the docs\ngit rm -r $basedir\n\n# assertion - basedir must not exist, but even if it does,subdir must definitely \n# not exist\n[[ ! -d $fileparentdir ]] || exit 2\n[[ ! -d $subdir ]] || echo \"git-rm didn't delete subdirectories recursively\" \\\n&& exit 3\n[[ ! -d $basedir ]] || echo \"Merge may suffer from this git-rm behaviour\" \\\n&& exit 4\n"},{"id":"135040","messageId":"20100219055721.GC22645@coredump.intra.peff.net","threadId":"22707","inReplyTo":"loom.20100218T113103-602@post.gmane.org","subject":"Re: Bug Report ( including test script ): Non-Fastforward merges misses directory deletion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-19T05:57:21Z","receivedAt":"2010-02-19T05:57:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 18, 2010 at 11:43:23AM +0000, Sebastian Thiel wrote:\n\n> To me it appears as if merge, once it detects a file deletion,\n> internally uses git-rm to delete the affected files from the working\n> tree.  Git-rm will only delete the file's immediate parent directory,\n> but does not consider other empty parent directories.\n\nHmm. It seems to be a bug.\n\n-- >8 --\nSubject: [PATCH] rm: fix bug in recursive subdirectory removal\n\nIf we remove a path in a/deep/subdirectory, we should try to\nremove as many trailing components as possible (i.e.,\nsubdirectory, then deep, then a). However, the test for the\nreturn value of rmdir was reversed, so we only ever deleted\nat most one level.\n\nThe fix is in remove_path, so \"apply\" and \"merge-recursive\"\nalso are fixed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was introduced by Alex's 4a92d1b (Add remove_path: a function to\nremove as much as possible of a path, 2008-09-27), which ironically\ncomplained about bugs in the code it was replacing. :)\n\nAs an added bonus, we used to see a failed rmdir as success and keep\nwalking backwards. So now we are avoiding some useless rmdir calls on\nthe parent directories (think of the microseconds we must be saving!).\n\n dir.c         |    2 +-\n t/t3600-rm.sh |    8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 67c3af6..133c333 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1044,7 +1044,7 @@ int remove_path(const char *name)\n \t\tslash = dirs + (slash - name);\n \t\tdo {\n \t\t\t*slash = '\\0';\n-\t\t} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));\n+\t\t} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));\n \t\tfree(dirs);\n \t}\n \treturn 0;\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 76b1bb4..0aaf0ad 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -271,4 +271,12 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \ttest \"$status\" != 0\n '\n \n+test_expect_success 'rm removes subdirectories recursively' '\n+\tmkdir -p dir/subdir/subsubdir &&\n+\techo content >dir/subdir/subsubdir/file &&\n+\tgit add dir/subdir/subsubdir/file &&\n+\tgit rm -f dir/subdir/subsubdir/file &&\n+\t! test -d dir\n+'\n+\n test_done\n-- \n1.7.0.77.gb5742\n"},{"id":"135042","messageId":"7v3a0xwxox.fsf@alter.siamese.dyndns.org","threadId":"22707","inReplyTo":"20100219055721.GC22645@coredump.intra.peff.net","subject":"Re: Bug Report ( including test script ): Non-Fastforward merges misses directory deletion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-19T06:23:42Z","receivedAt":"2010-02-19T06:23:42Z","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> This was introduced by Alex's 4a92d1b (Add remove_path: a function to\n> remove as much as possible of a path, 2008-09-27), which ironically\n> complained about bugs in the code it was replacing. :)\n\nGood catch.  I wish all bugs are this easy ;-)\n\nThanks.\n"},{"id":"135056","messageId":"81b0412b1002182340w71aa6364tfe2e17ad6fd1b1e8@mail.gmail.com","threadId":"22707","inReplyTo":"20100219055721.GC22645@coredump.intra.peff.net","subject":"Re: Bug Report ( including test script ): Non-Fastforward merges misses directory deletion","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-02-19T07:40:47Z","receivedAt":"2010-02-19T07:40:47Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Fri, Feb 19, 2010 at 06:57, Jeff King <peff@peff.net> wrote:\n> On Thu, Feb 18, 2010 at 11:43:23AM +0000, Sebastian Thiel wrote:\n> If we remove a path in a/deep/subdirectory, we should try to\n> remove as many trailing components as possible (i.e.,\n> subdirectory, then deep, then a). However, the test for the\n> return value of rmdir was reversed, so we only ever deleted\n> at most one level.\n\nWhat was I thinking!\n\nSincerely-sorry: Alex Riesen <raa.lkml@gmail.com>\n\n> This was introduced by Alex's 4a92d1b (Add remove_path: a function to\n> remove as much as possible of a path, 2008-09-27), which ironically\n> complained about bugs in the code it was replacing. :)\n\nWell, it did fix the bugs it claimed to fix\n"}]}