@matheusbsbs / matheus-bagliano / commits / b9cbd2958f

Fork 0

rebase: reinstate --no-keep-empty

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. 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). However, --no-keep-empty was named years ago and we are predominantly keeping it for backward compatibility; also we suspect it will only be used rarely since folks already have a simple way to drop commits they don't want with an interactive rebase. Reported-by: Bryan Turner <bturner@atlassian.com> Reported-by: Sami Boukortt <sami@boukortt.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Apr 11, 2020 at 02:44 UTC b9cbd2958f235614e9af687691c96bb001945f86

6 files changed +87 -30

Documentation/git-rebase.txt

+31 -17

@@ -277,20 +277,32 @@ See also INCOMPATIBLE OPTIONS below.

277 Other options, like --exec, will use the default of drop unless

278 -i/--interactive is explicitly specified.

279 +

280 -Note that commits which start empty are kept, and commits which are

281 -clean cherry-picks (as determined by `git log --cherry-mark ...`) are

282 -always dropped.

280 +Note that commits which start empty are kept (unless --no-keep-empty

281 +is specified), and commits which are clean cherry-picks (as determined

282 +by `git log --cherry-mark ...`) are always dropped.

283 +

284 See also INCOMPATIBLE OPTIONS below.

285

286 +--no-keep-empty::

287 --keep-empty::

287 - No-op. Rebasing commits that started empty (had no change

288 - relative to their parent) used to fail and this option would

289 - override that behavior, allowing commits with empty changes to

290 - be rebased. Now commits with no changes do not cause rebasing

291 - to halt.

288 + Do not keep commits that start empty before the rebase

289 + (i.e. that do not change anything from its parent) in the

290 + result. The default is to keep commits which start empty,

291 + since creating such commits requires passing the --allow-empty

292 + override flag to `git commit`, signifying that a user is very

293 + intentionally creating such a commit and thus wants to keep

294 + it.

295 +

293 -See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.

296 +Usage of this flag will probably be rare, since you can get rid of

297 +commits that start empty by just firing up an interactive rebase and

298 +removing the lines corresponding to the commits you don't want. This

299 +flag exists as a convenient shortcut, such as for cases where external

300 +tools generate many empty commits and you want them all removed.

301 ++

302 +For commits which do not start empty but become empty after rebasing,

303 +see the --empty flag.

304 ++

305 +See also INCOMPATIBLE OPTIONS below.

306

307 --allow-empty-message::

308 No-op. Rebasing commits with an empty message used to fail

@@ -587,7 +599,7 @@ are incompatible with the following options:

599 * --preserve-merges

600 * --interactive

601 * --exec

590 - * --keep-empty

602 + * --no-keep-empty

603 * --empty=

604 * --edit-todo

605 * --root when used in combination with --onto

@@ -620,13 +632,15 @@ commits that started empty, though these are rare in practice. It

632 also drops commits that become empty and has no option for controlling

633 this behavior.

634

623 -The merge backend keeps intentionally empty commits (though with -i

624 -they are marked as empty in the todo list editor). Similar to the

625 -apply backend, by default the merge backend drops commits that become

626 -empty unless -i/--interactive is specified (in which case it stops and

627 -asks the user what to do). The merge backend also has an

628 ---empty={drop,keep,ask} option for changing the behavior of handling

629 -commits that become empty.

635 +The merge backend keeps intentionally empty commits by default (though

636 +with -i they are marked as empty in the todo list editor, or they can

637 +be dropped automatically with --no-keep-empty).

638 +

639 +Similar to the apply backend, by default the merge backend drops

640 +commits that become empty unless -i/--interactive is specified (in

641 +which case it stops and asks the user what to do). The merge backend

642 +also has an --empty={drop,keep,ask} option for changing the behavior

643 +of handling commits that become empty.

644

645 Directory rename detection

646 ~~~~~~~~~~~~~~~~~~~~~~~~~~

builtin/rebase.c

+9 -6

@@ -85,6 +85,7 @@ struct rebase_options {

85 const char *action;

86 int signoff;

87 int allow_rerere_autoupdate;

88 + int keep_empty;

89 int autosquash;

90 char *gpg_sign_opt;

91 int autostash;

@@ -100,6 +101,7 @@ struct rebase_options {

101 #define REBASE_OPTIONS_INIT { \

102 .type = REBASE_UNSPECIFIED, \

103 .empty = EMPTY_UNSPECIFIED, \

104 + .keep_empty = 1, \

105 .default_backend = "merge", \

106 .flags = REBASE_NO_QUIET, \

107 .git_am_opts = ARGV_ARRAY_INIT, \

@@ -379,6 +381,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,

381

382 git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);

383

384 + flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0;

385 flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;

386 flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;

387 flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;

@@ -442,6 +445,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,

445 return ret;

446 }

447

448 +static void imply_merge(struct rebase_options *opts, const char *option);

449 static int parse_opt_keep_empty(const struct option *opt, const char *arg,

450 int unset)

451 {

@@ -449,10 +453,8 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,

453

454 BUG_ON_OPT_ARG(arg);

455

452 - /*

453 - * If we ever want to remap --keep-empty to --empty=keep, insert:

454 - * opts->empty = unset ? EMPTY_UNSPECIFIED : EMPTY_KEEP;

455 - */

456 + imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");

457 + opts->keep_empty = !unset;

458 opts->type = REBASE_MERGE;

459 return 0;

460 }

@@ -471,7 +473,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)

473 OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),

474 REBASE_FORCE),

475 { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,

474 - N_("(DEPRECATED) keep empty commits"),

476 + N_("keep commits which start empty"),

477 PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,

478 parse_opt_keep_empty },

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

