rebase: make the default backend configurable

From: Elijah Newren <[email protected]>

As established in the previous commit and commit b00bf1c9a8dd (git-rebase: make --allow-empty-message the default, 2018-06-27), the behavior for rebase with different backends in various edge or corner cases is often more happenstance than design. This commit addresses another such corner case: commits which "become empty".

A careful reader may note that there are two types of commits which would become empty due to a rebase:

* [clean cherry-pick] Commits which are clean cherry-picks of upstream commits, as determined by git log --cherry-mark .... Re-applying these commits would result in an empty set of changes and a duplicative commit message; i.e. these are commits that have "already been applied" upstream.

* [become empty] Commits which are not empty to start, are not clean cherry-picks of upstream commits, but which still become empty after being rebased. This happens e.g. when a commit has changes which are a strict subset of the changes in an upstream commit, or when the changes of a commit can be found spread across or among several upstream commits.

Clearly, in both cases the changes in the commit in question are found upstream already, but the commit message may not be in the latter case.

When cherry-mark can determine a commit is already upstream, then because of how cherry-mark works this means the upstream commit message was about the *exact* same set of changes. Thus, the commit messages can be assumed to be fully interchangeable (and are in fact likely to be completely identical). As such, the clean cherry-pick case represents a case when there is no information to be gained by keeping the extra commit around. All rebase types have always dropped these commits, and no one to my knowledge has ever requested that we do otherwise.

For many of the become empty cases (and likely even most), we will also be able to drop the commit without loss of information -- but this isn't quite always the case. Since these commits represent cases that were not clean cherry-picks, there is no upstream commit message explaining the same set of changes. Projects with good commit message hygiene will likely have the explanation from our commit message contained within or spread among the relevant upstream commits, but not all projects run that way. As such, the commit message of the commit being rebased may have reasoning that suggests additional changes that should be made to adapt to the new base, or it may have information that someone wants to add as a note to another commit, or perhaps someone even wants to create an empty commit with the commit message as-is.

Junio commented on the "become-empty" types of commits as follows[1]:

WRT a change that ends up being empty (as opposed to a change that is empty from the beginning), I'd think that the current behaviour is desireable one. "am" based rebase is solely to transplant an existing history and want to stop much less than "interactive" one whose purpose is to polish a series before making it publishable, and asking for confirmation ("this has become empty--do you want to drop it?") is more appropriate from the workflow point of view.

[1] https://lore.kernel.org/git/[email protected]/

I would simply add that his arguments for "am"-based rebases actually apply to all non-explicitly-interactive rebases. Also, since we are stating that different cases should have different defaults, it may be worth providing a flag to allow users to select which behavior they want for these commits.

Introduce a new command line flag for selecting the desired behavior:

     --empty={drop,keep,ask}

with the definitions:
drop: drop commits which become empty
keep: keep commits which become empty
ask: provide the user a chance to interact and pick what to do with
commits which become empty on a case-by-case basis

In line with Junio's suggestion, if the --empty flag is not specified, pick defaults as follows:
explicitly interactive: ask
otherwise: drop

Signed-off-by Elijah Newren <[email protected]>

---
  Documentation/git-rebase.txt | 27 ++++++++++++++++---
  builtin/rebase.c             | 52 ++++++++++++++++++++++++++++++++++++
  sequencer.c                  | 48 +++++++++++++++++++++++++--------
  sequencer.h                  |  1 +
  t/t3424-rebase-empty.sh      | 50 +++++++++++++++++++++++++++++-----
  t/t3427-rebase-subtree.sh    |  8 +++---
  6 files changed, 161 insertions(+), 25 deletions(-)

Documentation/git-rebase.txt +0 -0

index 1d19542d79..551a91d764 100644
@@ -258,6 +258,22 @@ See also INCOMPATIBLE OPTIONS below.
  	original branch. The index and working tree are also left
  	unchanged as a result.

Documentation/git-rebase.txt +14 -3

+Note that commits which start empty are kept, and commits which are
+clean cherry-picks (as determined by `git log --cherry-mark ...`) are
+always dropped.
++
+See also INCOMPATIBLE OPTIONS below.
+
  --keep-empty::
  	No-op.  Rebasing commits that started empty (had no change
  	relative to their parent) used to fail and this option would

@@ -561,6 +577,7 @@ are incompatible with the following options:
   * --interactive
   * --exec
   * --keep-empty
+ * --empty=
   * --edit-todo
   * --root when used in combination with --onto
 

@@ -569,6 +586,7 @@ In addition, the following pairs of options are incompatible:
   * --preserve-merges and --interactive
   * --preserve-merges and --signoff
   * --preserve-merges and --rebase-merges
+ * --preserve-merges and --empty=
   * --keep-base and --onto
   * --keep-base and --root
 

@@ -585,9 +603,12 @@ commits that started empty, though these are rare in practice.  It
  also drops commits that become empty and has no option for controlling
  this behavior.
 
-The interactive backend keeps intentionally empty commits.
-Unfortunately, it always halts whenever it runs across a commit that
-becomes empty, even when the rebase is not explicitly interactive.
+The interactive backend keeps intentionally empty commits.  Similar to
+the am backend, by default the interactive backend drops commits that
+become empty unless -i/--interactive is specified (in which case it
+stops and asks the user what to do).  The interactive backend also has
+an --empty={drop,keep,ask} option for changing the behavior of
+handling commits that become empty.
 
  Directory rename detection
  ~~~~~~~~~~~~~~~~~~~~~~~~~~

builtin/rebase.c +38 -0

index 537b3241ce..c299869e7b 100644
@@ -50,8 +50,16 @@ enum rebase_type {
  	REBASE_PRESERVE_MERGES
  };
 
+enum empty_type {
+	EMPTY_UNSPECIFIED = -1,
+	EMPTY_DROP,
+	EMPTY_KEEP,
+	EMPTY_ASK
+};
+
  struct rebase_options {
  	enum rebase_type type;
+	enum empty_type empty;
  	const char *state_dir;
  	struct commit *upstream;
  	const char *upstream_name;

@@ -91,6 +99,7 @@ struct rebase_options {
 
  #define REBASE_OPTIONS_INIT {			  	\
  		.type = REBASE_UNSPECIFIED,	  	\
+		.empty = EMPTY_UNSPECIFIED,	  	\
  		.flags = REBASE_NO_QUIET, 		\
  		.git_am_opts = ARGV_ARRAY_INIT,		\
  		.git_format_patch_opt = STRBUF_INIT	\

@@ -109,6 +118,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
  		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
  	replay.allow_empty = 1;
  	replay.allow_empty_message = opts->allow_empty_message;
+	replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
+	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
  	replay.verbose = opts->flags & REBASE_VERBOSE;
  	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
  	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);

@@ -444,6 +455,10 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 
  	BUG_ON_OPT_ARG(arg);
 
+	/*
+	 * If we ever want to remap --keep-empty to --empty=keep, insert:
+	 * 	opts->empty = unset ? EMPTY_UNSPECIFIED : EMPTY_KEEP;
+	 */
  	opts->type = REBASE_INTERACTIVE;
  	return 0;
  }

@@ -1350,6 +1365,29 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
  	return 0;
  }
 
+static enum empty_type parse_empty_value(const char *value)
+{
+	if (!strcasecmp(value, "drop"))
+		return EMPTY_DROP;
+	else if (!strcasecmp(value, "keep"))
+		return EMPTY_KEEP;
+	else if (!strcasecmp(value, "ask"))
+		return EMPTY_ASK;
+
+	die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
+}
+
+static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
+{
+	struct rebase_options *options = opt->value;
+	enum empty_type value = parse_empty_value(arg);
+
+	BUG_ON_OPT_NEG(unset);
+
+	options->empty = value;
+	return 0;
+}
+
  static void NORETURN error_on_missing_default_upstream(void)
  {
  	struct branch *current_branch = branch_get(NULL);

builtin/rebase.c +12 -0

+			       PARSE_OPT_NONEG, parse_opt_empty),
  		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
  			N_("(DEPRECATED) keep empty commits"),
  			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,

@@ -1760,6 +1801,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
  	if (!(options.flags & REBASE_NO_QUIET))
  		argv_array_push(&options.git_am_opts, "-q");
 
+	if (options.empty != EMPTY_UNSPECIFIED)
+		imply_interactive(&options, "--empty");
+
  	if (gpg_sign) {
  		free(options.gpg_sign_opt);
  		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);

@@ -1843,6 +1887,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
  		break;
  	}
 
+	if (options.empty == EMPTY_UNSPECIFIED) {
+		if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
+			options.empty = EMPTY_ASK;
+		else if (exec.nr > 0)
+			options.empty = EMPTY_KEEP;
+		else
+			options.empty = EMPTY_DROP;
+	}
  	if (reschedule_failed_exec > 0 && !is_interactive(&options))
  		die(_("--reschedule-failed-exec requires "
  		      "--exec or --interactive"));

sequencer.c +21 -9

index c21fc202b1..354d0b5a38 100644
@@ -158,6 +158,8 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
  static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
