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

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