Re: [PATCH] filter-branch fix and tests
- From
Michal Sojka <sojkam1@fel.cvut.cz>
- Date
- Jan 27, 2010, 23:41 UTC
- Message-ID
- <201001280041.23182.sojkam1@fel.cvut.cz>
- In-Reply-To
- <4B606748.2050209@viscovery.net>
On Wednesday 27 of January 2010 17:18:16 Johannes Sixt wrote:
Show 7 quoted lines
> 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.
Show 10 quoted lines
> > +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.
Show 15 quoted lines
> > +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