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

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

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....

> @@ -1682,13 +1699,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)) {
> +	if (can_fast_forward(options.onto, options.upstream, &options.orig_head, &merge_base) &&
> +	    !is_interactive(&options) && !options.restrict_revision) {

Ditto.  Shouldn't it be the matter of

	if (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&
	    !is_interactive(&options) && !options.restrict_revision) {

i.e. the original without paying any attention to the options.upstream?

> 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
>  '

The above two changes to 3400 and 3404 are not explained.


> 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'

This shows what the change wanted to achieve, which makes sense to
me.