From: Martin von Zweigbergk Date: Thu, 30 May 2013 06:14:33 GMT Subject: Re: [PATCH v2 2/7] add tests for rebasing with patch-equivalence present Message-ID: In-Reply-To: On Wed, May 29, 2013 at 10:41 PM, Felipe Contreras wrote: > On Thu, May 30, 2013 at 12:30 AM, Martin von Zweigbergk > wrote: >> On Wed, May 29, 2013 at 12:09 AM, Johannes Sixt wrote: >>> Am 5/29/2013 8:39, schrieb Martin von Zweigbergk: >>>> +# f >>>> +# / >>>> +# a---b---c---g---h >>>> +# \ >>>> +# d---G---i >>> ... >>>> +test_run_rebase () { >>>> + result=$1 >>>> + shift >>>> + test_expect_$result "rebase $* --onto drops patches in onto" " >>>> + reset_rebase && >>>> + git rebase $* --onto h f i && >>>> + test_cmp_rev h HEAD~2 && >>>> + test_linear_range 'd i' h.. >>> >>> Isn't this expectation wrong? The upstream of the rebased branch is f, and >>> it does not contain G. Hence, G should be replayed. Since h is the >>> reversal of g, the state at h is the same as at c, and applying G should >>> succeed (it is the same change as g). Therefore, I think the correct >>> expectation is: >>> >>> test_linear_range 'd G i' h.. >> >> Good question! It is really not obvious what the right answer is. Some >> arguments in favor of dropping 'G': > > I think the answer is obvious; G should not be dropped. Maybe it made > sense to drop g in upstream, but d fixes an issue, and it makes sense > to apply G on upstream. Well, maybe I was wrong in thinking that dropping 'G' in 'git rebase --onto f h i' is bad. It seems to complicate things a lot, so maybe we should just decide that it's fine to do that (to drop 'G' in that case). Since that's mostly how it has worked forever and no one seems to have reported a problem with it, I'm probably just being paranoid. Thoughts?