threads / patch / 41124

patchAdd a test for subtree rebase that loses commits

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

## tl;dr

24 messages between Jan 5, 2016 and Jun 28, 2016. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

David Greene· Jan 5, 2016, 04:40 UTC · re: David Greene · lore
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
Show changes to t/t3427-rebase-subtree.sh +68 −0
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
Torsten Bögershausen· Jan 5, 2016, 08:47 UTC · re: David Greene · lore

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

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

On 2016-01-05 05.40, David Greene wrote:
Show 54 quoted lines
> 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:

Show 6 quoted lines
> +	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)
Show 28 quoted lines
> +	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
>
Dennis Kaarsemaker· Jan 5, 2016, 09:57 UTC · re: Torsten Bögershausen · lore

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

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
Torsten Bögershausen· Jan 5, 2016, 11:18 UTC · re: Dennis Kaarsemaker · lore

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

On 2016-01-05 10.57, Dennis Kaarsemaker wrote:
Show 17 quoted lines
> 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.

Eric Sunshine· Jan 5, 2016, 20:34 UTC · re: David Greene · lore

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

On Mon, Jan 4, 2016 at 11:40 PM, David Greene <greened@obbligato.org> wrote:
Show 23 quoted lines
> 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.
Show 9 quoted lines
> +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 &&
        ...
    ) &&
    ...
Show 10 quoted lines
> +       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
Show 13 quoted lines
> +       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?

Show 5 quoted lines
> +'
> +
> +test_done
> --
> 2.6.1
David A. Greene· Jan 5, 2016, 21:14 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 26 quoted lines
> 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.
Show 12 quoted lines
>> +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.
Show 12 quoted lines
>> +               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.
Show 13 quoted lines
>> +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.

Show 23 quoted lines
>> +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.
Show 13 quoted lines
>> +       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.
Show 15 quoted lines
>> +       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.
Show 5 quoted lines
>> +       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
David Greene· Jan 10, 2016, 23:08 UTC · re: David Greene · lore

[PATCH v2] Test rebase -Xsubtree

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
David Greene· Jan 10, 2016, 23:08 UTC · re: David Greene · lore
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
Show changes to t/t3427-rebase-subtree.sh +79 −0
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
Eric Sunshine· Jan 15, 2016, 01:19 UTC · re: David Greene · lore

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

On Sun, Jan 10, 2016 at 6:08 PM, David Greene <greened@obbligato.org> wrote:
Show 6 quoted lines
> 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)...

Show 19 quoted lines
> 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().

Show 15 quoted lines
> +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?

Show 18 quoted lines
> +       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?

Show 32 quoted lines
> +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
David A. Greene· Jan 17, 2016, 22:50 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 10 quoted lines
> 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.
Show 22 quoted lines
>> 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.

Show 18 quoted lines
>> +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.
Show 20 quoted lines
>> +       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.
Show 5 quoted lines
> 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
David Greene· Jan 17, 2016, 23:13 UTC · re: Eric Sunshine · lore

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

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(+)
Show changes to t/t3427-rebase-subtree.sh +79 −0
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
Eric Sunshine· Jan 17, 2016, 23:32 UTC · re: David Greene · lore

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

On Sun, Jan 17, 2016 at 6:13 PM, David Greene <greened@obbligato.org> wrote:
Show 17 quoted lines
> 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.

Show 90 quoted lines
>  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
David A. Greene· Jan 17, 2016, 23:36 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 21 quoted lines
> 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
David Greene· Jan 17, 2016, 23:43 UTC · re: Eric Sunshine · lore

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

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(+)
Show changes to t/t3427-rebase-subtree.sh +70 −0
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
Eric Sunshine· Jan 18, 2016, 18:10 UTC · re: David Greene · lore

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

On Sun, Jan 17, 2016 at 6:43 PM, David Greene <greened@obbligato.org> wrote:
Show 17 quoted lines
> 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.
Show 25 quoted lines
> +'
> +
> +# 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
David A. Greene· Jan 19, 2016, 02:53 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 27 quoted lines
> 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
David Greene· Jan 19, 2016, 02:59 UTC · re: Eric Sunshine · lore

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

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(+)
Show changes to t/t3427-rebase-subtree.sh +70 −0
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
Eric Sunshine· Jan 19, 2016, 04:21 UTC · re: David Greene · lore

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

On Mon, Jan 18, 2016 at 9:59 PM, David Greene <greened@obbligato.org> wrote:
Show 16 quoted lines
> 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.

Junio C Hamano· Jan 19, 2016, 17:41 UTC · re: David Greene · lore

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

David Greene <greened@obbligato.org> writes:
Show 19 quoted lines
> 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.
Show 7 quoted lines
> +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.

Show 5 quoted lines
> +	(
> +		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.

David A. Greene· Jan 20, 2016, 04:10 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 27 quoted lines
> 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.
Show 18 quoted lines
>> +	(
>> +		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
Junio C Hamano· Apr 12, 2016, 23:27 UTC · re: David A. Greene · lore

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

greened@obbligato.org (David A. Greene) writes:
Show 9 quoted lines
>> 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...

David A. Greene· Jun 28, 2016, 10:55 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> 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
David Greene· Jun 28, 2016, 10:54 UTC · re: Junio C Hamano · lore

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

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(+)
Show changes to t/t3427-rebase-subtree.sh +119 −0
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

← back to recent threads