[PATCH] Test rebase -Xsubtree

[PATCH] Test rebase -Xsubtree

[David Greene]

Here is a test that finds a bug in rebase -Xsubtree.  With
--preserve-merges, commits are lost.

                    -David

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

[David Greene]

From: "David A. Greene" <greened@obbligato.org>

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>
---
 t/t3427-rebase-subtree.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100755 t/t3427-rebase-subtree.sh

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
new file mode 100755
index 0000000..7eb28ab
--- /dev/null
+++ 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})"
+}
+
+check_equal()
+{
+	test_debug 'echo'
+	test_debug "echo \"check a:\" \"{$1}\""
+	test_debug "echo \"      b:\" \"{$2}\""
+	if [ "$1" = "$2" ]; then
+		return 0
+	else
+		return 1
+	fi
+}
+
+last_commit_message()
+{
+	git log --pretty=format:%s -1
+}
+
+test_expect_success 'setup' '
+	test_commit README &&
+	mkdir files &&
+	cd files &&
+	git init &&
+	test_commit master1 &&
+	test_commit master2 &&
+	test_commit master3 &&
+	cd .. &&
+	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}) &&
+	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 &&
+	check_equal "$(last_commit_message)" "files_subtree/master5"
+'
+
+test_done
-- 
2.6.1

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

[Torsten Bögershausen]

Need to drop
David Greene <greened@obbligato.org>
from List, no MX record

On 2016-01-05 05.40, David Greene wrote:
> From: "David A. Greene" <greened@obbligato.org>
> 
> 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>
> ---
>  t/t3427-rebase-subtree.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100755 t/t3427-rebase-subtree.sh
> 
> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
> new file mode 100755
> index 0000000..7eb28ab
> --- /dev/null
> +++ 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})"
> +}
> +
> +check_equal()
> +{
> +	test_debug 'echo'
> +	test_debug "echo \"check a:\" \"{$1}\""
> +	test_debug "echo \"      b:\" \"{$2}\""
> +	if [ "$1" = "$2" ]; then
> +		return 0
> +	else
> +		return 1
> +	fi
> +}
> +
> +last_commit_message()
> +{
> +	git log --pretty=format:%s -1
> +}
> +
> +test_expect_success 'setup' '
> +	test_commit README &&
> +	mkdir files &&
When cd'ing into a directory,
we need to do it in a sub-shell:
> +	cd files &&
> +	git init &&
> +	test_commit master1 &&
> +	test_commit master2 &&
> +	test_commit master3 &&
> +	cd .. &&
	mkdir files &&
	(
	cd files &&
	git init &&
	test_commit master1 &&
	test_commit master2 &&
	test_commit master3
	)


(And similar below)
> +	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}) &&
> +	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 &&
> +	check_equal "$(last_commit_message)" "files_subtree/master5"
> +'
> +
> +test_done
> 

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

[Dennis Kaarsemaker]

On di, 2016-01-05 at 09:47 +0100, Torsten Bögershausen wrote:
> Need to drop
> David Greene <greened@obbligato.org>
> from List, no MX record

seahawk:~$ dig MX obbligato.org
obbligato.org.		1800	IN	MX	10
mail.obbligato.org.
seahawk:~$ dig mail.obbligato.org
mail.obbligato.org.	1800	IN	CNAME	obbligato
.org.
obbligato.org.		1800	IN	A	173.255.19
9.253

So it has an MX record, it's just incorrect: MX records must not point
to things that are CNAMEs.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

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

[Torsten Bögershausen]

On 2016-01-05 10.57, Dennis Kaarsemaker wrote:
> On di, 2016-01-05 at 09:47 +0100, Torsten Bögershausen wrote:
>> Need to drop
>> David Greene <greened@obbligato.org>
>> from List, no MX record
> 
> seahawk:~$ dig MX obbligato.org
> obbligato.org.		1800	IN	MX	10
> mail.obbligato.org.
> seahawk:~$ dig mail.obbligato.org
> mail.obbligato.org.	1800	IN	CNAME	obbligato
> .org.
> obbligato.org.		1800	IN	A	173.255.19
> 9.253
> 
> So it has an MX record, it's just incorrect: MX records must not point
> to things that are CNAMEs.
> 
This may be a problem from web.de:

