Message ID | 20210705044505.666977-1-alexhenrie24@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RFC] rebase: respect --ff-only option | expand |
Hi Alex On 05/07/2021 05:45, Alex Henrie wrote: > The warning about pulling without specifying how to reconcile divergent > branches says that after setting pull.rebase to true, --ff-only can > still be passed on the command line to require a fast-forward. Make that > actually work. As I understand it the motivation for this change is to have 'git -c pull.rebase=true pull --ff-only' actually fast forward. Why cant we just change pull not to rebase in that case? I've left some comments about the rebase changes below > [...] > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 12f093121d..b88f0cbcca 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > N_("ignore changes in whitespace")), > OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts, > N_("action"), N_("passed to 'git apply'"), 0), > - OPT_BIT('f', "force-rebase", &options.flags, > - N_("cherry-pick all commits, even if unchanged"), > - REBASE_FORCE), > - OPT_BIT(0, "no-ff", &options.flags, > - N_("cherry-pick all commits, even if unchanged"), > - REBASE_FORCE), > + OPT_SET_INT_F('f', "force-rebase", &options.fast_forward, > + N_("cherry-pick all commits, even if unchanged"), > + FF_NO, PARSE_OPT_NONEG), Why does this change rebase to start rejecting --no-force-rebase? This is the sort of behavior change that deserves a mention in the commit message. > + OPT_SET_INT(0, "ff", &options.fast_forward, N_("allow fast-forward"), > + FF_ALLOW), The useful option when rebasing is '--no-ff', now that will no longer appear in the output of 'git rebase -h' > + OPT_SET_INT_F(0, "ff-only", &options.fast_forward, > + N_("abort if fast-forward is not possible"), > + FF_ONLY, PARSE_OPT_NONEG), Is there a use for this outside of 'git pull --ff-only'? I'm far from convinced we want this new option but if we do end up adding it I think it should error out in combination with '-i' or '-x' as '-i' implies the user wants to change the existing commits and '-x' can end up changing them as well. I think this patch addresses a valid problem but it seems to me that the approach taken pushes complexity into rebase to handle a case when pull does not need to invoke rebase in the first place. Best Wishes Phillip > OPT_CMDMODE(0, "continue", &action, N_("continue"), > ACTION_CONTINUE), > OPT_CMDMODE(0, "skip", &action, > @@ -1635,7 +1641,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > allow_preemptive_ff = 0; > } > if (options.committer_date_is_author_date || options.ignore_date) > - options.flags |= REBASE_FORCE; > + options.fast_forward = FF_NO; > > for (i = 0; i < options.git_am_opts.nr; i++) { > const char *option = options.git_am_opts.v[i], *p; > @@ -1809,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > die("cannot combine '--signoff' with " > "'--preserve-merges'"); > strvec_push(&options.git_am_opts, "--signoff"); > - options.flags |= REBASE_FORCE; > + options.fast_forward = FF_NO; > } > > if (options.type == REBASE_PRESERVE_MERGES) { > @@ -1952,6 +1958,24 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (repo_read_index(the_repository) < 0) > die(_("could not read index")); > > + /* > + * Check if we are already based on onto with linear history, > + * in which case we could fast-forward without replacing the commits > + * with new commits recreated by replaying their changes. > + * > + * Note that can_fast_forward() initializes merge_base, so we have to > + * call it before checking allow_preemptive_ff. > + */ > + allow_preemptive_ff = can_fast_forward(options.onto, options.upstream, > + options.restrict_revision, > + &options.orig_head, &merge_base) > + && allow_preemptive_ff; > + > + if (!allow_preemptive_ff && options.fast_forward == FF_ONLY) { > + ret = error_ff_impossible(); > + goto cleanup; > + } > + > if (options.autostash) { > create_autostash(the_repository, state_dir_path("autostash", &options), > DEFAULT_REFLOG_ACTION); > @@ -1968,20 +1992,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > * everything leading up to orig_head) on top of onto. > */ > > - /* > - * Check if we are already based on onto with linear history, > - * in which case we could fast-forward without replacing the commits > - * with new commits recreated by replaying their changes. > - * > - * Note that can_fast_forward() initializes merge_base, so we have to > - * call it before checking allow_preemptive_ff. > - */ > - if (can_fast_forward(options.onto, options.upstream, options.restrict_revision, > - &options.orig_head, &merge_base) && > - allow_preemptive_ff) { > + if (allow_preemptive_ff) { > int flag; > > - if (!(options.flags & REBASE_FORCE)) { > + if (options.fast_forward != FF_NO) { > /* Lazily switch to the target branch if needed... */ > if (options.switch_to) { > strbuf_reset(&buf); > diff --git a/builtin/revert.c b/builtin/revert.c > index 237f2f18d4..f9b61478a2 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -123,7 +123,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > if (opts->action == REPLAY_PICK) { > struct option cp_extra[] = { > OPT_BOOL('x', NULL, &opts->record_origin, N_("append commit name")), > - OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")), > + OPT_SET_INT(0, "ff", &opts->fast_forward, N_("allow fast-forward"), FF_ALLOW), > OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")), > OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")), > OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")), > @@ -166,7 +166,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > "--strategy", opts->strategy ? 1 : 0, > "--strategy-option", opts->xopts ? 1 : 0, > "-x", opts->record_origin, > - "--ff", opts->allow_ff, > + "--ff", opts->fast_forward == FF_ALLOW, > "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, > "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE, > NULL); > @@ -177,7 +177,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > opts->default_strategy = NULL; > } > > - if (opts->allow_ff) > + if (opts->fast_forward == FF_ALLOW) > verify_opt_compatible(me, "--ff", > "--signoff", opts->signoff, > "--no-commit", opts->no_commit, > diff --git a/sequencer.c b/sequencer.c > index 0bec01cf38..84447937d2 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2120,18 +2120,23 @@ static int do_pick_commit(struct repository *r, > return error(_("cannot get commit message for %s"), > oid_to_hex(&commit->object.oid)); > > - if (opts->allow_ff && !is_fixup(command) && > - ((parent && oideq(&parent->object.oid, &head)) || > - (!parent && unborn))) { > - if (is_rebase_i(opts)) > - write_author_script(msg.message); > - res = fast_forward_to(r, &commit->object.oid, &head, unborn, > - opts); > - if (res || command != TODO_REWORD) > + if (opts->fast_forward != FF_NO) { > + if (!is_fixup(command) && > + ((parent && oideq(&parent->object.oid, &head)) || > + (!parent && unborn))) { > + if (is_rebase_i(opts)) > + write_author_script(msg.message); > + res = fast_forward_to(r, &commit->object.oid, &head, unborn, > + opts); > + if (res || command != TODO_REWORD) > + goto leave; > + reword = 1; > + msg_file = NULL; > + goto fast_forward_edit; > + } else if (opts->fast_forward == FF_ONLY) { > + res = error_ff_impossible(); > goto leave; > - reword = 1; > - msg_file = NULL; > - goto fast_forward_edit; > + } > } > if (parent && parse_commit(parent) < 0) > /* TRANSLATORS: The first %s will be a "todo" command like > @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) > else if (!strcmp(key, "options.record-origin")) > opts->record_origin = git_config_bool_or_int(key, value, &error_flag); > else if (!strcmp(key, "options.allow-ff")) > - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); > + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO; > else if (!strcmp(key, "options.mainline")) > opts->mainline = git_config_int(key, value); > else if (!strcmp(key, "options.strategy")) > @@ -2853,17 +2858,17 @@ static int read_populate_opts(struct replay_opts *opts) > opts->quiet = 1; > > if (file_exists(rebase_path_signoff())) { > - opts->allow_ff = 0; > + opts->fast_forward = FF_NO; > opts->signoff = 1; > } > > if (file_exists(rebase_path_cdate_is_adate())) { > - opts->allow_ff = 0; > + opts->fast_forward = FF_NO; > opts->committer_date_is_author_date = 1; > } > > if (file_exists(rebase_path_ignore_date())) { > - opts->allow_ff = 0; > + opts->fast_forward = FF_NO; > opts->ignore_date = 1; > } > > @@ -3320,7 +3325,7 @@ static int save_opts(struct replay_opts *opts) > if (opts->record_origin) > res |= git_config_set_in_file_gently(opts_file, > "options.record-origin", "true"); > - if (opts->allow_ff) > + if (opts->fast_forward == FF_ALLOW) > res |= git_config_set_in_file_gently(opts_file, > "options.allow-ff", "true"); > if (opts->mainline) { > @@ -3849,9 +3854,9 @@ static int do_merge(struct repository *r, > * If HEAD is not identical to the first parent of the original merge > * commit, we cannot fast-forward. > */ > - can_fast_forward = opts->allow_ff && commit && commit->parents && > - oideq(&commit->parents->item->object.oid, > - &head_commit->object.oid); > + can_fast_forward = opts->fast_forward != FF_NO && commit && > + commit->parents && oideq(&commit->parents->item->object.oid, > + &head_commit->object.oid); > > /* > * If any merge head is different from the original one, we cannot > @@ -3885,6 +3890,11 @@ static int do_merge(struct repository *r, > goto leave_merge; > } > > + if (opts->fast_forward == FF_ONLY && !can_fast_forward) { > + ret = error_ff_impossible(); > + goto leave_merge; > + } > + > if (strategy || to_merge->next) { > /* Octopus merge */ > struct child_process cmd = CHILD_PROCESS_INIT; > @@ -4276,7 +4286,7 @@ static int pick_commits(struct repository *r, > /* Note that 0 for 3rd parameter of setenv means set only if not set */ > setenv(GIT_REFLOG_ACTION, action_name(opts), 0); > prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION)); > - if (opts->allow_ff) > + if (opts->fast_forward != FF_NO) > assert(!(opts->signoff || opts->no_commit || > opts->record_origin || should_edit(opts) || > opts->committer_date_is_author_date || > @@ -5646,7 +5656,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > BUG("invalid todo list after expanding IDs:\n%s", > new_todo.buf.buf); > > - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) { > + if (opts->fast_forward != FF_NO > + && skip_unnecessary_picks(r, &new_todo, &oid)) { > todo_list_release(&new_todo); > return error(_("could not skip unnecessary pick commands")); > } > diff --git a/sequencer.h b/sequencer.h > index d57d8ea23d..e188dec312 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -28,6 +28,12 @@ enum commit_msg_cleanup_mode { > COMMIT_MSG_CLEANUP_ALL > }; > > +enum ff_type { > + FF_NO, > + FF_ALLOW, > + FF_ONLY > +}; > + > struct replay_opts { > enum replay_action action; > > @@ -38,7 +44,6 @@ struct replay_opts { > int record_origin; > int no_commit; > int signoff; > - int allow_ff; > int allow_rerere_auto; > int allow_empty; > int allow_empty_message; > @@ -50,6 +55,8 @@ struct replay_opts { > int committer_date_is_author_date; > int ignore_date; > > + enum ff_type fast_forward; > + > int mainline; > > char *gpg_sign; > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 66bcbbf952..858e406725 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -895,6 +895,11 @@ test_expect_success 'always cherry-pick with --no-ff' ' > test_must_be_empty out > ' > > +test_expect_success 'interactive rebase prevents non-fast-forward with --ff-only' ' > + git checkout primary && > + test_must_fail git rebase -i --ff-only branch1 > +' > + > test_expect_success 'set up commits with funny messages' ' > git checkout -b funny A && > echo >>file1 && > diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh > index ec8062a66a..e656b5bd28 100755 > --- a/t/t3409-rebase-preserve-merges.sh > +++ b/t/t3409-rebase-preserve-merges.sh > @@ -127,4 +127,8 @@ test_expect_success 'rebase -p ignores merge.log config' ' > ) > ' > > +test_expect_success 'rebase -p prevents non-fast-forward with --ff-only' ' > + test_must_fail git rebase -p --ff-only main > +' > + > test_done > diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh > index 43fcb68f27..69a85cb645 100755 > --- a/t/t3420-rebase-autostash.sh > +++ b/t/t3420-rebase-autostash.sh > @@ -246,6 +246,15 @@ test_expect_success "rebase: fast-forward rebase" ' > git checkout feature-branch > ' > > +test_expect_success "rebase: impossible fast-forward rebase" ' > + test_config rebase.autostash true && > + git reset --hard && > + echo dirty >>file1 && > + (git rebase --ff-only unrelated-onto-branch || true) && > + grep dirty file1 && > + git checkout feature-branch > +' > + > test_expect_success "rebase: noop rebase" ' > test_config rebase.autostash true && > git reset --hard && > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > index 52e8ccc933..af260b9faa 100755 > --- a/t/t7601-merge-pull-config.sh > +++ b/t/t7601-merge-pull-config.sh > @@ -183,6 +183,16 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' ' > test_must_fail git pull . c3 > ' > > +test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' ' > + git reset --hard c1 && > + test_must_fail git pull --rebase --ff-only . c3 > +' > + > +test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' ' > + git reset --hard c1 && > + test_must_fail git pull --no-rebase --ff-only . c3 > +' > + > test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' > git reset --hard c1 && > git config pull.twohead ours && >
On Sun, Jul 04 2021, Alex Henrie wrote: > I'm sending this patch as an RFC because I'm sure someone will find at > least one thing that needs to be changed before it's committed. FWIW that's true of most non-RFC patches :) > +int error_ff_impossible(void) > +{ > + error(_("Not possible to fast-forward, aborting.")); > + return -1; > +} Here's one, the idiom is just "return error", i.e it itself returns -1. FWIW I'd consider doing: /* earlier, static scope */ static const char error_ff_impossible = N_("Not possible..."); /* later, function scope */ return error(error_ff_impossible); It's a small difference, but for translators who use the jump-to-source while translating not having the indirection helps, and in this case it's just used 3 times... > [...] > if (parent && parse_commit(parent) < 0) > /* TRANSLATORS: The first %s will be a "todo" command like > @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) > else if (!strcmp(key, "options.record-origin")) > opts->record_origin = git_config_bool_or_int(key, value, &error_flag); > else if (!strcmp(key, "options.allow-ff")) > - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); > + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO; Since we're on nits, we try to wrap at 80 characters. > +test_expect_success "rebase: impossible fast-forward rebase" ' > + test_config rebase.autostash true && > + git reset --hard && > + echo dirty >>file1 && > + (git rebase --ff-only unrelated-onto-branch || true) && Never "||" git commands, better as test_might_fail. We don't want to hide segfaults. I saw Phillip Wood had some comments on the rest, I didn't test this, just skimmed it, but generally LGTM from a glance, not getting down to the nuts of more deeply inspecting the logic.
Phillip Wood <phillip.wood123@gmail.com> writes: > As I understand it the motivation for this change is to have 'git -c > pull.rebase=true pull --ff-only' actually fast forward. Why cant we > just change pull not to rebase in that case? > ... > Is there a use for this outside of 'git pull --ff-only'? I'm far from > convinced we want this new option but if we do end up adding it I > think it should error out in combination with '-i' or '-x' as '-i' > implies the user wants to change the existing commits and '-x' can end > up changing them as well. > > I think this patch addresses a valid problem but it seems to me that > the approach taken pushes complexity into rebase to handle a case when > pull does not need to invoke rebase in the first place. I share the sentiment, but my conclusion would be different. Even though we explain that "pull" is _like_ "fetch" followed by "merge" (or "rebase"), at the conceptual level, "pull --ff-only" should not have to invoke merge or rebase backend. If the branch fast-fowards to the fetched tip, "pull" should be able to just update the branch and check it out, and if it doesn't, "pull" should just be able to fail. Similarly, "pull --ff" (i.e. fast-forwading allowed) should be able to just update the branch and check it out when the current tip of the branch can be fast-forwarded to the fetched tip. But as you said, pull.rebase=interactive will break such a clean separation of concerns. You can leave "pull" oblivious of what "rebase -i" wants to do when seeing a fast-forwardable history by teaching "rebase" (and "merge") backend to take "--ff-only", "--ff", and "--no-ff" options and having "pull" pass them through. We can teach "pull" that certain backends (namely "rebase -i" in this case) want to handle "ff logic" [*] themselves, and other backends (i.e. "rebase" and "merge") do not mind if "pull" handles "ff logic" for them, but that will break the abstraction (e.g. do we really want to teach "pull" that "rebase -i" wants to rewrite all the commits even when the history fast-forwards?) and will become a source of duplicated logic. Another thing to worry about are hooks. post-rebase or post-merge operations would want to be carried out even when the history would fast-forward, and making "pull" to perform the fast-forwarding and know which hooks should be called with what parameter so that we could pretend as if the "merge" or "rebase" backend was indeed ran, breaks the abstraction. So, even though I wish that the world was simpler and we could handle "ff logic" inside "pull", I am not sure if it is a realistic wish. [Footnote] * By "ff logic", I am referring to what should happen in the 3x2=6 matrix when one of ("--ff", "--ff-only", or "--no-ff") is given and the history (does or does not) fast-forward.
Alex Henrie wrote: > The warning about pulling without specifying how to reconcile divergent > branches says that after setting pull.rebase to true, --ff-only can > still be passed on the command line to require a fast-forward. Make that > actually work. I don't see any value in doing `git rebase --ff-only`. What is the difference with `git merge --ff-only`? Presumably this isn't meant to to be called directly by the user, but only by `git pull`, so it's trying to address my comment [1]: > You can also pass --rebase, --no-rebase, or --ff-only on the command > line to override the configured default per invocation. Can I? git -c pull.rebase=true pull --ff-only `--ff-only` doesn't seem to be overriding the configuration. Passing --ff-only to `git rebase` doesn't solve the problem I was pointing out, only half of it. Sure, now this works: git -c pull.rebase=true pull --ff-only But --ff-only is not really overriding anything, now you are passing the problem along. Consider it the other way around: git -c pull.ff=only pull --rebase Is --rebase overriding anything? No, now all these three fail: git -c pull.ff=only pull git -c pull.ff=only pull --rebase git -c pull.ff=only pull --merge This is making the situation even worse. Junio already made the same mistake [2], and I already pointed out why that doesn't work [3]. This is what we want: 1. git pull # fail by default unless it's a fast-forward 2. git pull --merge # force a merge (unless it's a fast-forward) 3. git pull --rebase # force a rebase (unless it's a fast-forward) If you make `git rebase` honor --ff-only, and you make --ff-only the default, then `git pull --rebase` will *always* fail (unless it's a fast-forward). We want this: git -c pull.ff=only pull --rebase To *ignore* pull.ff=only, not honor it. I already explored all the options, which is why I maintain that the only reasonable option is an orthogonal configuration: pull.mode=fast-forward Cheers. [1] https://lore.kernel.org/git/60d7faaf5311a_b8dfe208bd@natae.notmuch/ [2] https://lore.kernel.org/git/xmqqy2irjy4f.fsf@gitster.c.googlers.com/ [3] https://lore.kernel.org/git/CAMP44s08mEyYqbjOeTeS46CngrbQMqP2=cMr1dtRLLk_BLAq3w@mail.gmail.com/
Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > As I understand it the motivation for this change is to have 'git -c > > pull.rebase=true pull --ff-only' actually fast forward. Why cant we > > just change pull not to rebase in that case? > > ... > > Is there a use for this outside of 'git pull --ff-only'? I'm far from > > convinced we want this new option but if we do end up adding it I > > think it should error out in combination with '-i' or '-x' as '-i' > > implies the user wants to change the existing commits and '-x' can end > > up changing them as well. > > > > I think this patch addresses a valid problem but it seems to me that > > the approach taken pushes complexity into rebase to handle a case when > > pull does not need to invoke rebase in the first place. > > I share the sentiment, but my conclusion would be different. > > Even though we explain that "pull" is _like_ "fetch" followed by > "merge" (or "rebase"), at the conceptual level, "pull --ff-only" > should not have to invoke merge or rebase backend. Indeed. I'm about to send a patch series that adds the `git fast-forward` command, so `git pull` doesn't even have to call either of those. This cleanly separates the logic, except --ff-only remains purely for `git merge`, and instead there's a new: git -c pull.mode=fast-forward pull Now it's 100% clear what these three do: git -c pull.mode=fast-forward pull git -c pull.mode=merge pull git -c pull.mode=rebase pull
Hi Junio On 05/07/2021 10:58, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> As I understand it the motivation for this change is to have 'git -c >> pull.rebase=true pull --ff-only' actually fast forward. Why cant we >> just change pull not to rebase in that case? >> ... >> Is there a use for this outside of 'git pull --ff-only'? I'm far from >> convinced we want this new option but if we do end up adding it I >> think it should error out in combination with '-i' or '-x' as '-i' >> implies the user wants to change the existing commits and '-x' can end >> up changing them as well. >> >> I think this patch addresses a valid problem but it seems to me that >> the approach taken pushes complexity into rebase to handle a case when >> pull does not need to invoke rebase in the first place. > > I share the sentiment, but my conclusion would be different. > > Even though we explain that "pull" is _like_ "fetch" followed by > "merge" (or "rebase"), at the conceptual level, "pull --ff-only" > should not have to invoke merge or rebase backend. If the branch > fast-fowards to the fetched tip, "pull" should be able to just > update the branch and check it out, and if it doesn't, "pull" should > just be able to fail. > > Similarly, "pull --ff" (i.e. fast-forwading allowed) should be able > to just update the branch and check it out when the current tip of > the branch can be fast-forwarded to the fetched tip. > > But as you said, pull.rebase=interactive will break such a clean > separation of concerns. You can leave "pull" oblivious of what > "rebase -i" wants to do when seeing a fast-forwardable history by > teaching "rebase" (and "merge") backend to take "--ff-only", "--ff", > and "--no-ff" options and having "pull" pass them through. My main concern with the new rebase option was about a user invoking 'git rebase -i --ff-only' directly. If a user has pull.rebase=interactive and runs 'git pull --ff-only' then I'm not clear what they expect to happen. Assuming we can fast-forward would they expect pull to run 'rebase -i' which would open their editor with the todo list or would they expect that '--ff-only' means "I just want to fast-forward, I don't want to run 'rebase -i'". If it is the latter then we can just invoke 'git merge --ff-only' (so long as we don't mind running the post-merge hook in this case) and not worry about adding more complexity to 'git rebase' The relevant section of the pull man page only talks about merging in relation to --ff-only --ff, --no-ff, --ff-only Specifies how a merge is handled when the merged-in history is already a descendant of the current history. --ff is the default unless merging an annotated (and possibly signed) tag that is not stored in its natural place in the refs/tags/ hierarchy, in which case --no-ff is assumed. With --ff, when possible resolve the merge as a fast-forward (only update the branch pointer to match the merged branch; do not create a merge commit). When not possible (when the merged-in history is not a descendant of the current history), create a merge commit. With --no-ff, create a merge commit in all cases, even when the merge could instead be resolved as a fast-forward. With --ff-only, resolve the merge as a fast-forward when possible. When not possible, refuse to merge and exit with a non-zero status. > We can teach "pull" that certain backends (namely "rebase -i" in > this case) want to handle "ff logic" [*] themselves, and other > backends (i.e. "rebase" and "merge") do not mind if "pull" handles > "ff logic" for them, but that will break the abstraction (e.g. do we > really want to teach "pull" that "rebase -i" wants to rewrite all > the commits even when the history fast-forwards?) and will become a > source of duplicated logic. > > Another thing to worry about are hooks. post-rebase There is no post-rebase hook. There is the post-rewrite hook, I haven't checked if we invoke it with no input of skip it entirely when we fast-forward. > or post-merge > operations would want to be carried out even when the history would > fast-forward, and making "pull" to perform the fast-forwarding and > know which hooks should be called with what parameter so that we > could pretend as if the "merge" or "rebase" backend was indeed ran, > breaks the abstraction. > > So, even though I wish that the world was simpler and we could > handle "ff logic" inside "pull", I am not sure if it is a realistic > wish. I think if we decide that 'pull --ff-only' always implies merging then the world stays fairly simple. On the other hand if we want to somehow combine rebasing with --ff-only it will be more complicated. If we go for the latter then unless someone comes up with a good use for 'rebase --ff-only' in another context I would prefer the new rebase option to be marked with PARSE_OPT_HIDDEN and that we also avoid making incidental changes the existing rebase options. Best Wishes Phillip > > [Footnote] > > * By "ff logic", I am referring to what should happen in the 3x2=6 > matrix when one of ("--ff", "--ff-only", or "--no-ff") is given > and the history (does or does not) fast-forward. >
On 05/07/2021 09:53, Phillip Wood wrote: > Hi Alex > > On 05/07/2021 05:45, Alex Henrie wrote: >> The warning about pulling without specifying how to reconcile divergent >> branches says that after setting pull.rebase to true, --ff-only can >> still be passed on the command line to require a fast-forward. Make that >> actually work. > > As I understand it the motivation for this change is to have 'git -c > pull.rebase=true pull --ff-only' actually fast forward. Why cant we just > change pull not to rebase in that case? Looking at origin/seen:builtin/pull.c we already check if we can fast-forward and unconditionally merge in that case irrespective of any '--rebase' option or pull.rebase config. It should be simple for pull to error out if '--ff-only' is given and we cannot fast-forward. Best Wishes Phillip > I've left some comments about > the rebase changes below > > > [...] >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index 12f093121d..b88f0cbcca 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, >> const char *prefix) >> N_("ignore changes in whitespace")), >> OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts, >> N_("action"), N_("passed to 'git apply'"), 0), >> - OPT_BIT('f', "force-rebase", &options.flags, >> - N_("cherry-pick all commits, even if unchanged"), >> - REBASE_FORCE), >> - OPT_BIT(0, "no-ff", &options.flags, >> - N_("cherry-pick all commits, even if unchanged"), >> - REBASE_FORCE), >> + OPT_SET_INT_F('f', "force-rebase", &options.fast_forward, >> + N_("cherry-pick all commits, even if unchanged"), >> + FF_NO, PARSE_OPT_NONEG), > > Why does this change rebase to start rejecting --no-force-rebase? This > is the sort of behavior change that deserves a mention in the commit > message. > >> + OPT_SET_INT(0, "ff", &options.fast_forward, N_("allow >> fast-forward"), >> + FF_ALLOW), > > The useful option when rebasing is '--no-ff', now that will no longer > appear in the output of 'git rebase -h' > >> + OPT_SET_INT_F(0, "ff-only", &options.fast_forward, >> + N_("abort if fast-forward is not possible"), >> + FF_ONLY, PARSE_OPT_NONEG), > > Is there a use for this outside of 'git pull --ff-only'? I'm far from > convinced we want this new option but if we do end up adding it I think > it should error out in combination with '-i' or '-x' as '-i' implies the > user wants to change the existing commits and '-x' can end up changing > them as well. > > I think this patch addresses a valid problem but it seems to me that the > approach taken pushes complexity into rebase to handle a case when pull > does not need to invoke rebase in the first place. > > Best Wishes > > Phillip > >> OPT_CMDMODE(0, "continue", &action, N_("continue"), >> ACTION_CONTINUE), >> OPT_CMDMODE(0, "skip", &action, >> @@ -1635,7 +1641,7 @@ int cmd_rebase(int argc, const char **argv, >> const char *prefix) >> allow_preemptive_ff = 0; >> } >> if (options.committer_date_is_author_date || options.ignore_date) >> - options.flags |= REBASE_FORCE; >> + options.fast_forward = FF_NO; >> for (i = 0; i < options.git_am_opts.nr; i++) { >> const char *option = options.git_am_opts.v[i], *p; >> @@ -1809,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, >> const char *prefix) >> die("cannot combine '--signoff' with " >> "'--preserve-merges'"); >> strvec_push(&options.git_am_opts, "--signoff"); >> - options.flags |= REBASE_FORCE; >> + options.fast_forward = FF_NO; >> } >> if (options.type == REBASE_PRESERVE_MERGES) { >> @@ -1952,6 +1958,24 @@ int cmd_rebase(int argc, const char **argv, >> const char *prefix) >> if (repo_read_index(the_repository) < 0) >> die(_("could not read index")); >> + /* >> + * Check if we are already based on onto with linear history, >> + * in which case we could fast-forward without replacing the commits >> + * with new commits recreated by replaying their changes. >> + * >> + * Note that can_fast_forward() initializes merge_base, so we >> have to >> + * call it before checking allow_preemptive_ff. >> + */ >> + allow_preemptive_ff = can_fast_forward(options.onto, >> options.upstream, >> + options.restrict_revision, >> + &options.orig_head, &merge_base) >> + && allow_preemptive_ff; >> + >> + if (!allow_preemptive_ff && options.fast_forward == FF_ONLY) { >> + ret = error_ff_impossible(); >> + goto cleanup; >> + } >> + >> if (options.autostash) { >> create_autostash(the_repository, state_dir_path("autostash", >> &options), >> DEFAULT_REFLOG_ACTION); >> @@ -1968,20 +1992,10 @@ int cmd_rebase(int argc, const char **argv, >> const char *prefix) >> * everything leading up to orig_head) on top of onto. >> */ >> - /* >> - * Check if we are already based on onto with linear history, >> - * in which case we could fast-forward without replacing the commits >> - * with new commits recreated by replaying their changes. >> - * >> - * Note that can_fast_forward() initializes merge_base, so we >> have to >> - * call it before checking allow_preemptive_ff. >> - */ >> - if (can_fast_forward(options.onto, options.upstream, >> options.restrict_revision, >> - &options.orig_head, &merge_base) && >> - allow_preemptive_ff) { >> + if (allow_preemptive_ff) { >> int flag; >> - if (!(options.flags & REBASE_FORCE)) { >> + if (options.fast_forward != FF_NO) { >> /* Lazily switch to the target branch if needed... */ >> if (options.switch_to) { >> strbuf_reset(&buf); >> diff --git a/builtin/revert.c b/builtin/revert.c >> index 237f2f18d4..f9b61478a2 100644 >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -123,7 +123,7 @@ static int run_sequencer(int argc, const char >> **argv, struct replay_opts *opts) >> if (opts->action == REPLAY_PICK) { >> struct option cp_extra[] = { >> OPT_BOOL('x', NULL, &opts->record_origin, N_("append >> commit name")), >> - OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow >> fast-forward")), >> + OPT_SET_INT(0, "ff", &opts->fast_forward, N_("allow >> fast-forward"), FF_ALLOW), >> OPT_BOOL(0, "allow-empty", &opts->allow_empty, >> N_("preserve initially empty commits")), >> OPT_BOOL(0, "allow-empty-message", >> &opts->allow_empty_message, N_("allow commits with empty messages")), >> OPT_BOOL(0, "keep-redundant-commits", >> &opts->keep_redundant_commits, N_("keep redundant, empty commits")), >> @@ -166,7 +166,7 @@ static int run_sequencer(int argc, const char >> **argv, struct replay_opts *opts) >> "--strategy", opts->strategy ? 1 : 0, >> "--strategy-option", opts->xopts ? 1 : 0, >> "-x", opts->record_origin, >> - "--ff", opts->allow_ff, >> + "--ff", opts->fast_forward == FF_ALLOW, >> "--rerere-autoupdate", opts->allow_rerere_auto == >> RERERE_AUTOUPDATE, >> "--no-rerere-autoupdate", opts->allow_rerere_auto == >> RERERE_NOAUTOUPDATE, >> NULL); >> @@ -177,7 +177,7 @@ static int run_sequencer(int argc, const char >> **argv, struct replay_opts *opts) >> opts->default_strategy = NULL; >> } >> - if (opts->allow_ff) >> + if (opts->fast_forward == FF_ALLOW) >> verify_opt_compatible(me, "--ff", >> "--signoff", opts->signoff, >> "--no-commit", opts->no_commit, >> diff --git a/sequencer.c b/sequencer.c >> index 0bec01cf38..84447937d2 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2120,18 +2120,23 @@ static int do_pick_commit(struct repository *r, >> return error(_("cannot get commit message for %s"), >> oid_to_hex(&commit->object.oid)); >> - if (opts->allow_ff && !is_fixup(command) && >> - ((parent && oideq(&parent->object.oid, &head)) || >> - (!parent && unborn))) { >> - if (is_rebase_i(opts)) >> - write_author_script(msg.message); >> - res = fast_forward_to(r, &commit->object.oid, &head, unborn, >> - opts); >> - if (res || command != TODO_REWORD) >> + if (opts->fast_forward != FF_NO) { >> + if (!is_fixup(command) && >> + ((parent && oideq(&parent->object.oid, &head)) || >> + (!parent && unborn))) { >> + if (is_rebase_i(opts)) >> + write_author_script(msg.message); >> + res = fast_forward_to(r, &commit->object.oid, &head, unborn, >> + opts); >> + if (res || command != TODO_REWORD) >> + goto leave; >> + reword = 1; >> + msg_file = NULL; >> + goto fast_forward_edit; >> + } else if (opts->fast_forward == FF_ONLY) { >> + res = error_ff_impossible(); >> goto leave; >> - reword = 1; >> - msg_file = NULL; >> - goto fast_forward_edit; >> + } >> } >> if (parent && parse_commit(parent) < 0) >> /* TRANSLATORS: The first %s will be a "todo" command like >> @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, >> const char *value, void *data) >> else if (!strcmp(key, "options.record-origin")) >> opts->record_origin = git_config_bool_or_int(key, value, >> &error_flag); >> else if (!strcmp(key, "options.allow-ff")) >> - opts->allow_ff = git_config_bool_or_int(key, value, >> &error_flag); >> + opts->fast_forward = git_config_bool_or_int(key, value, >> &error_flag) ? FF_ALLOW : FF_NO; >> else if (!strcmp(key, "options.mainline")) >> opts->mainline = git_config_int(key, value); >> else if (!strcmp(key, "options.strategy")) >> @@ -2853,17 +2858,17 @@ static int read_populate_opts(struct >> replay_opts *opts) >> opts->quiet = 1; >> if (file_exists(rebase_path_signoff())) { >> - opts->allow_ff = 0; >> + opts->fast_forward = FF_NO; >> opts->signoff = 1; >> } >> if (file_exists(rebase_path_cdate_is_adate())) { >> - opts->allow_ff = 0; >> + opts->fast_forward = FF_NO; >> opts->committer_date_is_author_date = 1; >> } >> if (file_exists(rebase_path_ignore_date())) { >> - opts->allow_ff = 0; >> + opts->fast_forward = FF_NO; >> opts->ignore_date = 1; >> } >> @@ -3320,7 +3325,7 @@ static int save_opts(struct replay_opts *opts) >> if (opts->record_origin) >> res |= git_config_set_in_file_gently(opts_file, >> "options.record-origin", "true"); >> - if (opts->allow_ff) >> + if (opts->fast_forward == FF_ALLOW) >> res |= git_config_set_in_file_gently(opts_file, >> "options.allow-ff", "true"); >> if (opts->mainline) { >> @@ -3849,9 +3854,9 @@ static int do_merge(struct repository *r, >> * If HEAD is not identical to the first parent of the original >> merge >> * commit, we cannot fast-forward. >> */ >> - can_fast_forward = opts->allow_ff && commit && commit->parents && >> - oideq(&commit->parents->item->object.oid, >> - &head_commit->object.oid); >> + can_fast_forward = opts->fast_forward != FF_NO && commit && >> + commit->parents && oideq(&commit->parents->item->object.oid, >> + &head_commit->object.oid); >> /* >> * If any merge head is different from the original one, we cannot >> @@ -3885,6 +3890,11 @@ static int do_merge(struct repository *r, >> goto leave_merge; >> } >> + if (opts->fast_forward == FF_ONLY && !can_fast_forward) { >> + ret = error_ff_impossible(); >> + goto leave_merge; >> + } >> + >> if (strategy || to_merge->next) { >> /* Octopus merge */ >> struct child_process cmd = CHILD_PROCESS_INIT; >> @@ -4276,7 +4286,7 @@ static int pick_commits(struct repository *r, >> /* Note that 0 for 3rd parameter of setenv means set only if not >> set */ >> setenv(GIT_REFLOG_ACTION, action_name(opts), 0); >> prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION)); >> - if (opts->allow_ff) >> + if (opts->fast_forward != FF_NO) >> assert(!(opts->signoff || opts->no_commit || >> opts->record_origin || should_edit(opts) || >> opts->committer_date_is_author_date || >> @@ -5646,7 +5656,8 @@ int complete_action(struct repository *r, struct >> replay_opts *opts, unsigned fla >> BUG("invalid todo list after expanding IDs:\n%s", >> new_todo.buf.buf); >> - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) { >> + if (opts->fast_forward != FF_NO >> + && skip_unnecessary_picks(r, &new_todo, &oid)) { >> todo_list_release(&new_todo); >> return error(_("could not skip unnecessary pick commands")); >> } >> diff --git a/sequencer.h b/sequencer.h >> index d57d8ea23d..e188dec312 100644 >> --- a/sequencer.h >> +++ b/sequencer.h >> @@ -28,6 +28,12 @@ enum commit_msg_cleanup_mode { >> COMMIT_MSG_CLEANUP_ALL >> }; >> +enum ff_type { >> + FF_NO, >> + FF_ALLOW, >> + FF_ONLY >> +}; >> + >> struct replay_opts { >> enum replay_action action; >> @@ -38,7 +44,6 @@ struct replay_opts { >> int record_origin; >> int no_commit; >> int signoff; >> - int allow_ff; >> int allow_rerere_auto; >> int allow_empty; >> int allow_empty_message; >> @@ -50,6 +55,8 @@ struct replay_opts { >> int committer_date_is_author_date; >> int ignore_date; >> + enum ff_type fast_forward; >> + >> int mainline; >> char *gpg_sign; >> diff --git a/t/t3404-rebase-interactive.sh >> b/t/t3404-rebase-interactive.sh >> index 66bcbbf952..858e406725 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -895,6 +895,11 @@ test_expect_success 'always cherry-pick with >> --no-ff' ' >> test_must_be_empty out >> ' >> +test_expect_success 'interactive rebase prevents non-fast-forward >> with --ff-only' ' >> + git checkout primary && >> + test_must_fail git rebase -i --ff-only branch1 >> +' >> + >> test_expect_success 'set up commits with funny messages' ' >> git checkout -b funny A && >> echo >>file1 && >> diff --git a/t/t3409-rebase-preserve-merges.sh >> b/t/t3409-rebase-preserve-merges.sh >> index ec8062a66a..e656b5bd28 100755 >> --- a/t/t3409-rebase-preserve-merges.sh >> +++ b/t/t3409-rebase-preserve-merges.sh >> @@ -127,4 +127,8 @@ test_expect_success 'rebase -p ignores merge.log >> config' ' >> ) >> ' >> +test_expect_success 'rebase -p prevents non-fast-forward with >> --ff-only' ' >> + test_must_fail git rebase -p --ff-only main >> +' >> + >> test_done >> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh >> index 43fcb68f27..69a85cb645 100755 >> --- a/t/t3420-rebase-autostash.sh >> +++ b/t/t3420-rebase-autostash.sh >> @@ -246,6 +246,15 @@ test_expect_success "rebase: fast-forward rebase" ' >> git checkout feature-branch >> ' >> +test_expect_success "rebase: impossible fast-forward rebase" ' >> + test_config rebase.autostash true && >> + git reset --hard && >> + echo dirty >>file1 && >> + (git rebase --ff-only unrelated-onto-branch || true) && >> + grep dirty file1 && >> + git checkout feature-branch >> +' >> + >> test_expect_success "rebase: noop rebase" ' >> test_config rebase.autostash true && >> git reset --hard && >> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh >> index 52e8ccc933..af260b9faa 100755 >> --- a/t/t7601-merge-pull-config.sh >> +++ b/t/t7601-merge-pull-config.sh >> @@ -183,6 +183,16 @@ test_expect_success 'pull prevents >> non-fast-forward with "only" in pull.ff' ' >> test_must_fail git pull . c3 >> ' >> +test_expect_success 'pull prevents non-fast-forward with --rebase >> --ff-only' ' >> + git reset --hard c1 && >> + test_must_fail git pull --rebase --ff-only . c3 >> +' >> + >> +test_expect_success 'pull prevents non-fast-forward with --no-rebase >> --ff-only' ' >> + git reset --hard c1 && >> + test_must_fail git pull --no-rebase --ff-only . c3 >> +' >> + >> test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' >> git reset --hard c1 && >> git config pull.twohead ours && >>
Phillip Wood <phillip.wood123@gmail.com> writes: > Looking at origin/seen:builtin/pull.c we already check if we can > fast-forward and unconditionally merge in that case irrespective of > any '--rebase' option or pull.rebase config. It should be simple for > pull to error out if '--ff-only' is given and we cannot fast-forward. Excellent. Even though teaching even more special case on the "git pull" side makes me feel somewhat dirty, but I think it would be a small price to pay, and the end result would save an useless fork whose sole purpose is to make the integration step after fetch fail when "pull" can easily tell, as you said, that it ought to fail, so overall it would probably be a net win. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Looking at origin/seen:builtin/pull.c we already check if we can >> fast-forward and unconditionally merge in that case irrespective of >> any '--rebase' option or pull.rebase config. It should be simple for >> pull to error out if '--ff-only' is given and we cannot fast-forward. > > Excellent. > > Even though teaching even more special case on the "git pull" side > makes me feel somewhat dirty, but I think it would be a small price > to pay, and the end result would save an useless fork whose sole > purpose is to make the integration step after fetch fail when "pull" > can easily tell, as you said, that it ought to fail, so overall it > would probably be a net win. A tangent after thinking a bit more. I do not think "pull.rebase=interactive" affects the "ff logic" at all. Rebasing integration rebuilds _your_ commits on the current branch on top of the tip of _their_ history, and if their history is a descendant of your history (i.e. the history fast-forwards), by definition, you do not have anything you need to rebuild on top of theirs, whether with the opportunity to make tweaks via "rebase -i" or without. This observation does not change the conclusion at all, but I should have spoken after thinking X-<. Thanks.
On Mon, Jul 5, 2021 at 2:53 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 05/07/2021 05:45, Alex Henrie wrote: > > index 12f093121d..b88f0cbcca 100644 > > --- a/builtin/rebase.c > > +++ b/builtin/rebase.c > > @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > N_("ignore changes in whitespace")), > > OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts, > > N_("action"), N_("passed to 'git apply'"), 0), > > - OPT_BIT('f', "force-rebase", &options.flags, > > - N_("cherry-pick all commits, even if unchanged"), > > - REBASE_FORCE), > > - OPT_BIT(0, "no-ff", &options.flags, > > - N_("cherry-pick all commits, even if unchanged"), > > - REBASE_FORCE), > > + OPT_SET_INT_F('f', "force-rebase", &options.fast_forward, > > + N_("cherry-pick all commits, even if unchanged"), > > + FF_NO, PARSE_OPT_NONEG), > > Why does this change rebase to start rejecting --no-force-rebase? This > is the sort of behavior change that deserves a mention in the commit > message. That was accidental, sorry. I copied and pasted option code from another place without paying attention to the PARSE_OPT_NONEG. > > + OPT_SET_INT(0, "ff", &options.fast_forward, N_("allow fast-forward"), > > + FF_ALLOW), > > The useful option when rebasing is '--no-ff', now that will no longer > appear in the output of 'git rebase -h' Yeah, that's a good point. On Mon, Jul 5, 2021 at 3:36 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Sun, Jul 04 2021, Alex Henrie wrote: > > > +int error_ff_impossible(void) > > +{ > > + error(_("Not possible to fast-forward, aborting.")); > > + return -1; > > +} > > Here's one, the idiom is just "return error", i.e it itself returns -1. That would indeed be simpler; thanks for pointing that out. > FWIW I'd consider doing: > > /* earlier, static scope */ > static const char error_ff_impossible = N_("Not possible..."); > /* later, function scope */ > return error(error_ff_impossible); > > It's a small difference, but for translators who use the jump-to-source > while translating not having the indirection helps, and in this case > it's just used 3 times... Wouldn't jump-to-source take the user to the English text in advice.c either way? How does putting it in a static variable help? > > [...] > > if (parent && parse_commit(parent) < 0) > > /* TRANSLATORS: The first %s will be a "todo" command like > > @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) > > else if (!strcmp(key, "options.record-origin")) > > opts->record_origin = git_config_bool_or_int(key, value, &error_flag); > > else if (!strcmp(key, "options.allow-ff")) > > - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); > > + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO; > > Since we're on nits, we try to wrap at 80 characters. Thanks, I didn't know what the exact cutoff was. > > +test_expect_success "rebase: impossible fast-forward rebase" ' > > + test_config rebase.autostash true && > > + git reset --hard && > > + echo dirty >>file1 && > > + (git rebase --ff-only unrelated-onto-branch || true) && > > Never "||" git commands, better as test_might_fail. We don't want to > hide segfaults. Also thanks for the advice here. On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano <gitster@pobox.com> wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > Looking at origin/seen:builtin/pull.c we already check if we can > > fast-forward and unconditionally merge in that case irrespective of > > any '--rebase' option or pull.rebase config. It should be simple for > > pull to error out if '--ff-only' is given and we cannot fast-forward. > > Excellent. > > Even though teaching even more special case on the "git pull" side > makes me feel somewhat dirty, but I think it would be a small price > to pay, and the end result would save an useless fork whose sole > purpose is to make the integration step after fetch fail when "pull" > can easily tell, as you said, that it ought to fail, so overall it > would probably be a net win. Okay, so it sounds like I should just scrap this patch and try again, after "pull: cleanup autostash check" is in master, with a patch that only modifies `git pull` and not `git rebase`. (Unless someone more experienced wants to take over, which would be fine by me.) Thanks anyway to Phillip and Ævar: Your feedback will help me write better patches in the future. -Alex
Hi Alex On 05/07/2021 20:48, Alex Henrie wrote: > On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >>> Looking at origin/seen:builtin/pull.c we already check if we can >>> fast-forward and unconditionally merge in that case irrespective of >>> any '--rebase' option or pull.rebase config. It should be simple for >>> pull to error out if '--ff-only' is given and we cannot fast-forward. >> >> Excellent. >> >> Even though teaching even more special case on the "git pull" side >> makes me feel somewhat dirty, but I think it would be a small price >> to pay, and the end result would save an useless fork whose sole >> purpose is to make the integration step after fetch fail when "pull" >> can easily tell, as you said, that it ought to fail, so overall it >> would probably be a net win. > > Okay, so it sounds like I should just scrap this patch and try again, > after "pull: cleanup autostash check" is in master, with a patch that > only modifies `git pull` and not `git rebase`. (Unless someone more > experienced wants to take over, which would be fine by me.) I don't think you need to wait, if necessary you could base your patches on top of that branch. Junio's latest what's cooking email said he was going to merge that branch into next so it is unlikely to change. In any case looking at the 'git diff origin/master origin/seen builtin/pull.c' I suspect the changes required won't conflict with that branch. > Thanks anyway to Phillip and Ævar: Your feedback will help me write > better patches in the future. You're welcome, I look forward to reading your future patches Best wishes Phillip > -Alex >
On Mon, Jul 05 2021, Alex Henrie wrote: > On Mon, Jul 5, 2021 at 3:36 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> On Sun, Jul 04 2021, Alex Henrie wrote: >> >> > +int error_ff_impossible(void) >> > +{ >> > + error(_("Not possible to fast-forward, aborting.")); >> > + return -1; >> > +} >> >> Here's one, the idiom is just "return error", i.e it itself returns -1. > > That would indeed be simpler; thanks for pointing that out. > >> FWIW I'd consider doing: >> >> /* earlier, static scope */ >> static const char error_ff_impossible = N_("Not possible..."); >> /* later, function scope */ >> return error(error_ff_impossible); >> >> It's a small difference, but for translators who use the jump-to-source >> while translating not having the indirection helps, and in this case >> it's just used 3 times... > > Wouldn't jump-to-source take the user to the English text in advice.c > either way? How does putting it in a static variable help? Yes, sorry. I don't know what I was thinking there. What I said would only apply if _() was used inline, nevermind. >> > [...] >> > if (parent && parse_commit(parent) < 0) >> > /* TRANSLATORS: The first %s will be a "todo" command like >> > @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) >> > else if (!strcmp(key, "options.record-origin")) >> > opts->record_origin = git_config_bool_or_int(key, value, &error_flag); >> > else if (!strcmp(key, "options.allow-ff")) >> > - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); >> > + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO; >> >> Since we're on nits, we try to wrap at 80 characters. > > Thanks, I didn't know what the exact cutoff was. It's not strictly adhered to, and depending on the code we'll have it be longer (sometimes much so) if it helps the general readability, e.g. long repetitive declarations or something. For this sort of thing it's generally preferred. > On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >> > Looking at origin/seen:builtin/pull.c we already check if we can >> > fast-forward and unconditionally merge in that case irrespective of >> > any '--rebase' option or pull.rebase config. It should be simple for >> > pull to error out if '--ff-only' is given and we cannot fast-forward. >> >> Excellent. >> >> Even though teaching even more special case on the "git pull" side >> makes me feel somewhat dirty, but I think it would be a small price >> to pay, and the end result would save an useless fork whose sole >> purpose is to make the integration step after fetch fail when "pull" >> can easily tell, as you said, that it ought to fail, so overall it >> would probably be a net win. > > Okay, so it sounds like I should just scrap this patch and try again, > after "pull: cleanup autostash check" is in master, with a patch that > only modifies `git pull` and not `git rebase`. (Unless someone more > experienced wants to take over, which would be fine by me.) I've just skimmed that whole part of the larger "what should we do" discussion here, and don't really have an opinion, but I see Phillip Wood replied to this downthread. You can grab the branches Junio mentions in What's Cooking from https://github.com/gitster/git.git, if you'd like to rebase on top of Felipe's (or from the ML). And if you fetch from it with: fetch = +refs/notes/amlog:refs/notes/amlog And set: [notes] displayRef = refs/notes/amlog You'll get mappings from the mails to the commits Junio applies. (Note that they're not always 1=1 the same, even after ignoring Junio's Signed-off-by header, he'll sometimes fix them up as he queues them, might use a different base for "master" than you did etc.)
Phillip Wood wrote: > On 05/07/2021 10:58, Junio C Hamano wrote: > My main concern with the new rebase option was about a user invoking > 'git rebase -i --ff-only' directly. > > If a user has pull.rebase=interactive and runs 'git pull --ff-only' then > I'm not clear what they expect to happen. Assuming we can fast-forward > would they expect pull to run 'rebase -i' which would open their editor > with the todo list or would they expect that '--ff-only' means "I just > want to fast-forward, I don't want to run 'rebase -i'". If it is the > latter then we can just invoke 'git merge --ff-only' (so long as we > don't mind running the post-merge hook in this case) and not worry about > adding more complexity to 'git rebase' Once again my suggestion is to keep these as orthogonal, then everything becomes clear: git -c pull.mode=ff-only -c pull.rebase=interactive pull This is just a fast-forward merge (git merge --ff-only). git -c pull.mode=ff-only -c pull.rebase=interactive pull --rebase This is an interactive rebase (--ff-only is ignored). git -c pull.mode=ff-only -c pull.rebase=interactive pull --merge This is a real merge (both --ff-only and --rebase=interactive are ignored). git -c pull.mode=ff-only -c pull.rebase=interactive pull --merge --ff-only This is a --ff-only merge. > The relevant section of the pull man page only talks about merging in > relation to --ff-only And my suggestion is that it should stay that way. > > or post-merge > > operations would want to be carried out even when the history would > > fast-forward, and making "pull" to perform the fast-forwarding and > > know which hooks should be called with what parameter so that we > > could pretend as if the "merge" or "rebase" backend was indeed ran, > > breaks the abstraction. > > > > So, even though I wish that the world was simpler and we could > > handle "ff logic" inside "pull", I am not sure if it is a realistic > > wish. > > I think if we decide that 'pull --ff-only' always implies merging then > the world stays fairly simple. Indeed, which is my suggestion. > On the other hand if we want to somehow > combine rebasing with --ff-only it will be more complicated. If we go > for the latter then unless someone comes up with a good use for 'rebase > --ff-only' in another context I would prefer the new rebase option to be > marked with PARSE_OPT_HIDDEN and that we also avoid making incidental > changes the existing rebase options. Once again, we cannot do that, because then this will be broken: git -c pull.ff=only pull --rebase
Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > Looking at origin/seen:builtin/pull.c we already check if we can > > fast-forward and unconditionally merge in that case irrespective of > > any '--rebase' option or pull.rebase config. It should be simple for > > pull to error out if '--ff-only' is given and we cannot fast-forward. > > Excellent. > > Even though teaching even more special case on the "git pull" side > makes me feel somewhat dirty, It makes you feeld dirty for a reason. Why don't you just actually try the proposal? --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1052,6 +1052,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) show_advice_pull_non_ff(); } + if (!can_ff && (!opt_ff || !strcmp(opt_ff, "--ff-only"))) + die("unable to fast-forward"); + if (opt_rebase) { int ret = 0; int ran_ff = 0; Then: ./git pull . topic ./git pull --no-rebase . topic ./git pull --rebase . topic From 1 to 10 how broken would you say that is? ------------------- t4013-diff-various.sh (Wstat: 256 Tests: 208 Failed: 91) Failed tests: 1, 49-63, 67-69, 71-91, 101-116, 120-124 127-130, 138-141, 144-145, 147-150, 152-153 171-175, 189-193, 201-204 Non-zero exit status: 1 t5521-pull-options.sh (Wstat: 256 Tests: 20 Failed: 2) Failed tests: 12, 16 Non-zero exit status: 1 t5524-pull-msg.sh (Wstat: 256 Tests: 3 Failed: 2) Failed tests: 2-3 Non-zero exit status: 1 t5520-pull.sh (Wstat: 256 Tests: 72 Failed: 39) Failed tests: 16, 19, 23-24, 26-29, 33-41, 43-46, 48-53 55-57, 59-64, 69, 71-72 Non-zero exit status: 1 t5533-push-cas.sh (Wstat: 256 Tests: 23 Failed: 2) Failed tests: 21-22 Non-zero exit status: 1 t5553-set-upstream.sh (Wstat: 256 Tests: 19 Failed: 6) Failed tests: 11-14, 16-17 Non-zero exit status: 1 t5604-clone-reference.sh (Wstat: 256 Tests: 33 Failed: 4) Failed tests: 13-16 Non-zero exit status: 1 t5572-pull-submodule.sh (Wstat: 256 Tests: 64 Failed: 3) Failed tests: 62-64 Non-zero exit status: 1 t6409-merge-subtree.sh (Wstat: 256 Tests: 12 Failed: 3) Failed tests: 9, 11-12 Non-zero exit status: 1 t6402-merge-rename.sh (Wstat: 256 Tests: 46 Failed: 8) Failed tests: 2-8, 11 Non-zero exit status: 1 t6417-merge-ours-theirs.sh (Wstat: 256 Tests: 7 Failed: 1) Failed test: 6 Non-zero exit status: 1 t7601-merge-pull-config.sh (Wstat: 256 Tests: 32 Failed: 3) Failed tests: 11, 15-16 Non-zero exit status: 1 t7603-merge-reduce-heads.sh (Wstat: 256 Tests: 13 Failed: 1) Failed test: 3 Non-zero exit status: 1
diff --git a/advice.c b/advice.c index 0b9c89c48a..8571ab7d0f 100644 --- a/advice.c +++ b/advice.c @@ -246,6 +246,12 @@ void list_config_advices(struct string_list *list, const char *prefix) list_config_item(list, prefix, advice_setting[i].key); } +int error_ff_impossible(void) +{ + error(_("Not possible to fast-forward, aborting.")); + return -1; +} + int error_resolve_conflict(const char *me) { if (!strcmp(me, "cherry-pick")) diff --git a/advice.h b/advice.h index bd26c385d0..49017f931e 100644 --- a/advice.h +++ b/advice.h @@ -93,6 +93,7 @@ int advice_enabled(enum advice_type type); void advise_if_enabled(enum advice_type type, const char *advice, ...); int error_resolve_conflict(const char *me); +int error_ff_impossible(void); void NORETURN die_resolve_conflict(const char *me); void NORETURN die_conclude_merge(void); void advise_on_updating_sparse_paths(struct string_list *pathspec_list); diff --git a/builtin/merge.c b/builtin/merge.c index a8a843b1f5..eb25c30e70 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -98,12 +98,6 @@ static struct strategy all_strategy[] = { static const char *pull_twohead, *pull_octopus; -enum ff_type { - FF_NO, - FF_ALLOW, - FF_ONLY -}; - static enum ff_type fast_forward = FF_ALLOW; static const char *cleanup_arg; diff --git a/builtin/pull.c b/builtin/pull.c index e8927fc2ff..c285e9cc89 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -889,6 +889,8 @@ static int run_rebase(const struct object_id *newbase, strvec_push(&args, "--interactive"); if (opt_diffstat) strvec_push(&args, opt_diffstat); + if (opt_ff) + strvec_push(&args, opt_ff); strvec_pushv(&args, opt_strategies.v); strvec_pushv(&args, opt_strategy_opts.v); if (opt_gpg_sign) diff --git a/builtin/rebase.c b/builtin/rebase.c index 12f093121d..b88f0cbcca 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -81,9 +81,9 @@ struct rebase_options { REBASE_NO_QUIET = 1<<0, REBASE_VERBOSE = 1<<1, REBASE_DIFFSTAT = 1<<2, - REBASE_FORCE = 1<<3, - REBASE_INTERACTIVE_EXPLICIT = 1<<4, + REBASE_INTERACTIVE_EXPLICIT = 1<<3, } flags; + enum ff_type fast_forward; struct strvec git_am_opts; const char *action; int signoff; @@ -110,6 +110,7 @@ struct rebase_options { .keep_empty = 1, \ .default_backend = "merge", \ .flags = REBASE_NO_QUIET, \ + .fast_forward = FF_ALLOW, \ .git_am_opts = STRVEC_INIT, \ .git_format_patch_opt = STRBUF_INIT, \ .fork_point = -1, \ @@ -124,7 +125,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts) sequencer_init_config(&replay); replay.signoff = opts->signoff; - replay.allow_ff = !(opts->flags & REBASE_FORCE); + replay.fast_forward = opts->fast_forward; if (opts->allow_rerere_autoupdate) replay.allow_rerere_auto = opts->allow_rerere_autoupdate; replay.allow_empty = 1; @@ -488,8 +489,11 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) struct object_id squash_onto = *null_oid(); enum action command = ACTION_NONE; struct option options[] = { - OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"), - REBASE_FORCE), + OPT_SET_INT(0, "ff", &opts.fast_forward, N_("allow fast-forward"), + FF_ALLOW), + OPT_SET_INT_F(0, "ff-only", &opts.fast_forward, + N_("abort if fast-forward is not possible"), + FF_ONLY, PARSE_OPT_NONEG), OPT_CALLBACK_F('k', "keep-empty", &options, NULL, N_("keep commits which start empty"), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, @@ -651,7 +655,7 @@ static int read_basic_state(struct rebase_options *opts) if (file_exists(state_dir_path("signoff", opts))) { opts->signoff = 1; - opts->flags |= REBASE_FORCE; + opts->fast_forward = FF_NO; } if (file_exists(state_dir_path("allow_rerere_autoupdate", opts))) { @@ -991,7 +995,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action) add_var(&script_snippet, "diffstat", opts->flags & REBASE_DIFFSTAT ? "t" : ""); add_var(&script_snippet, "force_rebase", - opts->flags & REBASE_FORCE ? "t" : ""); + opts->fast_forward == FF_NO ? "" : "t"); if (opts->switch_to) add_var(&script_snippet, "switch_to", opts->switch_to); add_var(&script_snippet, "action", opts->action ? opts->action : ""); @@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) N_("ignore changes in whitespace")), OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts, N_("action"), N_("passed to 'git apply'"), 0), - OPT_BIT('f', "force-rebase", &options.flags, - N_("cherry-pick all commits, even if unchanged"), - REBASE_FORCE), - OPT_BIT(0, "no-ff", &options.flags, - N_("cherry-pick all commits, even if unchanged"), - REBASE_FORCE), + OPT_SET_INT_F('f', "force-rebase", &options.fast_forward, + N_("cherry-pick all commits, even if unchanged"), + FF_NO, PARSE_OPT_NONEG), + OPT_SET_INT(0, "ff", &options.fast_forward, N_("allow fast-forward"), + FF_ALLOW), + OPT_SET_INT_F(0, "ff-only", &options.fast_forward, + N_("abort if fast-forward is not possible"), + FF_ONLY, PARSE_OPT_NONEG), OPT_CMDMODE(0, "continue", &action, N_("continue"), ACTION_CONTINUE), OPT_CMDMODE(0, "skip", &action, @@ -1635,7 +1641,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) allow_preemptive_ff = 0; } if (options.committer_date_is_author_date || options.ignore_date) - options.flags |= REBASE_FORCE; + options.fast_forward = FF_NO; for (i = 0; i < options.git_am_opts.nr; i++) { const char *option = options.git_am_opts.v[i], *p; @@ -1809,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) die("cannot combine '--signoff' with " "'--preserve-merges'"); strvec_push(&options.git_am_opts, "--signoff"); - options.flags |= REBASE_FORCE; + options.fast_forward = FF_NO; } if (options.type == REBASE_PRESERVE_MERGES) { @@ -1952,6 +1958,24 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (repo_read_index(the_repository) < 0) die(_("could not read index")); + /* + * Check if we are already based on onto with linear history, + * in which case we could fast-forward without replacing the commits + * with new commits recreated by replaying their changes. + * + * Note that can_fast_forward() initializes merge_base, so we have to + * call it before checking allow_preemptive_ff. + */ + allow_preemptive_ff = can_fast_forward(options.onto, options.upstream, + options.restrict_revision, + &options.orig_head, &merge_base) + && allow_preemptive_ff; + + if (!allow_preemptive_ff && options.fast_forward == FF_ONLY) { + ret = error_ff_impossible(); + goto cleanup; + } + if (options.autostash) { create_autostash(the_repository, state_dir_path("autostash", &options), DEFAULT_REFLOG_ACTION); @@ -1968,20 +1992,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) * everything leading up to orig_head) on top of onto. */ - /* - * Check if we are already based on onto with linear history, - * in which case we could fast-forward without replacing the commits - * with new commits recreated by replaying their changes. - * - * Note that can_fast_forward() initializes merge_base, so we have to - * call it before checking allow_preemptive_ff. - */ - if (can_fast_forward(options.onto, options.upstream, options.restrict_revision, - &options.orig_head, &merge_base) && - allow_preemptive_ff) { + if (allow_preemptive_ff) { int flag; - if (!(options.flags & REBASE_FORCE)) { + if (options.fast_forward != FF_NO) { /* Lazily switch to the target branch if needed... */ if (options.switch_to) { strbuf_reset(&buf); diff --git a/builtin/revert.c b/builtin/revert.c index 237f2f18d4..f9b61478a2 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -123,7 +123,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) if (opts->action == REPLAY_PICK) { struct option cp_extra[] = { OPT_BOOL('x', NULL, &opts->record_origin, N_("append commit name")), - OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")), + OPT_SET_INT(0, "ff", &opts->fast_forward, N_("allow fast-forward"), FF_ALLOW), OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")), OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")), OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")), @@ -166,7 +166,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) "--strategy", opts->strategy ? 1 : 0, "--strategy-option", opts->xopts ? 1 : 0, "-x", opts->record_origin, - "--ff", opts->allow_ff, + "--ff", opts->fast_forward == FF_ALLOW, "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE, "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE, NULL); @@ -177,7 +177,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) opts->default_strategy = NULL; } - if (opts->allow_ff) + if (opts->fast_forward == FF_ALLOW) verify_opt_compatible(me, "--ff", "--signoff", opts->signoff, "--no-commit", opts->no_commit, diff --git a/sequencer.c b/sequencer.c index 0bec01cf38..84447937d2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2120,18 +2120,23 @@ static int do_pick_commit(struct repository *r, return error(_("cannot get commit message for %s"), oid_to_hex(&commit->object.oid)); - if (opts->allow_ff && !is_fixup(command) && - ((parent && oideq(&parent->object.oid, &head)) || - (!parent && unborn))) { - if (is_rebase_i(opts)) - write_author_script(msg.message); - res = fast_forward_to(r, &commit->object.oid, &head, unborn, - opts); - if (res || command != TODO_REWORD) + if (opts->fast_forward != FF_NO) { + if (!is_fixup(command) && + ((parent && oideq(&parent->object.oid, &head)) || + (!parent && unborn))) { + if (is_rebase_i(opts)) + write_author_script(msg.message); + res = fast_forward_to(r, &commit->object.oid, &head, unborn, + opts); + if (res || command != TODO_REWORD) + goto leave; + reword = 1; + msg_file = NULL; + goto fast_forward_edit; + } else if (opts->fast_forward == FF_ONLY) { + res = error_ff_impossible(); goto leave; - reword = 1; - msg_file = NULL; - goto fast_forward_edit; + } } if (parent && parse_commit(parent) < 0) /* TRANSLATORS: The first %s will be a "todo" command like @@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data) else if (!strcmp(key, "options.record-origin")) opts->record_origin = git_config_bool_or_int(key, value, &error_flag); else if (!strcmp(key, "options.allow-ff")) - opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); + opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO; else if (!strcmp(key, "options.mainline")) opts->mainline = git_config_int(key, value); else if (!strcmp(key, "options.strategy")) @@ -2853,17 +2858,17 @@ static int read_populate_opts(struct replay_opts *opts) opts->quiet = 1; if (file_exists(rebase_path_signoff())) { - opts->allow_ff = 0; + opts->fast_forward = FF_NO; opts->signoff = 1; } if (file_exists(rebase_path_cdate_is_adate())) { - opts->allow_ff = 0; + opts->fast_forward = FF_NO; opts->committer_date_is_author_date = 1; } if (file_exists(rebase_path_ignore_date())) { - opts->allow_ff = 0; + opts->fast_forward = FF_NO; opts->ignore_date = 1; } @@ -3320,7 +3325,7 @@ static int save_opts(struct replay_opts *opts) if (opts->record_origin) res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true"); - if (opts->allow_ff) + if (opts->fast_forward == FF_ALLOW) res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true"); if (opts->mainline) { @@ -3849,9 +3854,9 @@ static int do_merge(struct repository *r, * If HEAD is not identical to the first parent of the original merge * commit, we cannot fast-forward. */ - can_fast_forward = opts->allow_ff && commit && commit->parents && - oideq(&commit->parents->item->object.oid, - &head_commit->object.oid); + can_fast_forward = opts->fast_forward != FF_NO && commit && + commit->parents && oideq(&commit->parents->item->object.oid, + &head_commit->object.oid); /* * If any merge head is different from the original one, we cannot @@ -3885,6 +3890,11 @@ static int do_merge(struct repository *r, goto leave_merge; } + if (opts->fast_forward == FF_ONLY && !can_fast_forward) { + ret = error_ff_impossible(); + goto leave_merge; + } + if (strategy || to_merge->next) { /* Octopus merge */ struct child_process cmd = CHILD_PROCESS_INIT; @@ -4276,7 +4286,7 @@ static int pick_commits(struct repository *r, /* Note that 0 for 3rd parameter of setenv means set only if not set */ setenv(GIT_REFLOG_ACTION, action_name(opts), 0); prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION)); - if (opts->allow_ff) + if (opts->fast_forward != FF_NO) assert(!(opts->signoff || opts->no_commit || opts->record_origin || should_edit(opts) || opts->committer_date_is_author_date || @@ -5646,7 +5656,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla BUG("invalid todo list after expanding IDs:\n%s", new_todo.buf.buf); - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) { + if (opts->fast_forward != FF_NO + && skip_unnecessary_picks(r, &new_todo, &oid)) { todo_list_release(&new_todo); return error(_("could not skip unnecessary pick commands")); } diff --git a/sequencer.h b/sequencer.h index d57d8ea23d..e188dec312 100644 --- a/sequencer.h +++ b/sequencer.h @@ -28,6 +28,12 @@ enum commit_msg_cleanup_mode { COMMIT_MSG_CLEANUP_ALL }; +enum ff_type { + FF_NO, + FF_ALLOW, + FF_ONLY +}; + struct replay_opts { enum replay_action action; @@ -38,7 +44,6 @@ struct replay_opts { int record_origin; int no_commit; int signoff; - int allow_ff; int allow_rerere_auto; int allow_empty; int allow_empty_message; @@ -50,6 +55,8 @@ struct replay_opts { int committer_date_is_author_date; int ignore_date; + enum ff_type fast_forward; + int mainline; char *gpg_sign; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 66bcbbf952..858e406725 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -895,6 +895,11 @@ test_expect_success 'always cherry-pick with --no-ff' ' test_must_be_empty out ' +test_expect_success 'interactive rebase prevents non-fast-forward with --ff-only' ' + git checkout primary && + test_must_fail git rebase -i --ff-only branch1 +' + test_expect_success 'set up commits with funny messages' ' git checkout -b funny A && echo >>file1 && diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh index ec8062a66a..e656b5bd28 100755 --- a/t/t3409-rebase-preserve-merges.sh +++ b/t/t3409-rebase-preserve-merges.sh @@ -127,4 +127,8 @@ test_expect_success 'rebase -p ignores merge.log config' ' ) ' +test_expect_success 'rebase -p prevents non-fast-forward with --ff-only' ' + test_must_fail git rebase -p --ff-only main +' + test_done diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index 43fcb68f27..69a85cb645 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -246,6 +246,15 @@ test_expect_success "rebase: fast-forward rebase" ' git checkout feature-branch ' +test_expect_success "rebase: impossible fast-forward rebase" ' + test_config rebase.autostash true && + git reset --hard && + echo dirty >>file1 && + (git rebase --ff-only unrelated-onto-branch || true) && + grep dirty file1 && + git checkout feature-branch +' + test_expect_success "rebase: noop rebase" ' test_config rebase.autostash true && git reset --hard && diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 52e8ccc933..af260b9faa 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -183,6 +183,16 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' ' test_must_fail git pull . c3 ' +test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' ' + git reset --hard c1 && + test_must_fail git pull --rebase --ff-only . c3 +' + +test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' ' + git reset --hard c1 && + test_must_fail git pull --no-rebase --ff-only . c3 +' + test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' git reset --hard c1 && git config pull.twohead ours &&
The warning about pulling without specifying how to reconcile divergent branches says that after setting pull.rebase to true, --ff-only can still be passed on the command line to require a fast-forward. Make that actually work. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- I'm sending this patch as an RFC because I'm sure someone will find at least one thing that needs to be changed before it's committed. --- advice.c | 6 +++ advice.h | 1 + builtin/merge.c | 6 --- builtin/pull.c | 2 + builtin/rebase.c | 68 +++++++++++++++++++------------ builtin/revert.c | 6 +-- sequencer.c | 53 ++++++++++++++---------- sequencer.h | 9 +++- t/t3404-rebase-interactive.sh | 5 +++ t/t3409-rebase-preserve-merges.sh | 4 ++ t/t3420-rebase-autostash.sh | 9 ++++ t/t7601-merge-pull-config.sh | 10 +++++ 12 files changed, 121 insertions(+), 58 deletions(-)