# [PATCH] Add a test for subtree rebase that loses commits 24 messages from 2016-01-05 to 2016-06-28. Participants: David Greene, Torsten Bögershausen, Dennis Kaarsemaker, Eric Sunshine, David A. Greene, Junio C Hamano. Thread: https://gitlist.dev/t/41124 ## David Greene, 2016-01-05 04:40 Subject: [PATCH] Test rebase -Xsubtree Message-ID: <1451968805-6948-1-git-send-email-greened@obbligato.org> URL: https://gitlist.dev/e/1451968805-6948-1-git-send-email-greened%40obbligato.org ``` Here is a test that finds a bug in rebase -Xsubtree. With --preserve-merges, commits are lost. -David ``` ## David Greene, 2016-01-05 04:40 Subject: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <1451968805-6948-2-git-send-email-greened@obbligato.org> URL: https://gitlist.dev/e/1451968805-6948-2-git-send-email-greened%40obbligato.org In-Reply-To: <1451968805-6948-1-git-send-email-greened@obbligato.org> ``` From: "David A. Greene" 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 --- 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 ``` ## Torsten Bögershausen, 2016-01-05 08:47 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <568B833B.4060001@web.de> URL: https://gitlist.dev/e/568B833B.4060001%40web.de In-Reply-To: <1451968805-6948-2-git-send-email-greened@obbligato.org> ``` Need to drop David Greene from List, no MX record On 2016-01-05 05.40, David Greene wrote: > From: "David A. Greene" > > 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 > --- > 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 > ``` ## Dennis Kaarsemaker, 2016-01-05 09:57 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <1451987857.2668.5.camel@kaarsemaker.net> URL: https://gitlist.dev/e/1451987857.2668.5.camel%40kaarsemaker.net In-Reply-To: <568B833B.4060001@web.de> ``` On di, 2016-01-05 at 09:47 +0100, Torsten Bögershausen wrote: > Need to drop > David Greene > 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, 2016-01-05 11:18 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <568BA670.6070104@web.de> URL: https://gitlist.dev/e/568BA670.6070104%40web.de In-Reply-To: <1451987857.2668.5.camel@kaarsemaker.net> ``` 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 >> 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, 2016-01-05 20:34 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSOzwGdp-FACM2%3DwK78KSjvEZoB6VKiEtBLnBX0G1L4QQ%40mail.gmail.com In-Reply-To: <1451968805-6948-2-git-send-email-greened@obbligato.org> ``` On Mon, Jan 4, 2016 at 11:40 PM, David Greene 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 > --- > 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 ``` ## David A. Greene, 2016-01-05 21:14 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <87si2bwwyd.fsf@waller.obbligato.org> URL: https://gitlist.dev/e/87si2bwwyd.fsf%40waller.obbligato.org In-Reply-To: ``` Eric Sunshine writes: > On Mon, Jan 4, 2016 at 11:40 PM, David Greene 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 >> --- >> 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 ``` ## David Greene, 2016-01-10 23:08 Subject: [PATCH v2] Test rebase -Xsubtree Message-ID: <1452467297-16868-1-git-send-email-greened@obbligato.org> URL: https://gitlist.dev/e/1452467297-16868-1-git-send-email-greened%40obbligato.org In-Reply-To: <1451968805-6948-2-git-send-email-greened@obbligato.org> ``` 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, 2016-01-10 23:08 Subject: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <1452467297-16868-2-git-send-email-greened@obbligato.org> URL: https://gitlist.dev/e/1452467297-16868-2-git-send-email-greened%40obbligato.org In-Reply-To: <1452467297-16868-1-git-send-email-greened@obbligato.org> ``` From: "David A. Greene" 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 --- 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 ``` ## Eric Sunshine, 2016-01-15 01:19 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ6Dfvc4dkQVZ6BqzD76nZ4mCcqkO4eAecrMENKWtgWEg%40mail.gmail.com In-Reply-To: <1452467297-16868-2-git-send-email-greened@obbligato.org> ``` On Sun, Jan 10, 2016 at 6:08 PM, David Greene wrote: > From: "David A. Greene" > > 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 > --- > 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 ``` ## David A. Greene, 2016-01-17 22:50 Subject: Re: [PATCH] Add a test for subtree rebase that loses commits Message-ID: <87y4bnaki9.fsf@waller.obbligato.org> URL: https://gitlist.dev/e/87y4bnaki9.fsf%40waller.obbligato.org In-Reply-To: ``` Eric Sunshine writes: > On Sun, Jan 10, 2016 at 6:08 PM, David Greene wrote: >> From: "David A. Greene" >> >> 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 >> --- >> 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 ``` ## David Greene, 2016-01-17 23:13 Subject: [PATCH v3 contrib/subtree 1/1] Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/ec1decfc5fd463f1e78a5aa2636c24fb11e80a62.1453072387.git.greened%40obbligato.org In-Reply-To: ``` From: David A. Greene 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 --- 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 ``` ## Eric Sunshine, 2016-01-17 23:32 Subject: Re: [PATCH v3 contrib/subtree 1/1] Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTMAnbyT3-FFN4juUooosiydOCX-ETwTghpnKoOeXcMpQ%40mail.gmail.com In-Reply-To: ``` On Sun, Jan 17, 2016 at 6:13 PM, David Greene wrote: > From: David A. Greene > > 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 > --- > > 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 ``` ## David A. Greene, 2016-01-17 23:36 Subject: Re: [PATCH v3 contrib/subtree 1/1] Add a test for subtree rebase that loses commits Message-ID: <87h9ibaido.fsf@waller.obbligato.org> URL: https://gitlist.dev/e/87h9ibaido.fsf%40waller.obbligato.org In-Reply-To: ``` Eric Sunshine writes: > On Sun, Jan 17, 2016 at 6:13 PM, David Greene wrote: >> From: David A. Greene >> >> 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 >> --- >> >> 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, 2016-01-17 23:43 Subject: [PATCH v4 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: <047e625a28954b8fd79225b55cab7620cb5f3b1f.1453074191.git.greened@obbligato.org> URL: https://gitlist.dev/e/047e625a28954b8fd79225b55cab7620cb5f3b1f.1453074191.git.greened%40obbligato.org In-Reply-To: ``` From: David A. Greene 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 --- 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 ``` ## Eric Sunshine, 2016-01-18 18:10 Subject: Re: [PATCH v4 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS6ouc%2BkdJaz10M2AApPoDODDcgDX9Azz8ih%3D4zxxD2zg%40mail.gmail.com In-Reply-To: <047e625a28954b8fd79225b55cab7620cb5f3b1f.1453074191.git.greened@obbligato.org> ``` On Sun, Jan 17, 2016 at 6:43 PM, David Greene 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 > --- > 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 ``` ## David A. Greene, 2016-01-19 02:53 Subject: Re: [PATCH v4 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: <87wpr6l1os.fsf@waller.obbligato.org> URL: https://gitlist.dev/e/87wpr6l1os.fsf%40waller.obbligato.org In-Reply-To: ``` Eric Sunshine writes: > On Sun, Jan 17, 2016 at 6:43 PM, David Greene 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 >> --- >> 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, 2016-01-19 02:59 Subject: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: <3eb25268597083cdb10303e3d5790302e719a803.1453172369.git.greened@obbligato.org> URL: https://gitlist.dev/e/3eb25268597083cdb10303e3d5790302e719a803.1453172369.git.greened%40obbligato.org In-Reply-To: ``` From: David A. Greene 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 --- 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 ``` ## Eric Sunshine, 2016-01-19 04:21 Subject: Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQyn-TGocV8Z6UTCJtBZMT8-HCtV7HqqJ%2ByinaczGUPvg%40mail.gmail.com In-Reply-To: <3eb25268597083cdb10303e3d5790302e719a803.1453172369.git.greened@obbligato.org> ``` On Mon, Jan 18, 2016 at 9:59 PM, David Greene 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 > --- > 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, 2016-01-19 17:41 Subject: Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/xmqqsi1tbh68.fsf%40gitster.mtv.corp.google.com In-Reply-To: <3eb25268597083cdb10303e3d5790302e719a803.1453172369.git.greened@obbligato.org> ``` David Greene writes: > From: David A. Greene > > 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 > --- > > 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. ``` ## David A. Greene, 2016-01-20 04:10 Subject: Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: <87wpr4ubzo.fsf@waller.obbligato.org> URL: https://gitlist.dev/e/87wpr4ubzo.fsf%40waller.obbligato.org In-Reply-To: ``` Junio C Hamano 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 ``` ## Junio C Hamano, 2016-04-12 23:27 Subject: Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: URL: https://gitlist.dev/e/xmqqpotummqn.fsf%40gitster.mtv.corp.google.com In-Reply-To: <87wpr4ubzo.fsf@waller.obbligato.org> ``` 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... ``` ## David Greene, 2016-06-28 10:54 Subject: [PATCH v6 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: <834520c69d67b6dbe804ff67b4be00a9ac17d556.1467111148.git.greened@obbligato.org> URL: https://gitlist.dev/e/834520c69d67b6dbe804ff67b4be00a9ac17d556.1467111148.git.greened%40obbligato.org In-Reply-To: ``` From: David A. Greene 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 --- 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 ``` ## David A. Greene, 2016-06-28 10:55 Subject: Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits Message-ID: <871t3heg65.fsf@waller.obbligato.org> URL: https://gitlist.dev/e/871t3heg65.fsf%40waller.obbligato.org In-Reply-To: ``` Junio C Hamano 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 ```