Re: [PATCH v5 3/5] 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)

This is minor, but comparing the above with below

> 		    F---G topic
> 		   /
> 	  A---B---C---D---E master

you'll notice that branches growing downwards in your picture (this
applies also to an illustration in your tests) are off by one
column.
		  D
                 /
	A---B---C
                 \
                  E

> @@ -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.
>  	 */

Two issues.

 - One is shared with the original (i.e. not a problem with this
   patch), but what "this" refers to is not what has already been
   stated in the sentence.  We check, to see if we are in a
   situation where a specific optimization is possible.  But this
   (== optimization to fast-forward without actually replaying the
   commit's changes) should be done only under such and such
   condition.

 - The other is more grave. "should be done if this is not an
   interactive rebase" drops "only" from "only if" in the original,
   which changes the meaning of the sentence (it can be read as "we
   check if we can optimize, but the optimization should be done for
   rebase-i regardless of the result of that said check", which is
   not what you mean).

Perhaps

	Check if we are ..., in which case we could fast-forward
	without replacing the commits with new commits recreated by
	replaying their changes.  This optimization must not be done
	if this is an interactive rebase.