Hi,

this is resend of the patch I sent two weeks ago. I've extended the commit
message to address Johannes' question and in the second patch I've added tests
for the fix.

Michal

--

[ message continues ]

Hi Johannes,


Thanks for your reply. I do not want to steal your time, but I may not
understand what is the issue. I extended my previous patch with tests
(see bellow) so that all the use cases which might be IMHO affected by
my "fix" are covered. If you can think of another use case let me
know.

The fastest way to test this patch is:
GIT_SKIP_TESTS='t7003.[12]? t7003.[2-9]' ./t7003-filter-branch.sh -d

From 849b105541ba4b5e3592de6769922b1264be0c77 Mon Sep 17 00:00:00 2001
From: Michal Sojka <sojkam1@fel.cvut.cz>
Date: Wed, 27 Jan 2010 15:57:17 +0100
Subject: [PATCH] filter-branch: Add tests for submodules

There are three important tests:
1) 'rewrite submodule with another content' passes only with the
   previous patch applied.

2) 'checkout submodule during rewrite' demonstrates that it is not
   possible to replace a submodule revision in tree-filter by checking
   the submodule out and reseting the submodule's HEAD. Fails both
   with and without the previous patch. This is because filter-branch
   sets GIT_WORKING_TREE to "." which causes clone (called from
   git-submodule) to fail.

3) 'replace submodule revision' shows that replacing submodule
   revision is possible by direct index manipulation. Succeeds both
   with and without the previous patch.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 t/t7003-filter-branch.sh |   48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 9503875..a218d7a 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -306,4 +306,52 @@ test_expect_success '--remap-to-ancestor with filename filters' '
 	test $orig_invariant = $(git rev-parse invariant)
 '
 
+test_expect_success 'setup submodule' '
+	rm -rf * .*
+	git init &&
+	test_commit file &&
+	mkdir submod &&
+	submodurl="$PWD/submod"
+	( cd submod &&
+	  git init &&
+	  test_commit file-in-submod ) &&
+	git submodule ...
[ message continues ]

You place the tree filter in double-quotes. Hence, orig_head will be

What is the purpose of the check in the last line?

As long as you have another command after the "} || : &&", you can just

You must not change the directory without changing back. Use a sub-shell.

I'm not sure whether it's worth catering for this use-case anyway.
Replacing a submodule commit should really be done only in the
--index-filter. The tree that --tree-filter checks out is intended only as
a temporary scratch area. It is not intended as a full worktree. In
particular, since 'submodule update --init' changes the configuration, it
is extremly dangerous to call from a filter.

-- Hannes
--

[ message continues ]

It should check that something was rewritten, but it was incorrectly put into 


I fully agree. I don't plan to put this test in the final version of the 
patch. I wrote this test because I didn't exactly know which issue has Dscho 
in mind. If it was this one, I wanted to show that this is not relevant.

I'm sending corrected version of the patch with tests.

Thanks
Michal
--

[ message continues ]

There are three important tests:
1) 'rewrite submodule with another content' passes only with the
   previous patch applied.

2) 'checkout submodule during rewrite' demonstrates that it is not
   possible to replace a submodule revision in tree-filter by checking
   the submodule out and reseting the submodule's HEAD. Fails both
   with and without the previous patch. This is because filter-branch
   sets GIT_WORKING_TREE to "." which causes clone (called from
   git-submodule) to fail.

3) 'replace submodule revision' shows that replacing submodule
   revision is possible by direct index manipulation. Succeeds both
   with and without the previous patch.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 t/t7003-filter-branch.sh |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 9503875..fabe038 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -306,4 +306,51 @@ test_expect_success '--remap-to-ancestor with filename filters' '
 	test $orig_invariant = $(git rev-parse invariant)
 '
 
