[PATCH RFC] rebase: respect --ff-only option

[PATCH RFC] rebase: respect --ff-only option

[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

Re: [PATCH RFC] rebase: respect --ff-only option

[Phillip Wood]

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 &&
> 

Re: [PATCH RFC] rebase: respect --ff-only option

[Junio C Hamano]

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.

Re: [PATCH RFC] rebase: respect --ff-only option

[Felipe Contreras]

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

Re: [PATCH RFC] rebase: respect --ff-only option

[Phillip Wood]

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.
> 

Re: [PATCH RFC] rebase: respect --ff-only option

[Felipe Contreras]

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

Re: [PATCH RFC] rebase: respect --ff-only option

[Phillip Wood]

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 &&
>>

Re: [PATCH RFC] rebase: respect --ff-only option

[Junio C Hamano]

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.

Re: [PATCH RFC] rebase: respect --ff-only option

[Junio C Hamano]

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.

Re: [PATCH RFC] rebase: respect --ff-only option

[Alex Henrie]

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

Re: [PATCH RFC] rebase: respect --ff-only option

[Phillip Wood]

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
> 

Re: [PATCH RFC] rebase: respect --ff-only option

[Ævar Arnfjörð Bjarmason]

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.)

Re: [PATCH RFC] rebase: respect --ff-only option

[Felipe Contreras]

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

Re: [PATCH RFC] rebase: respect --ff-only option

[Ævar Arnfjörð Bjarmason]

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.

RE: [PATCH RFC] rebase: respect --ff-only option

[Felipe Contreras]

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