Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Mon, Jan 4, 2016 at 11:40 PM, David Greene <greened@xxxxxxxxxxxxx> 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 <greened@xxxxxxxxxxxxx>
>> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html