[v2,2/3] rebase: reinstate --no-keep-empty

Commit Message

John Passaro via GitGitGadget April 10, 2020, 5:51 p.m. UTC

From: Elijah Newren <[e-mail]>

Commit d48e5e21da ("rebase (interactive-backend): make --keep-empty the
default", 2020-02-15) turned --keep-empty (for keeping commits which
start empty) into the default.  That commit viewed this, though as
turning that flag into a no-op.  As such, --no-keep-empty was translated
into undoing a no-op, i.e. also a no-op.  The logic underpinning that
commit was:

  1) 'git commit' errors out on the creation of empty commits without an
     override flag
  2) Once someone determines that the override is worthwhile, it's
     annoying and/or harmful to required them to take extra steps in
     order to keep such commits around (and to repeat such steps with
     every rebase).

While the logic on which the decision was made is sound, the result was
a bit of an overcorrection.  Instead of jumping to having --keep-empty
being the default, it jumped to making --keep-empty the only available
behavior.  There was a simple workaround, though, which was thought to
be good enough at the time.  People could still drop commits which
started empty the same way the could drop any commits: by firing up an
interactive rebase and picking out the commits they didn't want from the
list.  However, there are cases where external tools might create enough
empty commits that picking all of them out is painful.  As such, having
a flag to automatically remove start-empty commits may be beneficial.

Provide users a way to drop commits which start empty using a flag that
existed for years: --no-keep-empty.  Interpret --keep-empty as
countermanding any previous --no-keep-empty, but otherwise leaving
--keep-empty as the default.

This might lead to some slight weirdness since commands like
  git rebase --empty=drop --keep-empty
  git rebase --empty=keep --no-keep-empty
look really weird despite making perfect sense (the first will drop
commits which become empty, but keep commits that started empty; the
second will keep commits which become empty, but drop commits which
started empty).

Reported-by: Bryan Turner <[e-mail]>
Reported-by: Sami Boukortt <[e-mail]>
Signed-off-by: Elijah Newren <[e-mail]>
---
 Documentation/git-rebase.txt      | 37 +++++++++++++++++--------------
 builtin/rebase.c                  | 15 ++++++++-----
 sequencer.c                       |  6 +++++
 sequencer.h                       |  2 +-
 t/t3421-rebase-topology-linear.sh | 10 ++++-----
 t/t3424-rebase-empty.sh           | 36 ++++++++++++++++++++++++++++++
 6 files changed, 76 insertions(+), 30 deletions(-)

Comments

Junio C Hamano April 10, 2020, 8:37 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> That commit viewed this, though as
> turning that flag into a no-op.

Sorry, but I do not understand this sentence.

> Provide users a way to drop commits which start empty using a flag that
> existed for years: --no-keep-empty.  Interpret --keep-empty as
> countermanding any previous --no-keep-empty, but otherwise leaving
> --keep-empty as the default.

But everything after that sentence down to here was very clear.

> This might lead to some slight weirdness since commands like
>   git rebase --empty=drop --keep-empty
>   git rebase --empty=keep --no-keep-empty
> look really weird despite making perfect sense (the first will drop
> commits which become empty, but keep commits that started empty; the
> second will keep commits which become empty, but drop commits which
> started empty).

That is true.  Do we leave it to others (or our later selves) to
think about the UI further?  That is fine by me, but in that case we
may want to add " We may want to rethink the option names later",
perhaps?

> +--no-keep-empty::
>  --keep-empty::
> +	Do not keep commits that start empty before the rebase
> +	(i.e. that do not change anything from its parent) in the
> +	result.  For commits which become empty after rebasing, see
> +	the --empty and --keep-cherry-pick flags.

keep-cherry-pick will appear later, not here, right?

Thanks.

Elijah Newren April 10, 2020, 9:41 p.m. UTC | #2

On Fri, Apr 10, 2020 at 1:38 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > That commit viewed this, though as
> > turning that flag into a no-op.
>
> Sorry, but I do not understand this sentence.

Hmm...it wouldn't hurt to just drop the two sentences with "no-op" in them.

> > Provide users a way to drop commits which start empty using a flag that
> > existed for years: --no-keep-empty.  Interpret --keep-empty as
> > countermanding any previous --no-keep-empty, but otherwise leaving
> > --keep-empty as the default.
>
> But everything after that sentence down to here was very clear.

:-)

