@matheusbsbs / matheus-bagliano / commits / e98c4269c8

Fork 0

rebase (interactive-backend): fix handling of commits that become empty

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/xmqqfu1fswdh.fsf@gitster-ct.c.googlers.com/ 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 <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Feb 15, 2020 at 21:36 UTC e98c4269c86019bfe057a91b4305f784365b6f0b

6 files changed +183 -23

Documentation/git-rebase.txt

+26 -3

@@ -258,6 +258,24 @@ See also INCOMPATIBLE OPTIONS below.

258 original branch. The index and working tree are also left

259 unchanged as a result.

260

261 +--empty={drop,keep,ask}::

262 + How to handle commits that are not empty to start and are not

263 + clean cherry-picks of any upstream commit, but which become

264 + empty after rebasing (because they contain a subset of already

265 + upstream changes). With drop (the default), commits that

266 + become empty are dropped. With keep, such commits are kept.

267 + With ask (implied by --interactive), the rebase will halt when

268 + an empty commit is applied allowing you to choose whether to

269 + drop it, edit files more, or just commit the empty changes.

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

271 + -i/--interactive is explicitly specified.

272 ++

273 +Note that commits which start empty are kept, and commits which are

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

275 +always dropped.

276 ++

277 +See also INCOMPATIBLE OPTIONS below.

278 +

279 --keep-empty::

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

281 relative to their parent) used to fail and this option would

@@ -561,6 +579,7 @@ are incompatible with the following options:

579 * --interactive

580 * --exec

581 * --keep-empty

582 + * --empty=

583 * --edit-todo

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

585

@@ -569,6 +588,7 @@ In addition, the following pairs of options are incompatible:

588 * --preserve-merges and --interactive

589 * --preserve-merges and --signoff

590 * --preserve-merges and --rebase-merges

591 + * --preserve-merges and --empty=

592 * --keep-base and --onto

593 * --keep-base and --root

594

@@ -585,9 +605,12 @@ commits that started empty, though these are rare in practice. It

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

606 this behavior.

607

588 -The interactive backend keeps intentionally empty commits.

589 -Unfortunately, it always halts whenever it runs across a commit that

590 -becomes empty, even when the rebase is not explicitly interactive.

608 +The interactive backend keeps intentionally empty commits. Similar to

609 +the am backend, by default the interactive backend drops commits that

610 +become empty unless -i/--interactive is specified (in which case it

611 +stops and asks the user what to do). The interactive backend also has

612 +an --empty={drop,keep,ask} option for changing the behavior of

613 +handling commits that become empty.

614

615 Directory rename detection

616 ~~~~~~~~~~~~~~~~~~~~~~~~~~

builtin/rebase.c

+52

@@ -50,8 +50,16 @@ enum rebase_type {

50 REBASE_PRESERVE_MERGES

51 };

53 +enum empty_type {

54 + EMPTY_UNSPECIFIED = -1,

55 + EMPTY_DROP,

56 + EMPTY_KEEP,

57 + EMPTY_ASK

58 +};

59 +

60 struct rebase_options {

61 enum rebase_type type;

62 + enum empty_type empty;

63 const char *state_dir;

64 struct commit *upstream;

65 const char *upstream_name;

@@ -91,6 +99,7 @@ struct rebase_options {

100 #define REBASE_OPTIONS_INIT { \

101 .type = REBASE_UNSPECIFIED, \

102 + .empty = EMPTY_UNSPECIFIED, \

103 .flags = REBASE_NO_QUIET, \

104 .git_am_opts = ARGV_ARRAY_INIT, \

105 .git_format_patch_opt = STRBUF_INIT \

@@ -109,6 +118,8 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)

118 replay.allow_rerere_auto = opts->allow_rerere_autoupdate;

119 replay.allow_empty = 1;

120 replay.allow_empty_message = opts->allow_empty_message;

121 + replay.drop_redundant_commits = (opts->empty == EMPTY_DROP);

122 + replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);

123 replay.verbose = opts->flags & REBASE_VERBOSE;

124 replay.reschedule_failed_exec = opts->reschedule_failed_exec;

125 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,

455

