Re: [RFC PATCH] add t3420-rebase-topology
- From
Martin von Zweigbergk <martinvonz@gmail.com>
- Date
- Sep 26, 2012, 17:07 UTC
- Message-ID
- <CANiSa6i+A6fkWpkPMXiBRdT48LaSfPe2yki+AmWFAKYg02p=+g@mail.gmail.com>
- In-Reply-To
- <50582873.603@viscovery.net>
[+Chris Webb regarding "git rebase --root"]
First of all, thanks for a meticulous review!
On Tue, Sep 18, 2012 at 12:53 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 5 quoted lines
> Am 9/18/2012 8:31, schrieb Martin von Zweigbergk: > > Since here and in the following tests the test cases and test descriptions > vary in the same way, wouldn't it make sense to factor the description out > as well?
Definitely. I just couldn't think of a good way of doing it, so thanks for great and concrete suggestions!
> (Watch your quoting, though.)
Switched to putting the test body in double quotes as you suggested in your examples and used single quotes for strings within the test body.
Show 16 quoted lines
>> +run () {
>> +echo '
>> + reset &&
>> + git rebase '"$@"' --keep-empty p h &&
>> + test_range p.. "f g h"
>> +'
>> +}
>> +test_expect_success 'rebase --keep-empty keeps empty even if already in upstream' "$(run)"
>> +test_expect_failure 'rebase -m --keep-empty keeps empty even if already in upstream' "$(run -m)"
>> +test_expect_failure 'rebase -i --keep-empty keeps empty even if already in upstream' "$(run -i)"
>> +test_expect_failure 'rebase -p --keep-empty keeps empty even if already in upstream' "$(run -p)"
>
> "is in upstream" is decided by the patch text. If an empty commit is
> already in upstream, this adds another one with the same or a different
> commit message and authorship information. Dubious, but since it is
> opt-in, it should be OK.Yes, it is a little dubious. See http://thread.gmane.org/gmane.comp.version-control.git/203097/focus=203159 and Junio's answer, which I think makes sense.
Show 10 quoted lines
>> +run () {
>> +echo '
>> + reset &&
>> + git rebase '"$@"' j w &&
>> + test_range j.. "E n H" || test_range j.. "n H E"
>> +'
>
> Chaining tests with || is dangerous: you do not know whether the first
> failed because the condition is not satisfied or because of some other
> failure.Good point. Thanks.
> Why is this needed in the first place? Shouldn't the history be > deterministic, provided that the commit timestamps are all distinct?
It may be deterministic, but it's not specified, I think, so I didn't want to depend on the order. Thinking more about it, though, I think it's good to protect the current behavior from patches that change the order of the parents. Although it may not be incorrect to change the order, it would at least protect against accidental changes.
It turns out that "rebase -i" goes through the commits in --topo-order, while the others use default order, I think. Which flavor should pass the test case and which should fail (and be fixed)? I would personally prefer to say that "rebase -i" is correct in using --topo-order and that the others should be fixed. Again, it's not specified, but I would hate to have them behave differently.
Show 14 quoted lines
>> +run () {
>> +echo '
>> + reset &&
>> + git rebase '"$@"' --root c &&
>> + ! same_revision HEAD c &&
>> + test_range c "a b c"
>> +'
>> +}
>> +test_expect_success 'rebase --root is not a no-op' "$(run)"
>> +test_expect_success 'rebase -m --root is not a no-op' "$(run -m)"
>> +test_expect_success 'rebase -i --root is not a no-op' "$(run -i)"
>> +test_expect_success 'rebase -p --root is not a no-op' "$(run -p)"
>
> Why? Is it more like "--root implies --force"?It doesn't currently exactly imply --force, but the effect is the same. Also see my reply to Junio's email in this thread.
Maybe Chris has some thoughts on this?
Show 14 quoted lines
>> +run () {
>> +echo '
>> + reset &&
>> + git rebase '"$@"' --root --onto e y &&
>> + test_range e.. "x y"
>> +'
>> +}
>> +test_expect_success 'rebase --root --onto' "$(run)"
>> +test_expect_failure 'rebase -m --root --onto' "$(run -m)"
>> +test_expect_success 'rebase -i --root --onto' "$(run -i)"
>> +test_expect_success 'rebase -p --root --onto' "$(run -p)"
>
> Where does this rebase start? Ah, --root stands in for the "upstream"
> argument, hence, y is the tip to rebase. Right? Then it makes sense.Thanks for pointing that out. I changed the order to "git rebase --onto e --root y". I hope that makes it clearer.
Show 27 quoted lines
>> +test_expect_success 'rebase -p re-creates merge from upstream' ' >> + reset && >> + git rebase -p k w && >> + same_revision HEAD^ H && >> + same_revision HEAD^2 k >> +' > > IMO, this tests the wrong thing. You have this history: > > ---j-------E---k > \ \ > n---H---w > > where E is the second parent of w. What does it mean to rebase w onto k? > IMO, it is a meaningless operation, and the outcome is irrelevant. > > It would make sense to test that this history results after the upstream > at H moved forward: > > ---j-------E---k > \ \ > n---H \ > \ \ > z---w' > > That is, w began a topic by mergeing the sidebranch E; then upstream > advanced to z, and now you rebase the topic to the new upstream.
Fair enough. Changed accordingly.
Show 10 quoted lines
>> +test_expect_success 'rebase -p re-creates internal merge' ' >> + reset && >> + git rebase -p c w && >> + test_revisions "f j n E H w" HEAD~4 HEAD~3 HEAD~2 HEAD^2 HEAD^ HEAD > > You must also test for c; otherwise the test would succeed if rebase did > nothing at all. > > This comment applies to all other tests as well, even the "regular" rebase > tests above. (But I noticed only when I read this test.)
I did this only in one or two places thinking that that would be enough to make sure that rebase is not normally a no-op. But I think you are right that we should check it most of the time. It turns out that doing this caught a case where the rebase did do something and the right patches were in "c.." (or whatever it was; I forgot which test case), but the new base was not "c".
> After this plethora of tests, can we get rid of some or many from other > test scripts? (t34* tests are the ones that take the longest on Windows to > run.)
I was afraid that this file would be the slowest of all and it might very well be :-(. But, yes, it does replace a few test cases. I will send out an updated version of the patch later. That version should delete a few existing test cases as well.
I am having trouble finding enough time to get the patch into shape, but I didn't want to put off this reply for any longer.
Martin