An error occurred while sending mail. The mail server responded:
Requested action not taken: mailbox unavailable
invalid DNS MX or A/AAAA resource record.
Please check the message recipient "greened@obbligato.org" and try again.

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

[Eric Sunshine]

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

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

[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

[PATCH v2] Test rebase -Xsubtree

[David Greene]

Version 2 of the patch to test and expose problems with rebase -Xsubtree.
I've included an additional failure mode John reproduced and responded
to the comments from Eric.  The tests still check the last commit message
for correctness because I'm not sure what would work better.  I'm open
to learning new tricks.  :)

                       -David

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

[David Greene]

From: "David A. Greene" <greened@obbligato.org>

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 or the rebase aborts with an internal error.  The tests are
marked to expect failure so that we don't forget to fix it.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 t/t3427-rebase-subtree.sh | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100755 t/t3427-rebase-subtree.sh

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
new file mode 100755
index 0000000..add3b79
--- /dev/null
+++ b/t/t3427-rebase-subtree.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='git rebase tests for -Xsubtree
+
+This test runs git rebase and tests the subtree strategy.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+check_equal() {
+	test_debug 'echo'
+	test_debug "echo \"check a:\" \"{$1}\""
+	test_debug "echo \"      b:\" \"{$2}\""
+	test "$1" = "$2"
+}
+
+last_commit_message() {
+	git log --pretty=format:%s -1
+}
+
+test_expect_success 'setup' '
+	test_commit README &&
+	mkdir files &&
+	(
+		cd files &&
+		git init &&
+		test_commit master1 &&
+		test_commit master2 &&
+		test_commit master3
+	) &&
+	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) &&
+	git reset $commit &&
+	(
+		cd files_subtree &&
+		test_commit master4
+	) &&
+	test_commit files_subtree/master5
+'
+
+# Does not preserve master4 and master5.
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-preserve-merges 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 &&
+	check_equal "$(last_commit_message)" "files_subtree/master5"
+'
+
+# Does not preserve master4, master5 and empty.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	check_equal "$(last_commit_message)" "Empty commit"
+'
+
+# fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto' '
+	reset_rebase &&
+	git checkout -b rebase-onto master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	check_equal "$(last_commit_message)" "Empty commit"
+'
+
+test_done
-- 
2.6.1

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

[Eric Sunshine]

On Sun, Jan 10, 2016 at 6:08 PM, David Greene <greened@obbligato.org> wrote:
> From: "David A. Greene" <greened@obbligato.org>
>
> 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 or the rebase aborts with an internal error.  The tests are
> marked to expect failure so that we don't forget to fix it.

This version looks better. A few minor comments below (not necessarily
deserving a re-roll)...

> 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,79 @@
> +#!/bin/sh
> +
> +test_description='git rebase tests for -Xsubtree
> +
> +This test runs git rebase and tests the subtree strategy.
> +'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +check_equal() {
> +       test_debug 'echo'
> +       test_debug "echo \"check a:\" \"{$1}\""
> +       test_debug "echo \"      b:\" \"{$2}\""
> +       test "$1" = "$2"
> +}

I'm still curious as to why check_equal() is preferred over
test-lib-functions.sh:verbose().

> +last_commit_message() {
> +       git log --pretty=format:%s -1
> +}
> +
> +test_expect_success 'setup' '
> +       test_commit README &&
> +       mkdir files &&
> +       (
> +               cd files &&
> +               git init &&
> +               test_commit master1 &&
> +               test_commit master2 &&
> +               test_commit master3
> +       ) &&
> +       test_debug "echo Add project master to master" &&

Are these test_debug invocations still useful now that the test has
been fully developed?

> +       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) &&
> +       git reset $commit &&
> +       (
> +               cd files_subtree &&
> +               test_commit master4
> +       ) &&
> +       test_commit files_subtree/master5
> +'
> +
> +# Does not preserve master4 and master5.

