[PATCH v1] rebase -p: Command line option --no-ff is ignored

[PATCH v1] rebase -p: Command line option --no-ff is ignored

[Fabian Ruch]

The --no-ff option instructs git-rebase to always recreate commits as
they are being replayed, even if fast-forwards are possible.

However, if git-rebase is asked to recreate merge commits (via the -p
option), it suddenly ignores the --no-ff option and fast-forwards
both normal and merge commits whenever possible.

git-rebase--interactive, which is responsible for recreating merge
commits during a rebase, maintains a variable fast_forward to decide
whether the current replay should be tried as a fast-forward.
Previously, fast_forward was on by default and would get toggled only
if a parent was rewritten or a squash was in effect. Also turn
fast_forward off if --no-ff is in use, which is signalled by
git-rebase through the variable force_rebase.

If --no-ff is not in use, try to fast-forward HEAD using git-reset as
before. In contrast, if --no-ff is in use, replay normal commits
using git-cherry-pick and merge commits using git-merge. Note that
git-rebase--interactive already provides this machinery for enabling
and disabling fast-forwards, controlled by fast_forward being
assigned either t (for boolean true) or f (for boolean false).

As mentioned above, git-rebase--interactive needs to detect when a
squash is in effect. If several commits are squashed into one, each
of them is picked using the git-cherry-pick option -n and they get
all rewritten to the same commit, the squash commit. Previously,
fast_forward was assigned f if and only if -n was specified. This no
longer holds for fast_forward might be turned off due to a use of
--no-ff. To correctly notice squashes, explicitly check for -n.

Add test.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
Hi,

The code checking force_rebase is copied from pick_one, although
using a ternary operator to initialise fast_forward might be more
readable. Moreover, the code snippet used to detect squash mode is
copied from the f arm of the fast_forward case switch, although the
code base prefers to spell out test(1).

The test recreates a topic branch that merged a second topic branch.
Therefore, the test case tests the recreation of both normal and
merge commits.

Commit b499549 first introduced the --no-ff option to git-rebase and
since then force_rebase seems to respected only by pick_one but not
by its sibling pick_one_preserving_merges. I couldn't find a reason
why. Was pick_one_preserving_merges merely overlooked?

Is it a usability issue that conflicting merges will have to be
resolved again when being replayed now? The same applies to -p and
the replay of merges with rewritten parents. Should the possibly
required resolution be mentioned alongside git-rerere in the
git-rebase manual page?

   Fabian

 git-rebase--interactive.sh        |  3 ++-
 t/t3409-rebase-preserve-merges.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f267d8b..264a768 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -266,10 +266,11 @@ pick_one_preserving_merges () {
 		;;
 	esac
 	sha1=$(git rev-parse $sha1)
+	case "$force_rebase" in '') ;; ?*) fast_forward=f ;; esac
 
 	if test -f "$state_dir"/current-commit
 	then
-		if test "$fast_forward" = t
+		if [ "$1" != "-n" ]
 		then
 			while read current_commit
 			do
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 8c251c5..838937b 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -81,6 +81,18 @@ test_expect_success 'setup for merge-preserving rebase' \
 	git commit -a -m "Modify B2"
 '
 
