* [PATCH RFC] rebase: respect --ff-only option
@ 2021-07-05 4:45 Alex Henrie
2021-07-05 8:53 ` Phillip Wood
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Alex Henrie @ 2021-07-05 4:45 UTC (permalink / raw)
To: git, gitster, newren; +Cc: Alex Henrie
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(-)
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 &&
--
2.32.0.94.ge083fb24f6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
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 15:29 ` Phillip Wood
2021-07-05 9:27 ` Ævar Arnfjörð Bjarmason
2021-07-05 12:00 ` Felipe Contreras
2 siblings, 2 replies; 15+ messages in thread
From: Phillip Wood @ 2021-07-05 8:53 UTC (permalink / raw)
To: Alex Henrie, git, gitster, newren
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 &&
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
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:27 ` Ævar Arnfjörð Bjarmason
2021-07-05 12:00 ` Felipe Contreras
2 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-05 9:27 UTC (permalink / raw)
To: Alex Henrie; +Cc: git, gitster, newren
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
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-05 15:29 ` Phillip Wood
1 sibling, 2 replies; 15+ messages in thread
From: Junio C Hamano @ 2021-07-05 9:58 UTC (permalink / raw)
To: Phillip Wood; +Cc: Alex Henrie, git, newren
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH RFC] rebase: respect --ff-only option
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:27 ` Ævar Arnfjörð Bjarmason
@ 2021-07-05 12:00 ` Felipe Contreras
2 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-07-05 12:00 UTC (permalink / raw)
To: Alex Henrie, git, gitster, newren; +Cc: Alex Henrie
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/
--
Felipe Contreras
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 9:58 ` Junio C Hamano
@ 2021-07-05 12:09 ` Felipe Contreras
2021-07-05 13:54 ` Phillip Wood
1 sibling, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-07-05 12:09 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Alex Henrie, git, newren
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
--
Felipe Contreras
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
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
1 sibling, 1 reply; 15+ messages in thread
From: Phillip Wood @ 2021-07-05 13:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alex Henrie, git, newren
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.
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 8:53 ` Phillip Wood
2021-07-05 9:58 ` Junio C Hamano
@ 2021-07-05 15:29 ` Phillip Wood
2021-07-05 16:50 ` Junio C Hamano
1 sibling, 1 reply; 15+ messages in thread
From: Phillip Wood @ 2021-07-05 15:29 UTC (permalink / raw)
To: Alex Henrie, git, gitster, newren
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 &&
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 15:29 ` Phillip Wood
@ 2021-07-05 16:50 ` Junio C Hamano
2021-07-05 19:23 ` Junio C Hamano
2021-07-07 1:13 ` Felipe Contreras
0 siblings, 2 replies; 15+ messages in thread
From: Junio C Hamano @ 2021-07-05 16:50 UTC (permalink / raw)
To: Phillip Wood; +Cc: Alex Henrie, git, newren
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
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-07 1:13 ` Felipe Contreras
1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2021-07-05 19:23 UTC (permalink / raw)
To: Phillip Wood; +Cc: Alex Henrie, git, newren
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
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
0 siblings, 2 replies; 15+ messages in thread
From: Alex Henrie @ 2021-07-05 19:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood, Git mailing list, Elijah Newren,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 19:48 ` Alex Henrie
@ 2021-07-06 13:52 ` Phillip Wood
2021-07-06 14:43 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 15+ messages in thread
From: Phillip Wood @ 2021-07-06 13:52 UTC (permalink / raw)
To: Alex Henrie, Junio C Hamano
Cc: Git mailing list, Elijah Newren, Ævar Arnfjörð Bjarmason
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 19:48 ` Alex Henrie
2021-07-06 13:52 ` Phillip Wood
@ 2021-07-06 14:43 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-06 14:43 UTC (permalink / raw)
To: Alex Henrie; +Cc: Junio C Hamano, Phillip Wood, Git mailing list, Elijah Newren
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.)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 13:54 ` Phillip Wood
@ 2021-07-07 0:30 ` Felipe Contreras
0 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-07-07 0:30 UTC (permalink / raw)
To: Phillip Wood, Junio C Hamano; +Cc: Alex Henrie, git, newren
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
--
Felipe Contreras
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH RFC] rebase: respect --ff-only option
2021-07-05 16:50 ` Junio C Hamano
2021-07-05 19:23 ` Junio C Hamano
@ 2021-07-07 1:13 ` Felipe Contreras
1 sibling, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-07-07 1:13 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Alex Henrie, git, newren
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
--
Felipe Contreras
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-07 1:14 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.