{"thread":{"id":"56048","subject":"[PATCH RFC] rebase: respect --ff-only option","startedAt":"2021-07-05T04:45:34Z","lastAt":"2021-07-07T01:14:01Z","messageCount":15,"participants":["Alex Henrie","Phillip Wood","Ævar Arnfjörð Bjarmason","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429180","messageId":"20210705044505.666977-1-alexhenrie24@gmail.com","threadId":"56048","inReplyTo":null,"subject":"[PATCH RFC] rebase: respect --ff-only option","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-07-05T04:45:05Z","receivedAt":"2021-07-05T04:45:34Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The warning about pulling without specifying how to reconcile divergent\nbranches says that after setting pull.rebase to true, --ff-only can\nstill be passed on the command line to require a fast-forward. Make that\nactually work.\n\nSigned-off-by: Alex Henrie \n---\nI'm sending this patch as an RFC because I'm sure someone will find at\nleast one thing that needs to be changed before it's committed.\n---\n advice.c | 6 +++\n advice.h | 1 +\n builtin/merge.c | 6 ---\n builtin/pull.c | 2 +\n builtin/rebase.c | 68 +++++++++++++++++++------------\n builtin/revert.c | 6 +--\n sequencer.c | 53 ++++++++++++++----------\n sequencer.h | 9 +++-\n t/t3404-rebase-interactive.sh | 5 +++\n t/t3409-rebase-preserve-merges.sh | 4 ++\n t/t3420-rebase-autostash.sh | 9 ++++\n t/t7601-merge-pull-config.sh | 10 +++++\n 12 files changed, 121 insertions(+), 58 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0b9c89c48a..8571ab7d0f 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -246,6 +246,12 @@ void list_config_advices(struct string_list *list, const char *prefix)\n \t\tlist_config_item(list, prefix, advice_setting[i].key);\n }\n \n+int error_ff_impossible(void)\n+{\n+\terror(_(\"Not possible to fast-forward, aborting.\"));\n+\treturn -1;\n+}\n+\n int error_resolve_conflict(const char *me)\n {\n \tif (!strcmp(me, \"cherry-pick\"))\ndiff --git a/advice.h b/advice.h\nindex bd26c385d0..49017f931e 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -93,6 +93,7 @@ int advice_enabled(enum advice_type type);\n void advise_if_enabled(enum advice_type type, const char *advice, ...);\n \n int error_resolve_conflict(const char *me);\n+int error_ff_impossible(void);\n void NORETURN die_resolve_conflict(const char *me);\n void NORETURN die_conclude_merge(void);\n void advise_on_updating_sparse_paths(struct string_list *pathspec_list);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8a843b1f5..eb25c30e70 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -98,12 +98,6 @@ static struct strategy all_strategy[] = {\n \n static const char *pull_twohead, *pull_octopus;\n \n-enum ff_type {\n-\tFF_NO,\n-\tFF_ALLOW,\n-\tFF_ONLY\n-};\n-\n static enum ff_type fast_forward = FF_ALLOW;\n \n static const char *cleanup_arg;\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex e8927fc2ff..c285e9cc89 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -889,6 +889,8 @@ static int run_rebase(const struct object_id *newbase,\n \t\tstrvec_push(&args, \"--interactive\");\n \tif (opt_diffstat)\n \t\tstrvec_push(&args, opt_diffstat);\n+\tif (opt_ff)\n+\t\tstrvec_push(&args, opt_ff);\n \tstrvec_pushv(&args, opt_strategies.v);\n \tstrvec_pushv(&args, opt_strategy_opts.v);\n \tif (opt_gpg_sign)\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d..b88f0cbcca 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -81,9 +81,9 @@ struct rebase_options {\n \t\tREBASE_NO_QUIET = 1<<0,\n \t\tREBASE_VERBOSE = 1<<1,\n \t\tREBASE_DIFFSTAT = 1<<2,\n-\t\tREBASE_FORCE = 1<<3,\n-\t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n+\t\tREBASE_INTERACTIVE_EXPLICIT = 1<<3,\n \t} flags;\n+\tenum ff_type fast_forward;\n \tstruct strvec git_am_opts;\n \tconst char *action;\n \tint signoff;\n@@ -110,6 +110,7 @@ struct rebase_options {\n \t\t.keep_empty = 1,\t\t\t\\\n \t\t.default_backend = \"merge\",\t \t\\\n \t\t.flags = REBASE_NO_QUIET, \t\t\\\n+\t\t.fast_forward = FF_ALLOW, \t\t\\\n \t\t.git_am_opts = STRVEC_INIT,\t\t\\\n \t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n \t\t.fork_point = -1,\t\t\t\\\n@@ -124,7 +125,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \tsequencer_init_config(&replay);\n \n \treplay.signoff = opts->signoff;\n-\treplay.allow_ff = !(opts->flags & REBASE_FORCE);\n+\treplay.fast_forward = opts->fast_forward;\n \tif (opts->allow_rerere_autoupdate)\n \t\treplay.allow_rerere_auto = opts->allow_rerere_autoupdate;\n \treplay.allow_empty = 1;\n@@ -488,8 +489,11 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)\n \tstruct object_id squash_onto = *null_oid();\n \tenum action command = ACTION_NONE;\n \tstruct option options[] = {\n-\t\tOPT_NEGBIT(0, \"ff\", &opts.flags, N_(\"allow fast-forward\"),\n-\t\t\t REBASE_FORCE),\n+\t\tOPT_SET_INT(0, \"ff\", &opts.fast_forward, N_(\"allow fast-forward\"),\n+\t\t\t FF_ALLOW),\n+\t\tOPT_SET_INT_F(0, \"ff-only\", &opts.fast_forward,\n+\t\t\t N_(\"abort if fast-forward is not possible\"),\n+\t\t\t FF_ONLY, PARSE_OPT_NONEG),\n \t\tOPT_CALLBACK_F('k', \"keep-empty\", &options, NULL,\n \t\t\tN_(\"keep commits which start empty\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_HIDDEN,\n@@ -651,7 +655,7 @@ static int read_basic_state(struct rebase_options *opts)\n \n \tif (file_exists(state_dir_path(\"signoff\", opts))) {\n \t\topts->signoff = 1;\n-\t\topts->flags |= REBASE_FORCE;\n+\t\topts->fast_forward = FF_NO;\n \t}\n \n \tif (file_exists(state_dir_path(\"allow_rerere_autoupdate\", opts))) {\n@@ -991,7 +995,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)\n \tadd_var(&script_snippet, \"diffstat\",\n \t\topts->flags & REBASE_DIFFSTAT ? \"t\" : \"\");\n \tadd_var(&script_snippet, \"force_rebase\",\n-\t\topts->flags & REBASE_FORCE ? \"t\" : \"\");\n+\t\topts->fast_forward == FF_NO ? \"\" : \"t\");\n \tif (opts->switch_to)\n \t\tadd_var(&script_snippet, \"switch_to\", opts->switch_to);\n \tadd_var(&script_snippet, \"action\", opts->action ? opts->action : \"\");\n@@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"ignore changes in whitespace\")),\n \t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n \t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n-\t\tOPT_BIT('f', \"force-rebase\", &options.flags,\n-\t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n-\t\t\tREBASE_FORCE),\n-\t\tOPT_BIT(0, \"no-ff\", &options.flags,\n-\t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n-\t\t\tREBASE_FORCE),\n+\t\tOPT_SET_INT_F('f', \"force-rebase\", &options.fast_forward,\n+\t\t\t N_(\"cherry-pick all commits, even if unchanged\"),\n+\t\t\t FF_NO, PARSE_OPT_NONEG),\n+\t\tOPT_SET_INT(0, \"ff\", &options.fast_forward, N_(\"allow fast-forward\"),\n+\t\t\t FF_ALLOW),\n+\t\tOPT_SET_INT_F(0, \"ff-only\", &options.fast_forward,\n+\t\t\t N_(\"abort if fast-forward is not possible\"),\n+\t\t\t FF_ONLY, PARSE_OPT_NONEG),\n \t\tOPT_CMDMODE(0, \"continue\", &action, N_(\"continue\"),\n \t\t\t ACTION_CONTINUE),\n \t\tOPT_CMDMODE(0, \"skip\", &action,\n@@ -1635,7 +1641,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tallow_preemptive_ff = 0;\n \t}\n \tif (options.committer_date_is_author_date || options.ignore_date)\n-\t\toptions.flags |= REBASE_FORCE;\n+\t\toptions.fast_forward = FF_NO;\n \n \tfor (i = 0; i < options.git_am_opts.nr; i++) {\n \t\tconst char *option = options.git_am_opts.v[i], *p;\n@@ -1809,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"cannot combine '--signoff' with \"\n \t\t\t \"'--preserve-merges'\");\n \t\tstrvec_push(&options.git_am_opts, \"--signoff\");\n-\t\toptions.flags |= REBASE_FORCE;\n+\t\toptions.fast_forward = FF_NO;\n \t}\n \n \tif (options.type == REBASE_PRESERVE_MERGES) {\n@@ -1952,6 +1958,24 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (repo_read_index(the_repository) < 0)\n \t\tdie(_(\"could not read index\"));\n \n+\t/*\n+\t * Check if we are already based on onto with linear history,\n+\t * in which case we could fast-forward without replacing the commits\n+\t * with new commits recreated by replaying their changes.\n+\t *\n+\t * Note that can_fast_forward() initializes merge_base, so we have to\n+\t * call it before checking allow_preemptive_ff.\n+\t */\n+\tallow_preemptive_ff = can_fast_forward(options.onto, options.upstream,\n+\t\t\t\t\t options.restrict_revision,\n+\t\t\t\t\t &options.orig_head, &merge_base)\n+\t\t\t && allow_preemptive_ff;\n+\n+\tif (!allow_preemptive_ff && options.fast_forward == FF_ONLY) {\n+\t\tret = error_ff_impossible();\n+\t\tgoto cleanup;\n+\t}\n+\n \tif (options.autostash) {\n \t\tcreate_autostash(the_repository, state_dir_path(\"autostash\", &options),\n \t\t\t\t DEFAULT_REFLOG_ACTION);\n@@ -1968,20 +1992,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t * everything leading up to orig_head) on top of onto.\n \t */\n \n-\t/*\n-\t * Check if we are already based on onto with linear history,\n-\t * in which case we could fast-forward without replacing the commits\n-\t * with new commits recreated by replaying their changes.\n-\t *\n-\t * Note that can_fast_forward() initializes merge_base, so we have to\n-\t * call it before checking allow_preemptive_ff.\n-\t */\n-\tif (can_fast_forward(options.onto, options.upstream, options.restrict_revision,\n-\t\t &options.orig_head, &merge_base) &&\n-\t allow_preemptive_ff) {\n+\tif (allow_preemptive_ff) {\n \t\tint flag;\n \n-\t\tif (!(options.flags & REBASE_FORCE)) {\n+\t\tif (options.fast_forward != FF_NO) {\n \t\t\t/* Lazily switch to the target branch if needed... */\n \t\t\tif (options.switch_to) {\n \t\t\t\tstrbuf_reset(&buf);\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 237f2f18d4..f9b61478a2 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -123,7 +123,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOL('x', NULL, &opts->record_origin, N_(\"append commit name\")),\n-\t\t\tOPT_BOOL(0, \"ff\", &opts->allow_ff, N_(\"allow fast-forward\")),\n+\t\t\tOPT_SET_INT(0, \"ff\", &opts->fast_forward, N_(\"allow fast-forward\"), FF_ALLOW),\n \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOL(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n@@ -166,7 +166,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n \t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n \t\t\t\t\"-x\", opts->record_origin,\n-\t\t\t\t\"--ff\", opts->allow_ff,\n+\t\t\t\t\"--ff\", opts->fast_forward == FF_ALLOW,\n \t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n \t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n \t\t\t\tNULL);\n@@ -177,7 +177,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\topts->default_strategy = NULL;\n \t}\n \n-\tif (opts->allow_ff)\n+\tif (opts->fast_forward == FF_ALLOW)\n \t\tverify_opt_compatible(me, \"--ff\",\n \t\t\t\t\"--signoff\", opts->signoff,\n \t\t\t\t\"--no-commit\", opts->no_commit,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38..84447937d2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2120,18 +2120,23 @@ static int do_pick_commit(struct repository *r,\n \t\treturn error(_(\"cannot get commit message for %s\"),\n \t\t\toid_to_hex(&commit->object.oid));\n \n-\tif (opts->allow_ff && !is_fixup(command) &&\n-\t ((parent && oideq(&parent->object.oid, &head)) ||\n-\t (!parent && unborn))) {\n-\t\tif (is_rebase_i(opts))\n-\t\t\twrite_author_script(msg.message);\n-\t\tres = fast_forward_to(r, &commit->object.oid, &head, unborn,\n-\t\t\topts);\n-\t\tif (res || command != TODO_REWORD)\n+\tif (opts->fast_forward != FF_NO) {\n+\t\tif (!is_fixup(command) &&\n+\t\t ((parent && oideq(&parent->object.oid, &head)) ||\n+\t\t (!parent && unborn))) {\n+\t\t\tif (is_rebase_i(opts))\n+\t\t\t\twrite_author_script(msg.message);\n+\t\t\tres = fast_forward_to(r, &commit->object.oid, &head, unborn,\n+\t\t\t\topts);\n+\t\t\tif (res || command != TODO_REWORD)\n+\t\t\t\tgoto leave;\n+\t\t\treword = 1;\n+\t\t\tmsg_file = NULL;\n+\t\t\tgoto fast_forward_edit;\n+\t\t} else if (opts->fast_forward == FF_ONLY) {\n+\t\t\tres = error_ff_impossible();\n \t\t\tgoto leave;\n-\t\treword = 1;\n-\t\tmsg_file = NULL;\n-\t\tgoto fast_forward_edit;\n+\t\t}\n \t}\n \tif (parent && parse_commit(parent) < 0)\n \t\t/* TRANSLATORS: The first %s will be a \"todo\" command like\n@@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \telse if (!strcmp(key, \"options.record-origin\"))\n \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n \telse if (!strcmp(key, \"options.allow-ff\"))\n-\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n+\t\topts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;\n \telse if (!strcmp(key, \"options.mainline\"))\n \t\topts->mainline = git_config_int(key, value);\n \telse if (!strcmp(key, \"options.strategy\"))\n@@ -2853,17 +2858,17 @@ static int read_populate_opts(struct replay_opts *opts)\n \t\t\topts->quiet = 1;\n \n \t\tif (file_exists(rebase_path_signoff())) {\n-\t\t\topts->allow_ff = 0;\n+\t\t\topts->fast_forward = FF_NO;\n \t\t\topts->signoff = 1;\n \t\t}\n \n \t\tif (file_exists(rebase_path_cdate_is_adate())) {\n-\t\t\topts->allow_ff = 0;\n+\t\t\topts->fast_forward = FF_NO;\n \t\t\topts->committer_date_is_author_date = 1;\n \t\t}\n \n \t\tif (file_exists(rebase_path_ignore_date())) {\n-\t\t\topts->allow_ff = 0;\n+\t\t\topts->fast_forward = FF_NO;\n \t\t\topts->ignore_date = 1;\n \t\t}\n \n@@ -3320,7 +3325,7 @@ static int save_opts(struct replay_opts *opts)\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.record-origin\", \"true\");\n-\tif (opts->allow_ff)\n+\tif (opts->fast_forward == FF_ALLOW)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n \t\t\t\t\t\"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n@@ -3849,9 +3854,9 @@ static int do_merge(struct repository *r,\n \t * If HEAD is not identical to the first parent of the original merge\n \t * commit, we cannot fast-forward.\n \t */\n-\tcan_fast_forward = opts->allow_ff && commit && commit->parents &&\n-\t\toideq(&commit->parents->item->object.oid,\n-\t\t &head_commit->object.oid);\n+\tcan_fast_forward = opts->fast_forward != FF_NO && commit &&\n+\t\tcommit->parents && oideq(&commit->parents->item->object.oid,\n+\t\t\t\t\t &head_commit->object.oid);\n \n \t/*\n \t * If any merge head is different from the original one, we cannot\n@@ -3885,6 +3890,11 @@ static int do_merge(struct repository *r,\n \t\tgoto leave_merge;\n \t}\n \n+\tif (opts->fast_forward == FF_ONLY && !can_fast_forward) {\n+\t\tret = error_ff_impossible();\n+\t\tgoto leave_merge;\n+\t}\n+\n \tif (strategy || to_merge->next) {\n \t\t/* Octopus merge */\n \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -4276,7 +4286,7 @@ static int pick_commits(struct repository *r,\n \t/* Note that 0 for 3rd parameter of setenv means set only if not set */\n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n-\tif (opts->allow_ff)\n+\tif (opts->fast_forward != FF_NO)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n@@ -5646,7 +5656,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n \t\t new_todo.buf.buf);\n \n-\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n+\tif (opts->fast_forward != FF_NO\n+\t && skip_unnecessary_picks(r, &new_todo, &oid)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error(_(\"could not skip unnecessary pick commands\"));\n \t}\ndiff --git a/sequencer.h b/sequencer.h\nindex d57d8ea23d..e188dec312 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -28,6 +28,12 @@ enum commit_msg_cleanup_mode {\n \tCOMMIT_MSG_CLEANUP_ALL\n };\n \n+enum ff_type {\n+\tFF_NO,\n+\tFF_ALLOW,\n+\tFF_ONLY\n+};\n+\n struct replay_opts {\n \tenum replay_action action;\n \n@@ -38,7 +44,6 @@ struct replay_opts {\n \tint record_origin;\n \tint no_commit;\n \tint signoff;\n-\tint allow_ff;\n \tint allow_rerere_auto;\n \tint allow_empty;\n \tint allow_empty_message;\n@@ -50,6 +55,8 @@ struct replay_opts {\n \tint committer_date_is_author_date;\n \tint ignore_date;\n \n+\tenum ff_type fast_forward;\n+\n \tint mainline;\n \n \tchar *gpg_sign;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 66bcbbf952..858e406725 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -895,6 +895,11 @@ test_expect_success 'always cherry-pick with --no-ff' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'interactive rebase prevents non-fast-forward with --ff-only' '\n+\tgit checkout primary &&\n+\ttest_must_fail git rebase -i --ff-only branch1\n+'\n+\n test_expect_success 'set up commits with funny messages' '\n \tgit checkout -b funny A &&\n \techo >>file1 &&\ndiff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\nindex ec8062a66a..e656b5bd28 100755\n--- a/t/t3409-rebase-preserve-merges.sh\n+++ b/t/t3409-rebase-preserve-merges.sh\n@@ -127,4 +127,8 @@ test_expect_success 'rebase -p ignores merge.log config' '\n \t)\n '\n \n+test_expect_success 'rebase -p prevents non-fast-forward with --ff-only' '\n+\ttest_must_fail git rebase -p --ff-only main\n+'\n+\n test_done\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 43fcb68f27..69a85cb645 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -246,6 +246,15 @@ test_expect_success \"rebase: fast-forward rebase\" '\n \tgit checkout feature-branch\n '\n \n+test_expect_success \"rebase: impossible fast-forward rebase\" '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard &&\n+\techo dirty >>file1 &&\n+\t(git rebase --ff-only unrelated-onto-branch || true) &&\n+\tgrep dirty file1 &&\n+\tgit checkout feature-branch\n+'\n+\n test_expect_success \"rebase: noop rebase\" '\n \ttest_config rebase.autostash true &&\n \tgit reset --hard &&\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 52e8ccc933..af260b9faa 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -183,6 +183,16 @@ test_expect_success 'pull prevents non-fast-forward with \"only\" in pull.ff' '\n \ttest_must_fail git pull . c3\n '\n \n+test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git pull --rebase --ff-only . c3\n+'\n+\n+test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git pull --no-rebase --ff-only . c3\n+'\n+\n test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '\n \tgit reset --hard c1 &&\n \tgit config pull.twohead ours &&\n-- \n2.32.0.94.ge083fb24f6\n\n"},{"id":"429192","messageId":"349748b4-3c48-7ca7-eb0f-e859a15cab0f@gmail.com","threadId":"56048","inReplyTo":"20210705044505.666977-1-alexhenrie24@gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-05T08:53:16Z","receivedAt":"2021-07-05T08:53:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 05/07/2021 05:45, Alex Henrie wrote:\n> The warning about pulling without specifying how to reconcile divergent\n> branches says that after setting pull.rebase to true, --ff-only can\n> still be passed on the command line to require a fast-forward. Make that\n> actually work.\n\nAs I understand it the motivation for this change is to have 'git -c \npull.rebase=true pull --ff-only' actually fast forward. Why cant we just \nchange pull not to rebase in that case? I've left some comments about \nthe rebase changes below\n\n > [...]\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 12f093121d..b88f0cbcca 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"ignore changes in whitespace\")),\n> \t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n> \t\t\t\t N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n> -\t\tOPT_BIT('f', \"force-rebase\", &options.flags,\n> -\t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n> -\t\t\tREBASE_FORCE),\n> -\t\tOPT_BIT(0, \"no-ff\", &options.flags,\n> -\t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n> -\t\t\tREBASE_FORCE),\n> +\t\tOPT_SET_INT_F('f', \"force-rebase\", &options.fast_forward,\n> +\t\t\t N_(\"cherry-pick all commits, even if unchanged\"),\n> +\t\t\t FF_NO, PARSE_OPT_NONEG),\n\nWhy does this change rebase to start rejecting --no-force-rebase? This \nis the sort of behavior change that deserves a mention in the commit \nmessage.\n\n> +\t\tOPT_SET_INT(0, \"ff\", &options.fast_forward, N_(\"allow fast-forward\"),\n> +\t\t\t FF_ALLOW),\n\nThe useful option when rebasing is '--no-ff', now that will no longer \nappear in the output of 'git rebase -h'\n\n> +\t\tOPT_SET_INT_F(0, \"ff-only\", &options.fast_forward,\n> +\t\t\t N_(\"abort if fast-forward is not possible\"),\n> +\t\t\t FF_ONLY, PARSE_OPT_NONEG),\n\nIs there a use for this outside of 'git pull --ff-only'? I'm far from \nconvinced we want this new option but if we do end up adding it I think \nit should error out in combination with '-i' or '-x' as '-i' implies the \nuser wants to change the existing commits and '-x' can end up changing \nthem as well.\n\nI think this patch addresses a valid problem but it seems to me that the \napproach taken pushes complexity into rebase to handle a case when pull \ndoes not need to invoke rebase in the first place.\n\nBest Wishes\n\nPhillip\n\n> \t\tOPT_CMDMODE(0, \"continue\", &action, N_(\"continue\"),\n> \t\t\t ACTION_CONTINUE),\n> \t\tOPT_CMDMODE(0, \"skip\", &action,\n> @@ -1635,7 +1641,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tallow_preemptive_ff = 0;\n> \t}\n> \tif (options.committer_date_is_author_date || options.ignore_date)\n> -\t\toptions.flags |= REBASE_FORCE;\n> +\t\toptions.fast_forward = FF_NO;\n> \n> \tfor (i = 0; i < options.git_am_opts.nr; i++) {\n> \t\tconst char *option = options.git_am_opts.v[i], *p;\n> @@ -1809,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(\"cannot combine '--signoff' with \"\n> \t\t\t \"'--preserve-merges'\");\n> \t\tstrvec_push(&options.git_am_opts, \"--signoff\");\n> -\t\toptions.flags |= REBASE_FORCE;\n> +\t\toptions.fast_forward = FF_NO;\n> \t}\n> \n> \tif (options.type == REBASE_PRESERVE_MERGES) {\n> @@ -1952,6 +1958,24 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (repo_read_index(the_repository) < 0)\n> \t\tdie(_(\"could not read index\"));\n> \n> +\t/*\n> +\t * Check if we are already based on onto with linear history,\n> +\t * in which case we could fast-forward without replacing the commits\n> +\t * with new commits recreated by replaying their changes.\n> +\t *\n> +\t * Note that can_fast_forward() initializes merge_base, so we have to\n> +\t * call it before checking allow_preemptive_ff.\n> +\t */\n> +\tallow_preemptive_ff = can_fast_forward(options.onto, options.upstream,\n> +\t\t\t\t\t options.restrict_revision,\n> +\t\t\t\t\t &options.orig_head, &merge_base)\n> +\t\t\t && allow_preemptive_ff;\n> +\n> +\tif (!allow_preemptive_ff && options.fast_forward == FF_ONLY) {\n> +\t\tret = error_ff_impossible();\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> \tif (options.autostash) {\n> \t\tcreate_autostash(the_repository, state_dir_path(\"autostash\", &options),\n> \t\t\t\t DEFAULT_REFLOG_ACTION);\n> @@ -1968,20 +1992,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t * everything leading up to orig_head) on top of onto.\n> \t */\n> \n> -\t/*\n> -\t * Check if we are already based on onto with linear history,\n> -\t * in which case we could fast-forward without replacing the commits\n> -\t * with new commits recreated by replaying their changes.\n> -\t *\n> -\t * Note that can_fast_forward() initializes merge_base, so we have to\n> -\t * call it before checking allow_preemptive_ff.\n> -\t */\n> -\tif (can_fast_forward(options.onto, options.upstream, options.restrict_revision,\n> -\t\t &options.orig_head, &merge_base) &&\n> -\t allow_preemptive_ff) {\n> +\tif (allow_preemptive_ff) {\n> \t\tint flag;\n> \n> -\t\tif (!(options.flags & REBASE_FORCE)) {\n> +\t\tif (options.fast_forward != FF_NO) {\n> \t\t\t/* Lazily switch to the target branch if needed... */\n> \t\t\tif (options.switch_to) {\n> \t\t\t\tstrbuf_reset(&buf);\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 237f2f18d4..f9b61478a2 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -123,7 +123,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \tif (opts->action == REPLAY_PICK) {\n> \t\tstruct option cp_extra[] = {\n> \t\t\tOPT_BOOL('x', NULL, &opts->record_origin, N_(\"append commit name\")),\n> -\t\t\tOPT_BOOL(0, \"ff\", &opts->allow_ff, N_(\"allow fast-forward\")),\n> +\t\t\tOPT_SET_INT(0, \"ff\", &opts->fast_forward, N_(\"allow fast-forward\"), FF_ALLOW),\n> \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n> \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n> \t\t\tOPT_BOOL(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n> @@ -166,7 +166,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\t\t\"--strategy\", opts->strategy ? 1 : 0,\n> \t\t\t\t\"--strategy-option\", opts->xopts ? 1 : 0,\n> \t\t\t\t\"-x\", opts->record_origin,\n> -\t\t\t\t\"--ff\", opts->allow_ff,\n> +\t\t\t\t\"--ff\", opts->fast_forward == FF_ALLOW,\n> \t\t\t\t\"--rerere-autoupdate\", opts->allow_rerere_auto == RERERE_AUTOUPDATE,\n> \t\t\t\t\"--no-rerere-autoupdate\", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,\n> \t\t\t\tNULL);\n> @@ -177,7 +177,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\topts->default_strategy = NULL;\n> \t}\n> \n> -\tif (opts->allow_ff)\n> +\tif (opts->fast_forward == FF_ALLOW)\n> \t\tverify_opt_compatible(me, \"--ff\",\n> \t\t\t\t\"--signoff\", opts->signoff,\n> \t\t\t\t\"--no-commit\", opts->no_commit,\n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38..84447937d2 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2120,18 +2120,23 @@ static int do_pick_commit(struct repository *r,\n> \t\treturn error(_(\"cannot get commit message for %s\"),\n> \t\t\toid_to_hex(&commit->object.oid));\n> \n> -\tif (opts->allow_ff && !is_fixup(command) &&\n> -\t ((parent && oideq(&parent->object.oid, &head)) ||\n> -\t (!parent && unborn))) {\n> -\t\tif (is_rebase_i(opts))\n> -\t\t\twrite_author_script(msg.message);\n> -\t\tres = fast_forward_to(r, &commit->object.oid, &head, unborn,\n> -\t\t\topts);\n> -\t\tif (res || command != TODO_REWORD)\n> +\tif (opts->fast_forward != FF_NO) {\n> +\t\tif (!is_fixup(command) &&\n> +\t\t ((parent && oideq(&parent->object.oid, &head)) ||\n> +\t\t (!parent && unborn))) {\n> +\t\t\tif (is_rebase_i(opts))\n> +\t\t\t\twrite_author_script(msg.message);\n> +\t\t\tres = fast_forward_to(r, &commit->object.oid, &head, unborn,\n> +\t\t\t\topts);\n> +\t\t\tif (res || command != TODO_REWORD)\n> +\t\t\t\tgoto leave;\n> +\t\t\treword = 1;\n> +\t\t\tmsg_file = NULL;\n> +\t\t\tgoto fast_forward_edit;\n> +\t\t} else if (opts->fast_forward == FF_ONLY) {\n> +\t\t\tres = error_ff_impossible();\n> \t\t\tgoto leave;\n> -\t\treword = 1;\n> -\t\tmsg_file = NULL;\n> -\t\tgoto fast_forward_edit;\n> +\t\t}\n> \t}\n> \tif (parent && parse_commit(parent) < 0)\n> \t\t/* TRANSLATORS: The first %s will be a \"todo\" command like\n> @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n> \telse if (!strcmp(key, \"options.record-origin\"))\n> \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n> \telse if (!strcmp(key, \"options.allow-ff\"))\n> -\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n> +\t\topts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;\n> \telse if (!strcmp(key, \"options.mainline\"))\n> \t\topts->mainline = git_config_int(key, value);\n> \telse if (!strcmp(key, \"options.strategy\"))\n> @@ -2853,17 +2858,17 @@ static int read_populate_opts(struct replay_opts *opts)\n> \t\t\topts->quiet = 1;\n> \n> \t\tif (file_exists(rebase_path_signoff())) {\n> -\t\t\topts->allow_ff = 0;\n> +\t\t\topts->fast_forward = FF_NO;\n> \t\t\topts->signoff = 1;\n> \t\t}\n> \n> \t\tif (file_exists(rebase_path_cdate_is_adate())) {\n> -\t\t\topts->allow_ff = 0;\n> +\t\t\topts->fast_forward = FF_NO;\n> \t\t\topts->committer_date_is_author_date = 1;\n> \t\t}\n> \n> \t\tif (file_exists(rebase_path_ignore_date())) {\n> -\t\t\topts->allow_ff = 0;\n> +\t\t\topts->fast_forward = FF_NO;\n> \t\t\topts->ignore_date = 1;\n> \t\t}\n> \n> @@ -3320,7 +3325,7 @@ static int save_opts(struct replay_opts *opts)\n> \tif (opts->record_origin)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.record-origin\", \"true\");\n> -\tif (opts->allow_ff)\n> +\tif (opts->fast_forward == FF_ALLOW)\n> \t\tres |= git_config_set_in_file_gently(opts_file,\n> \t\t\t\t\t\"options.allow-ff\", \"true\");\n> \tif (opts->mainline) {\n> @@ -3849,9 +3854,9 @@ static int do_merge(struct repository *r,\n> \t * If HEAD is not identical to the first parent of the original merge\n> \t * commit, we cannot fast-forward.\n> \t */\n> -\tcan_fast_forward = opts->allow_ff && commit && commit->parents &&\n> -\t\toideq(&commit->parents->item->object.oid,\n> -\t\t &head_commit->object.oid);\n> +\tcan_fast_forward = opts->fast_forward != FF_NO && commit &&\n> +\t\tcommit->parents && oideq(&commit->parents->item->object.oid,\n> +\t\t\t\t\t &head_commit->object.oid);\n> \n> \t/*\n> \t * If any merge head is different from the original one, we cannot\n> @@ -3885,6 +3890,11 @@ static int do_merge(struct repository *r,\n> \t\tgoto leave_merge;\n> \t}\n> \n> +\tif (opts->fast_forward == FF_ONLY && !can_fast_forward) {\n> +\t\tret = error_ff_impossible();\n> +\t\tgoto leave_merge;\n> +\t}\n> +\n> \tif (strategy || to_merge->next) {\n> \t\t/* Octopus merge */\n> \t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> @@ -4276,7 +4286,7 @@ static int pick_commits(struct repository *r,\n> \t/* Note that 0 for 3rd parameter of setenv means set only if not set */\n> \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> \tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> -\tif (opts->allow_ff)\n> +\tif (opts->fast_forward != FF_NO)\n> \t\tassert(!(opts->signoff || opts->no_commit ||\n> \t\t\t opts->record_origin || should_edit(opts) ||\n> \t\t\t opts->committer_date_is_author_date ||\n> @@ -5646,7 +5656,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \t\tBUG(\"invalid todo list after expanding IDs:\\n%s\",\n> \t\t new_todo.buf.buf);\n> \n> -\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n> +\tif (opts->fast_forward != FF_NO\n> +\t && skip_unnecessary_picks(r, &new_todo, &oid)) {\n> \t\ttodo_list_release(&new_todo);\n> \t\treturn error(_(\"could not skip unnecessary pick commands\"));\n> \t}\n> diff --git a/sequencer.h b/sequencer.h\n> index d57d8ea23d..e188dec312 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -28,6 +28,12 @@ enum commit_msg_cleanup_mode {\n> \tCOMMIT_MSG_CLEANUP_ALL\n> };\n> \n> +enum ff_type {\n> +\tFF_NO,\n> +\tFF_ALLOW,\n> +\tFF_ONLY\n> +};\n> +\n> struct replay_opts {\n> \tenum replay_action action;\n> \n> @@ -38,7 +44,6 @@ struct replay_opts {\n> \tint record_origin;\n> \tint no_commit;\n> \tint signoff;\n> -\tint allow_ff;\n> \tint allow_rerere_auto;\n> \tint allow_empty;\n> \tint allow_empty_message;\n> @@ -50,6 +55,8 @@ struct replay_opts {\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> \n> +\tenum ff_type fast_forward;\n> +\n> \tint mainline;\n> \n> \tchar *gpg_sign;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 66bcbbf952..858e406725 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -895,6 +895,11 @@ test_expect_success 'always cherry-pick with --no-ff' '\n> \ttest_must_be_empty out\n> '\n> \n> +test_expect_success 'interactive rebase prevents non-fast-forward with --ff-only' '\n> +\tgit checkout primary &&\n> +\ttest_must_fail git rebase -i --ff-only branch1\n> +'\n> +\n> test_expect_success 'set up commits with funny messages' '\n> \tgit checkout -b funny A &&\n> \techo >>file1 &&\n> diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh\n> index ec8062a66a..e656b5bd28 100755\n> --- a/t/t3409-rebase-preserve-merges.sh\n> +++ b/t/t3409-rebase-preserve-merges.sh\n> @@ -127,4 +127,8 @@ test_expect_success 'rebase -p ignores merge.log config' '\n> \t)\n> '\n> \n> +test_expect_success 'rebase -p prevents non-fast-forward with --ff-only' '\n> +\ttest_must_fail git rebase -p --ff-only main\n> +'\n> +\n> test_done\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index 43fcb68f27..69a85cb645 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -246,6 +246,15 @@ test_expect_success \"rebase: fast-forward rebase\" '\n> \tgit checkout feature-branch\n> '\n> \n> +test_expect_success \"rebase: impossible fast-forward rebase\" '\n> +\ttest_config rebase.autostash true &&\n> +\tgit reset --hard &&\n> +\techo dirty >>file1 &&\n> +\t(git rebase --ff-only unrelated-onto-branch || true) &&\n> +\tgrep dirty file1 &&\n> +\tgit checkout feature-branch\n> +'\n> +\n> test_expect_success \"rebase: noop rebase\" '\n> \ttest_config rebase.autostash true &&\n> \tgit reset --hard &&\n> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> index 52e8ccc933..af260b9faa 100755\n> --- a/t/t7601-merge-pull-config.sh\n> +++ b/t/t7601-merge-pull-config.sh\n> @@ -183,6 +183,16 @@ test_expect_success 'pull prevents non-fast-forward with \"only\" in pull.ff' '\n> \ttest_must_fail git pull . c3\n> '\n> \n> +test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '\n> +\tgit reset --hard c1 &&\n> +\ttest_must_fail git pull --rebase --ff-only . c3\n> +'\n> +\n> +test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '\n> +\tgit reset --hard c1 &&\n> +\ttest_must_fail git pull --no-rebase --ff-only . c3\n> +'\n> +\n> test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '\n> \tgit reset --hard c1 &&\n> \tgit config pull.twohead ours &&\n> \n"},{"id":"429193","messageId":"87a6n16r1l.fsf@evledraar.gmail.com","threadId":"56048","inReplyTo":"20210705044505.666977-1-alexhenrie24@gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-05T09:27:16Z","receivedAt":"2021-07-05T09:36:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jul 04 2021, Alex Henrie wrote:\n\n> I'm sending this patch as an RFC because I'm sure someone will find at\n> least one thing that needs to be changed before it's committed.\n\nFWIW that's true of most non-RFC patches :)\n\n> +int error_ff_impossible(void)\n> +{\n> +\terror(_(\"Not possible to fast-forward, aborting.\"));\n> +\treturn -1;\n> +}\n\nHere's one, the idiom is just \"return error\", i.e it itself returns -1.\n\nFWIW I'd consider doing:\n\t\n\t/* earlier, static scope */\n\tstatic const char error_ff_impossible = N_(\"Not possible...\");\n\t/* later, function scope */\n\t return error(error_ff_impossible);\n\nIt's a small difference, but for translators who use the jump-to-source\nwhile translating not having the indirection helps, and in this case\nit's just used 3 times...\n\n> [...]\n> \tif (parent && parse_commit(parent) < 0)\n> \t\t/* TRANSLATORS: The first %s will be a \"todo\" command like\n> @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n> \telse if (!strcmp(key, \"options.record-origin\"))\n> \t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n> \telse if (!strcmp(key, \"options.allow-ff\"))\n> -\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n> +\t\topts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;\n\nSince we're on nits, we try to wrap at 80 characters.\n\n> +test_expect_success \"rebase: impossible fast-forward rebase\" '\n> +\ttest_config rebase.autostash true &&\n> +\tgit reset --hard &&\n> +\techo dirty >>file1 &&\n> +\t(git rebase --ff-only unrelated-onto-branch || true) &&\n\nNever \"||\" git commands, better as test_might_fail. We don't want to\nhide segfaults.\n\nI saw Phillip Wood had some comments on the rest, I didn't test this,\njust skimmed it, but generally LGTM from a glance, not getting down to\nthe nuts of more deeply inspecting the logic.\n"},{"id":"429194","messageId":"xmqq8s2lnkta.fsf@gitster.g","threadId":"56048","inReplyTo":"349748b4-3c48-7ca7-eb0f-e859a15cab0f@gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-05T09:58:57Z","receivedAt":"2021-07-05T09:59:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> As I understand it the motivation for this change is to have 'git -c\n> pull.rebase=true pull --ff-only' actually fast forward. Why cant we\n> just change pull not to rebase in that case?\n> ...\n> Is there a use for this outside of 'git pull --ff-only'? I'm far from\n> convinced we want this new option but if we do end up adding it I\n> think it should error out in combination with '-i' or '-x' as '-i'\n> implies the user wants to change the existing commits and '-x' can end\n> up changing them as well.\n>\n> I think this patch addresses a valid problem but it seems to me that\n> the approach taken pushes complexity into rebase to handle a case when\n> pull does not need to invoke rebase in the first place.\n\nI share the sentiment, but my conclusion would be different.\n\nEven though we explain that \"pull\" is _like_ \"fetch\" followed by\n\"merge\" (or \"rebase\"), at the conceptual level, \"pull --ff-only\"\nshould not have to invoke merge or rebase backend. If the branch\nfast-fowards to the fetched tip, \"pull\" should be able to just\nupdate the branch and check it out, and if it doesn't, \"pull\" should\njust be able to fail.\n\nSimilarly, \"pull --ff\" (i.e. fast-forwading allowed) should be able\nto just update the branch and check it out when the current tip of\nthe branch can be fast-forwarded to the fetched tip.\n\nBut as you said, pull.rebase=interactive will break such a clean\nseparation of concerns. You can leave \"pull\" oblivious of what\n\"rebase -i\" wants to do when seeing a fast-forwardable history by\nteaching \"rebase\" (and \"merge\") backend to take \"--ff-only\", \"--ff\",\nand \"--no-ff\" options and having \"pull\" pass them through.\n\nWe can teach \"pull\" that certain backends (namely \"rebase -i\" in\nthis case) want to handle \"ff logic\" [*] themselves, and other\nbackends (i.e. \"rebase\" and \"merge\") do not mind if \"pull\" handles\n\"ff logic\" for them, but that will break the abstraction (e.g. do we\nreally want to teach \"pull\" that \"rebase -i\" wants to rewrite all\nthe commits even when the history fast-forwards?) and will become a\nsource of duplicated logic.\n\nAnother thing to worry about are hooks. post-rebase or post-merge\noperations would want to be carried out even when the history would\nfast-forward, and making \"pull\" to perform the fast-forwarding and\nknow which hooks should be called with what parameter so that we\ncould pretend as if the \"merge\" or \"rebase\" backend was indeed ran,\nbreaks the abstraction.\n\nSo, even though I wish that the world was simpler and we could\nhandle \"ff logic\" inside \"pull\", I am not sure if it is a realistic\nwish.\n\n\n[Footnote]\n\n* By \"ff logic\", I am referring to what should happen in the 3x2=6\n matrix when one of (\"--ff\", \"--ff-only\", or \"--no-ff\") is given\n and the history (does or does not) fast-forward.\n"},{"id":"429195","messageId":"60e2f44749749_1b8e782081f@natae.notmuch","threadId":"56048","inReplyTo":"20210705044505.666977-1-alexhenrie24@gmail.com","subject":"RE: [PATCH RFC] rebase: respect --ff-only option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-05T12:00:07Z","receivedAt":"2021-07-05T12:00:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Alex Henrie wrote:\n> The warning about pulling without specifying how to reconcile divergent\n> branches says that after setting pull.rebase to true, --ff-only can\n> still be passed on the command line to require a fast-forward. Make that\n> actually work.\n\nI don't see any value in doing `git rebase --ff-only`. What is the\ndifference with `git merge --ff-only`?\n\nPresumably this isn't meant to to be called directly by the user, but\nonly by `git pull`, so it's trying to address my comment [1]:\n\n > You can also pass --rebase, --no-rebase, or --ff-only on the command\n > line to override the configured default per invocation.\n\n Can I?\n\n git -c pull.rebase=true pull --ff-only\n\n `--ff-only` doesn't seem to be overriding the configuration.\n\nPassing --ff-only to `git rebase` doesn't solve the problem I was\npointing out, only half of it.\n\nSure, now this works:\n\n git -c pull.rebase=true pull --ff-only\n\nBut --ff-only is not really overriding anything, now you are passing the\nproblem along.\n\nConsider it the other way around:\n\n git -c pull.ff=only pull --rebase\n\nIs --rebase overriding anything?\n\nNo, now all these three fail:\n\n git -c pull.ff=only pull\n git -c pull.ff=only pull --rebase\n git -c pull.ff=only pull --merge\n\nThis is making the situation even worse.\n\nJunio already made the same mistake [2], and I already pointed out why\nthat doesn't work [3].\n\nThis is what we want:\n\n 1. git pull # fail by default unless it's a fast-forward\n 2. git pull --merge # force a merge (unless it's a fast-forward)\n 3. git pull --rebase # force a rebase (unless it's a fast-forward)\n\nIf you make `git rebase` honor --ff-only, and you make --ff-only the\ndefault, then `git pull --rebase` will *always* fail (unless it's a\nfast-forward).\n\nWe want this:\n\n git -c pull.ff=only pull --rebase\n\nTo *ignore* pull.ff=only, not honor it.\n\nI already explored all the options, which is why I maintain that the\nonly reasonable option is an orthogonal configuration:\n\n pull.mode=fast-forward\n\nCheers.\n\n[1] https://lore.kernel.org/git/60d7faaf5311a_b8dfe208bd@natae.notmuch/\n[2] https://lore.kernel.org/git/xmqqy2irjy4f.fsf@gitster.c.googlers.com/\n[3] https://lore.kernel.org/git/CAMP44s08mEyYqbjOeTeS46CngrbQMqP2=cMr1dtRLLk_BLAq3w@mail.gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"429197","messageId":"60e2f68057ebe_1b8e782087d@natae.notmuch","threadId":"56048","inReplyTo":"xmqq8s2lnkta.fsf@gitster.g","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-05T12:09:36Z","receivedAt":"2021-07-05T12:09:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> > As I understand it the motivation for this change is to have 'git -c\n> > pull.rebase=true pull --ff-only' actually fast forward. Why cant we\n> > just change pull not to rebase in that case?\n> > ...\n> > Is there a use for this outside of 'git pull --ff-only'? I'm far from\n> > convinced we want this new option but if we do end up adding it I\n> > think it should error out in combination with '-i' or '-x' as '-i'\n> > implies the user wants to change the existing commits and '-x' can end\n> > up changing them as well.\n> >\n> > I think this patch addresses a valid problem but it seems to me that\n> > the approach taken pushes complexity into rebase to handle a case when\n> > pull does not need to invoke rebase in the first place.\n> \n> I share the sentiment, but my conclusion would be different.\n> \n> Even though we explain that \"pull\" is _like_ \"fetch\" followed by\n> \"merge\" (or \"rebase\"), at the conceptual level, \"pull --ff-only\"\n> should not have to invoke merge or rebase backend.\n\nIndeed. I'm about to send a patch series that adds the\n`git fast-forward` command, so `git pull` doesn't even have to call\neither of those.\n\nThis cleanly separates the logic, except --ff-only remains purely for\n`git merge`, and instead there's a new:\n\n git -c pull.mode=fast-forward pull\n\nNow it's 100% clear what these three do:\n\n git -c pull.mode=fast-forward pull\n git -c pull.mode=merge pull\n git -c pull.mode=rebase pull\n\n-- \nFelipe Contreras\n"},{"id":"429238","messageId":"41852b7c-6ac7-3a3b-9f2d-6e537f52cd2a@gmail.com","threadId":"56048","inReplyTo":"xmqq8s2lnkta.fsf@gitster.g","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-05T13:54:44Z","receivedAt":"2021-07-05T13:54:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 05/07/2021 10:58, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> As I understand it the motivation for this change is to have 'git -c\n>> pull.rebase=true pull --ff-only' actually fast forward. Why cant we\n>> just change pull not to rebase in that case?\n>> ...\n>> Is there a use for this outside of 'git pull --ff-only'? I'm far from\n>> convinced we want this new option but if we do end up adding it I\n>> think it should error out in combination with '-i' or '-x' as '-i'\n>> implies the user wants to change the existing commits and '-x' can end\n>> up changing them as well.\n>>\n>> I think this patch addresses a valid problem but it seems to me that\n>> the approach taken pushes complexity into rebase to handle a case when\n>> pull does not need to invoke rebase in the first place.\n> \n> I share the sentiment, but my conclusion would be different.\n> \n> Even though we explain that \"pull\" is _like_ \"fetch\" followed by\n> \"merge\" (or \"rebase\"), at the conceptual level, \"pull --ff-only\"\n> should not have to invoke merge or rebase backend. If the branch\n> fast-fowards to the fetched tip, \"pull\" should be able to just\n> update the branch and check it out, and if it doesn't, \"pull\" should\n> just be able to fail.\n> \n> Similarly, \"pull --ff\" (i.e. fast-forwading allowed) should be able\n> to just update the branch and check it out when the current tip of\n> the branch can be fast-forwarded to the fetched tip.\n> \n> But as you said, pull.rebase=interactive will break such a clean\n> separation of concerns. You can leave \"pull\" oblivious of what\n> \"rebase -i\" wants to do when seeing a fast-forwardable history by\n> teaching \"rebase\" (and \"merge\") backend to take \"--ff-only\", \"--ff\",\n> and \"--no-ff\" options and having \"pull\" pass them through.\n\nMy main concern with the new rebase option was about a user invoking \n'git rebase -i --ff-only' directly.\n\nIf a user has pull.rebase=interactive and runs 'git pull --ff-only' then \nI'm not clear what they expect to happen. Assuming we can fast-forward \nwould they expect pull to run 'rebase -i' which would open their editor \nwith the todo list or would they expect that '--ff-only' means \"I just \nwant to fast-forward, I don't want to run 'rebase -i'\". If it is the \nlatter then we can just invoke 'git merge --ff-only' (so long as we \ndon't mind running the post-merge hook in this case) and not worry about \nadding more complexity to 'git rebase'\n\nThe relevant section of the pull man page only talks about merging in \nrelation to --ff-only\n\n --ff, --no-ff, --ff-only\n\tSpecifies how a merge is handled when the merged-in history\n is already a descendant of the current history. --ff is the\n default unless merging an annotated (and possibly signed)\n tag that is not stored in its natural place in the\n refs/tags/ hierarchy, in which case --no-ff is assumed.\n\n With --ff, when possible resolve the merge as a fast-forward\n (only update the branch pointer to match the merged branch;\n do not create a merge commit). When not possible (when the\n merged-in history is not a descendant of the current\n history), create a merge commit.\n\n\tWith --no-ff, create a merge commit in all cases, even when\n\tthe merge could instead be resolved as a fast-forward.\n\n\tWith --ff-only, resolve the merge as a fast-forward when\n\tpossible. When not possible, refuse to merge and exit with a\n\tnon-zero status.\n\n> We can teach \"pull\" that certain backends (namely \"rebase -i\" in\n> this case) want to handle \"ff logic\" [*] themselves, and other\n> backends (i.e. \"rebase\" and \"merge\") do not mind if \"pull\" handles\n> \"ff logic\" for them, but that will break the abstraction (e.g. do we\n> really want to teach \"pull\" that \"rebase -i\" wants to rewrite all\n> the commits even when the history fast-forwards?) and will become a\n> source of duplicated logic.\n> \n> Another thing to worry about are hooks. post-rebase \n\nThere is no post-rebase hook. There is the post-rewrite hook, I haven't \nchecked if we invoke it with no input of skip it entirely when we \nfast-forward.\n\n> or post-merge\n> operations would want to be carried out even when the history would\n> fast-forward, and making \"pull\" to perform the fast-forwarding and\n> know which hooks should be called with what parameter so that we\n> could pretend as if the \"merge\" or \"rebase\" backend was indeed ran,\n> breaks the abstraction.\n> \n> So, even though I wish that the world was simpler and we could\n> handle \"ff logic\" inside \"pull\", I am not sure if it is a realistic\n> wish.\n\nI think if we decide that 'pull --ff-only' always implies merging then \nthe world stays fairly simple. On the other hand if we want to somehow \ncombine rebasing with --ff-only it will be more complicated. If we go \nfor the latter then unless someone comes up with a good use for 'rebase \n--ff-only' in another context I would prefer the new rebase option to be \nmarked with PARSE_OPT_HIDDEN and that we also avoid making incidental \nchanges the existing rebase options.\n\nBest Wishes\n\nPhillip\n\n> \n> [Footnote]\n> \n> * By \"ff logic\", I am referring to what should happen in the 3x2=6\n> matrix when one of (\"--ff\", \"--ff-only\", or \"--no-ff\") is given\n> and the history (does or does not) fast-forward.\n> \n"},{"id":"429246","messageId":"7ee36923-0806-4316-729c-8418df5b6555@gmail.com","threadId":"56048","inReplyTo":"349748b4-3c48-7ca7-eb0f-e859a15cab0f@gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-05T15:29:20Z","receivedAt":"2021-07-05T15:30:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/07/2021 09:53, Phillip Wood wrote:\n> Hi Alex\n> \n> On 05/07/2021 05:45, Alex Henrie wrote:\n>> The warning about pulling without specifying how to reconcile divergent\n>> branches says that after setting pull.rebase to true, --ff-only can\n>> still be passed on the command line to require a fast-forward. Make that\n>> actually work.\n> \n> As I understand it the motivation for this change is to have 'git -c \n> pull.rebase=true pull --ff-only' actually fast forward. Why cant we just \n> change pull not to rebase in that case? \n\nLooking at origin/seen:builtin/pull.c we already check if we can \nfast-forward and unconditionally merge in that case irrespective of any \n'--rebase' option or pull.rebase config. It should be simple for pull to \nerror out if '--ff-only' is given and we cannot fast-forward.\n\nBest Wishes\n\nPhillip\n\n> I've left some comments about \n> the rebase changes below\n> \n> > [...]\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 12f093121d..b88f0cbcca 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>                N_(\"ignore changes in whitespace\")),\n>>           OPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n>>                     N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n>> -        OPT_BIT('f', \"force-rebase\", &options.flags,\n>> -            N_(\"cherry-pick all commits, even if unchanged\"),\n>> -            REBASE_FORCE),\n>> -        OPT_BIT(0, \"no-ff\", &options.flags,\n>> -            N_(\"cherry-pick all commits, even if unchanged\"),\n>> -            REBASE_FORCE),\n>> +        OPT_SET_INT_F('f', \"force-rebase\", &options.fast_forward,\n>> +                  N_(\"cherry-pick all commits, even if unchanged\"),\n>> +                  FF_NO, PARSE_OPT_NONEG),\n> \n> Why does this change rebase to start rejecting --no-force-rebase? This \n> is the sort of behavior change that deserves a mention in the commit \n> message.\n> \n>> +        OPT_SET_INT(0, \"ff\", &options.fast_forward, N_(\"allow \n>> fast-forward\"),\n>> +                FF_ALLOW),\n> \n> The useful option when rebasing is '--no-ff', now that will no longer \n> appear in the output of 'git rebase -h'\n> \n>> +        OPT_SET_INT_F(0, \"ff-only\", &options.fast_forward,\n>> +                  N_(\"abort if fast-forward is not possible\"),\n>> +                  FF_ONLY, PARSE_OPT_NONEG),\n> \n> Is there a use for this outside of 'git pull --ff-only'? I'm far from \n> convinced we want this new option but if we do end up adding it I think \n> it should error out in combination with '-i' or '-x' as '-i' implies the \n> user wants to change the existing commits and '-x' can end up changing \n> them as well.\n> \n> I think this patch addresses a valid problem but it seems to me that the \n> approach taken pushes complexity into rebase to handle a case when pull \n> does not need to invoke rebase in the first place.\n> \n> Best Wishes\n> \n> Phillip\n> \n>>           OPT_CMDMODE(0, \"continue\", &action, N_(\"continue\"),\n>>                   ACTION_CONTINUE),\n>>           OPT_CMDMODE(0, \"skip\", &action,\n>> @@ -1635,7 +1641,7 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>           allow_preemptive_ff = 0;\n>>       }\n>>       if (options.committer_date_is_author_date || options.ignore_date)\n>> -        options.flags |= REBASE_FORCE;\n>> +        options.fast_forward = FF_NO;\n>>       for (i = 0; i < options.git_am_opts.nr; i++) {\n>>           const char *option = options.git_am_opts.v[i], *p;\n>> @@ -1809,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>               die(\"cannot combine '--signoff' with \"\n>>                   \"'--preserve-merges'\");\n>>           strvec_push(&options.git_am_opts, \"--signoff\");\n>> -        options.flags |= REBASE_FORCE;\n>> +        options.fast_forward = FF_NO;\n>>       }\n>>       if (options.type == REBASE_PRESERVE_MERGES) {\n>> @@ -1952,6 +1958,24 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>       if (repo_read_index(the_repository) < 0)\n>>           die(_(\"could not read index\"));\n>> +    /*\n>> +     * Check if we are already based on onto with linear history,\n>> +     * in which case we could fast-forward without replacing the commits\n>> +     * with new commits recreated by replaying their changes.\n>> +     *\n>> +     * Note that can_fast_forward() initializes merge_base, so we \n>> have to\n>> +     * call it before checking allow_preemptive_ff.\n>> +     */\n>> +    allow_preemptive_ff = can_fast_forward(options.onto, \n>> options.upstream,\n>> +                           options.restrict_revision,\n>> +                           &options.orig_head, &merge_base)\n>> +                  && allow_preemptive_ff;\n>> +\n>> +    if (!allow_preemptive_ff && options.fast_forward == FF_ONLY) {\n>> +        ret = error_ff_impossible();\n>> +        goto cleanup;\n>> +    }\n>> +\n>>       if (options.autostash) {\n>>           create_autostash(the_repository, state_dir_path(\"autostash\", \n>> &options),\n>>                    DEFAULT_REFLOG_ACTION);\n>> @@ -1968,20 +1992,10 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>        * everything leading up to orig_head) on top of onto.\n>>        */\n>> -    /*\n>> -     * Check if we are already based on onto with linear history,\n>> -     * in which case we could fast-forward without replacing the commits\n>> -     * with new commits recreated by replaying their changes.\n>> -     *\n>> -     * Note that can_fast_forward() initializes merge_base, so we \n>> have to\n>> -     * call it before checking allow_preemptive_ff.\n>> -     */\n>> -    if (can_fast_forward(options.onto, options.upstream, \n>> options.restrict_revision,\n>> -            &options.orig_head, &merge_base) &&\n>> -        allow_preemptive_ff) {\n>> +    if (allow_preemptive_ff) {\n>>           int flag;\n>> -        if (!(options.flags & REBASE_FORCE)) {\n>> +        if (options.fast_forward != FF_NO) {\n>>               /* Lazily switch to the target branch if needed... */\n>>               if (options.switch_to) {\n>>                   strbuf_reset(&buf);\n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 237f2f18d4..f9b61478a2 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -123,7 +123,7 @@ static int run_sequencer(int argc, const char \n>> **argv, struct replay_opts *opts)\n>>       if (opts->action == REPLAY_PICK) {\n>>           struct option cp_extra[] = {\n>>               OPT_BOOL('x', NULL, &opts->record_origin, N_(\"append \n>> commit name\")),\n>> -            OPT_BOOL(0, \"ff\", &opts->allow_ff, N_(\"allow \n>> fast-forward\")),\n>> +            OPT_SET_INT(0, \"ff\", &opts->fast_forward, N_(\"allow \n>> fast-forward\"), FF_ALLOW),\n>>               OPT_BOOL(0, \"allow-empty\", &opts->allow_empty, \n>> N_(\"preserve initially empty commits\")),\n>>               OPT_BOOL(0, \"allow-empty-message\", \n>> &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n>>               OPT_BOOL(0, \"keep-redundant-commits\", \n>> &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n>> @@ -166,7 +166,7 @@ static int run_sequencer(int argc, const char \n>> **argv, struct replay_opts *opts)\n>>                   \"--strategy\", opts->strategy ? 1 : 0,\n>>                   \"--strategy-option\", opts->xopts ? 1 : 0,\n>>                   \"-x\", opts->record_origin,\n>> -                \"--ff\", opts->allow_ff,\n>> +                \"--ff\", opts->fast_forward == FF_ALLOW,\n>>                   \"--rerere-autoupdate\", opts->allow_rerere_auto == \n>> RERERE_AUTOUPDATE,\n>>                   \"--no-rerere-autoupdate\", opts->allow_rerere_auto == \n>> RERERE_NOAUTOUPDATE,\n>>                   NULL);\n>> @@ -177,7 +177,7 @@ static int run_sequencer(int argc, const char \n>> **argv, struct replay_opts *opts)\n>>           opts->default_strategy = NULL;\n>>       }\n>> -    if (opts->allow_ff)\n>> +    if (opts->fast_forward == FF_ALLOW)\n>>           verify_opt_compatible(me, \"--ff\",\n>>                   \"--signoff\", opts->signoff,\n>>                   \"--no-commit\", opts->no_commit,\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 0bec01cf38..84447937d2 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -2120,18 +2120,23 @@ static int do_pick_commit(struct repository *r,\n>>           return error(_(\"cannot get commit message for %s\"),\n>>               oid_to_hex(&commit->object.oid));\n>> -    if (opts->allow_ff && !is_fixup(command) &&\n>> -        ((parent && oideq(&parent->object.oid, &head)) ||\n>> -         (!parent && unborn))) {\n>> -        if (is_rebase_i(opts))\n>> -            write_author_script(msg.message);\n>> -        res = fast_forward_to(r, &commit->object.oid, &head, unborn,\n>> -            opts);\n>> -        if (res || command != TODO_REWORD)\n>> +    if (opts->fast_forward != FF_NO) {\n>> +        if (!is_fixup(command) &&\n>> +            ((parent && oideq(&parent->object.oid, &head)) ||\n>> +             (!parent && unborn))) {\n>> +            if (is_rebase_i(opts))\n>> +                write_author_script(msg.message);\n>> +            res = fast_forward_to(r, &commit->object.oid, &head, unborn,\n>> +                opts);\n>> +            if (res || command != TODO_REWORD)\n>> +                goto leave;\n>> +            reword = 1;\n>> +            msg_file = NULL;\n>> +            goto fast_forward_edit;\n>> +        } else if (opts->fast_forward == FF_ONLY) {\n>> +            res = error_ff_impossible();\n>>               goto leave;\n>> -        reword = 1;\n>> -        msg_file = NULL;\n>> -        goto fast_forward_edit;\n>> +        }\n>>       }\n>>       if (parent && parse_commit(parent) < 0)\n>>           /* TRANSLATORS: The first %s will be a \"todo\" command like\n>> @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, \n>> const char *value, void *data)\n>>       else if (!strcmp(key, \"options.record-origin\"))\n>>           opts->record_origin = git_config_bool_or_int(key, value, \n>> &error_flag);\n>>       else if (!strcmp(key, \"options.allow-ff\"))\n>> -        opts->allow_ff = git_config_bool_or_int(key, value, \n>> &error_flag);\n>> +        opts->fast_forward = git_config_bool_or_int(key, value, \n>> &error_flag) ? FF_ALLOW : FF_NO;\n>>       else if (!strcmp(key, \"options.mainline\"))\n>>           opts->mainline = git_config_int(key, value);\n>>       else if (!strcmp(key, \"options.strategy\"))\n>> @@ -2853,17 +2858,17 @@ static int read_populate_opts(struct \n>> replay_opts *opts)\n>>               opts->quiet = 1;\n>>           if (file_exists(rebase_path_signoff())) {\n>> -            opts->allow_ff = 0;\n>> +            opts->fast_forward = FF_NO;\n>>               opts->signoff = 1;\n>>           }\n>>           if (file_exists(rebase_path_cdate_is_adate())) {\n>> -            opts->allow_ff = 0;\n>> +            opts->fast_forward = FF_NO;\n>>               opts->committer_date_is_author_date = 1;\n>>           }\n>>           if (file_exists(rebase_path_ignore_date())) {\n>> -            opts->allow_ff = 0;\n>> +            opts->fast_forward = FF_NO;\n>>               opts->ignore_date = 1;\n>>           }\n>> @@ -3320,7 +3325,7 @@ static int save_opts(struct replay_opts *opts)\n>>       if (opts->record_origin)\n>>           res |= git_config_set_in_file_gently(opts_file,\n>>                       \"options.record-origin\", \"true\");\n>> -    if (opts->allow_ff)\n>> +    if (opts->fast_forward == FF_ALLOW)\n>>           res |= git_config_set_in_file_gently(opts_file,\n>>                       \"options.allow-ff\", \"true\");\n>>       if (opts->mainline) {\n>> @@ -3849,9 +3854,9 @@ static int do_merge(struct repository *r,\n>>        * If HEAD is not identical to the first parent of the original \n>> merge\n>>        * commit, we cannot fast-forward.\n>>        */\n>> -    can_fast_forward = opts->allow_ff && commit && commit->parents &&\n>> -        oideq(&commit->parents->item->object.oid,\n>> -              &head_commit->object.oid);\n>> +    can_fast_forward = opts->fast_forward != FF_NO && commit &&\n>> +        commit->parents && oideq(&commit->parents->item->object.oid,\n>> +                     &head_commit->object.oid);\n>>       /*\n>>        * If any merge head is different from the original one, we cannot\n>> @@ -3885,6 +3890,11 @@ static int do_merge(struct repository *r,\n>>           goto leave_merge;\n>>       }\n>> +    if (opts->fast_forward == FF_ONLY && !can_fast_forward) {\n>> +        ret = error_ff_impossible();\n>> +        goto leave_merge;\n>> +    }\n>> +\n>>       if (strategy || to_merge->next) {\n>>           /* Octopus merge */\n>>           struct child_process cmd = CHILD_PROCESS_INIT;\n>> @@ -4276,7 +4286,7 @@ static int pick_commits(struct repository *r,\n>>       /* Note that 0 for 3rd parameter of setenv means set only if not \n>> set */\n>>       setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n>>       prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n>> -    if (opts->allow_ff)\n>> +    if (opts->fast_forward != FF_NO)\n>>           assert(!(opts->signoff || opts->no_commit ||\n>>                opts->record_origin || should_edit(opts) ||\n>>                opts->committer_date_is_author_date ||\n>> @@ -5646,7 +5656,8 @@ int complete_action(struct repository *r, struct \n>> replay_opts *opts, unsigned fla\n>>           BUG(\"invalid todo list after expanding IDs:\\n%s\",\n>>               new_todo.buf.buf);\n>> -    if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n>> +    if (opts->fast_forward != FF_NO\n>> +        && skip_unnecessary_picks(r, &new_todo, &oid)) {\n>>           todo_list_release(&new_todo);\n>>           return error(_(\"could not skip unnecessary pick commands\"));\n>>       }\n>> diff --git a/sequencer.h b/sequencer.h\n>> index d57d8ea23d..e188dec312 100644\n>> --- a/sequencer.h\n>> +++ b/sequencer.h\n>> @@ -28,6 +28,12 @@ enum commit_msg_cleanup_mode {\n>>       COMMIT_MSG_CLEANUP_ALL\n>>   };\n>> +enum ff_type {\n>> +    FF_NO,\n>> +    FF_ALLOW,\n>> +    FF_ONLY\n>> +};\n>> +\n>>   struct replay_opts {\n>>       enum replay_action action;\n>> @@ -38,7 +44,6 @@ struct replay_opts {\n>>       int record_origin;\n>>       int no_commit;\n>>       int signoff;\n>> -    int allow_ff;\n>>       int allow_rerere_auto;\n>>       int allow_empty;\n>>       int allow_empty_message;\n>> @@ -50,6 +55,8 @@ struct replay_opts {\n>>       int committer_date_is_author_date;\n>>       int ignore_date;\n>> +    enum ff_type fast_forward;\n>> +\n>>       int mainline;\n>>       char *gpg_sign;\n>> diff --git a/t/t3404-rebase-interactive.sh \n>> b/t/t3404-rebase-interactive.sh\n>> index 66bcbbf952..858e406725 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -895,6 +895,11 @@ test_expect_success 'always cherry-pick with \n>> --no-ff' '\n>>       test_must_be_empty out\n>>   '\n>> +test_expect_success 'interactive rebase prevents non-fast-forward \n>> with --ff-only' '\n>> +    git checkout primary &&\n>> +    test_must_fail git rebase -i --ff-only branch1\n>> +'\n>> +\n>>   test_expect_success 'set up commits with funny messages' '\n>>       git checkout -b funny A &&\n>>       echo >>file1 &&\n>> diff --git a/t/t3409-rebase-preserve-merges.sh \n>> b/t/t3409-rebase-preserve-merges.sh\n>> index ec8062a66a..e656b5bd28 100755\n>> --- a/t/t3409-rebase-preserve-merges.sh\n>> +++ b/t/t3409-rebase-preserve-merges.sh\n>> @@ -127,4 +127,8 @@ test_expect_success 'rebase -p ignores merge.log \n>> config' '\n>>       )\n>>   '\n>> +test_expect_success 'rebase -p prevents non-fast-forward with \n>> --ff-only' '\n>> +    test_must_fail git rebase -p --ff-only main\n>> +'\n>> +\n>>   test_done\n>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n>> index 43fcb68f27..69a85cb645 100755\n>> --- a/t/t3420-rebase-autostash.sh\n>> +++ b/t/t3420-rebase-autostash.sh\n>> @@ -246,6 +246,15 @@ test_expect_success \"rebase: fast-forward rebase\" '\n>>       git checkout feature-branch\n>>   '\n>> +test_expect_success \"rebase: impossible fast-forward rebase\" '\n>> +    test_config rebase.autostash true &&\n>> +    git reset --hard &&\n>> +    echo dirty >>file1 &&\n>> +    (git rebase --ff-only unrelated-onto-branch || true) &&\n>> +    grep dirty file1 &&\n>> +    git checkout feature-branch\n>> +'\n>> +\n>>   test_expect_success \"rebase: noop rebase\" '\n>>       test_config rebase.autostash true &&\n>>       git reset --hard &&\n>> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n>> index 52e8ccc933..af260b9faa 100755\n>> --- a/t/t7601-merge-pull-config.sh\n>> +++ b/t/t7601-merge-pull-config.sh\n>> @@ -183,6 +183,16 @@ test_expect_success 'pull prevents \n>> non-fast-forward with \"only\" in pull.ff' '\n>>       test_must_fail git pull . c3\n>>   '\n>> +test_expect_success 'pull prevents non-fast-forward with --rebase \n>> --ff-only' '\n>> +    git reset --hard c1 &&\n>> +    test_must_fail git pull --rebase --ff-only . c3\n>> +'\n>> +\n>> +test_expect_success 'pull prevents non-fast-forward with --no-rebase \n>> --ff-only' '\n>> +    git reset --hard c1 &&\n>> +    test_must_fail git pull --no-rebase --ff-only . c3\n>> +'\n>> +\n>>   test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '\n>>       git reset --hard c1 &&\n>>       git config pull.twohead ours &&\n>>\n"},{"id":"429249","messageId":"xmqqpmvwn1qp.fsf@gitster.g","threadId":"56048","inReplyTo":"7ee36923-0806-4316-729c-8418df5b6555@gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-05T16:50:54Z","receivedAt":"2021-07-05T16:51:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Looking at origin/seen:builtin/pull.c we already check if we can\n> fast-forward and unconditionally merge in that case irrespective of\n> any '--rebase' option or pull.rebase config. It should be simple for\n> pull to error out if '--ff-only' is given and we cannot fast-forward.\n\nExcellent.\n\nEven though teaching even more special case on the \"git pull\" side\nmakes me feel somewhat dirty, but I think it would be a small price\nto pay, and the end result would save an useless fork whose sole\npurpose is to make the integration step after fetch fail when \"pull\"\ncan easily tell, as you said, that it ought to fail, so overall it\nwould probably be a net win.\n\nThanks.\n"},{"id":"429252","messageId":"xmqqlf6kmup4.fsf@gitster.g","threadId":"56048","inReplyTo":"xmqqpmvwn1qp.fsf@gitster.g","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-05T19:23:03Z","receivedAt":"2021-07-05T19:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Phillip Wood writes:\n>\n>> Looking at origin/seen:builtin/pull.c we already check if we can\n>> fast-forward and unconditionally merge in that case irrespective of\n>> any '--rebase' option or pull.rebase config. It should be simple for\n>> pull to error out if '--ff-only' is given and we cannot fast-forward.\n>\n> Excellent.\n>\n> Even though teaching even more special case on the \"git pull\" side\n> makes me feel somewhat dirty, but I think it would be a small price\n> to pay, and the end result would save an useless fork whose sole\n> purpose is to make the integration step after fetch fail when \"pull\"\n> can easily tell, as you said, that it ought to fail, so overall it\n> would probably be a net win.\n\nA tangent after thinking a bit more.\n\nI do not think \"pull.rebase=interactive\" affects the \"ff logic\" at\nall. Rebasing integration rebuilds _your_ commits on the current\nbranch on top of the tip of _their_ history, and if their history is\na descendant of your history (i.e. the history fast-forwards), by\ndefinition, you do not have anything you need to rebuild on top of\ntheirs, whether with the opportunity to make tweaks via \"rebase -i\"\nor without.\n\nThis observation does not change the conclusion at all, but I should\nhave spoken after thinking X-<.\n\nThanks.\n"},{"id":"429256","messageId":"CAMMLpeQG-eYgWTXyG0YzgN3U8QDASAtNFpxyXDFFPSVNzfw18g@mail.gmail.com","threadId":"56048","inReplyTo":"xmqqlf6kmup4.fsf@gitster.g","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-07-05T19:48:05Z","receivedAt":"2021-07-05T19:48:20Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Jul 5, 2021 at 2:53 AM Phillip Wood wrote:\n>\n> On 05/07/2021 05:45, Alex Henrie wrote:\n> > index 12f093121d..b88f0cbcca 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > N_(\"ignore changes in whitespace\")),\n> > OPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n> > N_(\"action\"), N_(\"passed to 'git apply'\"), 0),\n> > - OPT_BIT('f', \"force-rebase\", &options.flags,\n> > - N_(\"cherry-pick all commits, even if unchanged\"),\n> > - REBASE_FORCE),\n> > - OPT_BIT(0, \"no-ff\", &options.flags,\n> > - N_(\"cherry-pick all commits, even if unchanged\"),\n> > - REBASE_FORCE),\n> > + OPT_SET_INT_F('f', \"force-rebase\", &options.fast_forward,\n> > + N_(\"cherry-pick all commits, even if unchanged\"),\n> > + FF_NO, PARSE_OPT_NONEG),\n>\n> Why does this change rebase to start rejecting --no-force-rebase? This\n> is the sort of behavior change that deserves a mention in the commit\n> message.\n\nThat was accidental, sorry. I copied and pasted option code from\nanother place without paying attention to the PARSE_OPT_NONEG.\n\n> > + OPT_SET_INT(0, \"ff\", &options.fast_forward, N_(\"allow fast-forward\"),\n> > + FF_ALLOW),\n>\n> The useful option when rebasing is '--no-ff', now that will no longer\n> appear in the output of 'git rebase -h'\n\nYeah, that's a good point.\n\nOn Mon, Jul 5, 2021 at 3:36 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> On Sun, Jul 04 2021, Alex Henrie wrote:\n>\n> > +int error_ff_impossible(void)\n> > +{\n> > + error(_(\"Not possible to fast-forward, aborting.\"));\n> > + return -1;\n> > +}\n>\n> Here's one, the idiom is just \"return error\", i.e it itself returns -1.\n\nThat would indeed be simpler; thanks for pointing that out.\n\n> FWIW I'd consider doing:\n>\n> /* earlier, static scope */\n> static const char error_ff_impossible = N_(\"Not possible...\");\n> /* later, function scope */\n> return error(error_ff_impossible);\n>\n> It's a small difference, but for translators who use the jump-to-source\n> while translating not having the indirection helps, and in this case\n> it's just used 3 times...\n\nWouldn't jump-to-source take the user to the English text in advice.c\neither way? How does putting it in a static variable help?\n\n> > [...]\n> > if (parent && parse_commit(parent) < 0)\n> > /* TRANSLATORS: The first %s will be a \"todo\" command like\n> > @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n> > else if (!strcmp(key, \"options.record-origin\"))\n> > opts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n> > else if (!strcmp(key, \"options.allow-ff\"))\n> > - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n> > + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;\n>\n> Since we're on nits, we try to wrap at 80 characters.\n\nThanks, I didn't know what the exact cutoff was.\n\n> > +test_expect_success \"rebase: impossible fast-forward rebase\" '\n> > + test_config rebase.autostash true &&\n> > + git reset --hard &&\n> > + echo dirty >>file1 &&\n> > + (git rebase --ff-only unrelated-onto-branch || true) &&\n>\n> Never \"||\" git commands, better as test_might_fail. We don't want to\n> hide segfaults.\n\nAlso thanks for the advice here.\n\nOn Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > Looking at origin/seen:builtin/pull.c we already check if we can\n> > fast-forward and unconditionally merge in that case irrespective of\n> > any '--rebase' option or pull.rebase config. It should be simple for\n> > pull to error out if '--ff-only' is given and we cannot fast-forward.\n>\n> Excellent.\n>\n> Even though teaching even more special case on the \"git pull\" side\n> makes me feel somewhat dirty, but I think it would be a small price\n> to pay, and the end result would save an useless fork whose sole\n> purpose is to make the integration step after fetch fail when \"pull\"\n> can easily tell, as you said, that it ought to fail, so overall it\n> would probably be a net win.\n\nOkay, so it sounds like I should just scrap this patch and try again,\nafter \"pull: cleanup autostash check\" is in master, with a patch that\nonly modifies `git pull` and not `git rebase`. (Unless someone more\nexperienced wants to take over, which would be fine by me.)\n\nThanks anyway to Phillip and Ævar: Your feedback will help me write\nbetter patches in the future.\n\n-Alex\n"},{"id":"429290","messageId":"87mtqz5wb2.fsf@evledraar.gmail.com","threadId":"56048","inReplyTo":"CAMMLpeQG-eYgWTXyG0YzgN3U8QDASAtNFpxyXDFFPSVNzfw18g@mail.gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-06T14:43:48Z","receivedAt":"2021-07-06T14:52:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 05 2021, Alex Henrie wrote:\n\n> On Mon, Jul 5, 2021 at 3:36 AM Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Sun, Jul 04 2021, Alex Henrie wrote:\n>>\n>> > +int error_ff_impossible(void)\n>> > +{\n>> > + error(_(\"Not possible to fast-forward, aborting.\"));\n>> > + return -1;\n>> > +}\n>>\n>> Here's one, the idiom is just \"return error\", i.e it itself returns -1.\n>\n> That would indeed be simpler; thanks for pointing that out.\n>\n>> FWIW I'd consider doing:\n>>\n>> /* earlier, static scope */\n>> static const char error_ff_impossible = N_(\"Not possible...\");\n>> /* later, function scope */\n>> return error(error_ff_impossible);\n>>\n>> It's a small difference, but for translators who use the jump-to-source\n>> while translating not having the indirection helps, and in this case\n>> it's just used 3 times...\n>\n> Wouldn't jump-to-source take the user to the English text in advice.c\n> either way? How does putting it in a static variable help?\n\nYes, sorry. I don't know what I was thinking there. What I said would\nonly apply if _() was used inline, nevermind.\n\n>> > [...]\n>> > if (parent && parse_commit(parent) < 0)\n>> > /* TRANSLATORS: The first %s will be a \"todo\" command like\n>> > @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n>> > else if (!strcmp(key, \"options.record-origin\"))\n>> > opts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n>> > else if (!strcmp(key, \"options.allow-ff\"))\n>> > - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n>> > + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;\n>>\n>> Since we're on nits, we try to wrap at 80 characters.\n>\n> Thanks, I didn't know what the exact cutoff was.\n\nIt's not strictly adhered to, and depending on the code we'll have it be\nlonger (sometimes much so) if it helps the general readability,\ne.g. long repetitive declarations or something. For this sort of thing\nit's generally preferred.\n\n> On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano wrote:\n>>\n>> Phillip Wood writes:\n>>\n>> > Looking at origin/seen:builtin/pull.c we already check if we can\n>> > fast-forward and unconditionally merge in that case irrespective of\n>> > any '--rebase' option or pull.rebase config. It should be simple for\n>> > pull to error out if '--ff-only' is given and we cannot fast-forward.\n>>\n>> Excellent.\n>>\n>> Even though teaching even more special case on the \"git pull\" side\n>> makes me feel somewhat dirty, but I think it would be a small price\n>> to pay, and the end result would save an useless fork whose sole\n>> purpose is to make the integration step after fetch fail when \"pull\"\n>> can easily tell, as you said, that it ought to fail, so overall it\n>> would probably be a net win.\n>\n> Okay, so it sounds like I should just scrap this patch and try again,\n> after \"pull: cleanup autostash check\" is in master, with a patch that\n> only modifies `git pull` and not `git rebase`. (Unless someone more\n> experienced wants to take over, which would be fine by me.)\n\nI've just skimmed that whole part of the larger \"what should we do\"\ndiscussion here, and don't really have an opinion, but I see Phillip\nWood replied to this downthread.\n\nYou can grab the branches Junio mentions in What's Cooking from\nhttps://github.com/gitster/git.git, if you'd like to rebase on top of\nFelipe's (or from the ML).\n\nAnd if you fetch from it with:\n\n fetch = +refs/notes/amlog:refs/notes/amlog\n\nAnd set:\n\n [notes]\n displayRef = refs/notes/amlog\n\nYou'll get mappings from the mails to the commits Junio applies.\n\n(Note that they're not always 1=1 the same, even after ignoring Junio's\nSigned-off-by header, he'll sometimes fix them up as he queues them,\nmight use a different base for \"master\" than you did etc.)\n"},{"id":"429293","messageId":"103eaf23-e55a-4583-bf02-0cebb78f00ae@gmail.com","threadId":"56048","inReplyTo":"CAMMLpeQG-eYgWTXyG0YzgN3U8QDASAtNFpxyXDFFPSVNzfw18g@mail.gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-07-06T13:52:45Z","receivedAt":"2021-07-06T15:18:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 05/07/2021 20:48, Alex Henrie wrote:\n> On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano wrote:\n>>\n>> Phillip Wood writes:\n>>\n>>> Looking at origin/seen:builtin/pull.c we already check if we can\n>>> fast-forward and unconditionally merge in that case irrespective of\n>>> any '--rebase' option or pull.rebase config. It should be simple for\n>>> pull to error out if '--ff-only' is given and we cannot fast-forward.\n>>\n>> Excellent.\n>>\n>> Even though teaching even more special case on the \"git pull\" side\n>> makes me feel somewhat dirty, but I think it would be a small price\n>> to pay, and the end result would save an useless fork whose sole\n>> purpose is to make the integration step after fetch fail when \"pull\"\n>> can easily tell, as you said, that it ought to fail, so overall it\n>> would probably be a net win.\n> \n> Okay, so it sounds like I should just scrap this patch and try again,\n> after \"pull: cleanup autostash check\" is in master, with a patch that\n> only modifies `git pull` and not `git rebase`. (Unless someone more\n> experienced wants to take over, which would be fine by me.)\n\nI don't think you need to wait, if necessary you could base your patches \non top of that branch. Junio's latest what's cooking email said he was \ngoing to merge that branch into next so it is unlikely to change. In any \ncase looking at the 'git diff origin/master origin/seen builtin/pull.c' \nI suspect the changes required won't conflict with that branch.\n\n> Thanks anyway to Phillip and Ævar: Your feedback will help me write\n> better patches in the future.\n\nYou're welcome, I look forward to reading your future patches\n\nBest wishes\n\nPhillip\n> -Alex\n> \n"},{"id":"429380","messageId":"60e4f5a354142_1c5ca520819@natae.notmuch","threadId":"56048","inReplyTo":"41852b7c-6ac7-3a3b-9f2d-6e537f52cd2a@gmail.com","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-07T00:30:27Z","receivedAt":"2021-07-07T00:30:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 05/07/2021 10:58, Junio C Hamano wrote:\n\n> My main concern with the new rebase option was about a user invoking \n> 'git rebase -i --ff-only' directly.\n> \n> If a user has pull.rebase=interactive and runs 'git pull --ff-only' then \n> I'm not clear what they expect to happen. Assuming we can fast-forward \n> would they expect pull to run 'rebase -i' which would open their editor \n> with the todo list or would they expect that '--ff-only' means \"I just \n> want to fast-forward, I don't want to run 'rebase -i'\". If it is the \n> latter then we can just invoke 'git merge --ff-only' (so long as we \n> don't mind running the post-merge hook in this case) and not worry about \n> adding more complexity to 'git rebase'\n\nOnce again my suggestion is to keep these as orthogonal, then everything\nbecomes clear:\n\n git -c pull.mode=ff-only -c pull.rebase=interactive pull\n\nThis is just a fast-forward merge (git merge --ff-only).\n\n git -c pull.mode=ff-only -c pull.rebase=interactive pull --rebase\n\nThis is an interactive rebase (--ff-only is ignored).\n\n git -c pull.mode=ff-only -c pull.rebase=interactive pull --merge\n\nThis is a real merge (both --ff-only and --rebase=interactive are ignored).\n\n git -c pull.mode=ff-only -c pull.rebase=interactive pull --merge --ff-only\n\nThis is a --ff-only merge.\n\n> The relevant section of the pull man page only talks about merging in \n> relation to --ff-only\n\nAnd my suggestion is that it should stay that way.\n\n> > or post-merge\n> > operations would want to be carried out even when the history would\n> > fast-forward, and making \"pull\" to perform the fast-forwarding and\n> > know which hooks should be called with what parameter so that we\n> > could pretend as if the \"merge\" or \"rebase\" backend was indeed ran,\n> > breaks the abstraction.\n> > \n> > So, even though I wish that the world was simpler and we could\n> > handle \"ff logic\" inside \"pull\", I am not sure if it is a realistic\n> > wish.\n> \n> I think if we decide that 'pull --ff-only' always implies merging then \n> the world stays fairly simple.\n\nIndeed, which is my suggestion.\n\n> On the other hand if we want to somehow \n> combine rebasing with --ff-only it will be more complicated. If we go \n> for the latter then unless someone comes up with a good use for 'rebase \n> --ff-only' in another context I would prefer the new rebase option to be \n> marked with PARSE_OPT_HIDDEN and that we also avoid making incidental \n> changes the existing rebase options.\n\nOnce again, we cannot do that, because then this will be broken:\n\n git -c pull.ff=only pull --rebase\n\n-- \nFelipe Contreras\n"},{"id":"429383","messageId":"60e4ffd4d2a93_1c5ca52081a@natae.notmuch","threadId":"56048","inReplyTo":"xmqqpmvwn1qp.fsf@gitster.g","subject":"Re: [PATCH RFC] rebase: respect --ff-only option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-07T01:13:56Z","receivedAt":"2021-07-07T01:14:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> > Looking at origin/seen:builtin/pull.c we already check if we can\n> > fast-forward and unconditionally merge in that case irrespective of\n> > any '--rebase' option or pull.rebase config. It should be simple for\n> > pull to error out if '--ff-only' is given and we cannot fast-forward.\n> \n> Excellent.\n> \n> Even though teaching even more special case on the \"git pull\" side\n> makes me feel somewhat dirty,\n\nIt makes you feeld dirty for a reason.\n\nWhy don't you just actually try the proposal?\n\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1052,6 +1052,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n show_advice_pull_non_ff();\n }\n \n+ if (!can_ff && (!opt_ff || !strcmp(opt_ff, \"--ff-only\")))\n+ die(\"unable to fast-forward\");\n+\n if (opt_rebase) {\n int ret = 0;\n int ran_ff = 0;\n\nThen:\n\n ./git pull . topic\n ./git pull --no-rebase . topic\n ./git pull --rebase . topic\n\nFrom 1 to 10 how broken would you say that is?\n\n-------------------\nt4013-diff-various.sh (Wstat: 256 Tests: 208 Failed: 91)\n Failed tests: 1, 49-63, 67-69, 71-91, 101-116, 120-124\n 127-130, 138-141, 144-145, 147-150, 152-153\n 171-175, 189-193, 201-204\n Non-zero exit status: 1\nt5521-pull-options.sh (Wstat: 256 Tests: 20 Failed: 2)\n Failed tests: 12, 16\n Non-zero exit status: 1\nt5524-pull-msg.sh (Wstat: 256 Tests: 3 Failed: 2)\n Failed tests: 2-3\n Non-zero exit status: 1\nt5520-pull.sh (Wstat: 256 Tests: 72 Failed: 39)\n Failed tests: 16, 19, 23-24, 26-29, 33-41, 43-46, 48-53\n 55-57, 59-64, 69, 71-72\n Non-zero exit status: 1\nt5533-push-cas.sh (Wstat: 256 Tests: 23 Failed: 2)\n Failed tests: 21-22\n Non-zero exit status: 1\nt5553-set-upstream.sh (Wstat: 256 Tests: 19 Failed: 6)\n Failed tests: 11-14, 16-17\n Non-zero exit status: 1\nt5604-clone-reference.sh (Wstat: 256 Tests: 33 Failed: 4)\n Failed tests: 13-16\n Non-zero exit status: 1\nt5572-pull-submodule.sh (Wstat: 256 Tests: 64 Failed: 3)\n Failed tests: 62-64\n Non-zero exit status: 1\nt6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 3)\n Failed tests: 9, 11-12\n Non-zero exit status: 1\nt6402-merge-rename.sh (Wstat: 256 Tests: 46 Failed: 8)\n Failed tests: 2-8, 11\n Non-zero exit status: 1\nt6417-merge-ours-theirs.sh (Wstat: 256 Tests: 7 Failed: 1)\n Failed test: 6\n Non-zero exit status: 1\nt7601-merge-pull-config.sh (Wstat: 256 Tests: 32 Failed: 3)\n Failed tests: 11, 15-16\n Non-zero exit status: 1\nt7603-merge-reduce-heads.sh (Wstat: 256 Tests: 13 Failed: 1)\n Failed test: 3\n Non-zero exit status: 1\n\n-- \nFelipe Contreras\n"}]}