{"thread":{"id":"37067","subject":"[PATCH v1] rebase -p: Command line option --no-ff is ignored","startedAt":"2014-07-07T03:50:00Z","lastAt":"2014-07-16T18:07:38Z","messageCount":3,"participants":["Fabian Ruch","Marc Branchaud"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"245453","messageId":"d8a1d5015e5562a706c1e8cf574d6011f1f1ac38.1404704884.git.bafain@gmail.com","threadId":"37067","inReplyTo":null,"subject":"[PATCH v1] rebase -p: Command line option --no-ff is ignored","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-07-07T03:50:00Z","receivedAt":"2014-07-07T03:50:00Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"The --no-ff option instructs git-rebase to always recreate commits as\nthey are being replayed, even if fast-forwards are possible.\n\nHowever, if git-rebase is asked to recreate merge commits (via the -p\noption), it suddenly ignores the --no-ff option and fast-forwards\nboth normal and merge commits whenever possible.\n\ngit-rebase--interactive, which is responsible for recreating merge\ncommits during a rebase, maintains a variable fast_forward to decide\nwhether the current replay should be tried as a fast-forward.\nPreviously, fast_forward was on by default and would get toggled only\nif a parent was rewritten or a squash was in effect. Also turn\nfast_forward off if --no-ff is in use, which is signalled by\ngit-rebase through the variable force_rebase.\n\nIf --no-ff is not in use, try to fast-forward HEAD using git-reset as\nbefore. In contrast, if --no-ff is in use, replay normal commits\nusing git-cherry-pick and merge commits using git-merge. Note that\ngit-rebase--interactive already provides this machinery for enabling\nand disabling fast-forwards, controlled by fast_forward being\nassigned either t (for boolean true) or f (for boolean false).\n\nAs mentioned above, git-rebase--interactive needs to detect when a\nsquash is in effect. If several commits are squashed into one, each\nof them is picked using the git-cherry-pick option -n and they get\nall rewritten to the same commit, the squash commit. Previously,\nfast_forward was assigned f if and only if -n was specified. This no\nlonger holds for fast_forward might be turned off due to a use of\n--no-ff. To correctly notice squashes, explicitly check for -n.\n\nAdd test.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\nHi,\n\nThe code checking force_rebase is copied from pick_one, although\nusing a ternary operator to initialise fast_forward might be more\nreadable. Moreover, the code snippet used to detect squash mode is\ncopied from the f arm of the fast_forward case switch, although the\ncode base prefers to spell out test(1).\n\nThe test recreates a topic branch that merged a second topic branch.\nTherefore, the test case tests the recreation of both normal and\nmerge commits.\n\nCommit b499549 first introduced the --no-ff option to git-rebase and\nsince then force_rebase seems to respected only by pick_one but not\nby its sibling pick_one_preserving_merges. I couldn't find a reason\nwhy. Was pick_one_preserving_merges merely overlooked?\n\nIs it a usability issue that conflicting merges will have to be\nresolved again when being replayed now? The same applies to -p and\nthe replay of merges with rewritten parents. Should the possibly\nrequired resolution be mentioned alongside git-rerere in the\ngit-rebase manual page?\n\n   Fabian\n\n git-rebase--interactive.sh        |  3 ++-\n t/t3409-rebase-preserve-merges.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f267d8b..264a768 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -266,10 +266,11 @@ pick_one_preserving_merges () {\n \t\t;;\n \tesac\n \tsha1=$(git rev-parse $sha1)\n+\tcase \"$force_rebase\" in '') ;; ?*) fast_forward=f ;; esac\n \n \tif test -f \"$state_dir\"/current-commit\n \tthen\n-\t\tif test \"$fast_forward\" = t\n+\t\tif [ \"$1\" != \"-n\" ]\n \t\tthen\n \t\t\twhile read current_commit\n \t\t\tdo\ndiff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\nindex 8c251c5..838937b 100755\n--- a/t/t3409-rebase-preserve-merges.sh\n+++ b/t/t3409-rebase-preserve-merges.sh\n@@ -81,6 +81,18 @@ test_expect_success 'setup for merge-preserving rebase' \\\n \tgit commit -a -m \"Modify B2\"\n '\n \n+test_expect_success '--no-ff records new commits' '\n+\t(\n+\tcd clone3 &&\n+\ttest_when_finished 'cd clone3 && git checkout topic' &&\n+\tgit checkout -b recreated-topic &&\n+\t# recreate topic with merged topic2 (branching-off point A1)\n+\tgit rebase -p --no-ff HEAD~2 &&\n+\ttest $(git rev-parse new-topic^) != $(git rev-parse topic^) &&\n+\ttest $(git rev-parse new-topic) != $(git rev-parse topic)\n+\t)\n+'\n+\n test_expect_success '--continue works after a conflict' '\n \t(\n \tcd clone2 &&\n-- \n2.0.0\n"},{"id":"246163","messageId":"53C6A1CC.2000306@gmail.com","threadId":"37067","inReplyTo":"d8a1d5015e5562a706c1e8cf574d6011f1f1ac38.1404704884.git.bafain@gmail.com","subject":"Re: [PATCH v1] rebase -p: Command line option --no-ff is ignored","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-07-16T16:01:16Z","receivedAt":"2014-07-16T16:01:16Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Marc,\n\nI forgot to cc your mailbox when I posted this patch last week. Do you\nstill remember whether there was a particular reason why\npick_one_preserving_merges wasn't touched by the commit b499549 (\"Teach\nrebase the --no-ff option.\"), by any chance?\n\nKind regards,\n   Fabian\n\nFabian Ruch writes:\n> The --no-ff option instructs git-rebase to always recreate commits as\n> they are being replayed, even if fast-forwards are possible.\n> \n> However, if git-rebase is asked to recreate merge commits (via the -p\n> option), it suddenly ignores the --no-ff option and fast-forwards\n> both normal and merge commits whenever possible.\n> \n> git-rebase--interactive, which is responsible for recreating merge\n> commits during a rebase, maintains a variable fast_forward to decide\n> whether the current replay should be tried as a fast-forward.\n> Previously, fast_forward was on by default and would get toggled only\n> if a parent was rewritten or a squash was in effect. Also turn\n> fast_forward off if --no-ff is in use, which is signalled by\n> git-rebase through the variable force_rebase.\n> \n> If --no-ff is not in use, try to fast-forward HEAD using git-reset as\n> before. In contrast, if --no-ff is in use, replay normal commits\n> using git-cherry-pick and merge commits using git-merge. Note that\n> git-rebase--interactive already provides this machinery for enabling\n> and disabling fast-forwards, controlled by fast_forward being\n> assigned either t (for boolean true) or f (for boolean false).\n> \n> As mentioned above, git-rebase--interactive needs to detect when a\n> squash is in effect. If several commits are squashed into one, each\n> of them is picked using the git-cherry-pick option -n and they get\n> all rewritten to the same commit, the squash commit. Previously,\n> fast_forward was assigned f if and only if -n was specified. This no\n> longer holds for fast_forward might be turned off due to a use of\n> --no-ff. To correctly notice squashes, explicitly check for -n.\n> \n> Add test.\n> \n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n> Hi,\n> \n> The code checking force_rebase is copied from pick_one, although\n> using a ternary operator to initialise fast_forward might be more\n> readable. Moreover, the code snippet used to detect squash mode is\n> copied from the f arm of the fast_forward case switch, although the\n> code base prefers to spell out test(1).\n> \n> The test recreates a topic branch that merged a second topic branch.\n> Therefore, the test case tests the recreation of both normal and\n> merge commits.\n> \n> Commit b499549 first introduced the --no-ff option to git-rebase and\n> since then force_rebase seems to respected only by pick_one but not\n> by its sibling pick_one_preserving_merges. I couldn't find a reason\n> why. Was pick_one_preserving_merges merely overlooked?\n> \n> Is it a usability issue that conflicting merges will have to be\n> resolved again when being replayed now? The same applies to -p and\n> the replay of merges with rewritten parents. Should the possibly\n> required resolution be mentioned alongside git-rerere in the\n> git-rebase manual page?\n> \n>    Fabian\n> \n>  git-rebase--interactive.sh        |  3 ++-\n>  t/t3409-rebase-preserve-merges.sh | 12 ++++++++++++\n>  2 files changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index f267d8b..264a768 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -266,10 +266,11 @@ pick_one_preserving_merges () {\n>  \t\t;;\n>  \tesac\n>  \tsha1=$(git rev-parse $sha1)\n> +\tcase \"$force_rebase\" in '') ;; ?*) fast_forward=f ;; esac\n>  \n>  \tif test -f \"$state_dir\"/current-commit\n>  \tthen\n> -\t\tif test \"$fast_forward\" = t\n> +\t\tif [ \"$1\" != \"-n\" ]\n>  \t\tthen\n>  \t\t\twhile read current_commit\n>  \t\t\tdo\n> diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\n> index 8c251c5..838937b 100755\n> --- a/t/t3409-rebase-preserve-merges.sh\n> +++ b/t/t3409-rebase-preserve-merges.sh\n> @@ -81,6 +81,18 @@ test_expect_success 'setup for merge-preserving rebase' \\\n>  \tgit commit -a -m \"Modify B2\"\n>  '\n>  \n> +test_expect_success '--no-ff records new commits' '\n> +\t(\n> +\tcd clone3 &&\n> +\ttest_when_finished 'cd clone3 && git checkout topic' &&\n> +\tgit checkout -b recreated-topic &&\n> +\t# recreate topic with merged topic2 (branching-off point A1)\n> +\tgit rebase -p --no-ff HEAD~2 &&\n> +\ttest $(git rev-parse new-topic^) != $(git rev-parse topic^) &&\n> +\ttest $(git rev-parse new-topic) != $(git rev-parse topic)\n> +\t)\n> +'\n> +\n>  test_expect_success '--continue works after a conflict' '\n>  \t(\n>  \tcd clone2 &&\n"},{"id":"246175","messageId":"53C6BF6A.8030105@xiplink.com","threadId":"37067","inReplyTo":"53C6A1CC.2000306@gmail.com","subject":"Re: [PATCH v1] rebase -p: Command line option --no-ff is ignored","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2014-07-16T18:07:38Z","receivedAt":"2014-07-16T18:07:38Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 14-07-16 12:01 PM, Fabian Ruch wrote:\n> Hi Marc,\n> \n> I forgot to cc your mailbox when I posted this patch last week. Do you\n> still remember whether there was a particular reason why\n> pick_one_preserving_merges wasn't touched by the commit b499549 (\"Teach\n> rebase the --no-ff option.\"), by any chance?\n\nI think it was simply overlooked.  (Though it was very long ago and my brain\ncan barely keep track of what I did last week...)\n\n\t\tM.\n"}]}