This comment is explaining why the test is marked "failure", right?
When someone gets around to fixing the breakage and toggling this to
"success", there is a reasonably good chance that the comment will be
overlooked and thus become stale. Perhaps prefixing the comment with a
bold "FAILURE:" would serve as a reminder that the comment should be
dropped when the problem is fixed?

> +test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-preserve-merges 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 &&
> +       check_equal "$(last_commit_message)" "files_subtree/master5"
> +'
> +
> +# Does not preserve master4, master5 and empty.
> +test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-keep-empty master &&
> +       git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> +       git commit -m "Empty commit" --allow-empty &&
> +       git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
> +       check_equal "$(last_commit_message)" "Empty commit"
> +'
> +
> +# fatal: Could not parse object
> +test_expect_failure 'Rebase -Xsubtree --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-onto master &&
> +       git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> +       git commit -m "Empty commit" --allow-empty &&
> +       git rebase -Xsubtree=files_subtree --onto files-master master &&
> +       check_equal "$(last_commit_message)" "Empty commit"
> +'
> +
> +test_done
> --
> 2.6.1

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

[David A. Greene]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Jan 10, 2016 at 6:08 PM, David Greene <greened@obbligato.org> wrote:
>> From: "David A. Greene" <greened@obbligato.org>
>>
>> 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 or the rebase aborts with an internal error.  The tests are
>> marked to expect failure so that we don't forget to fix it.
>
> This version looks better. A few minor comments below (not necessarily
> deserving a re-roll)...

I'll re-roll because I think your comments make sense.

>> 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,79 @@
>> +#!/bin/sh
>> +
>> +test_description='git rebase tests for -Xsubtree
>> +
>> +This test runs git rebase and tests the subtree strategy.
>> +'
>> +. ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-rebase.sh
>> +
>> +check_equal() {
>> +       test_debug 'echo'
>> +       test_debug "echo \"check a:\" \"{$1}\""
>> +       test_debug "echo \"      b:\" \"{$2}\""
>> +       test "$1" = "$2"
>> +}
>
> I'm still curious as to why check_equal() is preferred over
> test-lib-functions.sh:verbose().

I can change it.  Better to use standard tools when available.  I like
the output from test_debug when I want to look at it but that's a
relatively minor thing.

>> +last_commit_message() {
>> +       git log --pretty=format:%s -1
>> +}
>> +
>> +test_expect_success 'setup' '
>> +       test_commit README &&
>> +       mkdir files &&
>> +       (
>> +               cd files &&
>> +               git init &&
>> +               test_commit master1 &&
>> +               test_commit master2 &&
>> +               test_commit master3
>> +       ) &&
>> +       test_debug "echo Add project master to master" &&
>
> Are these test_debug invocations still useful now that the test has
> been fully developed?

Yeah, I'll remove these.

>> +       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) &&
>> +       git reset $commit &&
>> +       (
>> +               cd files_subtree &&
>> +               test_commit master4
>> +       ) &&
>> +       test_commit files_subtree/master5
>> +'
>> +
>> +# Does not preserve master4 and master5.
>
> This comment is explaining why the test is marked "failure", right?

Right.

> When someone gets around to fixing the breakage and toggling this to
> "success", there is a reasonably good chance that the comment will be
> overlooked and thus become stale. Perhaps prefixing the comment with a
> bold "FAILURE:" would serve as a reminder that the comment should be
> dropped when the problem is fixed?

Good idea.

                        -David

[PATCH v3 contrib/subtree 1/1] Add a test for subtree rebase that loses commits

[David Greene]

From: David A. Greene <greened@obbligato.org>

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 or the rebase aborts with an internal error.  The tests are
marked to expect failure so that we don't forget to fix it.

Signed-off-by: David A. Greene <greened@obbligato.org>
---

Notes:
    Change History:
    
    v1 - Initial version
    v2 - Additional tests and code cleanup
    v3 - Remove check_equal, mark comments on failure and remove
         test_debug statements

 t/t3427-rebase-subtree.sh | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
