Re: [PATCH] Add a test for subtree rebase that loses commits

[greened@obbligato.org (David A. Greene)]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jan 4, 2016 at 11:40 PM, David Greene <greened@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 <greened@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