Eric Sunshine <sunsh...@sunshineco.com> writes: > On Mon, Jan 4, 2016 at 11:40 PM, David Greene <gree...@obbligato.org> wrote: >> This test merges an external tree in as a subtree, makes some commits >> on top of it and splits it back out. In the process the added commits >> are lost. This is marked to expect failure so that we don't forget to >> fix it. >> >> Signed-off-by: David A. Greene <gree...@obbligato.org> >> --- >> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh >> @@ -0,0 +1,68 @@ >> +#!/bin/sh >> + >> +test_description='git rebase tests for -Xsubtree >> + >> +This test runs git rebase and tests the subtree strategy. >> +' >> +. ./test-lib.sh >> + >> +addfile() { >> + name=$1 >> + echo $(basename ${name}) > ${name} >> + ${git} add ${name} >> + ${git} commit -m "Add $(basename ${name})" >> +} > > What is this function for? It doesn't seem to be used at all by this script.
Nothing. I had sent a mail saying not to apply the patch but it bounced. :) Will fix. >> +check_equal() >> +{ > > Style: Place brace on the same line as the function declaration. > >> + test_debug 'echo' >> + test_debug "echo \"check a:\" \"{$1}\"" >> + test_debug "echo \" b:\" \"{$2}\"" >> + if [ "$1" = "$2" ]; then > > Style: Use 'test' rather than '[', drop semi-colon, and place 'then' > on its own line. Ok. >> + return 0 >> + else >> + return 1 >> + fi > > This entire if/else/fi can be rephrased as just a single line at the > end of the function: > > test "$1" = "$2" > > the result of which will be 0 if the strings are equal, else 1, thus > there's no need for if/else/fi. Ok. >> +} > > Isn't check_equal() pretty much a (less generic) re-invention of > t/test-lib-functions.sh:verbose()? Dunno. I'll have to look. >> +last_commit_message() >> +{ >> + git log --pretty=format:%s -1 >> +} > > Are there plans to re-use this function by more than the current > single call site? If not, it might be just as clear to assign the > result of the expression to an aptly named variable directly in the > caller: > > last_commit_msg=$(git log --pretty=format:%s -1) > > or something. The intent is to add more tests later. In fact I have at least a couple more to add. >> +test_expect_success 'setup' ' >> + test_commit README && >> + mkdir files && >> + cd files && >> + git init && >> + test_commit master1 && >> + test_commit master2 && >> + test_commit master3 && >> + cd .. && > > Mentioned by Torsten: If any command before "cd .." fails, then "cd > .." won't be invoked, and subsequent tests will be executed in the > wrong directory. Use a subshell to overcome this problem since the > current directory of the parent shell is not impacted by the subshell > (thus you can drop the "cd .." altogether): > > mkdir files && > ( > cd files && > git init && > ... > ) && > ... Yep. Thanks. >> + test_debug "echo Add project master to master" && >> + git fetch files master && >> + git branch files-master FETCH_HEAD && >> + test_debug "echo Add subtree master to master via subtree" && >> + git read-tree --prefix=files_subtree files-master && >> + git checkout -- files_subtree && >> + tree=$(git write-tree) && >> + head=$(git rev-parse HEAD) && >> + rev=$(git rev-parse --verify files-master^0) && >> + commit=$(git commit-tree -p ${head} -p ${rev} -m "Add subproject >> master" ${tree}) && > > Nit: This could be less syntactically noisy by dropping the > unnecessary braces: ${head} -> $head Ok. It's the style I usually use but I'll go with the git convention. >> + git reset ${commit} && >> + cd files_subtree && >> + test_commit master4 && >> + cd .. && >> + test_commit files_subtree/master5 >> +' >> + >> +# Does not preserve master4 and master5. >> +test_expect_failure 'Rebase default' ' >> + git checkout -b rebase-default master && >> + git filter-branch --prune-empty -f --subdirectory-filter >> files_subtree && >> + git commit -m "Empty commit" --allow-empty && >> + git rebase -Xsubtree=files_subtree --preserve-merges --onto >> files-master master && > > Style: Too many spaces before --preserve-merges. Thanks. >> + check_equal "$(last_commit_message)" "files_subtree/master5" > > Hmm, is checking the commit message the best way to determine if the > expected commit was there? Why not check the commit ID instead or > something? I'll look into that. Thanks for the good feedback! -David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html