new file mode 100755
index 0000000..add3b79
--- /dev/null
+++ b/t/t3427-rebase-subtree.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='git rebase tests for -Xsubtree
+
+This test runs git rebase and tests the subtree strategy.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+check_equal() {
+	test_debug 'echo'
+	test_debug "echo \"check a:\" \"{$1}\""
+	test_debug "echo \"      b:\" \"{$2}\""
+	test "$1" = "$2"
+}
+
+last_commit_message() {
+	git log --pretty=format:%s -1
+}
+
+test_expect_success 'setup' '
+	test_commit README &&
+	mkdir files &&
+	(
+		cd files &&
+		git init &&
+		test_commit master1 &&
+		test_commit master2 &&
+		test_commit master3
+	) &&
+	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) &&
+	git reset $commit &&
+	(
+		cd files_subtree &&
+		test_commit master4
+	) &&
+	test_commit files_subtree/master5
+'
+
+# Does not preserve master4 and master5.
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-preserve-merges 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 &&
+	check_equal "$(last_commit_message)" "files_subtree/master5"
+'
+
+# Does not preserve master4, master5 and empty.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	check_equal "$(last_commit_message)" "Empty commit"
+'
+
+# fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto' '
+	reset_rebase &&
+	git checkout -b rebase-onto master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	check_equal "$(last_commit_message)" "Empty commit"
+'
+
+test_done
-- 
2.6.1

Re: [PATCH v3 contrib/subtree 1/1] Add a test for subtree rebase that loses commits

[Eric Sunshine]

On Sun, Jan 17, 2016 at 6:13 PM, David Greene <greened@obbligato.org> wrote:
> From: David A. Greene <greened@obbligato.org>
>
> 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 or the rebase aborts with an internal error.  The tests are
> marked to expect failure so that we don't forget to fix it.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
>
> Notes:
>     Change History:
>
>     v1 - Initial version
>     v2 - Additional tests and code cleanup
>     v3 - Remove check_equal, mark comments on failure and remove
>          test_debug statements

Hmm, the v3 changes described here don't appear in this version. In
fact, v2 and v3 are identical.

>  t/t3427-rebase-subtree.sh | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
> new file mode 100755
> index 0000000..add3b79
> --- /dev/null
> +++ b/t/t3427-rebase-subtree.sh
> @@ -0,0 +1,79 @@
> +#!/bin/sh
> +
> +test_description='git rebase tests for -Xsubtree
> +
> +This test runs git rebase and tests the subtree strategy.
> +'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +check_equal() {
> +       test_debug 'echo'
> +       test_debug "echo \"check a:\" \"{$1}\""
> +       test_debug "echo \"      b:\" \"{$2}\""
> +       test "$1" = "$2"
> +}
> +
> +last_commit_message() {
> +       git log --pretty=format:%s -1
> +}
> +
> +test_expect_success 'setup' '
> +       test_commit README &&
> +       mkdir files &&
> +       (
> +               cd files &&
> +               git init &&
> +               test_commit master1 &&
> +               test_commit master2 &&
> +               test_commit master3
> +       ) &&
> +       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) &&
> +       git reset $commit &&
> +       (
> +               cd files_subtree &&
> +               test_commit master4
> +       ) &&
> +       test_commit files_subtree/master5
> +'
> +
> +# Does not preserve master4 and master5.
> +test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-preserve-merges 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 &&
> +       check_equal "$(last_commit_message)" "files_subtree/master5"
> +'
> +
> +# Does not preserve master4, master5 and empty.
> +test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-keep-empty master &&
> +       git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> +       git commit -m "Empty commit" --allow-empty &&
> +       git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
> +       check_equal "$(last_commit_message)" "Empty commit"
> +'
> +
> +# fatal: Could not parse object
> +test_expect_failure 'Rebase -Xsubtree --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-onto master &&
> +       git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> +       git commit -m "Empty commit" --allow-empty &&
> +       git rebase -Xsubtree=files_subtree --onto files-master master &&
> +       check_equal "$(last_commit_message)" "Empty commit"
> +'
> +
> +test_done
> --
> 2.6.1

