From: Phillip Wood <phillip.wood123@gmail.com>
To: Alex Henrie <alexhenrie24@gmail.com>,
git@vger.kernel.org, gitster@pobox.com, newren@gmail.com
Subject: Re: [PATCH RFC] rebase: respect --ff-only option
Date: Mon, 5 Jul 2021 16:29:20 +0100 [thread overview]
Message-ID: <7ee36923-0806-4316-729c-8418df5b6555@gmail.com> (raw)
In-Reply-To: <349748b4-3c48-7ca7-eb0f-e859a15cab0f@gmail.com>
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 &&
>>
next prev parent reply other threads:[~2021-07-05 15:34 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-05 4:45 [PATCH RFC] rebase: respect --ff-only option Alex Henrie
2021-07-05 8:53 ` Phillip Wood
2021-07-05 9:58 ` Junio C Hamano
2021-07-05 12:09 ` Felipe Contreras
2021-07-05 13:54 ` Phillip Wood
2021-07-07 0:30 ` Felipe Contreras
2021-07-05 15:29 ` Phillip Wood [this message]
2021-07-05 16:50 ` Junio C Hamano
2021-07-05 19:23 ` Junio C Hamano
2021-07-05 19:48 ` Alex Henrie
2021-07-06 13:52 ` Phillip Wood
2021-07-06 14:43 ` Ævar Arnfjörð Bjarmason
2021-07-07 1:13 ` Felipe Contreras
2021-07-05 9:27 ` Ævar Arnfjörð Bjarmason
2021-07-05 12:00 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7ee36923-0806-4316-729c-8418df5b6555@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=alexhenrie24@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).