> > This might lead to some slight weirdness since commands like
> >   git rebase --empty=drop --keep-empty
> >   git rebase --empty=keep --no-keep-empty
> > look really weird despite making perfect sense (the first will drop
> > commits which become empty, but keep commits that started empty; the
> > second will keep commits which become empty, but drop commits which
> > started empty).
>
> That is true.  Do we leave it to others (or our later selves) to
> think about the UI further?  That is fine by me, but in that case we
> may want to add " We may want to rethink the option names later",
> perhaps?

The names might not be great, but since --no-keep-empty and
--keep-empty already existed for years and prior to 2.16 and were
about how to handle commits which started empty, it seemed reasonable
to (re)use them.  My personal preference would be that for commits
whose keep/drop state can be determined solely by looking at its
contents of the commit before the rebase (whether that's because they
start empty or for whatever other reasons), I'd rather recommend that
people just fire up an interactive rebase and pick out the commits
they don't want to keep.  So, my own bias would be that I wouldn't
recommend that new people use either --keep-empty or --no-keep-empty.
In my mind, the --[no-]keep-empty flags are just for backward
compatibility, so going through effort to rename or alias doesn't seem
like it makes a lot of sense.

I don't have a strongly held position here and I'm happy to hear
alternate viewpoints, but that's the thought process I went through
when creating this change.

> > +--no-keep-empty::
> >  --keep-empty::
> > +     Do not keep commits that start empty before the rebase
> > +     (i.e. that do not change anything from its parent) in the
> > +     result.  For commits which become empty after rebasing, see
> > +     the --empty and --keep-cherry-pick flags.
>
> keep-cherry-pick will appear later, not here, right?

--keep-cherry-pick is the new flag added by jt/rebase-allow-duplicate.
I guess I should technically probably wait until I create a commit on
top of that series and this one to mention it.  Or rebase Jonathan's
commit, and combine the two series (adding his second since mine might
be viewed as a bugfix).

Patch

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8ab0558aca2..e58ca37786c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -277,20 +277,21 @@  See also INCOMPATIBLE OPTIONS below.
 	Other options, like --exec, will use the default of drop unless
 	-i/--interactive is explicitly specified.
 +
-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.
+Note that commits which start empty are kept (unless --no-keep-empty
+is specified), and commits which are clean cherry-picks (as determined
+by `git log --cherry-mark ...`) are detected and dropped as a preliminary
+step (unless --keep-cherry-picks is passed).
 +
 See also INCOMPATIBLE OPTIONS below.
 
+--no-keep-empty::
 --keep-empty::
-	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.
+	Do not keep commits that start empty before the rebase
+	(i.e. that do not change anything from its parent) in the
+	result.  For commits which become empty after rebasing, see
+	the --empty and --keep-cherry-pick flags.
 +
-See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.
+See also INCOMPATIBLE OPTIONS below.
 
 --allow-empty-message::
 	No-op.  Rebasing commits with an empty message used to fail
@@ -587,7 +588,7 @@  are incompatible with the following options:
  * --preserve-merges
  * --interactive
  * --exec
- * --keep-empty
+ * --no-keep-empty
  * --empty=
  * --edit-todo
  * --root when used in combination with --onto
@@ -620,13 +621,15 @@  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 merge backend keeps intentionally empty commits (though with -i
-they are marked as empty in the todo list editor).  Similar to the
-apply backend, by default the merge backend drops commits that become
-empty unless -i/--interactive is specified (in which case it stops and
-asks the user what to do).  The merge backend also has an
---empty={drop,keep,ask} option for changing the behavior of handling
-commits that become empty.
+The merge backend keeps intentionally empty commits by default (though
+with -i they are marked as empty in the todo list editor, or they can
+be dropped automatically with --no-keep-empty).
+
+Similar to the apply backend, by default the merge backend drops
+commits that become empty unless -i/--interactive is specified (in
+which case it stops and asks the user what to do).  The merge backend
+also has an --empty={drop,keep,ask} option for changing the behavior
+of handling commits that become empty.
 
 Directory rename detection
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/rebase.c b/builtin/rebase.c
index bff53d5d167..022aa2589a5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -85,6 +85,7 @@  struct rebase_options {
 	const char *action;
 	int signoff;
 	int allow_rerere_autoupdate;
+	int keep_empty;
 	int autosquash;
 	char *gpg_sign_opt;
 	int autostash;
@@ -100,6 +101,7 @@  struct rebase_options {
 #define REBASE_OPTIONS_INIT {			  	\
 		.type = REBASE_UNSPECIFIED,	  	\
 		.empty = EMPTY_UNSPECIFIED,	  	\
+		.keep_empty = 1,			\
 		.default_backend = "merge",	  	\
 		.flags = REBASE_NO_QUIET, 		\
 		.git_am_opts = ARGV_ARRAY_INIT,		\
@@ -379,6 +381,7 @@  static int run_sequencer_rebase(struct rebase_options *opts,
 
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
+	flags |= opts->keep_empty ? TODO_LIST_KEEP_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;
@@ -442,6 +445,7 @@  static int run_sequencer_rebase(struct rebase_options *opts,
 	return ret;
 }
 
+static void imply_merge(struct rebase_options *opts, const char *option);
 static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 				int unset)
 {
@@ -449,10 +453,8 @@  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;
-	 */
+	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+	opts->keep_empty = !unset;
 	opts->type = REBASE_MERGE;
 	return 0;
 }
@@ -471,7 +473,7 @@  int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
 			   REBASE_FORCE),
 		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
-			N_("(DEPRECATED) keep empty commits"),
+			N_("keep commits which start empty"),
 			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
 			parse_opt_keep_empty },
 		OPT_BOOL_F(0, "allow-empty-message", &opts.allow_empty_message,
@@ -1162,6 +1164,7 @@  static int run_specific_rebase(struct rebase_options *opts, enum action action)
 		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, "autosquash", opts->autosquash ? "t" : "");
 	add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);
 	add_var(&script_snippet, "cmd", opts->cmd);