Re: [PATCH v3 contrib/subtree 1/1] Add a test for subtree rebase that loses commits

[David A. Greene]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Jan 17, 2016 at 6:13 PM, David Greene <greened@obbligato.org> wrote:
>> From: David A. Greene <greened@obbligato.org>
>>
>> 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 or the rebase aborts with an internal error.  The tests are
>> marked to expect failure so that we don't forget to fix it.
>>
>> Signed-off-by: David A. Greene <greened@obbligato.org>
>> ---
>>
>> Notes:
>>     Change History:
>>
>>     v1 - Initial version
>>     v2 - Additional tests and code cleanup
>>     v3 - Remove check_equal, mark comments on failure and remove
>>          test_debug statements
>
> Hmm, the v3 changes described here don't appear in this version. In
> fact, v2 and v3 are identical.

Dang, you caught me before I could reply.  :)

Yes, I botched this one.  Sending v4... ;)

                          -David

[PATCH v4 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[David Greene]

From: David A. Greene <greened@obbligato.org>

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 or the rebase aborts with an internal error.  The tests are
marked to expect failure so that we don't forget to fix it.

Signed-off-by: David A. Greene <greened@obbligato.org>
---

Notes:
    Change History:
    
    v1 - Initial version
    v2 - Additional tests and code cleanup
    v3 - Remove check_equal, mark comments on failure and remove
         test_debug statements
    v4 - Send correct v3 test (botched v3)

 t/t3427-rebase-subtree.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
new file mode 100755
index 0000000..4d47f77
--- /dev/null
+++ b/t/t3427-rebase-subtree.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='git rebase tests for -Xsubtree
+
+This test runs git rebase and tests the subtree strategy.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+last_commit_message() {
+	git log --pretty=format:%s -1
+}
+
+test_expect_success 'setup' '
+	test_commit README &&
+	mkdir files &&
+	(
+		cd files &&
+		git init &&
+		test_commit master1 &&
+		test_commit master2 &&
+		test_commit master3
+	) &&
+	git fetch files master &&
+	git branch files-master FETCH_HEAD &&
+	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) &&
+	git reset $commit &&
+	(
+		cd files_subtree &&
+		test_commit master4
+	) &&
+	test_commit files_subtree/master5
+'
+
+# FAILURE: Does not preserve master4 and master5.
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-preserve-merges 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 &&
+	verbose "$(last_commit_message)" "files_subtree/master5"
+'
+
+# FAILURE: Does not preserve master4, master5 and empty.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose "$(last_commit_message)" "Empty commit"
+'
+
+# FAILURE: fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto' '
+	reset_rebase &&
+	git checkout -b rebase-onto master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose "$(last_commit_message)" "Empty commit"
+'
+
+test_done
-- 
2.6.1