456 BUG_ON_OPT_ARG(arg);

457

458 + /*

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

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

461 + */

462 opts->type = REBASE_INTERACTIVE;

463 return 0;

464 }

@@ -1350,6 +1365,29 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,

1365 return 0;

1366 }

1367

1368 +static enum empty_type parse_empty_value(const char *value)

1369 +{

1370 + if (!strcasecmp(value, "drop"))

1371 + return EMPTY_DROP;

1372 + else if (!strcasecmp(value, "keep"))

1373 + return EMPTY_KEEP;

1374 + else if (!strcasecmp(value, "ask"))

1375 + return EMPTY_ASK;

1376 +

1377 + die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);

1378 +}

1379 +

1380 +static int parse_opt_empty(const struct option *opt, const char *arg, int unset)

1381 +{

1382 + struct rebase_options *options = opt->value;

1383 + enum empty_type value = parse_empty_value(arg);

1384 +

1385 + BUG_ON_OPT_NEG(unset);

1386 +

1387 + options->empty = value;

1388 + return 0;

1389 +}

1390 +

1391 static void NORETURN error_on_missing_default_upstream(void)

1392 {

1393 struct branch *current_branch = branch_get(NULL);

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

1532 "ignoring them"),

1533 REBASE_PRESERVE_MERGES, PARSE_OPT_HIDDEN),

1534 OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),

1535 + OPT_CALLBACK_F(0, "empty", &options, N_("{drop,keep,ask}"),

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

1537 + PARSE_OPT_NONEG, parse_opt_empty),

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

1539 N_("(DEPRECATED) keep empty commits"),

1540 PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,

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

1801 if (!(options.flags & REBASE_NO_QUIET))

1802 argv_array_push(&options.git_am_opts, "-q");

1803

1804 + if (options.empty != EMPTY_UNSPECIFIED)

1805 + imply_interactive(&options, "--empty");

1806 +

1807 if (gpg_sign) {

1808 free(options.gpg_sign_opt);

1809 options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);

@@ -1843,6 +1887,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)

1887 break;

1888 }

1889

1890 + if (options.empty == EMPTY_UNSPECIFIED) {

1891 + if (options.flags & REBASE_INTERACTIVE_EXPLICIT)

1892 + options.empty = EMPTY_ASK;

1893 + else if (exec.nr > 0)

1894 + options.empty = EMPTY_KEEP;

1895 + else

1896 + options.empty = EMPTY_DROP;

1897 + }

1898 if (reschedule_failed_exec > 0 && !is_interactive(&options))

1899 die(_("--reschedule-failed-exec requires "

1900 "--exec or --interactive"));

sequencer.c

+39 -11

@@ -158,6 +158,8 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")

158 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")

159 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")

160 static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")

161 +static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")

162 +static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")

163

164 static int git_sequencer_config(const char *k, const char *v, void *cb)

165 {

@@ -1483,7 +1485,11 @@ static int is_original_commit_empty(struct commit *commit)

1485 }

1486

1487 /*

1486 - * Do we run "git commit" with "--allow-empty"?

1488 + * Should empty commits be allowed? Return status:

1489 + * <0: Error in is_index_unchanged(r) or is_original_commit_empty(commit)

1490 + * 0: Halt on empty commit

1491 + * 1: Allow empty commit

1492 + * 2: Drop empty commit

1493 */