@@ -1547,7 +1550,7 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 			       N_("how to handle commits that become empty"),
 			       PARSE_OPT_NONEG, parse_opt_empty),
 		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
-			N_("(DEPRECATED) keep empty commits"),
+			N_("keep commits which start empty"),
 			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
 			parse_opt_keep_empty },
 		OPT_BOOL(0, "autosquash", &options.autosquash,
diff --git a/sequencer.c b/sequencer.c
index ce9fd27a878..d973e19729e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4591,6 +4591,7 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 				   struct rev_info *revs, struct strbuf *out,
 				   unsigned flags)
 {
+	int keep_empty = flags & TODO_LIST_KEEP_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;
@@ -4645,6 +4646,8 @@  static int make_script_with_merges(struct pretty_print_context *pp,
 		is_empty = is_original_commit_empty(commit);
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
+		if (is_empty && !keep_empty)
+			continue;
 
 		strbuf_reset(&oneline);
 		pretty_print_commit(pp, commit, &oneline);
@@ -4822,6 +4825,7 @@  int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	struct pretty_print_context pp = {0};
 	struct rev_info revs;
 	struct commit *commit;
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
 
@@ -4861,6 +4865,8 @@  int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
+		if (is_empty && !keep_empty)
+			continue;
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, out);
diff --git a/sequencer.h b/sequencer.h
index 718a07426da..7ebaa237340 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -134,7 +134,7 @@  int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-/* #define TODO_LIST_KEEP_EMPTY (1U << 0) */ /* No longer used */
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
 #define TODO_LIST_SHORTEN_IDS (1U << 1)
 #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
 #define TODO_LIST_REBASE_MERGES (1U << 3)
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index cf8dfd6c203..4a9204b4b64 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -220,14 +220,13 @@  test_have_prereq !REBASE_P || test_run_rebase failure -p
 test_run_rebase () {
 	result=$1
 	shift
-	test_expect_$result "rebase $* --keep-empty" "
+	test_expect_$result "rebase $* --no-keep-empty drops begin-empty commits" "
 		reset_rebase &&
-		git rebase $* --keep-empty c l &&
-		test_cmp_rev c HEAD~3 &&
-		test_linear_range 'd k l' c..
+		git rebase $* --no-keep-empty c l &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'd l' c..
 	"
 }
-test_run_rebase success --apply
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -242,7 +241,6 @@  test_run_rebase () {
 		test_linear_range 'd k l' j..
 	"
 }
-test_run_rebase success --apply
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index e1e30517ea6..5e1045a0afc 100755
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -123,6 +123,42 @@  test_expect_success 'rebase --interactive uses default of --empty=ask' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase --merge --empty=drop --keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=drop --keep-empty upstream &&
+
+	test_write_lines D C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=drop --no-keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=drop --no-keep-empty upstream &&
+
+	test_write_lines C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep --keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep --keep-empty 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=keep --no-keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep --no-keep-empty upstream &&
+
+	test_write_lines C2 C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase --merge does not leave state laying around' '
 	git checkout -B testing localmods~2 &&
 	git rebase --merge upstream &&