Re: [PATCH v4 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[Eric Sunshine]

On Sun, Jan 17, 2016 at 6:43 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 or the rebase aborts with an internal error.  The tests are
> 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,70 @@
> +# FAILURE: Does not preserve master4 and master5.
> +test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-preserve-merges 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 &&
> +       verbose "$(last_commit_message)" "files_subtree/master5"

Hmm, does this test succeed? If it does, it's only by accident.
verbose() is just a helper for printing the the expression being
tested upon failure, but you still need to supply a proper expression
for testing. It is intended to be used like this:

    verbose test "$(last_commit_message)" = files_subtree/master5

Same comment applies to the remaining tests.

> +'
> +
> +# FAILURE: Does not preserve master4, master5 and empty.
> +test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-keep-empty master &&
> +       git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> +       git commit -m "Empty commit" --allow-empty &&
> +       git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
> +       verbose "$(last_commit_message)" "Empty commit"
> +'
> +
> +# FAILURE: fatal: Could not parse object
> +test_expect_failure 'Rebase -Xsubtree --onto' '
> +       reset_rebase &&
> +       git checkout -b rebase-onto master &&
> +       git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
> +       git commit -m "Empty commit" --allow-empty &&
> +       git rebase -Xsubtree=files_subtree --onto files-master master &&
> +       verbose "$(last_commit_message)" "Empty commit"
> +'
> +
> +test_done
> --
> 2.6.1

Re: [PATCH v4 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[David A. Greene]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Jan 17, 2016 at 6:43 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 or the rebase aborts with an internal error.  The tests are
>> 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,70 @@
>> +# FAILURE: Does not preserve master4 and master5.
>> +test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
>> +       reset_rebase &&
>> +       git checkout -b rebase-preserve-merges 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 &&
>> +       verbose "$(last_commit_message)" "files_subtree/master5"
>
> Hmm, does this test succeed? If it does, it's only by accident.
> verbose() is just a helper for printing the the expression being
> tested upon failure, but you still need to supply a proper expression
> for testing. It is intended to be used like this:
>
>     verbose test "$(last_commit_message)" = files_subtree/master5
>
> Same comment applies to the remaining tests.

Blast.  Yes, it did pass (expect failure) but it's definitely wrong.  On
to v5!

                           -David

[PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[David Greene]

From: David A. Greene <greened@obbligato.org>

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 or the rebase aborts with an internal error.  The tests are
marked to expect failure so that we don't forget to fix it.

Signed-off-by: David A. Greene <greened@obbligato.org>
---

Notes:
    Change History:
    
    v1 - Initial version
    v2 - Additional tests and code cleanup
    v3 - Remove check_equal, mark comments on failure and remove
         test_debug statements
    v4 - Send correct v3 test (botched v3)
    v5 - Fix use of verbose

 t/t3427-rebase-subtree.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
new file mode 100755
index 0000000..a68a1b1
--- /dev/null
+++ b/t/t3427-rebase-subtree.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='git rebase tests for -Xsubtree
+
+This test runs git rebase and tests the subtree strategy.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+last_commit_message() {
+	git log --pretty=format:%s -1
+}
+
+test_expect_success 'setup' '
+	test_commit README &&
+	mkdir files &&
+	(
+		cd files &&
+		git init &&
+		test_commit master1 &&
+		test_commit master2 &&
+		test_commit master3
+	) &&
+	git fetch files master &&
+	git branch files-master FETCH_HEAD &&
+	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) &&
+	git reset $commit &&
+	(
+		cd files_subtree &&
+		test_commit master4
+	) &&
+	test_commit files_subtree/master5
+'
+
+# FAILURE: Does not preserve master4 and master5.
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-preserve-merges 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 &&
+	verbose test "$(last_commit_message)" = "files_subtree/master5"
+'
+
+# FAILURE: Does not preserve master4, master5 and empty.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose test "$(last_commit_message)" = "Empty commit"
+'
+
+# FAILURE: fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto' '
+	reset_rebase &&
+	git checkout -b rebase-onto master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose test "$(last_commit_message)" = "Empty commit"
+'
+
+test_done
-- 
2.6.1

Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[Eric Sunshine]

On Mon, Jan 18, 2016 at 9:59 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 or the rebase aborts with an internal error.  The tests are
> marked to expect failure so that we don't forget to fix it.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
> Notes:
>     Change History:
>
>     v1 - Initial version
>     v2 - Additional tests and code cleanup
>     v3 - Remove check_equal, mark comments on failure and remove
>          test_debug statements
>     v4 - Send correct v3 test (botched v3)
>     v5 - Fix use of verbose

Thanks, I think this re-roll addresses all the (relatively
superficial) issues raised by my reviews of previous versions.

Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[Junio C Hamano]

David Greene <greened@obbligato.org> writes:

> From: David A. Greene <greened@obbligato.org>
>
> 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 or the rebase aborts with an internal error.  The tests are
> marked to expect failure so that we don't forget to fix it.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
>
> Notes:
>     Change History:
>     
>     v1 - Initial version
>     v2 - Additional tests and code cleanup
>     v3 - Remove check_equal, mark comments on failure and remove
>          test_debug statements
>     v4 - Send correct v3 test (botched v3)
>     v5 - Fix use of verbose

Thanks, both.  Will queue.

I have a couple of questions and comments, though.

> +test_expect_success 'setup' '
> +	test_commit README &&
> +...
> +	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) &&

I think at this point, your index and working tree states match that
of $commit.  So the next command ...

> +	git reset $commit &&

... made me wonder what its significance was.  I think you are doing
this solely to move the HEAD pointer to point at $commit, but then
it would be much better and more readable to use update-ref, i.e.
making this line to:

	git update-ref HEAD $commit &&

instead, as "write-tree && commit-tree && update-ref" is a familiar
pattern to reimplement "git commit" using the plumbing.  Ending that
three-command sequence with "reset" breaks the pattern.

> +	(
> +		cd files_subtree &&
> +		test_commit master4
> +	) &&
> +	test_commit files_subtree/master5

I understand that you are creating these two commits both in the
top-level repository (the one the history initially created in
"files" repository gets merged into), but you are creating them
slightly differently.  Is that significant?  I am not complaining
about the style of writing tests, but I am wondering if having these
two commits created differently has any effect on the bug you
observed, which may be a good starting point for anybody who wants
to fix it to start digging from.  IOW, would the resulting history
different if you did this instead?

	test_commit files_subtree/master4 &&
	test_commit files_subtree/master5

I also notice that files_subtree/master4 does not appear in any of
the verification in the three tests that use the history being
prepared here, i.e. if master4 is silently dropped while master5 is
kept, such a bug won't be caught by them.

Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[David A. Greene]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks, both.  Will queue.
>
> I have a couple of questions and comments, though.
>
>> +test_expect_success 'setup' '
>> +	test_commit README &&
>> +...
>> +	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) &&
>
> I think at this point, your index and working tree states match that
> of $commit.  So the next command ...
>
>> +	git reset $commit &&
>
> ... made me wonder what its significance was.  I think you are doing
> this solely to move the HEAD pointer to point at $commit, but then
> it would be much better and more readable to use update-ref, i.e.
> making this line to:
>
> 	git update-ref HEAD $commit &&
>
> instead, as "write-tree && commit-tree && update-ref" is a familiar
> pattern to reimplement "git commit" using the plumbing.  Ending that
> three-command sequence with "reset" breaks the pattern.