1164 opts->allow_rerere_autoupdate ?

1165 opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?

1166 "--rerere-autoupdate" : "--no-rerere-autoupdate" : "");

1167 + add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : "");

1168 add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : "");

1169 add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);

1170 add_var(&script_snippet, "cmd", opts->cmd);

@@ -1547,7 +1550,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)

1550 N_("how to handle commits that become empty"),

1551 PARSE_OPT_NONEG, parse_opt_empty),

1552 { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,

1550 - N_("(DEPRECATED) keep empty commits"),

1553 + N_("keep commits which start empty"),

1554 PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,

1555 parse_opt_keep_empty },

1556 OPT_BOOL(0, "autosquash", &options.autosquash,

sequencer.c

@@ -4591,6 +4591,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,

4591 struct rev_info *revs, struct strbuf *out,

4592 unsigned flags)

4593 {

4594 + int keep_empty = flags & TODO_LIST_KEEP_EMPTY;

4595 int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;

4596 int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;

4597 struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;

@@ -4645,6 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,

4646 is_empty = is_original_commit_empty(commit);

4647 if (!is_empty && (commit->object.flags & PATCHSAME))

4648 continue;

4649 + if (is_empty && !keep_empty)

4650 + continue;

4651

4652 strbuf_reset(&oneline);

4653 pretty_print_commit(pp, commit, &oneline);

@@ -4822,6 +4825,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,

4825 struct pretty_print_context pp = {0};

4826 struct rev_info revs;

4827 struct commit *commit;

4828 + int keep_empty = flags & TODO_LIST_KEEP_EMPTY;

4829 const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";

4830 int rebase_merges = flags & TODO_LIST_REBASE_MERGES;

4831

@@ -4861,6 +4865,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,

4865

4866 if (!is_empty && (commit->object.flags & PATCHSAME))

4867 continue;

4868 + if (is_empty && !keep_empty)

4869 + continue;

4870 strbuf_addf(out, "%s %s ", insn,

4871 oid_to_hex(&commit->object.oid));

4872 pretty_print_commit(&pp, commit, out);

sequencer.h

+1 -1

@@ -134,7 +134,7 @@ int sequencer_rollback(struct repository *repo, struct replay_opts *opts);

134 int sequencer_skip(struct repository *repo, struct replay_opts *opts);

135 int sequencer_remove_state(struct replay_opts *opts);

136

137 -/* #define TODO_LIST_KEEP_EMPTY (1U << 0) */ /* No longer used */

137 +#define TODO_LIST_KEEP_EMPTY (1U << 0)

138 #define TODO_LIST_SHORTEN_IDS (1U << 1)

139 #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)

140 #define TODO_LIST_REBASE_MERGES (1U << 3)

t/t3421-rebase-topology-linear.sh

+4 -6

@@ -220,14 +220,13 @@ test_have_prereq !REBASE_P || test_run_rebase failure -p

220 test_run_rebase () {

221 result=$1

222 shift

223 - test_expect_$result "rebase $* --keep-empty" "

223 + test_expect_$result "rebase $* --no-keep-empty drops begin-empty commits" "

224 reset_rebase &&

225 - git rebase $* --keep-empty c l &&

226 - test_cmp_rev c HEAD~3 &&

227 - test_linear_range 'd k l' c..

225 + git rebase $* --no-keep-empty c l &&

226 + test_cmp_rev c HEAD~2 &&

227 + test_linear_range 'd l' c..

228 "

229 }

230 -test_run_rebase success --apply

230 test_run_rebase success -m

231 test_run_rebase success -i

232 test_have_prereq !REBASE_P || test_run_rebase success -p

@@ -242,7 +241,6 @@ test_run_rebase () {

241 test_linear_range 'd k l' j..

242 "

243 }

245 -test_run_rebase success --apply

244 test_run_rebase success -m

245 test_run_rebase success -i

246 test_have_prereq !REBASE_P || test_run_rebase success -p

t/t3424-rebase-empty.sh

+36

@@ -123,6 +123,42 @@ test_expect_success 'rebase --interactive uses default of --empty=ask' '

123 test_cmp expect actual

124 '

125

126 +test_expect_success 'rebase --merge --empty=drop --keep-empty' '

127 + git checkout -B testing localmods &&

128 + git rebase --merge --empty=drop --keep-empty upstream &&

129 +

130 + test_write_lines D C B A >expect &&

131 + git log --format=%s >actual &&

132 + test_cmp expect actual

133 +'

134 +

135 +test_expect_success 'rebase --merge --empty=drop --no-keep-empty' '

136 + git checkout -B testing localmods &&

137 + git rebase --merge --empty=drop --no-keep-empty upstream &&

138 +

139 + test_write_lines C B A >expect &&

140 + git log --format=%s >actual &&

141 + test_cmp expect actual

142 +'

143 +

144 +test_expect_success 'rebase --merge --empty=keep --keep-empty' '

145 + git checkout -B testing localmods &&

146 + git rebase --merge --empty=keep --keep-empty upstream &&

147 +

148 + test_write_lines D C2 C B A >expect &&

149 + git log --format=%s >actual &&

150 + test_cmp expect actual

151 +'

152 +

153 +test_expect_success 'rebase --merge --empty=keep --no-keep-empty' '

154 + git checkout -B testing localmods &&

155 + git rebase --merge --empty=keep --no-keep-empty upstream &&

156 +

157 + test_write_lines C2 C B A >expect &&

158 + git log --format=%s >actual &&

159 + test_cmp expect actual

160 +'

161 +

162 test_expect_success 'rebase --merge does not leave state laying around' '

163 git checkout -B testing localmods~2 &&

164 git rebase --merge upstream &&