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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> +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.

> +               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.

> +}

Isn't check_equal() pretty much a (less generic) re-invention of
t/test-lib-functions.sh:verbose()?

> +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.

> +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 &&
        ...
    ) &&
    ...

> +       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

> +       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.

> +       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?

> +'
> +
> +test_done
> --
> 2.6.1