Ok, that makes sense.

>> +	(
>> +		cd files_subtree &&
>> +		test_commit master4
>> +	) &&
>> +	test_commit files_subtree/master5
>
> I understand that you are creating these two commits both in the
> top-level repository (the one the history initially created in
> "files" repository gets merged into), but you are creating them
> slightly differently.  Is that significant?  I am not complaining
> about the style of writing tests, but I am wondering if having these
> two commits created differently has any effect on the bug you
> observed, which may be a good starting point for anybody who wants
> to fix it to start digging from.  IOW, would the resulting history
> different if you did this instead?
>
> 	test_commit files_subtree/master4 &&
> 	test_commit files_subtree/master5

That is a good question.  I originally created the test to see if making
these two commits differently would cause any problems with
git-subtree's split command.  Obviously, I didn't get that far.  :)

So I think it makes sense for me to at least test and see what happens.
I will add another test to this set if it makes a difference.

> I also notice that files_subtree/master4 does not appear in any of
> the verification in the three tests that use the history being
> prepared here, i.e. if master4 is silently dropped while master5 is
> kept, such a bug won't be caught by them.

Ah, good catch.  I should add a test for that.

Let me do a re-roll of this since I think you bring up some excellent
points.  Might be a few days due to work obbligations.

                        -David

Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[Junio C Hamano]

greened@obbligato.org (David A. Greene) writes:

>> I also notice that files_subtree/master4 does not appear in any of
>> the verification in the three tests that use the history being
>> prepared here, i.e. if master4 is silently dropped while master5 is
>> kept, such a bug won't be caught by them.
>
> Ah, good catch.  I should add a test for that.
>
> Let me do a re-roll of this since I think you bring up some excellent
> points.  Might be a few days due to work obbligations.

A friendly ping to see if I missed anything that happened after this
message...

Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[David A. Greene]

Junio C Hamano <gitster@pobox.com> writes:

