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

Eric Sunshine [off-list ref] writes:

On Mon, Jan 4, 2016 at 11:40 PM, David Greene [off-list ref] wrote:

quoted

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

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.

quoted

+check_equal()
+{

Style: Place brace on the same line as the function declaration.

quoted

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

quoted

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

quoted

+}

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

Dunno.  I'll have to look.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+       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