1494 static int allow_empty(struct repository *r,

1495 struct replay_opts *opts,

@@ -1492,14 +1498,17 @@ static int allow_empty(struct repository *r,

1498 int index_unchanged, originally_empty;

1499

1500 /*

1495 - * Three cases:

1501 + * Four cases:

1502 *

1503 * (1) we do not allow empty at all and error out.

1504 *

1499 - * (2) we allow ones that were initially empty, but

1500 - * forbid the ones that become empty;

1505 + * (2) we allow ones that were initially empty, and

1506 + * just drop the ones that become empty

1507 *

1502 - * (3) we allow both.

1508 + * (3) we allow ones that were initially empty, but

1509 + * halt for the ones that become empty;

1510 + *

1511 + * (4) we allow both.

1512 */

1513 if (!opts->allow_empty)

1514 return 0; /* let "git commit" barf as necessary */

@@ -1516,10 +1525,12 @@ static int allow_empty(struct repository *r,

1525 originally_empty = is_original_commit_empty(commit);

1526 if (originally_empty < 0)

1527 return originally_empty;

1519 - if (!originally_empty)

1520 - return 0;

1521 - else

1528 + if (originally_empty)

1529 return 1;

1530 + else if (opts->drop_redundant_commits)

1531 + return 2;

1532 + else

1533 + return 0;

1534 }

1535

1536 static struct {

@@ -1730,7 +1741,7 @@ static int do_pick_commit(struct repository *r,

1741 char *author = NULL;

1742 struct commit_message msg = { NULL, NULL, NULL, NULL };

1743 struct strbuf msgbuf = STRBUF_INIT;

1733 - int res, unborn = 0, reword = 0, allow;

1744 + int res, unborn = 0, reword = 0, allow, drop_commit;

1745

1746 if (opts->no_commit) {

1747 /*

@@ -1935,13 +1946,20 @@ static int do_pick_commit(struct repository *r,

1946 goto leave;

1947 }

1948

1949 + drop_commit = 0;

1950 allow = allow_empty(r, opts, commit);

1951 if (allow < 0) {

1952 res = allow;

1953 goto leave;

1942 - } else if (allow)

1954 + } else if (allow == 1) {

1955 flags |= ALLOW_EMPTY;

1944 - if (!opts->no_commit) {

1956 + } else if (allow == 2) {

1957 + drop_commit = 1;

1958 + fprintf(stderr,

1959 + _("dropping %s %s -- patch contents already upstream\n"),

1960 + oid_to_hex(&commit->object.oid), msg.subject);

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

1962 + if (!opts->no_commit && !drop_commit) {

1963 if (author || command == TODO_REVERT || (flags & AMEND_MSG))

1964 res = do_commit(r, msg_file, author, opts, flags);

1965 else

@@ -2495,6 +2513,12 @@ static int read_populate_opts(struct replay_opts *opts)

2513 if (file_exists(rebase_path_reschedule_failed_exec()))

2514 opts->reschedule_failed_exec = 1;

2515

2516 + if (file_exists(rebase_path_drop_redundant_commits()))

2517 + opts->drop_redundant_commits = 1;

2518 +

2519 + if (file_exists(rebase_path_keep_redundant_commits()))

2520 + opts->keep_redundant_commits = 1;

2521 +

2522 read_strategy_opts(opts, &buf);

2523 strbuf_release(&buf);

2524

@@ -2574,6 +2598,10 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,

2598 write_file(rebase_path_gpg_sign_opt(), "-S%s\n", opts->gpg_sign);

2599 if (opts->signoff)

2600 write_file(rebase_path_signoff(), "--signoff\n");

2601 + if (opts->drop_redundant_commits)

2602 + write_file(rebase_path_drop_redundant_commits(), "%s", "");

2603 + if (opts->keep_redundant_commits)

2604 + write_file(rebase_path_keep_redundant_commits(), "%s", "");

2605 if (opts->reschedule_failed_exec)

2606 write_file(rebase_path_reschedule_failed_exec(), "%s", "");

2607

sequencer.h

@@ -39,6 +39,7 @@ struct replay_opts {

39 int allow_rerere_auto;

40 int allow_empty;

41 int allow_empty_message;

42 + int drop_redundant_commits;

43 int keep_redundant_commits;

44 int verbose;

45 int quiet;

t/t3424-rebase-empty.sh

+61 -5

@@ -34,7 +34,7 @@ test_expect_success 'setup test repository' '

34 git commit -m "Five letters ought to be enough for anybody"

35 '

37 -test_expect_failure 'rebase (am-backend) with a variety of empty commits' '

37 +test_expect_failure 'rebase (am-backend)' '

38 test_when_finished "git rebase --abort" &&

39 git checkout -B testing localmods &&

40 # rebase (--am) should not drop commits that start empty

@@ -45,10 +45,17 @@ test_expect_failure 'rebase (am-backend) with a variety of empty commits' '

45 test_cmp expect actual

46 '

48 -test_expect_failure 'rebase --merge with a variety of empty commits' '

49 - test_when_finished "git rebase --abort" &&

48 +test_expect_success 'rebase --merge --empty=drop' '

49 + git checkout -B testing localmods &&

50 + git rebase --merge --empty=drop upstream &&

51 +

52 + test_write_lines D C B A >expect &&

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

54 + test_cmp expect actual

55 +'

56 +

57 +test_expect_success 'rebase --merge uses default of --empty=drop' '

58 git checkout -B testing localmods &&

51 - # rebase --merge should not halt on the commit that becomes empty

59 git rebase --merge upstream &&

61 test_write_lines D C B A >expect &&

@@ -56,7 +63,56 @@ test_expect_failure 'rebase --merge with a variety of empty commits' '

63 test_cmp expect actual

64 '

59 -test_expect_success 'rebase --interactive with a variety of empty commits' '

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

67 + git checkout -B testing localmods &&

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

69 +

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

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

72 + test_cmp expect actual

73 +'

74 +

75 +test_expect_success 'rebase --merge --empty=ask' '

76 + git checkout -B testing localmods &&

77 + test_must_fail git rebase --merge --empty=ask upstream &&

78 +

79 + git rebase --skip &&

80 +

81 + test_write_lines D C B A >expect &&

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

83 + test_cmp expect actual

84 +'

85 +

86 +test_expect_success 'rebase --interactive --empty=drop' '

87 + git checkout -B testing localmods &&

88 + git rebase --interactive --empty=drop upstream &&

89 +

90 + test_write_lines D C B A >expect &&

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

92 + test_cmp expect actual

93 +'

94 +

95 +test_expect_success 'rebase --interactive --empty=keep' '

96 + git checkout -B testing localmods &&

97 + git rebase --interactive --empty=keep upstream &&

98 +

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

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

101 + test_cmp expect actual

102 +'

103 +

104 +test_expect_success 'rebase --interactive --empty=ask' '

105 + git checkout -B testing localmods &&

106 + test_must_fail git rebase --interactive --empty=ask upstream &&

107 +

108 + git rebase --skip &&

109 +

110 + test_write_lines D C B A >expect &&

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

112 + test_cmp expect actual

113 +'

114 +

115 +test_expect_success 'rebase --interactive uses default of --empty=ask' '

116 git checkout -B testing localmods &&

117 test_must_fail git rebase --interactive upstream &&

118

t/t3427-rebase-subtree.sh

+4 -4

@@ -85,10 +85,10 @@ test_expect_failure REBASE_P 'Rebase -Xsubtree --keep-empty --preserve-merges --

85 verbose test "$(commit_message HEAD)" = "Empty commit"

86 '

88 -test_expect_success 'Rebase -Xsubtree --onto commit' '

88 +test_expect_success 'Rebase -Xsubtree --empty=ask --onto commit' '

89 reset_rebase &&

90 git checkout -b rebase-onto to-rebase &&

91 - test_must_fail git rebase -Xsubtree=files_subtree --onto files-master master &&

91 + test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --onto files-master master &&

92 : first pick results in no changes &&

93 git rebase --skip &&

94 verbose test "$(commit_message HEAD~2)" = "master4" &&

@@ -96,10 +96,10 @@ test_expect_success 'Rebase -Xsubtree --onto commit' '

96 verbose test "$(commit_message HEAD)" = "Empty commit"

97 '

99 -test_expect_success 'Rebase -Xsubtree --rebase-merges --onto commit' '

99 +test_expect_success 'Rebase -Xsubtree --empty=ask --rebase-merges --onto commit' '

100 reset_rebase &&

101 git checkout -b rebase-merges-onto to-rebase &&

102 - test_must_fail git rebase -Xsubtree=files_subtree --rebase-merges --onto files-master --root &&

102 + test_must_fail git rebase -Xsubtree=files_subtree --empty=ask --rebase-merges --onto files-master --root &&

103 : first pick results in no changes &&

104 git rebase --skip &&

105 verbose test "$(commit_message HEAD~2)" = "master4" &&