Re: [PATCH v2 2/2] rebase: omit patch-identical commits with --fork-point

[John Keeping <john@keeping.me.uk>]

On Wed, Jul 16, 2014 at 01:26:32PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > When the `--fork-point` argument was added to `git rebase`, we changed
> > the value of $upstream to be the fork point instead of the point from
> > which we want to rebase.  When $orig_head..$upstream is empty this does
> > not change the behaviour, but when there are new changes in the upstream
> > we are no longer checking if any of them are patch-identical with
> > changes in $upstream..$orig_head.
> >
> > Fix this by introducing a new variable to hold the fork point and using
> > this to restrict the range as an extra (negative) revision argument so
> > that the set of desired revisions becomes (in fork-point mode):
> >
> > 	git rev-list --cherry-pick --right-only \
> > 		$upstream...$orig_head ^$fork_point
> >
> > This allows us to correctly handle the scenario where we have the
> > following topology:
> >
> > 	    C --- D --- E  <- dev
> > 	   /
> > 	  B  <- master@{1}
> > 	 /
> > 	o --- B' --- C* --- D*  <- master
> >
> > where:
> > - B' is a fixed-up version of B that is not patch-identical with B;
> > - C* and D* are patch-identical to C and D respectively and conflict
> >   textually if applied in the wrong order;
> > - E depends textually on D.
> >
> > The correct result of `git rebase master dev` is that B is identified as
> > the fork-point of dev and master, so that C, D, E are the commits that
> > need to be replayed onto master; but C and D are patch-identical with C*
> > and D* and so can be dropped, so that the end result is:
> >
> > 	o --- B' --- C* --- D* --- E  <- dev
> >
> > If the fork-point is not identified, then picking B onto a branch
> > containing B' results in a conflict and if the patch-identical commits
> > are not correctly identified then picking C onto a branch containing D
> > (or equivalently D*) results in a conflict.
> >
> > This change allows us to handle both of these cases, where previously we
> > either identified the fork-point (with `--fork-point`) but not the
> > patch-identical commits *or* (with `--no-fork-point`) identified the
> > patch-identical commits but not the fact that master had been rewritten.
> >
> > Reported-by: Ted Felix <ted@tedfelix.com>
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >
> > Change from v1:
> >     - add a test case
> 
> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> > index 80e0a95..47b5682 100755
> > --- a/t/t3400-rebase.sh
> > +++ b/t/t3400-rebase.sh
> > @@ -169,6 +169,29 @@ test_expect_success 'default to common base in @{upstream}s reflog if no upstrea
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'cherry-picked commits and fork-point work together' '
> > +	git checkout default-base &&
> > +	echo Amended >A &&
> > +	git commit -a --no-edit --amend &&
> > +	test_commit B B &&
> > +	test_commit new_B B "New B" &&
> > +	test_commit C C &&
> > +	git checkout default &&
> > +	git reset --hard default-base@{4} &&
> > +	test_commit D D &&
> > +	git cherry-pick -2 default-base^ &&
> > +	test_commit final_B B "Final B" &&
> > +	git rebase &&
> 
> mental note: this rebases default (i.e. the current branch) on
> default-base; it depends on branch.default.{remote,merge} being set
> by the previous test piece.
> 
> > +	echo Amended >expect &&
> > +	test_cmp A expect &&
> > +	echo "Final B" >expect &&
> > +	test_cmp B expect &&
> > +	echo C >expect &&
> > +	test_cmp C expect &&
> > +	echo D >expect &&
> > +	test_cmp D expect
> > +'
> 
> Thanks.  Do these labels on the commits have any relation to the
> topology illustrated in the log message?

No, I didn't consider the log message when adding the test; it simply
uses the next letters that are unused in the test repository.

> It looks like the above produces this:
> 
>       a'---D---B'--new_B'---final_B (default)
>      /
>     o----a---B---new_B---C (default-base)
>                           \
>                            D''---final_B''
> 
> where 'a' is "Modify A." from the original set-up and B and new_B
> are the cherry-picks to be filtered out during the rebase.  Am I
> reading the test correctly?

Yes, that's right.  The idea is to make sure that we skip the
cherry-picked commits, which requires two commits that will conflict if
we try to apply the first on top of the second and to make sure that we
don't lose the new commit on the upstream (C).

> >  test_expect_success 'rebase -q is quiet' '
> >  	git checkout -b quiet topic &&
> >  	git rebase -q master >output.out 2>&1 &&