> greened@obbligato.org (David A. Greene) writes:
>
>>> I also notice that files_subtree/master4 does not appear in any of
>>> the verification in the three tests that use the history being
>>> prepared here, i.e. if master4 is silently dropped while master5 is
>>> kept, such a bug won't be caught by them.
>>
>> Ah, good catch.  I should add a test for that.
>>
>> Let me do a re-roll of this since I think you bring up some excellent
>> points.  Might be a few days due to work obbligations.
>
> A friendly ping to see if I missed anything that happened after this
> message...

Just sent it.  I guess it took more than a few days... :)

                   -David

[PATCH v6 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

[David Greene]

From: David A. Greene <greened@obbligato.org>

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 or the rebase aborts with an internal error.  The tests are
marked to expect failure so that we don't forget to fix it.

Signed-off-by: David A. Greene <greened@obbligato.org>
---

Notes:
    Change History:
    
    v1 - Initial version
    v2 - Additional tests and code cleanup
    v3 - Remove check_equal, mark comments on failure and remove
         test_debug statements
    v4 - Send correct v3 test (botched v3)
    v5 - Fix use of verbose
    v6 - Add individual tests for each potentially dropped commit

 t/t3427-rebase-subtree.sh | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
new file mode 100755
index 0000000..3780877
--- /dev/null
+++ b/t/t3427-rebase-subtree.sh
@@ -0,0 +1,119 @@
+#!/bin/sh
+
+test_description='git rebase tests for -Xsubtree
+
+This test runs git rebase and tests the subtree strategy.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+commit_message() {
+	git log --pretty=format:%s -1 "$1"
+}
+
+test_expect_success 'setup' '
+	test_commit README &&
+	mkdir files &&
+	(
+		cd files &&
+		git init &&
+		test_commit master1 &&
+		test_commit master2 &&
+		test_commit master3
+	) &&
+	git fetch files master &&
+	git branch files-master FETCH_HEAD &&
+	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) &&
+	git update-ref HEAD $commit &&
+	(
+		cd files_subtree &&
+		test_commit master4
+	) &&
+	test_commit files_subtree/master5
+'
+
+# FAILURE: Does not preserve master4.
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 4' '
+	reset_rebase &&
+	git checkout -b rebase-preserve-merges-4 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 &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master4"
+'
+
+# FAILURE: Does not preserve master5.
+test_expect_failure 'Rebase -Xsubtree --preserve-merges --onto commit 5' '
+	reset_rebase &&
+	git checkout -b rebase-preserve-merges-5 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 &&
+	verbose test "$(commit_message HEAD)" = "files_subtree/master5"
+'
+
+# FAILURE: Does not preserve master4.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 4' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty-4 master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
+'
+
+# FAILURE: Does not preserve master5.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto commit 5' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty-5 master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
+'
+
+# FAILURE: Does not preserve Empty.
+test_expect_failure 'Rebase -Xsubtree --keep-empty --preserve-merges --onto empty commit' '
+	reset_rebase &&
+	git checkout -b rebase-keep-empty-empty master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --keep-empty --preserve-merges --onto files-master master &&
+	verbose test "$(commit_message HEAD)" = "Empty commit"
+'
+
+# FAILURE: fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto commit 4' '
+	reset_rebase &&
+	git checkout -b rebase-onto-4 master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose test "$(commit_message HEAD~2)" = "files_subtree/master4"
+'
+
+# FAILURE: fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto commit 5' '
+	reset_rebase &&
+	git checkout -b rebase-onto-5 master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose test "$(commit_message HEAD~)" = "files_subtree/master5"
+'
+# FAILURE: fatal: Could not parse object
+test_expect_failure 'Rebase -Xsubtree --onto empty commit' '
+	reset_rebase &&
+	git checkout -b rebase-onto-empty master &&
+	git filter-branch --prune-empty -f --subdirectory-filter files_subtree &&
+	git commit -m "Empty commit" --allow-empty &&
+	git rebase -Xsubtree=files_subtree --onto files-master master &&
+	verbose test "$(commit_message HEAD)" = "Empty commit"
+'
+
+test_done
-- 
2.8.1