+test_expect_success 'setup submodule' '
+	rm -rf * .*
+	git init &&
+	test_commit file &&
+	mkdir submod &&
+	submodurl="$PWD/submod"
+	( cd submod &&
+	  git init &&
+	  test_commit file-in-submod ) &&
+	git submodule add "$submodurl"
+	git commit -m "added submodule" &&
+	test_commit add-file &&
+	( cd submod && test_commit add-in-submodule ) &&
+	git add submod &&
+	git commit -m "changed submodule" &&
+	git branch original HEAD
+'
+
+orig_head=`git show-ref --hash --head HEAD`
+
+test_expect_success 'rewrite submodule with another content' '
+	git filter-branch --tree-filter "test -d submod && {
+					 rm -rf submod &&
+					 git rm -rf --quiet submod &&
+					 mkdir submod &&
+					 : > submod/file
+					 } || :" HEAD &&
+	test $orig_head != `git show-ref --hash --head HEAD`
+'
+
+test_expect_failure 'checkout ...
[ message continues ]

It is unnecessary and counterproductive to self-proclaim the importance of
a patch or new tests.  If anything, what are important are not tests
themselves but the conditions that they check, so "Add tests to check
three important cases:" is slightly more palatable.


Sorry, but I think I am missing some context here to understand this

I thought you agreed with Hannes that this is not something we would even

Thanks.
--

[ message continues ]

Yes. I'm removing this test.

Cheers,
Michal
--

[ message continues ]

When git filter-branch is used to replace a submodule with another
content, it always fails on the first commit. Consider a repository with
submod directory containing a submodule. If I want to remove the
submodule and replace it with a file, the following command fails.

git filter-branch --tree-filter 'rm -rf submod &&
				 git rm -q submod &&
				 mkdir submod &&
				 touch submod/file'

The error message is:
error: submod: is a directory - add files inside instead

The reason is that git diff-index, which generates the first part of the
list of files updated by the tree filter, emits also the removed
submodule even if it was replaced by a real directory.

Adding --ignored-submodules solves the problem for me and
tests in t7003-filter-branch.sh pass correctly.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 git-filter-branch.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 195b5ef..7c4ad7d 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -331,7 +331,7 @@ while read commit parents; do
 			die "tree filter failed: $filter_tree"
 
 		(
-			git diff-index -r --name-only $commit &&
+			git diff-index -r --name-only --ignore-submodules $commit &&
 			git ls-files --others
 		) > "$tempdir"/tree-state || exit
 		git update-index --add --replace --remove --stdin \
-- 
1.6.6

--

[ message continues ]

Add tests to make sure:
1) a submodule can be removed and its content replaced with regular
   files ('rewrite submodule with another content'). This test passes
   only with the previous patch applied.

2) it is possible to replace submodule revision by direct index
   manipulation ('replace submodule revision'). Although it would be
   better to run such a filter in --index-filter, this test shows that
   this functionality is not broken by the previous patch. This
   succeeds both with and without the previous patch.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 t/t7003-filter-branch.sh |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 9503875..4ee8237 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -306,4 +306,43 @@ test_expect_success '--remap-to-ancestor with filename filters' '
 	test $orig_invariant = $(git rev-parse invariant)
 '
 
+test_expect_success 'setup submodule' '
+	rm -rf * .*
+	git init &&
+	test_commit file &&
+	mkdir submod &&
+	submodurl="$PWD/submod"
+	( cd submod &&
+	  git init &&
+	  test_commit file-in-submod ) &&
+	git submodule add "$submodurl"
+	git commit -m "added submodule" &&
+	test_commit add-file &&
+	( cd submod && test_commit add-in-submodule ) &&
+	git add submod &&
+	git commit -m "changed submodule" &&
+	git branch original HEAD
+'
+
+orig_head=`git show-ref --hash --head HEAD`
+
+test_expect_success 'rewrite submodule with another content' '
+	git filter-branch --tree-filter "test -d submod && {
+					 rm -rf submod &&
+					 git rm -rf --quiet submod &&
+					 mkdir submod &&
+					 : > submod/file
+					 } || :" HEAD &&
+	test $orig_head != `git show-ref --hash --head HEAD`
+'
+
+test_expect_success 'replace submodule revision' '
+	git reset --hard original &&
+	git filter-branch -f --tree-filter \
+	    "if git ls-files --error-unmatch -- submod > /dev/null ...
[ message continues ]

