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

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

[Sebastian Thiel]

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 !"

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

[Sebastian Thiel]

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

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

[Jeff King]

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

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

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(-)

diff --git a/dir.c b/dir.c
index 67c3af6..133c333 100644
--- a/dir.c
+++ b/dir.c
@@ -1044,7 +1044,7 @@ int remove_path(const char *name)
 		slash = dirs + (slash - name);
 		do {
 			*slash = '\0';
-		} while (rmdir(dirs) && (slash = strrchr(dirs, '/')));
+		} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
 		free(dirs);
 	}
 	return 0;
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 76b1bb4..0aaf0ad 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -271,4 +271,12 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 	test "$status" != 0
 '
 
+test_expect_success 'rm removes subdirectories recursively' '
+	mkdir -p dir/subdir/subsubdir &&
+	echo content >dir/subdir/subsubdir/file &&
+	git add dir/subdir/subsubdir/file &&
+	git rm -f dir/subdir/subsubdir/file &&
+	! test -d dir
+'
+
 test_done
-- 
1.7.0.77.gb5742

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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. :)

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

Thanks.

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

[Alex Riesen]

On Fri, Feb 19, 2010 at 06:57, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 18, 2010 at 11:43:23AM +0000, Sebastian Thiel wrote:
> 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.

What was I thinking!

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

> 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. :)

Well, it did fix the bugs it claimed to fix