+test_expect_success '--no-ff records new commits' '
+	(
+	cd clone3 &&
+	test_when_finished 'cd clone3 && git checkout topic' &&
+	git checkout -b recreated-topic &&
+	# recreate topic with merged topic2 (branching-off point A1)
+	git rebase -p --no-ff HEAD~2 &&
+	test $(git rev-parse new-topic^) != $(git rev-parse topic^) &&
+	test $(git rev-parse new-topic) != $(git rev-parse topic)
+	)
+'
+
 test_expect_success '--continue works after a conflict' '
 	(
 	cd clone2 &&
-- 
2.0.0

Re: [PATCH v1] rebase -p: Command line option --no-ff is ignored

[Fabian Ruch]

Hi Marc,

I forgot to cc your mailbox when I posted this patch last week. Do you
still remember whether there was a particular reason why
pick_one_preserving_merges wasn't touched by the commit b499549 ("Teach
rebase the --no-ff option."), by any chance?

Kind regards,
   Fabian

Fabian Ruch writes:
> The --no-ff option instructs git-rebase to always recreate commits as
> they are being replayed, even if fast-forwards are possible.
> 
> However, if git-rebase is asked to recreate merge commits (via the -p
> option), it suddenly ignores the --no-ff option and fast-forwards
> both normal and merge commits whenever possible.
> 
> git-rebase--interactive, which is responsible for recreating merge
> commits during a rebase, maintains a variable fast_forward to decide
> whether the current replay should be tried as a fast-forward.
> Previously, fast_forward was on by default and would get toggled only
> if a parent was rewritten or a squash was in effect. Also turn
> fast_forward off if --no-ff is in use, which is signalled by
> git-rebase through the variable force_rebase.
> 
> If --no-ff is not in use, try to fast-forward HEAD using git-reset as
> before. In contrast, if --no-ff is in use, replay normal commits
> using git-cherry-pick and merge commits using git-merge. Note that
> git-rebase--interactive already provides this machinery for enabling
> and disabling fast-forwards, controlled by fast_forward being
> assigned either t (for boolean true) or f (for boolean false).
> 
> As mentioned above, git-rebase--interactive needs to detect when a
> squash is in effect. If several commits are squashed into one, each
> of them is picked using the git-cherry-pick option -n and they get
> all rewritten to the same commit, the squash commit. Previously,
> fast_forward was assigned f if and only if -n was specified. This no
> longer holds for fast_forward might be turned off due to a use of
> --no-ff. To correctly notice squashes, explicitly check for -n.
> 
> Add test.
> 
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
> Hi,
> 
> The code checking force_rebase is copied from pick_one, although
> using a ternary operator to initialise fast_forward might be more
> readable. Moreover, the code snippet used to detect squash mode is
> copied from the f arm of the fast_forward case switch, although the
> code base prefers to spell out test(1).
> 
> The test recreates a topic branch that merged a second topic branch.
> Therefore, the test case tests the recreation of both normal and
> merge commits.
> 
> Commit b499549 first introduced the --no-ff option to git-rebase and
> since then force_rebase seems to respected only by pick_one but not
> by its sibling pick_one_preserving_merges. I couldn't find a reason
> why. Was pick_one_preserving_merges merely overlooked?
> 
> Is it a usability issue that conflicting merges will have to be
> resolved again when being replayed now? The same applies to -p and
> the replay of merges with rewritten parents. Should the possibly
> required resolution be mentioned alongside git-rerere in the
> git-rebase manual page?
> 
>    Fabian
> 
>  git-rebase--interactive.sh        |  3 ++-
>  t/t3409-rebase-preserve-merges.sh | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f267d8b..264a768 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -266,10 +266,11 @@ pick_one_preserving_merges () {
>  		;;
>  	esac
>  	sha1=$(git rev-parse $sha1)
> +	case "$force_rebase" in '') ;; ?*) fast_forward=f ;; esac
>  
>  	if test -f "$state_dir"/current-commit
>  	then
> -		if test "$fast_forward" = t
> +		if [ "$1" != "-n" ]
>  		then
>  			while read current_commit
>  			do
> diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
> index 8c251c5..838937b 100755
> --- a/t/t3409-rebase-preserve-merges.sh
> +++ b/t/t3409-rebase-preserve-merges.sh
> @@ -81,6 +81,18 @@ test_expect_success 'setup for merge-preserving rebase' \
>  	git commit -a -m "Modify B2"
>  '
>  
> +test_expect_success '--no-ff records new commits' '
> +	(
> +	cd clone3 &&
> +	test_when_finished 'cd clone3 && git checkout topic' &&
> +	git checkout -b recreated-topic &&
> +	# recreate topic with merged topic2 (branching-off point A1)
> +	git rebase -p --no-ff HEAD~2 &&
> +	test $(git rev-parse new-topic^) != $(git rev-parse topic^) &&
> +	test $(git rev-parse new-topic) != $(git rev-parse topic)
> +	)
> +'
> +
>  test_expect_success '--continue works after a conflict' '
>  	(
>  	cd clone2 &&

Re: [PATCH v1] rebase -p: Command line option --no-ff is ignored

[Marc Branchaud]

On 14-07-16 12:01 PM, Fabian Ruch wrote:
> Hi Marc,
> 
> I forgot to cc your mailbox when I posted this patch last week. Do you
> still remember whether there was a particular reason why
> pick_one_preserving_merges wasn't touched by the commit b499549 ("Teach
> rebase the --no-ff option."), by any chance?

I think it was simply overlooked.  (Though it was very long ago and my brain
can barely keep track of what I did last week...)

		M.