Yikes.  Please don't do this.

If you cannot structure your tests following what has already been done by
the previous tests, at least name the things that you want to remove a bit
more explicitly to avoid mistakes.  The loosest form that is reasonable
would probably be (to catch a, actual, backup-refs, ... and .git):

	rm -fr ?* .?* &&

but it would be preferable to be even more explicit "rm -fr ?* .git".


"&&"?

--

[ message continues ]

Fixed.

I've also split each my test into two separate tests. In the first one I 
perform the rewrite and in the second I test that the result is what was 
expected. I did it because the other tests in this file are structured the 
same way.

Cheers,
Michal
--

[ message continues ]

When git filter-branch is used to replace a submodule with another
content, it always fails on the first commit. Consider a repository with
submod directory containing a submodule. If I want to remove the
submodule and replace it with a file, the following command fails.

git filter-branch --tree-filter 'rm -rf submod &&
				 git rm -q submod &&
				 mkdir submod &&
				 touch submod/file'

The error message is:
error: submod: is a directory - add files inside instead

The reason is that git diff-index, which generates the first part of the
list of files updated by the tree filter, emits also the removed
submodule even if it was replaced by a real directory.

Adding --ignored-submodules solves the problem for me and
tests in t7003-filter-branch.sh pass correctly.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 git-filter-branch.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 195b5ef..7c4ad7d 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -331,7 +331,7 @@ while read commit parents; do
 			die "tree filter failed: $filter_tree"
 
 		(
-			git diff-index -r --name-only $commit &&
+			git diff-index -r --name-only --ignore-submodules $commit &&
 			git ls-files --others
 		) > "$tempdir"/tree-state || exit
 		git update-index --add --replace --remove --stdin \
-- 
1.6.6

--

[ message continues ]

Add tests to make sure:
1) a submodule can be removed and its content replaced with regular
   files ('rewrite submodule with another content'). This test passes
   only with the previous patch applied.

2) it is possible to replace submodule revision by direct index
   manipulation ('replace submodule revision'). Although it would be
   better to run such a filter in --index-filter, this test shows that
   this functionality is not broken by the previous patch. This
   succeeds both with and without the previous patch.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 t/t7003-filter-branch.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 9503875..a7f0791 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -306,4 +306,49 @@ test_expect_success '--remap-to-ancestor with filename filters' '
 	test $orig_invariant = $(git rev-parse invariant)
 '
 
+test_expect_success 'setup submodule' '
+	rm -rf ?* .git &&
+	git init &&
+	test_commit file &&
+	mkdir submod &&
+	submodurl="$PWD/submod" &&
+	( cd submod &&
+	  git init &&
+	  test_commit file-in-submod ) &&
+	git submodule add "$submodurl" &&
+	git commit -m "added submodule" &&
+	test_commit add-file &&
+	( cd submod && test_commit add-in-submodule ) &&
+	git add submod &&
+	git commit -m "changed submodule" &&
+	git branch original HEAD
+'
+
+orig_head=`git rev-parse HEAD`
+
+test_expect_success 'rewrite submodule with another content' '
+	git filter-branch --tree-filter "test -d submod && {
+					 rm -rf submod &&
+					 git rm -rf --quiet submod &&
+					 mkdir submod &&
+					 : > submod/file
+					 } || :" HEAD
+'
+test_expect_success 'test that submodule was rewritten' '
+	test -f submod/file &&
+	test $orig_head != `git rev-parse HEAD`
+'
+
+test_expect_success 'replace submodule revision' '
+	git reset --hard original &&
+	git filter-branch -f ...
[ message continues ]