Bug Report ( including test script ): Non-Fastforward merges misses directory deletion

Hello, 

I recently recognized a bug that is related to the merge of deletions. If there is a single file at path 'dir/subdir/file', and the file is deleted in one branch called 'del', git merge fails to delete 'dir' if 'del' is merged into another branch where the path still existed if --no-ff is given ( or if a fast-forward is not possible ). Apparently, it will only delete the immediate parent directory, but cannot work its way up to the remaining empty directories. If a fast-forward is possible, 'dir' will be deleted as one would expect it - perhaps git will internally just do a checkout which is implemented differently.

The issue could be reproduced on git 1.7.0 and 1.6.5, I have not tested other versions though.

To reproduce the issue, execute the following script. It will exit with status 5 to indicate the base top-level directory still exists.

Regards, Sebastian

--------------------------------------------------------------------------

#!/bin/bash reponame=testrepo basedir=dir dirpath=$basedir/subdir filepath=$dirpath/file

# setup git repo mkdir $reponame cd $reponame git init

# make dir and file mkdir -p $dirpath echo data > $filepath

# initial commit git add $dirpath git commit -m "initial commit"

# create branch with deletion git co -b del git rm -r $dirpath git commit -m "deleted folder"

# merge fast forward - it works git co master git merge del

# assertion - directory must not exist [[ ! -d $dirpath ]] || exit 1 [[ ! -d $basedir ]] || exit 2

# undo merge, again with non-fastforward git reset --hard master~1

# as a test, one can make a fast-forward impossible - the issue still shows up #echo "some data" > new_file #git add new_file #git commit -m "new file" #git merge del

git merge --no-ff del

# the directory should be gone, but effectively only the file is AND the files # empty parent directory [[ ! -f $filepath ]] || exit 3 [[ ! -d $dirpath ]] || exit 4 [[ ! -d $basedir ]] || exit 5

echo "It worked actually !"

I did some additional testing and now this issue makes more sense to me. 

To me it appears as if merge, once it detects a file deletion, internally uses git-rm to delete the affected files from the working tree. Git-rm will only delete the file's immediate parent directory, but does not consider other empty parent directories. Given the working tree ...

dir/subdir/subsubdir/file

... if git-rm receives only one file for deletion, i.e. 

git rm dir/subdir/subsubdir/file

it will also delete subsubdir if it turns out to be empty after the deletion of file. This might already be too much as the user might have had a reason not to specify dir/subdir/subsubdir ( perhaps he wants to copy another file into it which sadly doesn't exist anymore ).

Conversely, if the user wants to delete a whole directory tree recursively, rm seems to resolve commands like ...

git rm -r dir

... to a list of file paths in the index, and applies the same logic as previously mentioned. This results in unexpected behaviour regarding the working tree state, as it will leave 'dir/subdir' untouched although it was supposed to be deleted recursively ( /bin/rm -R would have done it )

To my mind, this behaviour of git-rm is incorrect, when reading the docs I would come to the conclusion that it will in fact delete subdirectories recursively, although I could expect that 'dir' should stay as it only cares about subdirectories:

"A leading directory name (e.g. dir to remove dir/file1 and dir/file2) can be given to remove all files in the directory, and recursively all sub-directories, but this requires the -r option to be explicitly given."

Perhaps this behaviour is desired here, but it might be good to update the git-rm docs to clearly reflect that.

Considering my previous findings about git-rm, the behaviour of git-merge is understandable. As git only tracks files, it would even be okay to keep possibly empty directories after a merge. The problem here is that git-merge in fact deletes empty parent directories after file deletions which implies it cares, but it does not do so recursively. I would suggest that it either does not touch the empty parent directories of deleted files at all or that it removes empty parent directories to more closely match the actual index.

To increase the understanding for the severity of the working tree inconsistency, let me present the case I am working on. There is a file server with a git repository. It keeps its working tree up-to-date with the tree of the head commit at all times, hence empty folders may not exist as there are no empty trees. Clients push their changes into separate branches. A git-update hook checks it and will at some point allow the change to be merged into the checked-out main branch. If this merge involves a deletion that effectively removes directories, these would remain in the working tree if the merge ends up not to be fast-forwarded. This confuses the users as they see the server's working tree.

I can workaround this issue by verifying that the merge was a fast-forward one. If this was not the case, I use git-clean to remove everything not in the index.

To illustrate the git-rm recursive deletion issue, I appended the 'test_git_rm_recursive' script.

Please see this post as an amendment to my previous post.

Thank you, Sebastian

This test exits with 3 and a comment. --------------- test_git_rm ------------------- #!/bin/bash reponame=testrepo_rm basedir=dir subdir=$basedir/subdir fileparentdir=$subdir/subsubdir filepath=$fileparentdir/file

# setup git repo mkdir $reponame cd $reponame git init

# make dir and file mkdir -p $fileparentdir echo data > $filepath

# initial commit git add $filepath git commit -m "initial commit"

# delete the top-level dir - we expect recursive deletion as stated in the docs git rm -r $basedir

# assertion - basedir must not exist, but even if it does,subdir must definitely # not exist [[ ! -d $fileparentdir ]] || exit 2 [[ ! -d $subdir ]] || echo "git-rm didn't delete subdirectories recursively" \ && exit 3 [[ ! -d $basedir ]] || echo "Merge may suffer from this git-rm behaviour" \ && exit 4

On Thu, Feb 18, 2010 at 11:43:23AM +0000, Sebastian Thiel wrote:

Hmm. It seems to be a bug.

-- >8 --
Subject: [PATCH] rm: fix bug in recursive subdirectory removal

If we remove a path in a/deep/subdirectory, we should try to remove as many trailing components as possible (i.e., subdirectory, then deep, then a). However, the test for the return value of rmdir was reversed, so we only ever deleted at most one level.

The fix is in remove_path, so "apply" and "merge-recursive" also are fixed.

Signed-off-by: Jeff King <peff@peff.net>
---
This was introduced by Alex's 4a92d1b (Add remove_path: a function to
remove as much as possible of a path, 2008-09-27), which ironically
complained about bugs in the code it was replacing. :)

As an added bonus, we used to see a failed rmdir as success and keep walking backwards. So now we are avoiding some useless rmdir calls on the parent directories (think of the microseconds we must be saving!).

 dir.c         |    2 +-
 t/t3600-rm.sh |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

Jeff King <peff@peff.net> writes:

Good catch.  I wish all bugs are this easy ;-)

Thanks.

On Fri, Feb 19, 2010 at 06:57, Jeff King <peff@peff.net> wrote:

What was I thinking!

Sincerely-sorry: Alex Riesen <raa.lkml@gmail.com>

Well, it did fix the bugs it claimed to fix