filter-branch fix and tests
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
threads / patch / 22386
Subject: [PATCH 2/2] filter-branch: Add tests for submodules
filter-branch fix and tests
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
[PATCH 1/2] filter-branch: Fix to allow replacing submodules with another content
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.
If somebody wants to replace one revision of the module with another, it can be done with --index-filter. Using --tree-filter for this has no sense since --tree-filter "git submodule update --init" fails with Clone of '/tmp/submod' into submodule path 'submod' failed so that the revision must be replaced only in index.
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..d4ac7fb 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
The test 'rewrite submodule with another content' passes only with the previous patch.
Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz> --- t/t7003-filter-branch.sh | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh index 9503875..39d4153 100755 --- a/t/t7003-filter-branch.sh +++ b/t/t7003-filter-branch.sh @@ -306,4 +306,30 @@ 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" +' + +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 + } || :" +' + test_done
-- 1.6.6
Re: [PATCH] filter-branch fix and tests
Hi Johannes,
On Tuesday 26 of January 2010 19:40:47 Johannes Schindelin wrote:
> On Mon, 25 Jan 2010, Michal Sojka wrote: > > 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. > > From a quick look, it does not seem that I will be happy with your "fix"; > you changed the commit message, but did not address the issue. > > Unfortunately, I do not have time to review anything that takes longer > than 30 minutes in total, and today not even that, so that will have to > wait.
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 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` +export orig_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 + } || : && + test $orig_head != `git show-ref --hash HEAD`" +' + +test_expect_failure 'checkout submodule during rewrite' ' + git reset --hard original && + git filter-branch -f --tree-filter \ + "git submodule update --init && + cd submod && + git reset --hard origin/master" HEAD +' + +test_expect_success 'replace submodule revision' ' + git reset --hard original && + git filter-branch -f --tree-filter \ + "git ls-files --error-unmatch -- submod > /dev/null 2>&1 && + git update-index --cacheinfo 160000 0123456789012345678901234567890123456789 submod || + : && + test $orig_head != `git show-ref --hash HEAD`" +' + test_done
-- 1.6.6
Re: [PATCH] filter-branch fix and tests
Michal Sojka schrieb:
> +orig_head=`git show-ref --hash HEAD` > +export orig_head
You place the tree filter in double-quotes. Hence, orig_head will be interpolated on the filter-branch command line. You don't need to export it.
> +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
> + } || : &&
> + test $orig_head != `git show-ref --hash HEAD`"What is the purpose of the check in the last line?
As long as you have another command after the "} || : &&", you can just write "}" instead.
> +test_expect_failure 'checkout submodule during rewrite' ' > + git reset --hard original && > + git filter-branch -f --tree-filter \ > + "git submodule update --init && > + cd submod && > + git reset --hard origin/master" HEAD
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
Re: [PATCH] filter-branch fix and tests
On Wednesday 27 of January 2010 17:18:16 Johannes Sixt wrote:
> Michal Sojka schrieb: > > +orig_head=`git show-ref --hash HEAD` > > +export orig_head > > You place the tree filter in double-quotes. Hence, orig_head will be > interpolated on the filter-branch command line. You don't need to export > it.
You're right. Fixed.
> > +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
> > + } || : &&
> > + test $orig_head != `git show-ref --hash HEAD`"
>
> What is the purpose of the check in the last line?It should check that something was rewritten, but it was incorrectly put into the filed. Fixed.
> As long as you have another command after the "} || : &&", you can just > write "}" instead.
OK.
> > +test_expect_failure 'checkout submodule during rewrite' ' > > + git reset --hard original && > > + git filter-branch -f --tree-filter \ > > + "git submodule update --init && > > + cd submod && > > + git reset --hard origin/master" HEAD > > 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.
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
[PATCHv3] 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 | 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 submodule during rewrite' ' + git reset --hard original && + git filter-branch -f --tree-filter \ + "git submodule update --init && + ( test -d submod && cd submod && + git reset --hard origin/master )" 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 2>&1 + then git update-index --cacheinfo 160000 0123456789012345678901234567890123456789 submod + fi" HEAD && + test $orig_head != `git show-ref --hash --head HEAD` +' + test_done
-- 1.6.6
Re: [PATCHv3] filter-branch: Add tests for submodules
Michal Sojka <sojkam1@fel.cvut.cz> writes:
> There are three important tests:
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.
I'd suggest to just start with "Add three tests to make sure:".
> 1) 'rewrite submodule with another content' passes only with the > previous patch applied.
Sorry, but I think I am missing some context here to understand this sentence. Which previous patch?
> 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.
I thought you agreed with Hannes that this is not something we would even want to support?
> 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> > ---
Thanks.
Re: [PATCHv3] filter-branch: Add tests for submodules
On Thursday 28 of January 2010 01:14:07 Junio C Hamano wrote:
> Michal Sojka <sojkam1@fel.cvut.cz> writes: > > There are three important tests: > > 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. > > I'd suggest to just start with "Add three tests to make sure:".
OK
> > 1) 'rewrite submodule with another content' passes only with the > > previous patch applied. > > Sorry, but I think I am missing some context here to understand this > sentence. Which previous patch?
I should have sent both patches. Sorry for that. I'll send them now.
> > 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. > > I thought you agreed with Hannes that this is not something we would even > want to support?
Yes. I'm removing this test.
Cheers, Michal
[PATCHv4 1/2] filter-branch: Fix to allow replacing submodules with another content
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
[PATCHv4 2/2] filter-branch: Add tests for submodules in tree-filter
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 2>&1 + then git update-index --cacheinfo 160000 0123456789012345678901234567890123456789 submod + fi" HEAD && + test $orig_head != `git show-ref --hash --head HEAD` +' + test_done
-- 1.6.6
Re: [PATCHv4 2/2] filter-branch: Add tests for submodules in tree-filter
Michal Sojka <sojkam1@fel.cvut.cz> writes:
> @@ -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 * .*
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".
Also make sure you don't break the chain of "&&" unnecessarily.
> + git init && > + test_commit file && > + mkdir submod && > + submodurl="$PWD/submod" > + ( cd submod && > + git init && > + test_commit file-in-submod ) && > + git submodule add "$submodurl"
"&&"?
Re: [PATCHv4 2/2] filter-branch: Add tests for submodules in tree-filter
On Thursday 28 of January 2010 22:57:16 Junio C Hamano wrote:
> Michal Sojka <sojkam1@fel.cvut.cz> writes: > > +test_expect_success 'setup submodule' ' > > + rm -rf * .* > > Yikes. Please don't do this. > > [...] > > but it would be preferable to be even more explicit "rm -fr ?* .git".
Fixed. I did it like you suggested.
> Also make sure you don't break the chain of "&&" unnecessarily. >
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
[PATCHv5 1/2] filter-branch: Fix to allow replacing submodules with another content
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
[PATCHv5 2/2] filter-branch: Add tests for submodules in tree-filter
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 --tree-filter \ + "if git ls-files --error-unmatch -- submod > /dev/null 2>&1 + then git update-index --cacheinfo 160000 0123456789012345678901234567890123456789 submod + fi" HEAD +' +test_expect_success 'test that revision was replaced' ' + test "`git ls-files --stage submod`" = "160000 0123456789012345678901234567890123456789 0 submod" && + test $orig_head != `git rev-parse HEAD` +' + test_done
-- 1.6.6