Re: [PATCH v3 3/4] rebase: fast-forward --onto in more cases

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Denton Liu <liu.denton@gmail.com> writes:
>
>> Before, when we had the following graph,
>>
>> 	A---B---C (master)
>> 	    \
>> 	     D (side)
>>
>> running 'git rebase --onto master... master side' would result in D
>> being always rebased, no matter what. However, the desired behavior is
>> that rebase should notice that this is fast-forwardable and do that
>> instead.
>>
>> Add detection to `can_fast_forward` so that this case can be detected
>> and a fast-forward will be performed.
>
> OK.  As long as the 'onto' commit is a strict ancestor of the side
> branch being rebased, the 'upstream' that is used only to determine
> which commits are on the side branch (essentially those that are not
> reachable from upstream but that are from the side branch) should
> not count in the equation to decide if we fast-forward or not.  That
> makes sense.
>
>> ---
>>  builtin/rebase.c               | 40 +++++++++++++++++++++++-----------
>>  t/t3400-rebase.sh              |  2 +-
>>  t/t3404-rebase-interactive.sh  |  2 +-
>>  t/t3432-rebase-fast-forward.sh |  2 +-
>>  4 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 77deebc65c..7aa6a090d4 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -895,12 +895,12 @@ static int is_linear_history(struct commit *from, struct commit *to)
>>  	return 1;
>>  }
>>  
>> -static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
>> -			    struct object_id *merge_base)
>> +static int can_fast_forward(struct commit *onto, struct commit *upstream,
>> +			    struct object_id *head_oid, struct object_id *merge_base)
>>  {
>>  	struct commit *head = lookup_commit(the_repository, head_oid);
>> -	struct commit_list *merge_bases;
>> -	int res;
>> +	struct commit_list *merge_bases = NULL;
>> +	int res = 0;
>>  
>>  	if (!head)
>>  		return 0;
>> @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
>>  	merge_bases = get_merge_bases(onto, head);
>>  	if (merge_bases && !merge_bases->next) {
>>  		oidcpy(merge_base, &merge_bases->item->object.oid);
>> -		res = oideq(merge_base, &onto->object.oid);
>> +		if (!oideq(merge_base, &onto->object.oid))
>> +			goto done;
>>  	} else {
>>  		oidcpy(merge_base, &null_oid);
>> -		res = 0;
>> +		goto done;
>>  	}
>
> The above does not change any existing logic, but purely simplifies
> the code.  In your picture in the log message
>
>     A---B---C (master)
>          \
>           D (side)
>
> where "rebase --onto master... master side" is run, "onto" in this
> function is B, and "head" is D.  There is a single merge base B
> (i.e. merge_bases->next == NULL), so we jump to the label "done:"
> with res==0.
>
> So why does the remainder of the function need to be changed?
>
>> +	if (!upstream)
>> +		goto done;
>> +
>>  	free_commit_list(merge_bases);
>> +	merge_bases = get_merge_bases(upstream, head);
>> +	if (merge_bases && !merge_bases->next) {
>> +		if (!oideq(&onto->object.oid, &merge_bases->item->object.oid))
>> +			goto done;
>> +	} else
>> +		goto done;
>
> This computes the same between C and D (instead of B and D).  Why is
> this needed?
>
> Stepping back a bit, I understand that your argument we saw in the
> log message was that we only need to know if the commit we are
> transplanting the history on (i.e. "onto") already is an ancestor of
> the history being transplanted (i.e. "onto..head"), and it does not
> matter what upstream is.  Am I mistaken?  Why does this function now
> need to know what 'upstream' is?
>
> Puzzled....

So I replaced the part for builtin/rebase.c from your patch with the
above suggestion (attached) and the result seems to pass the three
tests you touched.  I am still puzzled.

Thanks.

 builtin/rebase.c               | 7 ++-----
 t/t3400-rebase.sh              | 2 +-
 t/t3404-rebase-interactive.sh  | 2 +-
 t/t3432-rebase-fast-forward.sh | 2 +-
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 77deebc65c..fe61c2a899 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1682,13 +1682,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Check if we are already based on onto with linear history,
-	 * but this should be done only when upstream and onto are the same
-	 * and if this is not an interactive rebase.
+	 * but this should be done if this is not an interactive rebase.
 	 */
 	if (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&
-	    !is_interactive(&options) && !options.restrict_revision &&
-	    options.upstream &&
-	    !oidcmp(&options.upstream->object.oid, &options.onto->object.oid)) {
+	    !is_interactive(&options) && !options.restrict_revision) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 460d0523be..604d624ff8 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh and --show-current-patch' '
 		echo two >>init.t &&
 		git commit -a -m two &&
 		git tag two &&
-		test_must_fail git rebase --onto init HEAD^ &&
+		test_must_fail git rebase -f --onto init HEAD^ &&
 		GIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&
 		grep "show.*$(git rev-parse two)" stderr
 	)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b60b11f9f2..f054186cc7 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1066,7 +1066,7 @@ test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-int
 	git reset --hard &&
 	git checkout conflict-branch &&
 	set_fake_editor &&
-	test_must_fail git rebase --onto HEAD~2 HEAD~ &&
+	test_must_fail git rebase -f --onto HEAD~2 HEAD~ &&
 	test_must_fail git rebase --edit-todo &&
 	git rebase --abort
 '
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 3e6362dd9c..414b4216d6 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -54,6 +54,6 @@ test_expect_success 'add work to upstream' '
 changes='our and their changes'
 test_rebase_same_head success '--onto B B'
 test_rebase_same_head success '--onto B... B'
-test_rebase_same_head failure '--onto master... master'
+test_rebase_same_head success '--onto master... master'
 
 test_done