+static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
  static int git_sequencer_config(const char *k, const char *v, void *cb)
  {

@@ -1483,7 +1485,11 @@ static int is_original_commit_empty(struct commit *commit)
  }
 
  /*
- * Do we run "git commit" with "--allow-empty"?
+ * Should empty commits be allowed?  Return status:
+ *    <0: Error in is_index_unchanged(r) or is_original_commit_empty(commit)
+ *     0: Halt on empty commit
+ *     1: Allow empty commit
+ *     2: Drop empty commit
   */
  static int allow_empty(struct repository *r,
  		       struct replay_opts *opts,

@@ -1492,14 +1498,17 @@ static int allow_empty(struct repository *r,
  	int index_unchanged, originally_empty;
 
  	/*
-	 * Three cases:
+	 * Four cases:
  	 *
  	 * (1) we do not allow empty at all and error out.
  	 *
-	 * (2) we allow ones that were initially empty, but
-	 * forbid the ones that become empty;
+	 * (2) we allow ones that were initially empty, and
+	 *     just drop the ones that become empty
  	 *
-	 * (3) we allow both.
+	 * (3) we allow ones that were initially empty, but
+	 *     halt for the ones that become empty;
+	 *
+	 * (4) we allow both.
  	 */
  	if (!opts->allow_empty)
  		return 0; /* let "git commit" barf as necessary */

@@ -1516,10 +1525,12 @@ static int allow_empty(struct repository *r,
  	originally_empty = is_original_commit_empty(commit);
  	if (originally_empty < 0)
  		return originally_empty;
-	if (!originally_empty)
-		return 0;
-	else
+	if (originally_empty)
  		return 1;
+	else if (opts->drop_redundant_commits)
+		return 2;
+	else
+		return 0;
  }
 
  static struct {

@@ -1730,7 +1741,7 @@ static int do_pick_commit(struct repository *r,
  	char *author = NULL;
  	struct commit_message msg = { NULL, NULL, NULL, NULL };
  	struct strbuf msgbuf = STRBUF_INIT;
-	int res, unborn = 0, reword = 0, allow;
+	int res, unborn = 0, reword = 0, allow, drop_commit;
 
  	if (opts->no_commit) {
  		/*

@@ -1935,13 +1946,18 @@ static int do_pick_commit(struct repository *r,
  		goto leave;
  	}
 
+	drop_commit = 0;
  	allow = allow_empty(r, opts, commit);

sequencer.c +11 -0

+	if (!opts->no_commit && !drop_commit) {
  		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
  			res = do_commit(r, msg_file, author, opts, flags);
  		else

@@ -2495,6 +2511,12 @@ static int read_populate_opts(struct replay_opts *opts)
  		if (file_exists(rebase_path_reschedule_failed_exec()))
  			opts->reschedule_failed_exec = 1;
 
+		if (file_exists(rebase_path_drop_redundant_commits()))
+			opts->drop_redundant_commits = 1;
+
+		if (file_exists(rebase_path_keep_redundant_commits()))
+			opts->keep_redundant_commits = 1;
+
  		read_strategy_opts(opts, &buf);
  		strbuf_release(&buf);
 

@@ -2574,6 +2596,10 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
  		write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
  	if (opts->signoff)
  		write_file(rebase_path_signoff(), "--signoff\n");
+	if (opts->drop_redundant_commits)
+		write_file(rebase_path_drop_redundant_commits(), "%s", "");
+	if (opts->keep_redundant_commits)
+		write_file(rebase_path_keep_redundant_commits(), "%s", "");
  	if (opts->reschedule_failed_exec)
  		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 

sequencer.h +1 -0

index c165e0ff25..3b0ab9141f 100644
@@ -39,6 +39,7 @@ struct replay_opts {
  	int allow_rerere_auto;
  	int allow_empty;
  	int allow_empty_message;
+	int drop_redundant_commits;
  	int keep_redundant_commits;
  	int verbose;
  	int quiet;

t/t3424-rebase-empty.sh +37 -6

index 22d97e143b..dcb4cb4751 100755
@@ -34,7 +34,7 @@ test_expect_success 'setup test repository' '
  	git commit -m "Five letters ought to be enough for anybody"
  '
 
-test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
+test_expect_failure 'rebase (am-backend)' '
  	test_when_finished "git rebase --abort" &&
  	git checkout -B testing localmods &&
  	# rebase (--am) should not drop commits that start empty

@@ -45,11 +45,29 @@ test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
  	test_cmp expect actual
  '
 
-test_expect_failure 'rebase --merge with a variety of empty commits' '
-	test_when_finished "git rebase --abort" &&
+test_expect_success 'rebase --merge --empty=drop' '
  	git checkout -B testing localmods &&
-	# rebase --merge should not halt on the commit that becomes empty
-	git rebase --merge upstream &&
+	git rebase --merge --empty=drop upstream &&
+
+	test_write_lines D C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep upstream &&
+
+	test_write_lines D C2 C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=ask' '
+	git checkout -B testing localmods &&
+	test_must_fail git rebase --merge --empty=ask upstream &&
+
+	git rebase --skip &&
 
  	test_write_lines D C B A >expect &&
  	git log --format=%s >actual &&

@@ -58,9 +76,27 @@ test_expect_failure 'rebase --merge with a variety of empty commits' '
 
  GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 
-test_expect_success 'rebase --interactive with a variety of empty commits' '
+test_expect_success 'rebase --interactive --empty=drop' '
+	git checkout -B testing localmods &&
+	git rebase --interactive --empty=drop upstream &&
+
+	test_write_lines D C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --interactive --empty=keep' '
+	git checkout -B testing localmods &&
+	git rebase --interactive --empty=keep upstream &&
+
+	test_write_lines D C2 C B A >expect &&

t/t3427-rebase-subtree.sh +4 -4

index 8dceef61cf..79e43a370b 100755
@@ -85,10 +85,10 @@ test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --
  	verbose test "$(commit_message HEAD)" = "Empty commit"
  '
 
-test_expect_success 'Rebase -Xsubtree --onto commit' '
+test_expect_success 'Rebase -Xsubtree --empty=ask --onto commit' '
  	reset_rebase &&
  	git checkout -b rebase-onto to-rebase &&
-	test_must_fail git rebase -Xsubtree=files_subtree --onto files-master master &&
+	test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --onto files-master master &&
  	: first pick results in no changes &&
  	git rebase --skip &&
  	verbose test "$(commit_message HEAD~2)" = "master4" &&

@@ -96,10 +96,10 @@ test_expect_success 'Rebase -Xsubtree --onto commit' '
  	verbose test "$(commit_message HEAD)" = "Empty commit"
  '
 
-test_expect_success 'Rebase -Xsubtree --rebase-merges --onto commit' '
+test_expect_success 'Rebase -Xsubtree --empty=ask --rebase-merges --onto commit' '
  	reset_rebase &&
  	git checkout -b rebase-merges-onto to-rebase &&
-	test_must_fail git rebase -Xsubtree=files_subtree --rebase-merges --onto files-master --root &&
+	test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --rebase-merges --onto files-master --root &&
  	: first pick results in no changes &&
  	git rebase --skip &&
  	verbose test "$(commit_message HEAD~2)" = "master4" &&

Hi Elijah

Quoted 75 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

As established in the previous commit and commit b00bf1c9a8dd (git-rebase: make --allow-empty-message the default, 2018-06-27), the behavior for rebase with different backends in various edge or corner cases is often more happenstance than design. This commit addresses another such corner case: commits which "become empty".

A careful reader may note that there are two types of commits which would become empty due to a rebase:

* [clean cherry-pick] Commits which are clean cherry-picks of upstream commits, as determined by git log --cherry-mark .... Re-applying these commits would result in an empty set of changes and a duplicative commit message; i.e. these are commits that have "already been applied" upstream.

* [become empty] Commits which are not empty to start, are not clean cherry-picks of upstream commits, but which still become empty after being rebased. This happens e.g. when a commit has changes which are a strict subset of the changes in an upstream commit, or when the changes of a commit can be found spread across or among several upstream commits.

Clearly, in both cases the changes in the commit in question are found upstream already, but the commit message may not be in the latter case.

When cherry-mark can determine a commit is already upstream, then because of how cherry-mark works this means the upstream commit message was about the *exact* same set of changes. Thus, the commit messages can be assumed to be fully interchangeable (and are in fact likely to be completely identical). As such, the clean cherry-pick case represents a case when there is no information to be gained by keeping the extra commit around. All rebase types have always dropped these commits, and no one to my knowledge has ever requested that we do otherwise.

For many of the become empty cases (and likely even most), we will also be able to drop the commit without loss of information -- but this isn't quite always the case. Since these commits represent cases that were not clean cherry-picks, there is no upstream commit message explaining the same set of changes. Projects with good commit message hygiene will likely have the explanation from our commit message contained within or spread among the relevant upstream commits, but not all projects run that way. As such, the commit message of the commit being rebased may have reasoning that suggests additional changes that should be made to adapt to the new base, or it may have information that someone wants to add as a note to another commit, or perhaps someone even wants to create an empty commit with the commit message as-is.

Junio commented on the "become-empty" types of commits as follows[1]:

WRT a change that ends up being empty (as opposed to a change that is empty from the beginning), I'd think that the current behaviour is desireable one. "am" based rebase is solely to transplant an existing history and want to stop much less than "interactive" one whose purpose is to polish a series before making it publishable, and asking for confirmation ("this has become empty--do you want to drop it?") is more appropriate from the workflow point of view.

[1] https://lore.kernel.org/git/[email protected]/

I would simply add that his arguments for "am"-based rebases actually apply to all non-explicitly-interactive rebases. Also, since we are stating that different cases should have different defaults, it may be worth providing a flag to allow users to select which behavior they want for these commits.

Introduce a new command line flag for selecting the desired behavior:

     --empty={drop,keep,ask}

with the definitions:
drop: drop commits which become empty
keep: keep commits which become empty
ask: provide the user a chance to interact and pick what to do with
commits which become empty on a case-by-case basis

Quoted 4 lines

In line with Junio's suggestion, if the --empty flag is not specified, pick defaults as follows:
explicitly interactive: ask
otherwise: drop

Looking at the implementation there is a third option - if --exec is given without -i then the default is "keep". I'm not sure if having different defaults is convenient or confusing but don't feel that strongly about it.

I've got a few minor comments below (the mains ones
are saying which commit has been dropped and testing the default behavior when --empty is not given) but basically I like the new patch. Thanks for working on this, the commit message does a good job of explaining the changes.

Quoted 18 lines

Signed-off-by Elijah Newren <[email protected]>

---
  Documentation/git-rebase.txt | 27 ++++++++++++++++---
  builtin/rebase.c             | 52 ++++++++++++++++++++++++++++++++++++
  sequencer.c                  | 48 +++++++++++++++++++++++++--------
  sequencer.h                  |  1 +
  t/t3424-rebase-empty.sh      | 50 +++++++++++++++++++++++++++++-----
  t/t3427-rebase-subtree.sh    |  8 +++---
  6 files changed, 161 insertions(+), 25 deletions(-)

Documentation/git-rebase.txt +0 -0

index 1d19542d79..551a91d764 100644
@@ -258,6 +258,22 @@ See also INCOMPATIBLE OPTIONS below.
      original branch. The index and working tree are also left
      unchanged as a result.
 

Documentation/git-rebase.txt See also INCOMPATIBLE OPTIONS below. · line 269

Quoted 9 lines

Documentation/git-rebase.txt +9 -0

+--empty={drop,keep,ask}::
+     How to handle commits that are not empty to start and are not
+     clean cherry-picks of any upstream commit, but which become
+     empty after rebasing (because they contain a subset of already
+     upstream changes).  With drop (the default), commits that
+     become empty are dropped.  With keep, such commits are kept.
+     With ask (implied by --interactive), the rebase will halt when
+     an empty commit is applied allowing you to choose whether to
+     drop it, edit files more, or just commit the empty changes.

We should probably document the default for --exec without -i

Quoted 121 lines

Documentation/git-rebase.txt +14 -3

+Note that commits which start empty are kept, and commits which are
+clean cherry-picks (as determined by `git log --cherry-mark ...`) are
+always dropped.
++
+See also INCOMPATIBLE OPTIONS below.
+
  --keep-empty::
      No-op.  Rebasing commits that started empty (had no change
      relative to their parent) used to fail and this option would

@@ -561,6 +577,7 @@ are incompatible with the following options:
   * --interactive
   * --exec
   * --keep-empty
+ * --empty=
   * --edit-todo
   * --root when used in combination with --onto
 

@@ -569,6 +586,7 @@ In addition, the following pairs of options are incompatible:
   * --preserve-merges and --interactive
   * --preserve-merges and --signoff
   * --preserve-merges and --rebase-merges
+ * --preserve-merges and --empty=
   * --keep-base and --onto
   * --keep-base and --root
 

@@ -585,9 +603,12 @@ commits that started empty, though these are rare in practice.  It
  also drops commits that become empty and has no option for controlling
  this behavior.
 
-The interactive backend keeps intentionally empty commits.
-Unfortunately, it always halts whenever it runs across a commit that
-becomes empty, even when the rebase is not explicitly interactive.
+The interactive backend keeps intentionally empty commits.  Similar to
+the am backend, by default the interactive backend drops commits that
+become empty unless -i/--interactive is specified (in which case it
+stops and asks the user what to do).  The interactive backend also has
+an --empty={drop,keep,ask} option for changing the behavior of
+handling commits that become empty.
 
  Directory rename detection
  ~~~~~~~~~~~~~~~~~~~~~~~~~~

builtin/rebase.c +38 -0

index 537b3241ce..c299869e7b 100644
@@ -50,8 +50,16 @@ enum rebase_type {
      REBASE_PRESERVE_MERGES
  };
 
+enum empty_type {
+     EMPTY_UNSPECIFIED = -1,
+     EMPTY_DROP,
+     EMPTY_KEEP,
+     EMPTY_ASK
+};
+
  struct rebase_options {
      enum rebase_type type;
+     enum empty_type empty;
      const char *state_dir;
      struct commit *upstream;
      const char *upstream_name;

@@ -91,6 +99,7 @@ struct rebase_options {
 
  #define REBASE_OPTIONS_INIT {                               \
              .type = REBASE_UNSPECIFIED,             \
+             .empty = EMPTY_UNSPECIFIED,             \
              .flags = REBASE_NO_QUIET,               \
              .git_am_opts = ARGV_ARRAY_INIT,         \
              .git_format_patch_opt = STRBUF_INIT     \

@@ -109,6 +118,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
              replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
      replay.allow_empty = 1;
      replay.allow_empty_message = opts->allow_empty_message;
+     replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
+     replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
      replay.verbose = opts->flags & REBASE_VERBOSE;
      replay.reschedule_failed_exec = opts->reschedule_failed_exec;
      replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);

@@ -444,6 +455,10 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 
      BUG_ON_OPT_ARG(arg);
 
+     /*
+      * If we ever want to remap --keep-empty to --empty=keep, insert:
+      *      opts->empty = unset ? EMPTY_UNSPECIFIED : EMPTY_KEEP;
+      */
      opts->type = REBASE_INTERACTIVE;
      return 0;
  }

@@ -1350,6 +1365,29 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
      return 0;
  }
 
+static enum empty_type parse_empty_value(const char *value)
+{
+     if (!strcasecmp(value, "drop"))
+             return EMPTY_DROP;
+     else if (!strcasecmp(value, "keep"))
+             return EMPTY_KEEP;
+     else if (!strcasecmp(value, "ask"))
+             return EMPTY_ASK;
+
+     die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
+}
+
+static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
+{
+     struct rebase_options *options = opt->value;
+     enum empty_type value = parse_empty_value(arg);
+
+     BUG_ON_OPT_NEG(unset);
+
+     options->empty = value;
+     return 0;
+}
+
  static void NORETURN error_on_missing_default_upstream(void)
  {
      struct branch *current_branch = branch_get(NULL);

@@ -1494,6 +1532,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1536

Quoted 5 lines

builtin/rebase.c +2 -0

                               "ignoring them"),
                            REBASE_PRESERVE_MERGES, PARSE_OPT_HIDDEN),
              OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
+             OPT_CALLBACK_F(0, "empty", &options, N_("{drop,keep,ask}"),
+                            N_("how to handle empty commits"),

Maybe we should say "how to handle commits that become empty" to distinguish them from commits that start empty which we always keep

Quoted 115 lines

builtin/rebase.c +12 -0

+                            PARSE_OPT_NONEG, parse_opt_empty),
              { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
                      N_("(DEPRECATED) keep empty commits"),
                      PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,

@@ -1760,6 +1801,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
      if (!(options.flags & REBASE_NO_QUIET))
              argv_array_push(&options.git_am_opts, "-q");
 
+     if (options.empty != EMPTY_UNSPECIFIED)
+             imply_interactive(&options, "--empty");
+
      if (gpg_sign) {
              free(options.gpg_sign_opt);
              options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);

@@ -1843,6 +1887,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
              break;
      }
 
+     if (options.empty == EMPTY_UNSPECIFIED) {
+             if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
+                     options.empty = EMPTY_ASK;
+             else if (exec.nr > 0)
+                     options.empty = EMPTY_KEEP;
+             else
+                     options.empty = EMPTY_DROP;
+     }
      if (reschedule_failed_exec > 0 && !is_interactive(&options))
              die(_("--reschedule-failed-exec requires "
                    "--exec or --interactive"));

sequencer.c +22 -11

index c21fc202b1..354d0b5a38 100644
@@ -158,6 +158,8 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
  static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
+static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
  static int git_sequencer_config(const char *k, const char *v, void *cb)
  {

@@ -1483,7 +1485,11 @@ static int is_original_commit_empty(struct commit *commit)
  }
 
  /*
- * Do we run "git commit" with "--allow-empty"?
+ * Should empty commits be allowed?  Return status:
+ *    <0: Error in is_index_unchanged(r) or is_original_commit_empty(commit)
+ *     0: Halt on empty commit
+ *     1: Allow empty commit
+ *     2: Drop empty commit
   */
  static int allow_empty(struct repository *r,
                     struct replay_opts *opts,

@@ -1492,14 +1498,17 @@ static int allow_empty(struct repository *r,
      int index_unchanged, originally_empty;
 
      /*
-      * Three cases:
+      * Four cases:
       *
       * (1) we do not allow empty at all and error out.
       *
-      * (2) we allow ones that were initially empty, but
-      * forbid the ones that become empty;
+      * (2) we allow ones that were initially empty, and
+      *     just drop the ones that become empty
       *
-      * (3) we allow both.
+      * (3) we allow ones that were initially empty, but
+      *     halt for the ones that become empty;
+      *
+      * (4) we allow both.
       */
      if (!opts->allow_empty)
              return 0; /* let "git commit" barf as necessary */

@@ -1516,10 +1525,12 @@ static int allow_empty(struct repository *r,
      originally_empty = is_original_commit_empty(commit);
      if (originally_empty < 0)
              return originally_empty;
-     if (!originally_empty)
-             return 0;
-     else
+     if (originally_empty)
              return 1;
+     else if (opts->drop_redundant_commits)
+             return 2;
+     else
+             return 0;
  }
 
  static struct {

@@ -1730,7 +1741,7 @@ static int do_pick_commit(struct repository *r,
      char *author = NULL;
      struct commit_message msg = { NULL, NULL, NULL, NULL };
      struct strbuf msgbuf = STRBUF_INIT;
-     int res, unborn = 0, reword = 0, allow;
+     int res, unborn = 0, reword = 0, allow, drop_commit;
 
      if (opts->no_commit) {
              /*

@@ -1935,13 +1946,18 @@ static int do_pick_commit(struct repository *r,
              goto leave;
      }
 
+     drop_commit = 0;
      allow = allow_empty(r, opts, commit);
      if (allow < 0) {
              res = allow;
              goto leave;
-     } else if (allow)
+     } else if (allow == 1) {
              flags |= ALLOW_EMPTY;
-     if (!opts->no_commit) {

sequencer.c static int do_pick_commit(struct repository *r, · line 1958

Quoted 3 lines

sequencer.c +3 -0

+     } else if (allow == 2) {
+             drop_commit = 1;
+             fprintf(stderr, _("No changes -- Patch already applied.\n"));

nit pick - usually messages start with a lowercase letter. Would it be helpful to explicitly state which commit is being dropped as well as why? Something like
dropping <oid> <subject> - patch contents already upstream

sequencer.c static int do_pick_commit(struct repository *r, · line 1959

Quoted 2 lines

sequencer.c +1 -0

+     } // else allow == 0 and there's nothing special to do

We don't use // for comments

Quoted 116 lines

sequencer.c +11 -0

+     if (!opts->no_commit && !drop_commit) {
              if (author || command == TODO_REVERT || (flags & AMEND_MSG))
                      res = do_commit(r, msg_file, author, opts, flags);
              else

@@ -2495,6 +2511,12 @@ static int read_populate_opts(struct replay_opts *opts)
              if (file_exists(rebase_path_reschedule_failed_exec()))
                      opts->reschedule_failed_exec = 1;
 
+             if (file_exists(rebase_path_drop_redundant_commits()))
+                     opts->drop_redundant_commits = 1;
+
+             if (file_exists(rebase_path_keep_redundant_commits()))
+                     opts->keep_redundant_commits = 1;
+
              read_strategy_opts(opts, &buf);
              strbuf_release(&buf);
 

@@ -2574,6 +2596,10 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
              write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
      if (opts->signoff)
              write_file(rebase_path_signoff(), "--signoff\n");
+     if (opts->drop_redundant_commits)
+             write_file(rebase_path_drop_redundant_commits(), "%s", "");
+     if (opts->keep_redundant_commits)
+             write_file(rebase_path_keep_redundant_commits(), "%s", "");
      if (opts->reschedule_failed_exec)
              write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 

sequencer.h +1 -0

index c165e0ff25..3b0ab9141f 100644
@@ -39,6 +39,7 @@ struct replay_opts {
      int allow_rerere_auto;
      int allow_empty;
      int allow_empty_message;
+     int drop_redundant_commits;
      int keep_redundant_commits;
      int verbose;
      int quiet;

t/t3424-rebase-empty.sh +43 -7

index 22d97e143b..dcb4cb4751 100755
@@ -34,7 +34,7 @@ test_expect_success 'setup test repository' '
      git commit -m "Five letters ought to be enough for anybody"
  '
 
-test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
+test_expect_failure 'rebase (am-backend)' '
      test_when_finished "git rebase --abort" &&
      git checkout -B testing localmods &&
      # rebase (--am) should not drop commits that start empty

@@ -45,11 +45,29 @@ test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
      test_cmp expect actual
  '
 
-test_expect_failure 'rebase --merge with a variety of empty commits' '
-     test_when_finished "git rebase --abort" &&
+test_expect_success 'rebase --merge --empty=drop' '
      git checkout -B testing localmods &&
-     # rebase --merge should not halt on the commit that becomes empty
-     git rebase --merge upstream &&
+     git rebase --merge --empty=drop upstream &&
+
+     test_write_lines D C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep' '
+     git checkout -B testing localmods &&
+     git rebase --merge --empty=keep upstream &&
+
+     test_write_lines D C2 C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=ask' '
+     git checkout -B testing localmods &&
+     test_must_fail git rebase --merge --empty=ask upstream &&
+
+     git rebase --skip &&
 
      test_write_lines D C B A >expect &&
      git log --format=%s >actual &&

@@ -58,9 +76,27 @@ test_expect_failure 'rebase --merge with a variety of empty commits' '
 
  GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 
-test_expect_success 'rebase --interactive with a variety of empty commits' '
+test_expect_success 'rebase --interactive --empty=drop' '
+     git checkout -B testing localmods &&
+     git rebase --interactive --empty=drop upstream &&
+
+     test_write_lines D C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --interactive --empty=keep' '
+     git checkout -B testing localmods &&
+     git rebase --interactive --empty=keep upstream &&
+
+     test_write_lines D C2 C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --interactive --empty=ask' '
      git checkout -B testing localmods &&
-     test_must_fail git rebase --interactive upstream &&
+     test_must_fail git rebase --interactive --empty=ask upstream &&
 

t/t3424-rebase-empty.sh test_expect_failure 'rebase --merge with a variety of empty commits' ' · line 101

Quoted 1 line

t/t3424-rebase-empty.sh +0 -0

      git rebase --skip &&

As the default if --empty is not given is supposed to vary depending on the other options given it would be good to test that I think

Quoted 84 lines · Phillip Wood

Hi Elijah

Quoted 75 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

As established in the previous commit and commit b00bf1c9a8dd (git-rebase: make --allow-empty-message the default, 2018-06-27), the behavior for rebase with different backends in various edge or corner cases is often more happenstance than design. This commit addresses another such corner case: commits which "become empty".

A careful reader may note that there are two types of commits which would become empty due to a rebase:

* [clean cherry-pick] Commits which are clean cherry-picks of upstream commits, as determined by git log --cherry-mark .... Re-applying these commits would result in an empty set of changes and a duplicative commit message; i.e. these are commits that have "already been applied" upstream.

* [become empty] Commits which are not empty to start, are not clean cherry-picks of upstream commits, but which still become empty after being rebased. This happens e.g. when a commit has changes which are a strict subset of the changes in an upstream commit, or when the changes of a commit can be found spread across or among several upstream commits.

Clearly, in both cases the changes in the commit in question are found upstream already, but the commit message may not be in the latter case.

When cherry-mark can determine a commit is already upstream, then because of how cherry-mark works this means the upstream commit message was about the *exact* same set of changes. Thus, the commit messages can be assumed to be fully interchangeable (and are in fact likely to be completely identical). As such, the clean cherry-pick case represents a case when there is no information to be gained by keeping the extra commit around. All rebase types have always dropped these commits, and no one to my knowledge has ever requested that we do otherwise.

For many of the become empty cases (and likely even most), we will also be able to drop the commit without loss of information -- but this isn't quite always the case. Since these commits represent cases that were not clean cherry-picks, there is no upstream commit message explaining the same set of changes. Projects with good commit message hygiene will likely have the explanation from our commit message contained within or spread among the relevant upstream commits, but not all projects run that way. As such, the commit message of the commit being rebased may have reasoning that suggests additional changes that should be made to adapt to the new base, or it may have information that someone wants to add as a note to another commit, or perhaps someone even wants to create an empty commit with the commit message as-is.

Junio commented on the "become-empty" types of commits as follows[1]:

WRT a change that ends up being empty (as opposed to a change that is empty from the beginning), I'd think that the current behaviour is desireable one. "am" based rebase is solely to transplant an existing history and want to stop much less than "interactive" one whose purpose is to polish a series before making it publishable, and asking for confirmation ("this has become empty--do you want to drop it?") is more appropriate from the workflow point of view.

[1] https://lore.kernel.org/git/[email protected]/

I would simply add that his arguments for "am"-based rebases actually apply to all non-explicitly-interactive rebases. Also, since we are stating that different cases should have different defaults, it may be worth providing a flag to allow users to select which behavior they want for these commits.

Introduce a new command line flag for selecting the desired behavior:

      --empty={drop,keep,ask}

with the definitions:
drop: drop commits which become empty
keep: keep commits which become empty
ask: provide the user a chance to interact and pick what to do with
commits which become empty on a case-by-case basis

Quoted 4 lines

In line with Junio's suggestion, if the --empty flag is not specified, pick defaults as follows:
explicitly interactive: ask
otherwise: drop

Quoted 4 lines

Looking at the implementation there is a third option - if --exec is given without -i then the default is "keep". I'm not sure if having different defaults is convenient or confusing but don't feel that strongly about it.

Heh, in https://lore.kernel.org/git/[email protected]/
you argued that having different defaults was confusing and sounded like you felt strongly about it. Granted, there has been a lot of simplification to the implementation (and description) since then but I'm still inclined to go with the simpler and more easily explained behavior for the defaults based on what you said there.

Quoted 5 lines

I've got a few minor comments below (the mains ones
are saying which commit has been dropped and testing the default behavior when --empty is not given) but basically I like the new patch. Thanks for working on this, the commit message does a good job of explaining the changes.

:-)

Quoted 18 lines

Quoted 18 lines

Signed-off-by Elijah Newren <[email protected]>

---
   Documentation/git-rebase.txt | 27 ++++++++++++++++---
   builtin/rebase.c             | 52 ++++++++++++++++++++++++++++++++++++
   sequencer.c                  | 48 +++++++++++++++++++++++++--------
   sequencer.h                  |  1 +
   t/t3424-rebase-empty.sh      | 50 +++++++++++++++++++++++++++++-----
   t/t3427-rebase-subtree.sh    |  8 +++---
   6 files changed, 161 insertions(+), 25 deletions(-)

Documentation/git-rebase.txt +0 -0

index 1d19542d79..551a91d764 100644
@@ -258,6 +258,22 @@ See also INCOMPATIBLE OPTIONS below.
       original branch. The index and working tree are also left
       unchanged as a result.
 

Quoted 11 lines

Documentation/git-rebase.txt See also INCOMPATIBLE OPTIONS below. · line 269

Quoted 9 lines

Documentation/git-rebase.txt +9 -0

+--empty={drop,keep,ask}::
+     How to handle commits that are not empty to start and are not
+     clean cherry-picks of any upstream commit, but which become
+     empty after rebasing (because they contain a subset of already
+     upstream changes).  With drop (the default), commits that
+     become empty are dropped.  With keep, such commits are kept.
+     With ask (implied by --interactive), the rebase will halt when
+     an empty commit is applied allowing you to choose whether to
+     drop it, edit files more, or just commit the empty changes.

We should probably document the default for --exec without -i

I did, but I guess it wasn't clear enough. Maybe I should add "Note that other options like --exec will use the default of drop unless -i/--interactive is specified."?

Quoted 121 lines

Quoted 121 lines

Documentation/git-rebase.txt +14 -3

+Note that commits which start empty are kept, and commits which are
+clean cherry-picks (as determined by `git log --cherry-mark ...`) are
+always dropped.
++
+See also INCOMPATIBLE OPTIONS below.
+
   --keep-empty::
       No-op.  Rebasing commits that started empty (had no change
       relative to their parent) used to fail and this option would

@@ -561,6 +577,7 @@ are incompatible with the following options:
    * --interactive
    * --exec
    * --keep-empty
+ * --empty=
    * --edit-todo
    * --root when used in combination with --onto
 

@@ -569,6 +586,7 @@ In addition, the following pairs of options are incompatible:
    * --preserve-merges and --interactive
    * --preserve-merges and --signoff
    * --preserve-merges and --rebase-merges
+ * --preserve-merges and --empty=
    * --keep-base and --onto
    * --keep-base and --root
 

@@ -585,9 +603,12 @@ commits that started empty, though these are rare in practice.  It
   also drops commits that become empty and has no option for controlling
   this behavior.
 
-The interactive backend keeps intentionally empty commits.
-Unfortunately, it always halts whenever it runs across a commit that
-becomes empty, even when the rebase is not explicitly interactive.
+The interactive backend keeps intentionally empty commits.  Similar to
+the am backend, by default the interactive backend drops commits that
+become empty unless -i/--interactive is specified (in which case it
+stops and asks the user what to do).  The interactive backend also has
+an --empty={drop,keep,ask} option for changing the behavior of
+handling commits that become empty.
 
   Directory rename detection
   ~~~~~~~~~~~~~~~~~~~~~~~~~~

builtin/rebase.c +38 -0

index 537b3241ce..c299869e7b 100644
@@ -50,8 +50,16 @@ enum rebase_type {
       REBASE_PRESERVE_MERGES
   };
 
+enum empty_type {
+     EMPTY_UNSPECIFIED = -1,
+     EMPTY_DROP,
+     EMPTY_KEEP,
+     EMPTY_ASK
+};
+
   struct rebase_options {
       enum rebase_type type;
+     enum empty_type empty;
       const char *state_dir;
       struct commit *upstream;
       const char *upstream_name;

@@ -91,6 +99,7 @@ struct rebase_options {
 
   #define REBASE_OPTIONS_INIT {                               \
               .type = REBASE_UNSPECIFIED,             \
+             .empty = EMPTY_UNSPECIFIED,             \
               .flags = REBASE_NO_QUIET,               \
               .git_am_opts = ARGV_ARRAY_INIT,         \
               .git_format_patch_opt = STRBUF_INIT     \

@@ -109,6 +118,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
               replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
       replay.allow_empty = 1;
       replay.allow_empty_message = opts->allow_empty_message;
+     replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
+     replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
       replay.verbose = opts->flags & REBASE_VERBOSE;
       replay.reschedule_failed_exec = opts->reschedule_failed_exec;
       replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);

@@ -444,6 +455,10 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 
       BUG_ON_OPT_ARG(arg);
 
+     /*
+      * If we ever want to remap --keep-empty to --empty=keep, insert:
+      *      opts->empty = unset ? EMPTY_UNSPECIFIED : EMPTY_KEEP;
+      */
       opts->type = REBASE_INTERACTIVE;
       return 0;
   }

@@ -1350,6 +1365,29 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
       return 0;
   }
 
+static enum empty_type parse_empty_value(const char *value)
+{
+     if (!strcasecmp(value, "drop"))
+             return EMPTY_DROP;
+     else if (!strcasecmp(value, "keep"))
+             return EMPTY_KEEP;
+     else if (!strcasecmp(value, "ask"))
+             return EMPTY_ASK;
+
+     die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
+}
+
+static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
+{
+     struct rebase_options *options = opt->value;
+     enum empty_type value = parse_empty_value(arg);
+
+     BUG_ON_OPT_NEG(unset);
+
+     options->empty = value;
+     return 0;
+}
+
   static void NORETURN error_on_missing_default_upstream(void)
   {
       struct branch *current_branch = branch_get(NULL);

@@ -1494,6 +1532,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)

Quoted 8 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1536

Quoted 5 lines

builtin/rebase.c +2 -0

                                "ignoring them"),
                             REBASE_PRESERVE_MERGES, PARSE_OPT_HIDDEN),
               OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
+             OPT_CALLBACK_F(0, "empty", &options, N_("{drop,keep,ask}"),
+                            N_("how to handle empty commits"),

Maybe we should say "how to handle commits that become empty" to distinguish them from commits that start empty which we always keep

Ooh, good catch; will fix.

Quoted 115 lines

Quoted 115 lines

builtin/rebase.c +12 -0

+                            PARSE_OPT_NONEG, parse_opt_empty),
               { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
                       N_("(DEPRECATED) keep empty commits"),
                       PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,

@@ -1760,6 +1801,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
       if (!(options.flags & REBASE_NO_QUIET))
               argv_array_push(&options.git_am_opts, "-q");
 
+     if (options.empty != EMPTY_UNSPECIFIED)
+             imply_interactive(&options, "--empty");
+
       if (gpg_sign) {
               free(options.gpg_sign_opt);
               options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);

@@ -1843,6 +1887,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
               break;
       }
 
+     if (options.empty == EMPTY_UNSPECIFIED) {
+             if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
+                     options.empty = EMPTY_ASK;
+             else if (exec.nr > 0)
+                     options.empty = EMPTY_KEEP;
+             else
+                     options.empty = EMPTY_DROP;
+     }
       if (reschedule_failed_exec > 0 && !is_interactive(&options))
               die(_("--reschedule-failed-exec requires "
                     "--exec or --interactive"));

sequencer.c +22 -11

index c21fc202b1..354d0b5a38 100644
@@ -158,6 +158,8 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
   static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
   static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
   static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
+static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
+static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
   static int git_sequencer_config(const char *k, const char *v, void *cb)
   {

@@ -1483,7 +1485,11 @@ static int is_original_commit_empty(struct commit *commit)
   }
 
   /*
- * Do we run "git commit" with "--allow-empty"?
+ * Should empty commits be allowed?  Return status:
+ *    <0: Error in is_index_unchanged(r) or is_original_commit_empty(commit)
+ *     0: Halt on empty commit
+ *     1: Allow empty commit
+ *     2: Drop empty commit
    */
   static int allow_empty(struct repository *r,
                      struct replay_opts *opts,

@@ -1492,14 +1498,17 @@ static int allow_empty(struct repository *r,
       int index_unchanged, originally_empty;
 
       /*
-      * Three cases:
+      * Four cases:
        *
        * (1) we do not allow empty at all and error out.
        *
-      * (2) we allow ones that were initially empty, but
-      * forbid the ones that become empty;
+      * (2) we allow ones that were initially empty, and
+      *     just drop the ones that become empty
        *
-      * (3) we allow both.
+      * (3) we allow ones that were initially empty, but
+      *     halt for the ones that become empty;
+      *
+      * (4) we allow both.
        */
       if (!opts->allow_empty)
               return 0; /* let "git commit" barf as necessary */

@@ -1516,10 +1525,12 @@ static int allow_empty(struct repository *r,
       originally_empty = is_original_commit_empty(commit);
       if (originally_empty < 0)
               return originally_empty;
-     if (!originally_empty)
-             return 0;
-     else
+     if (originally_empty)
               return 1;
+     else if (opts->drop_redundant_commits)
+             return 2;
+     else
+             return 0;
   }
 
   static struct {

@@ -1730,7 +1741,7 @@ static int do_pick_commit(struct repository *r,
       char *author = NULL;
       struct commit_message msg = { NULL, NULL, NULL, NULL };
       struct strbuf msgbuf = STRBUF_INIT;
-     int res, unborn = 0, reword = 0, allow;
+     int res, unborn = 0, reword = 0, allow, drop_commit;
 
       if (opts->no_commit) {
               /*

@@ -1935,13 +1946,18 @@ static int do_pick_commit(struct repository *r,
               goto leave;
       }
 
+     drop_commit = 0;
       allow = allow_empty(r, opts, commit);
       if (allow < 0) {
               res = allow;
               goto leave;
-     } else if (allow)
+     } else if (allow == 1) {
               flags |= ALLOW_EMPTY;
-     if (!opts->no_commit) {

Quoted 8 lines

sequencer.c static int do_pick_commit(struct repository *r, · line 1958

Quoted 3 lines

sequencer.c +3 -0

+     } else if (allow == 2) {
+             drop_commit = 1;
+             fprintf(stderr, _("No changes -- Patch already applied.\n"));

nit pick - usually messages start with a lowercase letter. Would it be helpful to explicitly state which commit is being dropped as well as why? Something like
dropping <oid> <subject> - patch contents already upstream

I was actually just trying to mimic the am-backend here, and copied its message verbatim for this case (see am_run() in builtin/am.c). However, your version does seem more helpful and informative. I'll look into it implementing it here.

Quoted 4 lines

sequencer.c static int do_pick_commit(struct repository *r, · line 1959

Quoted 2 lines

sequencer.c +1 -0

+     } // else allow == 0 and there's nothing special to do

We don't use // for comments

Oops, sorry. Will fix.

Quoted 116 lines

Quoted 116 lines

sequencer.c +11 -0

+     if (!opts->no_commit && !drop_commit) {
               if (author || command == TODO_REVERT || (flags & AMEND_MSG))
                       res = do_commit(r, msg_file, author, opts, flags);
               else

@@ -2495,6 +2511,12 @@ static int read_populate_opts(struct replay_opts *opts)
               if (file_exists(rebase_path_reschedule_failed_exec()))
                       opts->reschedule_failed_exec = 1;
 
+             if (file_exists(rebase_path_drop_redundant_commits()))
+                     opts->drop_redundant_commits = 1;
+
+             if (file_exists(rebase_path_keep_redundant_commits()))
+                     opts->keep_redundant_commits = 1;
+
               read_strategy_opts(opts, &buf);
               strbuf_release(&buf);
 

@@ -2574,6 +2596,10 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
               write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);
       if (opts->signoff)
               write_file(rebase_path_signoff(), "--signoff\n");
+     if (opts->drop_redundant_commits)
+             write_file(rebase_path_drop_redundant_commits(), "%s", "");
+     if (opts->keep_redundant_commits)
+             write_file(rebase_path_keep_redundant_commits(), "%s", "");
       if (opts->reschedule_failed_exec)
               write_file(rebase_path_reschedule_failed_exec(), "%s", "");
 

sequencer.h +1 -0

index c165e0ff25..3b0ab9141f 100644
@@ -39,6 +39,7 @@ struct replay_opts {
       int allow_rerere_auto;
       int allow_empty;
       int allow_empty_message;
+     int drop_redundant_commits;
       int keep_redundant_commits;
       int verbose;
       int quiet;

t/t3424-rebase-empty.sh +43 -7

index 22d97e143b..dcb4cb4751 100755
@@ -34,7 +34,7 @@ test_expect_success 'setup test repository' '
       git commit -m "Five letters ought to be enough for anybody"
   '
 
-test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
+test_expect_failure 'rebase (am-backend)' '
       test_when_finished "git rebase --abort" &&
       git checkout -B testing localmods &&
       # rebase (--am) should not drop commits that start empty

@@ -45,11 +45,29 @@ test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
       test_cmp expect actual
   '
 
-test_expect_failure 'rebase --merge with a variety of empty commits' '
-     test_when_finished "git rebase --abort" &&
+test_expect_success 'rebase --merge --empty=drop' '
       git checkout -B testing localmods &&
-     # rebase --merge should not halt on the commit that becomes empty
-     git rebase --merge upstream &&
+     git rebase --merge --empty=drop upstream &&
+
+     test_write_lines D C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep' '
+     git checkout -B testing localmods &&
+     git rebase --merge --empty=keep upstream &&
+
+     test_write_lines D C2 C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=ask' '
+     git checkout -B testing localmods &&
+     test_must_fail git rebase --merge --empty=ask upstream &&
+
+     git rebase --skip &&
 
       test_write_lines D C B A >expect &&
       git log --format=%s >actual &&

@@ -58,9 +76,27 @@ test_expect_failure 'rebase --merge with a variety of empty commits' '
 
   GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
 
-test_expect_success 'rebase --interactive with a variety of empty commits' '
+test_expect_success 'rebase --interactive --empty=drop' '
+     git checkout -B testing localmods &&
+     git rebase --interactive --empty=drop upstream &&
+
+     test_write_lines D C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --interactive --empty=keep' '
+     git checkout -B testing localmods &&
+     git rebase --interactive --empty=keep upstream &&
+
+     test_write_lines D C2 C B A >expect &&
+     git log --format=%s >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'rebase --interactive --empty=ask' '
       git checkout -B testing localmods &&
-     test_must_fail git rebase --interactive upstream &&
+     test_must_fail git rebase --interactive --empty=ask upstream &&
 

Quoted 4 lines

t/t3424-rebase-empty.sh test_expect_failure 'rebase --merge with a variety of empty commits' ' · line 101

Quoted 1 line

t/t3424-rebase-empty.sh +0 -0

       git rebase --skip &&

As the default if --empty is not given is supposed to vary depending on the other options given it would be good to test that I think

I'll add some tests.

Thanks for the thorough review!
Elijah

This is a re-roll of en/rebase-backend which has been rebased on v2.25.0 and updated to remove the dependence on ra/rebase-i-more-options, and also tries to address feedback from Phillip, Junio, and Jonathan. This series does a lot of work around making the default rebase backend configurable, and switches the default from the am backend to the merge/interactive one.

Changes since v3:

* Rebased on origin/master and updated to remove the dependence on ra/rebase-i-more-options.
* Added two new patches at the start of the series.
* Split the old first patch into two, while modifying them based on Phillip's feedback (though slightly differently than discussed on the list; instead of making --keep-empty a synonym for --empty=keep, I instead kept backward compatibility by making it a no-op).
* I noted the post-commit hook in the differences between backends. Emily is investigating what changes need to happen there, so I merely documented the existing differences.
* dropped '-i' from the reflog messages; now they just refer to 'rebase'

Changes possibly missing from this version, for discussion:

* I did not remove the --am option as suggested by Phillip, since Junio and
Phillip were still discussing whether it is wanted/needed. > * I did not address the last two items Jonathan brought up as I

find enough information to reproduce or understand the problems.

Elijah Newren (19):
git-rebase.txt: update description of --allow-empty-message
t3404: directly test the behavior of interest
rebase (interactive-backend): make --keep-empty the default rebase (interactive-backend): fix handling of commits that become
empty
t3406: simplify an already simple test
rebase, sequencer: remove the broken GIT_QUIET handling
rebase: make sure to pass along the quiet flag to the sequencer
rebase: fix handling of restrict_revision
t3432: make these tests work with either am or merge backends
rebase: allow more types of rebases to fast-forward git-rebase.txt: add more details about behavioral differences of
backends
rebase: move incompatibility checks between backend options a bit
earlier
rebase: add an --am option
git-prompt: change the prompt for interactive-based rebases
rebase: drop '-i' from the reflog for interactive-based rebases rebase tests: mark tests specific to the am-backend with --am rebase tests: repeat some tests using the merge backend instead of am
rebase: make the backend configurable via config setting
rebase: change the default backend from "am" to "merge"

  Documentation/config/rebase.txt        |   8 ++
  Documentation/git-rebase.txt           | 150 +++++++++++++++++---
  builtin/rebase.c                       | 186 +++++++++++++++++++------
  contrib/completion/git-prompt.sh       |   6 +-
  rebase-interactive.c                   |   7 +-
  rebase-interactive.h                   |   2 +-
  sequencer.c                            |  84 ++++++-----
  sequencer.h                            |   3 +-
  t/t3400-rebase.sh                      |  36 ++++-
  t/t3401-rebase-and-am-rename.sh        |   4 +-
  t/t3404-rebase-interactive.sh          |  19 +--
  t/t3406-rebase-message.sh              |  19 ++-
  t/t3407-rebase-abort.sh                |   6 +-
  t/t3420-rebase-autostash.sh            |   2 +-
  t/t3421-rebase-topology-linear.sh      |  16 +--
  t/t3424-rebase-empty.sh                | 108 ++++++++++++++
  t/t3425-rebase-topology-merges.sh      |   8 +-
  t/t3427-rebase-subtree.sh              |  12 +-
  t/t3432-rebase-fast-forward.sh         |  54 ++++---
  t/t5407-post-rewrite-hook.sh           |  12 +-
  t/t5520-pull.sh                        |  27 +++-
  t/t6047-diff3-conflict-markers.sh      |  13 +-
  t/t7512-status-help.sh                 |  12 +-
  t/t9106-git-svn-commit-diff-clobber.sh |   3 +-
  t/t9903-bash-prompt.sh                 |   8 +-
  25 files changed, 595 insertions(+), 210 deletions(-)

create mode 100755 t/t3424-rebase-empty.sh

base-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-679%2Fnewren%2Frebase-fixes-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-679/newren/rebase-fixes-v4
Pull-Request: https://github.com/git/git/pull/679

Range-diff vs v3:

   -:  ---------- >  1:  3ea48d5394 git-rebase.txt: update description of --allow-empty-message
   -:  ---------- >  2:  10fdd162a0 t3404: directly test the behavior of interest
   1:  1c2b77e94d !  3:  179f82ab83 rebase: extend the options for handling of empty commits
      @@ -1,49 +1,46 @@
       Author: Elijah Newren <[email protected]>
       
      -    rebase: extend the options for handling of empty commits
      +    rebase (interactive-backend): make --keep-empty the default
       
      -    Extend the interactive machinery with the ability to handle the full
      -    spread of options for how to handle commits that either start or become
      -    empty (by "become empty" I mean the changes in a commit are a subset of
      -    changes that exist upstream, so the net effect of applying the commit is
      -    no changes).  Introduce a new command line flag for selecting the
      -    desired behavior:
      -        --empty={drop,keep,ask}
      -    with the definitions:
      -        drop: drop empty commits
      -        keep: keep empty commits
      -        ask:  provide the user a chance to interact and pick what to do with
      -              empty commits on a case-by-case basis
      +    Different rebase backends have different treatment for commits which
      +    start empty (i.e. have no changes relative to their parent), and the
      +    --keep-empty option was added at some point to allow adjusting behavior
      +    for the interactive backend.  The handling of commits which start empty
      +    is actually quite similar to commit b00bf1c9a8dd (git-rebase: make
      +    --allow-empty-message the default, 2018-06-27), which pointed out that
      +    the behavior for various backends is often more happenstance than
      +    design.  The specific change made in that commit is actually quite
      +    relevant as well and much of the logic there directly applies here.
       
      -    Note that traditionally, am-based rebases have always dropped commits
      -    that either started or became empty, while interactive-based rebases
      -    have defaulted to ask (and provided an option to keep commits that
      -    started empty).  This difference made sense since users of an am-based
      -    rebase just wanted to quickly batch apply a sequence of commits, while
      -    users editing a todo list will likely want the chance to interact and
      -    handle unusual cases on a case-by-case basis.  However, not all rebases
      -    using the interactive machinery are explicitly interactive anymore.  In
      -    particular --merge was always meant to behave more like --am: just
      -    rebase a batch of commits without popping up a todo list.
      +    It makes a lot of sense in 'git commit' to error out on the creation of
      +    empty commits, unless an override flag is provided.  However, once
      +    someone determines that there is a rare case that merits using the
      +    manual override to create such a commit, it is somewhere between
      +    annoying and harmful to have to take extra steps to keep such
      +    intentional commits around.  Granted, empty commits are quite rare,
      +    which is why handling of them doesn't get considered much and folks tend
      +    to defer to existing (accidental) behavior and assume there was a reason
      +    for it, leading them to just add flags (--keep-empty in this case) that
      +    allow them to override the bad defaults.  Fix the interactive backend so
      +    that --keep-empty is the default, much like we did with
      +    --allow-empty-message.  The am backend should also be fixed to have
      +    --keep-empty semantics for commits that start empty, but that is not
      +    included in this patch other than a testcase documenting the failure.
       
      -    If the --empty flag is not specified, pick defaults as follows:
      -        explicitly interactive: ask
      -        --exec: keep (exec is about checking existing commits, and often
      -                      used without actually changing the base.  Thus the
      -                      expectation is that the user doesn't necessarily want
      -                      anything to change; they just want to test).
      -        otherwise: drop
      -
      -    Also, this commit makes --keep-empty just imply --empty=keep, and hides
      -    it from help so that we aren't confusing users with different ways to do
      -    the same thing.  (I could have added a --drop-empty flag, but then that
      -    invites users to specify both --keep-empty and --drop-empty and we have
      -    to add sanity checking around that; it seems cleaner to have a single
      -    multi-valued option.)  This actually fixes --keep-empty too; previously,
      -    it only meant to sometimes keep empty commits, in particular commits
      -    which started empty would be kept.  But it would still error out and ask
      -    the user what to do with commits that became empty.  Now it keeps empty
      -    commits, as instructed.
      +    Note that there was one test in t3421 which appears to have been written
      +    expecting --keep-empty to not be the default as correct behavior.  This
      +    test was introduced in commit 00b8be5a4d38 ("add tests for rebasing of
      +    empty commits", 2013-06-06), which was part of a series focusing on
      +    rebase topology and which had an interesting original cover letter at
      +    https://lore.kernel.org/git/[email protected]/
      +    which noted
      +        Your input especially appreciated on whether you agree with the
      +        intent of the test cases.
      +    and then went into a long example about how one of the many tests added
      +    had several questions about whether it was correct.  As such, I believe
      +    most the tests in that series were about testing rebase topology with as
      +    many different flags as possible and were not trying to state in general
      +    how those flags should behave otherwise.
       
           Signed-off-by: Elijah Newren <[email protected]>
       
      @@ -51,126 +48,62 @@
        --- a/Documentation/git-rebase.txt
        +++ b/Documentation/git-rebase.txt
       @@
      - 	original branch. The index and working tree are also left
        	unchanged as a result.
        
      -+--empty={drop,keep,ask}::
      -+	How to handle commits that become empty (because they contain a
      -+	subset of already upstream changes) or start empty.  With drop
      -+	(the default), commits that start or become empty are dropped.
      -+	With keep (implied by --exec), such commits are kept.  With ask
      -+	(implied by --interactive), the rebase will halt when an empty
      -+	commit is applied allowing you to choose whether to drop it or
      -+	commit it.  Also with ask, if the rebase is interactive then
      -+	commits which start empty will be commented out in the todo
      -+	action list (giving you a chance to uncomment).
      -++
      -+Note that this has no effect on commits which are already upstream (as
      -+can be checked via `git log --cherry-mark ...`), which are always
      -+dropped by rebase.
      -++
      -+See also INCOMPATIBLE OPTIONS below.
      -+
        --keep-empty::
       -	Keep the commits that do not change anything from its
       -	parents in the result.
      -+	Deprecated alias for what is now known as --empty=keep.
      ++	No-op.  Rebasing commits that started empty (had no change
      ++	relative to their parent) used to fail and this option would
      ++	override that behavior, allowing commits with empty changes to
      ++	be rebased.  Now commits with no changes do not cause rebasing
      ++	to halt.
        +
      - See also INCOMPATIBLE OPTIONS below.
      -
      -@@
      -  * --interactive
      -  * --exec
      -  * --keep-empty
      -+ * --empty=
      -  * --edit-todo
      -  * --root when used in combination with --onto
      -
      -@@
      -  * --preserve-merges and --ignore-whitespace
      -  * --preserve-merges and --committer-date-is-author-date
      -  * --preserve-merges and --ignore-date
      -+ * --preserve-merges and --empty=
      -  * --keep-base and --onto
      -  * --keep-base and --root
      +-See also INCOMPATIBLE OPTIONS below.
      ++See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.
        
      + --allow-empty-message::
      + 	No-op.  Rebasing commits with an empty message used to fail
       @@
      + Empty commits
      + ~~~~~~~~~~~~~
        
      - There are some subtle differences how the backends behave.
      -
      --Empty commits
      --~~~~~~~~~~~~~
      --
       -The am backend drops any "empty" commits, regardless of whether the
       -commit started empty (had no changes relative to its parent to
       -start with) or ended empty (all changes were already applied
       -upstream in other commits).
      --
      ++The am backend unfortunately drops intentionally empty commits, i.e.
      ++commits that started empty, though these are rare in practice.  It
      ++also drops commits that become empty and has no option for controlling
      ++this behavior.
      +
       -The interactive backend drops commits by default that
       -started empty and halts if it hits a commit that ended up empty.
       -The `--keep-empty` option exists for the interactive backend to allow
       -it to keep commits that started empty.
      --
      ++The interactive backend keeps intentionally empty commits.
      ++Unfortunately, it always halts whenever it runs across a commit that
      ++becomes empty, even when the rebase is not explicitly interactive.
      +
        Directory rename detection
        ~~~~~~~~~~~~~~~~~~~~~~~~~~
      -
       
        diff --git a/builtin/rebase.c b/builtin/rebase.c
        --- a/builtin/rebase.c
        +++ b/builtin/rebase.c
      -@@
      - 	REBASE_PRESERVE_MERGES
      - };
      -
      -+enum empty_type {
      -+	EMPTY_UNSPECIFIED = -1,
      -+	EMPTY_DROP,
      -+	EMPTY_KEEP,
      -+	EMPTY_ASK
      -+};
      -+
      - struct rebase_options {
      - 	enum rebase_type type;
      -+	enum empty_type empty;
      - 	const char *state_dir;
      - 	struct commit *upstream;
      - 	const char *upstream_name;
       @@
        	const char *action;
        	int signoff;
        	int allow_rerere_autoupdate;
       -	int keep_empty;
        	int autosquash;
      - 	int ignore_whitespace;
        	char *gpg_sign_opt;
      -@@
      -
      - #define REBASE_OPTIONS_INIT {			  	\
      - 		.type = REBASE_UNSPECIFIED,	  	\
      -+		.empty = EMPTY_UNSPECIFIED,	  	\
      - 		.flags = REBASE_NO_QUIET, 		\
      - 		.git_am_opts = ARGV_ARRAY_INIT,		\
      - 		.git_format_patch_opt = STRBUF_INIT	\
      -@@
      - 		replay.allow_rerere_auto = opts->allow_rerere_autoupdate;
      - 	replay.allow_empty = 1;
      - 	replay.allow_empty_message = opts->allow_empty_message;
      -+	replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
      -+	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
      -+	replay.ask_on_initially_empty = (opts->empty == EMPTY_ASK &&
      -+					 !(opts->flags & REBASE_INTERACTIVE_EXPLICIT));
      - 	replay.verbose = opts->flags & REBASE_VERBOSE;
      - 	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
      - 	replay.committer_date_is_author_date =
      + 	int autostash;
       @@
        
        	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
        
       -	flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
      -+	flags |= (opts->empty == EMPTY_DROP) ? TODO_LIST_DROP_EMPTY : 0;
      -+	flags |= (opts->empty == EMPTY_ASK &&
      -+		  opts->flags & REBASE_INTERACTIVE_EXPLICIT) ?
      -+			TODO_LIST_ASK_EMPTY : 0;
        	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
        	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
        	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
      @@ -183,10 +116,8 @@
       +{
       +	struct rebase_options *opts = opt->value;
       +
      -+	BUG_ON_OPT_NEG(unset);
       +	BUG_ON_OPT_ARG(arg);
       +
      -+	opts->empty = EMPTY_KEEP;
       +	opts->type = REBASE_INTERACTIVE;
       +	return 0;
       +}
      @@ -201,62 +132,28 @@
       -		OPT_BOOL(0, "keep-empty", &opts.keep_empty, N_("keep empty commits")),
       +		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
       +			N_("(DEPRECATED) keep empty commits"),
      -+			PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
      ++			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
       +			parse_opt_keep_empty },
      - 		OPT_BOOL(0, "allow-empty-message", &opts.allow_empty_message,
      - 			 N_("allow commits with empty messages")),
      - 		OPT_BOOL(0, "rebase-merges", &opts.rebase_merges, N_("rebase merge commits")),
      + 		OPT_BOOL_F(0, "allow-empty-message", &opts.allow_empty_message,
      + 			   N_("allow commits with empty messages"),
      + 			   PARSE_OPT_HIDDEN),
       @@
        		opts->allow_rerere_autoupdate ?
        			opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
        			"--rerere-autoupdate" : "--no-rerere-autoupdate" : "");
       -	add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : "");
      -+	add_var(&script_snippet, "empty", opts->empty == EMPTY_KEEP ? "yes" : "");
        	add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : "");
        	add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);
        	add_var(&script_snippet, "cmd", opts->cmd);
      -@@
      - 	return 0;
      - }
      -
      -+static enum empty_type parse_empty_value(const char *value)
      -+{
      -+	if (!strcasecmp(value, "drop"))
      -+		return EMPTY_DROP;
      -+	else if (!strcasecmp(value, "keep"))
      -+		return EMPTY_KEEP;
      -+	else if (!strcasecmp(value, "ask"))
      -+		return EMPTY_ASK;
      -+
      -+	die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
      -+}
      -+
      -+static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
      -+{
      -+	struct rebase_options *options = opt->value;
      -+	enum empty_type value = parse_empty_value(arg);
      -+
      -+	BUG_ON_OPT_NEG(unset);
      -+
      -+	options->empty = value;
      -+	return 0;
      -+}
      -+
      - static void NORETURN error_on_missing_default_upstream(void)
      - {
      - 	struct branch *current_branch = branch_get(NULL);
       @@
        				 "ignoring them"),
        			      REBASE_PRESERVE_MERGES, PARSE_OPT_HIDDEN),
        		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
       -		OPT_BOOL('k', "keep-empty", &options.keep_empty,
       -			 N_("preserve empty commits during rebase")),
      -+		OPT_CALLBACK_F(0, "empty", &options, N_("{drop,keep,ask}"),
      -+			       N_("how to handle empty commits"),
      -+			       PARSE_OPT_NONEG, parse_opt_empty),
       +		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
       +			N_("(DEPRECATED) keep empty commits"),
      -+			PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
      ++			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
       +			parse_opt_keep_empty },
        		OPT_BOOL(0, "autosquash", &options.autosquash,
        			 N_("move commits that begin with "
      @@ -267,26 +164,10 @@
        
       -	if (options.keep_empty)
       -		imply_interactive(&options, "--keep-empty");
      -+	if (options.empty != EMPTY_UNSPECIFIED)
      -+		imply_interactive(&options, "--empty");
      -
      +-
        	if (gpg_sign) {
        		free(options.gpg_sign_opt);
      -@@
      - 		break;
      - 	}
      -
      -+	if (options.empty == EMPTY_UNSPECIFIED) {
      -+		if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
      -+			options.empty = EMPTY_ASK;
      -+		else if (exec.nr > 0)
      -+			options.empty = EMPTY_KEEP;
      -+		else
      -+			options.empty = EMPTY_DROP;
      -+	}
      - 	if (reschedule_failed_exec > 0 && !is_interactive(&options))
      - 		die(_("--reschedule-failed-exec requires "
      - 		      "--exec or --interactive"));
      + 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
       
        diff --git a/rebase-interactive.c b/rebase-interactive.c
        --- a/rebase-interactive.c
      @@ -296,19 +177,22 @@
        }
        
       -void append_todo_help(unsigned keep_empty, int command_count,
      -+void append_todo_help(unsigned no_ask_empty, int command_count,
      ++void append_todo_help(int command_count,
        		      const char *shortrevisions, const char *shortonto,
        		      struct strbuf *buf)
        {
       @@
      + 			"the rebase will be aborted.\n\n");
        
        	strbuf_add_commented_lines(buf, msg, strlen(msg));
      -
      +-
       -	if (!keep_empty) {
      -+	if (!no_ask_empty) {
      - 		msg = _("Note that empty commits are commented out");
      - 		strbuf_add_commented_lines(buf, msg, strlen(msg));
      - 	}
      +-		msg = _("Note that empty commits are commented out");
      +-		strbuf_add_commented_lines(buf, msg, strlen(msg));
      +-	}
      + }
      +
      + int edit_todo_list(struct repository *r, struct todo_list *todo_list,
       
        diff --git a/rebase-interactive.h b/rebase-interactive.h
        --- a/rebase-interactive.h
      @@ -318,7 +202,7 @@
        struct todo_list;
        
       -void append_todo_help(unsigned keep_empty, int command_count,
      -+void append_todo_help(unsigned no_ask_empty, int command_count,
      ++void append_todo_help(int command_count,
        		      const char *shortrevisions, const char *shortonto,
        		      struct strbuf *buf);
        int edit_todo_list(struct repository *r, struct todo_list *todo_list,
      @@ -327,157 +211,51 @@
        --- a/sequencer.c
        +++ b/sequencer.c
       @@
      - static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
      - static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
      - static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
      -+static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
      -+static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
      -+static GIT_PATH_FUNC(rebase_path_ask_on_initially_empty, "rebase-merge/ask_on_initially_empty")
      -
      - static int git_sequencer_config(const char *k, const char *v, void *cb)
      + 		       struct replay_opts *opts,
      + 		       struct commit *commit)
        {
      +-	int index_unchanged, empty_commit;
      ++	int index_unchanged, originally_empty;
      +
      + 	/*
      + 	 * Three cases:
       @@
      - 	empty_commit = is_original_commit_empty(commit);
      - 	if (empty_commit < 0)
      - 		return empty_commit;
      + 	if (opts->keep_redundant_commits)
      + 		return 1;
      +
      +-	empty_commit = is_original_commit_empty(commit);
      +-	if (empty_commit < 0)
      +-		return empty_commit;
       -	if (!empty_commit)
      -+	if (!empty_commit || opts->ask_on_initially_empty)
      ++	originally_empty = is_original_commit_empty(commit);
      ++	if (originally_empty < 0)
      ++		return originally_empty;
      ++	if (!originally_empty)
        		return 0;
        	else
        		return 1;
      -@@
      - 	char *author = NULL;
      - 	struct commit_message msg = { NULL, NULL, NULL, NULL };
      - 	struct strbuf msgbuf = STRBUF_INIT;
      --	int res, unborn = 0, reword = 0, allow;
      -+	int res, unborn = 0, reword = 0, allow, drop_commit;
      -
      - 	if (opts->no_commit) {
      - 		/*
      -@@
      - 		goto leave;
      - 	}
      -
      --	allow = allow_empty(r, opts, commit);
      --	if (allow < 0) {
      --		res = allow;
      --		goto leave;
      --	} else if (allow)
      --		flags |= ALLOW_EMPTY;
      --	if (!opts->no_commit) {
      -+	drop_commit = 0;
      -+	if (opts->drop_redundant_commits && is_index_unchanged(r)) {
      -+		drop_commit = 1;
      -+		fprintf(stderr, _("No changes -- Patch already applied."));
      -+	} else {
      -+		allow = allow_empty(r, opts, commit);
      -+		if (allow < 0) {
      -+			res = allow;
      -+			goto leave;
      -+		} else if (allow) {
      -+			flags |= ALLOW_EMPTY;
      -+		}
      -+	}
      -+	if (!opts->no_commit && !drop_commit) {
      - 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
      - 			res = do_commit(r, msg_file, author, opts, flags);
      - 		else
      -@@
      - 	else if (!strcmp(key, "options.allow-empty-message"))
      - 		opts->allow_empty_message =
      - 			git_config_bool_or_int(key, value, &error_flag);
      -+	else if (!strcmp(key, "options.drop-redundant-commits"))
      -+		opts->drop_redundant_commits =
      -+			git_config_bool_or_int(key, value, &error_flag);
      - 	else if (!strcmp(key, "options.keep-redundant-commits"))
      - 		opts->keep_redundant_commits =
      - 			git_config_bool_or_int(key, value, &error_flag);
      -+	else if (!strcmp(key, "options.ask_on_initially_empty"))
      -+		opts->ask_on_initially_empty =
      -+			git_config_bool_or_int(key, value, &error_flag);
      - 	else if (!strcmp(key, "options.signoff"))
      - 		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
      - 	else if (!strcmp(key, "options.record-origin"))
      -@@
      - 		if (file_exists(rebase_path_reschedule_failed_exec()))
      - 			opts->reschedule_failed_exec = 1;
      -
      -+		if (file_exists(rebase_path_drop_redundant_commits()))
      -+			opts->drop_redundant_commits = 1;
      -+
      -+		if (file_exists(rebase_path_keep_redundant_commits()))
      -+			opts->keep_redundant_commits = 1;
      -+
      -+		if (file_exists(rebase_path_ask_on_initially_empty()))
      -+			opts->ask_on_initially_empty = 1;
      -+
      - 		read_strategy_opts(opts, &buf);
      - 		strbuf_release(&buf);
      -
      -@@
      - 		write_file(rebase_path_cdate_is_adate(), "%s", "");
      - 	if (opts->ignore_date)
      - 		write_file(rebase_path_ignore_date(), "%s", "");
      -+	if (opts->drop_redundant_commits)
      -+		write_file(rebase_path_drop_redundant_commits(), "%s", "");
      -+	if (opts->keep_redundant_commits)
      -+		write_file(rebase_path_keep_redundant_commits(), "%s", "");
      -+	if (opts->ask_on_initially_empty)
      -+		write_file(rebase_path_ask_on_initially_empty(), "%s", "");
      - 	if (opts->reschedule_failed_exec)
      - 		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
      -
      -@@
      - 	if (opts->allow_empty_message)
      - 		res |= git_config_set_in_file_gently(opts_file,
      - 				"options.allow-empty-message", "true");
      -+	if (opts->drop_redundant_commits)
      -+		res |= git_config_set_in_file_gently(opts_file,
      -+				"options.drop-redundant-commits", "true");
      - 	if (opts->keep_redundant_commits)
      - 		res |= git_config_set_in_file_gently(opts_file,
      - 				"options.keep-redundant-commits", "true");
      -+	if (opts->ask_on_initially_empty)
      -+		res |= git_config_set_in_file_gently(opts_file,
      -+				"options.ask_on_initially_empty", "true");
      - 	if (opts->signoff)
      - 		res |= git_config_set_in_file_gently(opts_file,
      - 					"options.signoff", "true");
       @@
        				   struct rev_info *revs, struct strbuf *out,
        				   unsigned flags)
        {
       -	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
      -+	int drop_empty = flags & TODO_LIST_DROP_EMPTY;
      -+	int ask_empty = flags & TODO_LIST_ASK_EMPTY;
        	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
        	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
        	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
      -@@
      - 		is_empty = is_original_commit_empty(commit);
      - 		if (!is_empty && (commit->object.flags & PATCHSAME))
      - 			continue;
      -+		if (is_empty && drop_empty)
      -+			continue;
      -
      - 		strbuf_reset(&oneline);
      - 		pretty_print_commit(pp, commit, &oneline);
       @@
        		if (!to_merge) {
        			/* non-merge commit: easy case */
        			strbuf_reset(&buf);
       -			if (!keep_empty && is_empty)
      -+			if (is_empty && ask_empty)
      - 				strbuf_addf(&buf, "%c ", comment_line_char);
      +-				strbuf_addf(&buf, "%c ", comment_line_char);
        			strbuf_addf(&buf, "%s %s %s", cmd_pick,
        				    oid_to_hex(&commit->object.oid),
      + 				    oneline.buf);
       @@
        	struct pretty_print_context pp = {0};
        	struct rev_info revs;
        	struct commit *commit;
       -	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
      -+	int drop_empty = flags & TODO_LIST_DROP_EMPTY;
      -+	int ask_empty = flags & TODO_LIST_ASK_EMPTY;
        	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
        	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
        
      @@ -491,19 +269,16 @@
        		if (!is_empty && (commit->object.flags & PATCHSAME))
        			continue;
       -		if (!keep_empty && is_empty)
      -+		if (is_empty && drop_empty)
      -+			continue;
      -+		if (is_empty && ask_empty)
      - 			strbuf_addf(out, "%c ", comment_line_char);
      +-			strbuf_addf(out, "%c ", comment_line_char);
        		strbuf_addf(out, "%s %s ", insn,
        			    oid_to_hex(&commit->object.oid));
      + 		pretty_print_commit(&pp, commit, out);
       @@
        
        	todo_list_to_strbuf(r, todo_list, &buf, num, flags);
        	if (flags & TODO_LIST_APPEND_TODO_HELP)
       -		append_todo_help(flags & TODO_LIST_KEEP_EMPTY, count_commands(todo_list),
      -+		append_todo_help(!(flags & TODO_LIST_ASK_EMPTY),
      -+				 count_commands(todo_list),
      ++		append_todo_help(count_commands(todo_list),
        				 shortrevisions, shortonto, &buf);
        
        	res = write_message(buf.buf, buf.len, file, 0);
      @@ -511,16 +286,6 @@
        diff --git a/sequencer.h b/sequencer.h
        --- a/sequencer.h
        +++ b/sequencer.h
      -@@
      - 	int allow_rerere_auto;
      - 	int allow_empty;
      - 	int allow_empty_message;
      -+	int drop_redundant_commits;
      - 	int keep_redundant_commits;
      -+	int ask_on_initially_empty;
      - 	int verbose;
      - 	int quiet;
      - 	int reschedule_failed_exec;
       @@
        int sequencer_skip(struct repository *repo, struct replay_opts *opts);
        int sequencer_remove_state(struct replay_opts *opts);
      @@ -530,19 +295,34 @@
        #define TODO_LIST_SHORTEN_IDS (1U << 1)
        #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
        #define TODO_LIST_REBASE_MERGES (1U << 3)
      -@@
      -  * `--onto`, we do not want to re-generate the root commits.
      -  */
      - #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
      -+#define TODO_LIST_DROP_EMPTY (1U << 7)
      -+#define TODO_LIST_ASK_EMPTY (1U << 8)
      -
      -
      - int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
       
        diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
        --- a/t/t3421-rebase-topology-linear.sh
        +++ b/t/t3421-rebase-topology-linear.sh
      +@@
      + test_run_rebase () {
      + 	result=$1
      + 	shift
      +-	test_expect_$result "rebase $* drops empty commit" "
      ++	test_expect_$result "rebase $* keeps begin-empty commits" "
      + 		reset_rebase &&
      +-		git rebase $* c l &&
      +-		test_cmp_rev c HEAD~2 &&
      +-		test_linear_range 'd l' c..
      ++		git rebase $* j l &&
      ++		test_cmp_rev c HEAD~4 &&
      ++		test_linear_range 'j d k l' c..
      + 	"
      + }
      +-test_run_rebase success ''
      ++test_run_rebase failure ''
      + test_run_rebase success -m
      + test_run_rebase success -i
      +-test_have_prereq !REBASE_P || test_run_rebase success -p
      ++test_have_prereq !REBASE_P || test_run_rebase failure -p
      +
      + test_run_rebase () {
      + 	result=$1
       @@
        test_run_rebase success ''
        test_run_rebase success -m
      @@ -603,31 +383,22 @@
       +	git commit -m "Five letters ought to be enough for anybody"
       +'
       +
      -+test_expect_success 'rebase --merge --empty=drop' '
      ++test_expect_failure 'rebase (am-backend) with a variety of empty commits' '
      ++	test_when_finished "git rebase --abort" &&
       +	git checkout -B testing localmods &&
      -+	git rebase --merge --empty=drop upstream &&
      -+
      -+	test_write_lines C B A >expect &&
      -+	git log --format=%s >actual &&
      -+	test_cmp expect actual
      -+'
      ++	# rebase (--am) should not drop commits that start empty
      ++	git rebase upstream &&
       +
      -+test_expect_success 'rebase --merge --empty=keep' '
      -+	git checkout -B testing localmods &&
      -+	git rebase --merge --empty=keep upstream &&
      -+
      -+	test_write_lines D C2 C B A >expect &&
      ++	test_write_lines D C B A >expect &&
       +	git log --format=%s >actual &&
       +	test_cmp expect actual
       +'
       +
      -+test_expect_success 'rebase --merge --empty=ask' '
      ++test_expect_failure 'rebase --merge with a variety of empty commits' '
      ++	test_when_finished "git rebase --abort" &&
       +	git checkout -B testing localmods &&
      -+	test_must_fail git rebase --merge --empty=ask upstream &&
      -+
      -+	test_must_fail git rebase --skip &&
      -+	git commit --allow-empty &&
      -+	git rebase --continue &&
      ++	# rebase --merge should not halt on the commit that becomes empty
      ++	git rebase --merge upstream &&
       +
       +	test_write_lines D C B A >expect &&
       +	git log --format=%s >actual &&
      @@ -636,25 +407,17 @@
       +
       +GIT_SEQUENCE_EDITOR=: && export GIT_SEQUENCE_EDITOR
       +
      -+test_expect_success 'rebase --interactive --empty=drop' '
      ++test_expect_success 'rebase --interactive with a variety of empty commits' '
       +	git checkout -B testing localmods &&
      -+	git rebase --interactive --empty=drop upstream &&
      ++	test_must_fail git rebase --interactive upstream &&
       +
      -+	test_write_lines C B A >expect &&
      -+	git log --format=%s >actual &&
      -+	test_cmp expect actual
      -+'
      -+
      -+test_expect_success 'rebase --interactive --empty=keep' '
      -+	git checkout -B testing localmods &&
      -+	git rebase --interactive --empty=keep upstream &&
      ++	git rebase --skip &&
       +
      -+	test_write_lines D C2 C B A >expect &&
      ++	test_write_lines D C B A >expect &&
       +	git log --format=%s >actual &&
       +	test_cmp expect actual
       +'
       +
      -+
       +test_done
       
        diff --git a/t/t3427-rebase-subtree.sh b/t/t3427-rebase-subtree.sh
      @@ -665,32 +428,28 @@
        '
        
       -test_expect_success 'Rebase -Xsubtree --keep-empty --onto commit' '
      -+test_expect_success 'Rebase -Xsubtree --empty=ask --onto commit' '
      ++test_expect_success 'Rebase -Xsubtree --onto commit' '
        	reset_rebase &&
        	git checkout -b rebase-onto to-rebase &&
       -	test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --onto files-master master &&
      -+	test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --onto files-master master &&
      ++	test_must_fail git rebase -Xsubtree=files_subtree --onto files-master master &&
        	: first pick results in no changes &&
       -	git rebase --continue &&
      -+	test_must_fail git rebase --skip &&
      -+	: last pick was an empty commit that has no changes, but we want to keep it &&
      -+	git commit --allow-empty &&
      ++	git rebase --skip &&
        	verbose test "$(commit_message HEAD~2)" = "master4" &&
        	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
        	verbose test "$(commit_message HEAD)" = "Empty commit"
        '
        
       -test_expect_success 'Rebase -Xsubtree --keep-empty --rebase-merges --onto commit' '
      -+test_expect_success 'Rebase -Xsubtree --empty=ask --rebase-merges --onto commit' '
      ++test_expect_success 'Rebase -Xsubtree --rebase-merges --onto commit' '
        	reset_rebase &&
        	git checkout -b rebase-merges-onto to-rebase &&
       -	test_must_fail git rebase -Xsubtree=files_subtree --keep-empty --rebase-merges --onto files-master --root &&
      -+	test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --rebase-merges --onto files-master --root &&
      ++	test_must_fail git rebase -Xsubtree=files_subtree --rebase-merges --onto files-master --root &&
        	: first pick results in no changes &&
       -	git rebase --continue &&
      -+	test_must_fail git rebase --skip &&
      -+	: last pick was an empty commit that has no changes, but we want to keep it &&
      -+	git commit --allow-empty &&
      ++	git rebase --skip &&
        	verbose test "$(commit_message HEAD~2)" = "master4" &&
        	verbose test "$(commit_message HEAD~)" = "files_subtree/master5" &&
        	verbose test "$(commit_message HEAD)" = "Empty commit"
   -:  ---------- >  4:  c9542a2abe rebase (interactive-backend): fix handling of commits that become empty
   2:  bd3c5ec155 =  5:  9f66229d5c t3406: simplify an already simple test
   3:  49388b79fd =  6:  8d731fa39c rebase, sequencer: remove the broken GIT_QUIET handling
   4:  478479358f !  7:  b6b6597eef rebase: make sure to pass along the quiet flag to the sequencer
      @@ -8,13 +8,13 @@
        --- a/builtin/rebase.c
        +++ b/builtin/rebase.c
       @@
      + 	replay.allow_empty_message = opts->allow_empty_message;
      + 	replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);
        	replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
      - 	replay.ask_on_initially_empty = (opts->empty == EMPTY_ASK &&
      - 					 !(opts->flags & REBASE_INTERACTIVE_EXPLICIT));
       +	replay.quiet = !(opts->flags & REBASE_NO_QUIET);
        	replay.verbose = opts->flags & REBASE_VERBOSE;
        	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
      - 	replay.committer_date_is_author_date =
      + 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
       @@
        			 N_("allow pre-rebase hook to run")),
        		OPT_NEGBIT('q', "quiet", &options.flags,
   5:  ee26f5a161 =  8:  0acefa988b rebase: fix handling of restrict_revision
   6:  34a69def33 =  9:  8c5b5b5133 t3432: make these tests work with either am or merge backends
   7:  f2c92853b4 ! 10:  b8c087d6fb rebase: allow more types of rebases to fast-forward
      @@ -35,8 +35,8 @@
        		OPT_STRING(0, "onto", &options.onto_name,
        			   N_("revision"),
       @@
      - 	    options.ignore_date)
      - 		options.flags |= REBASE_FORCE;
      + 		    state_dir_base, cmd_live_rebase, buf.buf);
      + 	}
        
       +	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
       +	    (action != ACTION_NONE) ||
      @@ -47,7 +47,9 @@
       +
        	for (i = 0; i < options.git_am_opts.argc; i++) {
        		const char *option = options.git_am_opts.argv[i], *p;
      - 		if (!strcmp(option, "--whitespace=fix") ||
      + 		if (!strcmp(option, "--committer-date-is-author-date") ||
      + 		    !strcmp(option, "--ignore-date") ||
      + 		    !strcmp(option, "--whitespace=fix") ||
        		    !strcmp(option, "--whitespace=strip"))
       -			options.flags |= REBASE_FORCE;
       +			allow_preemptive_ff = 0;

8: b307340f7c ! 11: b50a1741e0 git-rebase.txt: add more details about behavioral differences of backends
@@ -8,23 +8,24 @@
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@
- with --keep-base in order to drop those commits from your branch.

--ignore-whitespace::
-- Behaves differently depending on which backend is selected.
--+
--'am' backend: When applying a patch, ignore changes in whitespace in --context lines if necessary.
--+
--'interactive' backend: Treat lines with only whitespace changes as --unchanged for the sake of a three-way merge.
-+ Ignore whitespace-only changes in the commits being rebased,
-+ which may avoid "unnecessary" conflicts. (Both backends
-+ currently have differing edgecase bugs with this option; see
-+ BEHAVIORAL DIFFERENCES.)
-
--whitespace=<option>::
- This flag is passed to the 'git apply' program
+- These flag are passed to the 'git apply' program
++ These flags are passed to the 'git apply' program
+ (see linkgit:git-apply[1]) that applies the patch.
+ +
+ See also INCOMPATIBLE OPTIONS below.
+@@
+
+ * --committer-date-is-author-date
+ * --ignore-date
+- * --whitespace
+ * --ignore-whitespace
++ * --whitespace
+ * -C
+
+ are incompatible with the following options:
@@
Directory rename detection
~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -82,24 +83,14 @@
+The interactive backend works with the full commits on both sides of +history and thus has no such limitations.
+
-+--ignore-whitespace
-+~~~~~~~~~~~~~~~~~~~
++Hooks
++~~~~~
+
-+The --ignore-whitespace option is supposed to ignore whitespace-only -+changes if it allows the code to merge cleanly. Unfortunately, the -+different backends implement this differently, and both have different -+edge case bugs.
-++
-+'am' backend: When applying a patch, ignore changes in whitespace in -+context lines if necessary. (Which implies that if the whitespace -+change was not in the context lines but on a line with a real change, -+then the rebase will still fail with "unnecessary" content conflicts.)
-++
-+'interactive' backend: Treat lines with only whitespace changes as -+unchanged for the sake of a three-way merge. This means that if one -+side made no changes and the commits being rebased had whitespace-only -+changes, those whitespaces fixups will be discarded despite the fact -+that they present no content conflict.
++The am backend has not traditionally called the post-commit hook, ++while the merge/interactive backend has. However, this was by ++accident of implementation rather than by design. Both backends ++should have the same behavior, though it is not clear which one is ++correct.
+
+Miscellaneous differences
+~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -123,19 +114,3 @@

include::merge-strategies.txt[]

-
- diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
- --- a/t/t3433-rebase-options-compatibility.sh
- +++ b/t/t3433-rebase-options-compatibility.sh
-@@
- GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
- export GIT_AUTHOR_DATE
-
--# This is a special case in which both am and interactive backends
--# provide the same output. It was done intentionally because
--# both the backends fall short of optimal behaviour.
-+# This is a common case in which both am and interactive backends
-+# provide the same output with --ignore-whitespace.
- test_expect_success 'setup' '
- git checkout -b topic &&
- q_to_tab >file <<-\EOF &&

   9:  7c3f2e07f3 = 12:  58e6e4ffb3 rebase: move incompatibility checks between backend options a bit earlier
  10:  1df11f0b51 ! 13:  5478c730ac rebase: add an --am option

@@ -24,8 +24,8 @@
+See also INCOMPATIBLE OPTIONS below.
+
--empty={drop,keep,ask}::
- How to handle commits that become empty (because they contain a
- subset of already upstream changes) or start empty. With drop
+ How to handle commits that are not empty to start and are not
+ clean cherry-picks of any upstream commit, but which become
@@
Ensure at least <n> lines of surrounding context match before and after each change. When fewer lines of surrounding
@@ -37,7 +37,7 @@

@@
--whitespace=<option>::
- This flag is passed to the 'git apply' program
+ These flags are passed to the 'git apply' program
(see linkgit:git-apply[1]) that applies the patch.
+ Implies --am.
+
@@ -48,9 +48,9 @@
The following options:

+ * --am
- * --whitespace
- * -C
-
+ * --committer-date-is-author-date
+ * --ignore-date
+ * --ignore-whitespace

diff --git a/builtin/rebase.c b/builtin/rebase.c --- a/builtin/rebase.c

  11:  ff43593211 = 14:  db5e29bd81 git-prompt: change the prompt for interactive-based rebases
   -:  ---------- > 15:  413e190ac9 rebase: drop '-i' from the reflog for interactive-based rebases
  12:  99388f24e5 ! 16:  170be283a8 rebase tests: mark tests specific to the am-backend with --am

@@ -228,40 +228,6 @@
test_rebase_same_head_ $status_f $what_f $cmp_f " --merge --no-ff" "$*"
}

- diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
- --- a/t/t3433-rebase-options-compatibility.sh
- +++ b/t/t3433-rebase-options-compatibility.sh
-@@
- new line 2
- line 3
- EOF
-- test_must_fail git rebase main side &&
-+ test_must_fail git rebase --am main side &&
- git rebase --abort &&
-- git rebase --ignore-whitespace main side &&
-+ git rebase --am --ignore-whitespace main side &&
- test_cmp expect file
- '
-
-@@
-
- test_expect_success '--committer-date-is-author-date works with am backend' '
- git commit --amend &&
-- git rebase --committer-date-is-author-date HEAD^ &&
-+ git rebase --am --committer-date-is-author-date HEAD^ &&
- git show HEAD --pretty="format:%ai" >authortime &&
- git show HEAD --pretty="format:%ci" >committertime &&
- test_cmp authortime committertime
-@@
- # sets to +0530.
- test_expect_success '--ignore-date works with am backend' '
- git commit --amend --date="$GIT_AUTHOR_DATE" &&
-- git rebase --ignore-date HEAD^ &&
-+ git rebase --am --ignore-date HEAD^ &&
- git show HEAD --pretty="format:%ai" >authortime &&
- grep "+0000" authortime
- '
-
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh

  13:  c2ba6317bf = 17:  1e3d4066c4 rebase tests: repeat some tests using the merge backend instead of am
  14:  8bec6df51a = 18:  9b4ac83d2d rebase: make the backend configurable via config setting
  15:  044853fd61 = 19:  859a4a94d7 rebase: change the default backend from "am" to "merge"

Quoted 6 lines

Changes possibly missing from this version, for discussion:

* I did not remove the --am option as suggested by Phillip, since Junio and Phillip were still discussing whether it is wanted/needed.
* I did not address the last two items Jonathan brought up as I couldn't find enough information to reproduce or understand the problems.

I think I've got a fix for the failure with --committer-date-is-author-date that Jonathan reported which I'll post next week - the bug was not in this series, it was just exposed by it. I'll try and read through this series next week as well.

Best Wishes

Quoted 12 lines · Phillip Wood

Quoted 6 lines

Changes possibly missing from this version, for discussion:

* I did not remove the --am option as suggested by Phillip, since Junio and Phillip were still discussing whether it is wanted/needed.
* I did not address the last two items Jonathan brought up as I couldn't find enough information to reproduce or understand the problems.

I think I've got a fix for the failure with --committer-date-is-author-date that Jonathan reported which I'll post next week - the bug was not in this series, it was just exposed by it. I'll try and read through this series next week as well.

Quoted 1 line

Best Wishes

Thanks.

As to the "--am" option, I do not care too deeply about it anymore, so if there is nothing else that we need to further polish, should we move this forward to 'next' soonish?