@matheusbsbs / matheus-bagliano / commits / 39edfd5cbc

sequencer: fix edit handling for cherry-pick and revert messages

save_opts() should save any non-default values. It was intended to do this, but since most options in struct replay_opts default to 0, it only saved non-zero values. Unfortunately, this does not always work for options.edit. Roughly speaking, options.edit had a default value of 0 for cherry-pick but a default value of 1 for revert. Make save_opts() record a value whenever it differs from the default. options.edit was also overly simplistic; we had more than two cases. The behavior that previously existed was as follows: Non-conflict commits Right after Conflict revert Edit iff isatty(0) Edit (ignore isatty(0)) cherry-pick No edit See above Specify --edit Edit (ignore isatty(0)) See above Specify --no-edit (*) See above (*) Before stopping for conflicts, No edit is the behavior. After stopping for conflicts, the --no-edit flag is not saved so see the first two rows. However, the expected behavior is: Non-conflict commits Right after Conflict revert Edit iff isatty(0) Edit iff isatty(0) cherry-pick No edit Edit iff isatty(0) Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0)) Specify --no-edit No edit No edit In order to get the expected behavior, we need to change options.edit to a tri-state: unspecified, false, or true. When specified, we follow what it says. When unspecified, we need to check whether the current commit being created is resolving a conflict as well as consulting options.action and isatty(0). While at it, add a should_edit() utility function that compresses options.edit down to a boolean based on the additional information for the non-conflict case. continue_single_pick() is the function responsible for resuming after conflict cases, regardless of whether there is one commit being picked or many. Make this function stop assuming edit behavior in all cases, so that it can correctly handle !isatty(0) and specific requests to not edit the commit message. Reported-by: Renato Botelho <garga@freebsd.org> Signed-off-by: Elijah Newren <newren@gmail.com> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Mar 31, 2021 at 06:52 UTC 39edfd5cbc4d168db19ec1bc867d78ec7211ec39

4 files changed +77 -17

builtin/revert.c

+1 -3

@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)

182 "--signoff", opts->signoff,

183 "--no-commit", opts->no_commit,

184 "-x", opts->record_origin,

185 - "--edit", opts->edit,

185 + "--edit", opts->edit > 0,

186 NULL);

187

188 if (cmd) {

@@ -230,8 +230,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)

230 struct replay_opts opts = REPLAY_OPTS_INIT;

231 int res;

232

233 - if (isatty(0))

234 - opts.edit = 1;

233 opts.action = REPLAY_REVERT;

234 sequencer_init_config(&opts);

235 res = run_sequencer(argc, argv, &opts);

sequencer.c

+41 -11

@@ -1860,14 +1860,25 @@ static void record_in_rewritten(struct object_id *oid,

1860 flush_rewritten_pending();

1861 }

1862

1863 +static int should_edit(struct replay_opts *opts) {

1864 + if (opts->edit < 0)

1865 + /*

1866 + * Note that we only handle the case of non-conflicted

1867 + * commits; continue_single_pick() handles the conflicted

1868 + * commits itself instead of calling this function.

1869 + */

1870 + return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;

1871 + return opts->edit;

1872 +}

1873 +

1874 static int do_pick_commit(struct repository *r,

1875 enum todo_command command,

1876 struct commit *commit,

1877 struct replay_opts *opts,

1878 int final_fixup, int *check_todo)

1879 {

1869 - unsigned int flags = opts->edit ? EDIT_MSG : 0;

1870 - const char *msg_file = opts->edit ? NULL : git_path_merge_msg(r);

1880 + unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;

1881 + const char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);

1882 struct object_id head;

1883 struct commit *base, *next, *parent;

1884 const char *base_label, *next_label;

@@ -3101,9 +3112,9 @@ static int save_opts(struct replay_opts *opts)

3112 if (opts->no_commit)

3113 res |= git_config_set_in_file_gently(opts_file,

3114 "options.no-commit", "true");

3104 - if (opts->edit)

3105 - res |= git_config_set_in_file_gently(opts_file,

3106 - "options.edit", "true");

3115 + if (opts->edit >= 0)

3116 + res |= git_config_set_in_file_gently(opts_file, "options.edit",

3117 + opts->edit ? "true" : "false");

3118 if (opts->allow_empty)

3119 res |= git_config_set_in_file_gently(opts_file,

3120 "options.allow-empty", "true");

@@ -4077,7 +4088,7 @@ static int pick_commits(struct repository *r,

4088 prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));

4089 if (opts->allow_ff)

4090 assert(!(opts->signoff || opts->no_commit ||

4080 - opts->record_origin || opts->edit ||

4091 + opts->record_origin || should_edit(opts) ||

4092 opts->committer_date_is_author_date ||

4093 opts->ignore_date));

4094 if (read_and_refresh_cache(r, opts))

@@ -4370,14 +4381,33 @@ cleanup_head_ref:

4381 return sequencer_remove_state(opts);

4382 }

4383

4373 -static int continue_single_pick(struct repository *r)

4384 +static int continue_single_pick(struct repository *r, struct replay_opts *opts)

4385 {

4375 - const char *argv[] = { "commit", NULL };

4386 + struct strvec argv = STRVEC_INIT;

4387 + int ret;

4388

4389 if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&

4390 !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))

4391 return error(_("no cherry-pick or revert in progress"));

4380 - return run_command_v_opt(argv, RUN_GIT_CMD);

4392 +

4393 + strvec_push(&argv, "commit");

4394 +

4395 + /*

4396 + * continue_single_pick() handles the case of recovering from a

4397 + * conflict. should_edit() doesn't handle that case; for a conflict,

4398 + * we want to edit if the user asked for it, or if they didn't specify

4399 + * and stdin is a tty.

4400 + */

4401 + if (!opts->edit || (opts->edit < 0 && !isatty(0)))

4402 + /*

4403 + * Include --cleanup=strip as well because we don't want the

4404 + * "# Conflicts:" messages.

4405 + */

4406 + strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL);

4407 +

4408 + ret = run_command_v_opt(argv.v, RUN_GIT_CMD);

4409 + strvec_clear(&argv);

4410 + return ret;

4411 }

4412

4413 static int commit_staged_changes(struct repository *r,

@@ -4547,7 +4577,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)

4577 goto release_todo_list;

4578 }

4579 } else if (!file_exists(get_todo_path(opts)))

4550 - return continue_single_pick(r);

4580 + return continue_single_pick(r, opts);

4581 else if ((res = read_populate_todo(r, &todo_list, opts)))

4582 goto release_todo_list;

4583

@@ -4556,7 +4586,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)

4586 if (refs_ref_exists(get_main_ref_store(r),

4587 "CHERRY_PICK_HEAD") ||

4588 refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD")) {

4559 - res = continue_single_pick(r);

4589 + res = continue_single_pick(r, opts);

4590 if (res)

4591 goto release_todo_list;

4592 }

sequencer.h

+4 -2

@@ -31,8 +31,10 @@ enum commit_msg_cleanup_mode {

31 struct replay_opts {

32 enum replay_action action;

34 - /* Boolean options */

34 + /* Tri-state options: unspecified, false, or true */

35 int edit;

36 +

37 + /* Boolean options */

38 int record_origin;

39 int no_commit;

40 int signoff;

@@ -71,7 +73,7 @@ struct replay_opts {

73 /* Only used by REPLAY_NONE */

74 struct rev_info *revs;

75 };

74 -#define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }

76 +#define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }

78 /*

79 * Note that ordering matters in this enum. Not only must it match the mapping

t/t3510-cherry-pick-sequence.sh

+31 -1

@@ -65,7 +65,7 @@ test_expect_success 'cherry-pick persists opts correctly' '

65 # gets interrupted, use a high-enough number that is larger

66 # than the number of parents of any commit we have created

67 mainline=4 &&

68 - test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&

68 + test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours --edit initial..anotherpick &&

69 test_path_is_dir .git/sequencer &&

70 test_path_is_file .git/sequencer/head &&

71 test_path_is_file .git/sequencer/todo &&

@@ -84,6 +84,36 @@ test_expect_success 'cherry-pick persists opts correctly' '

84 ours

85 EOF

86 git config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&

87 + test_cmp expect actual &&

88 + echo "true" >expect &&

89 + git config --file=.git/sequencer/opts --get-all options.edit >actual &&

90 + test_cmp expect actual

91 +'

92 +

93 +test_expect_success 'revert persists opts correctly' '

94 + pristine_detach initial &&

95 + # to make sure that the session to revert a sequence

96 + # gets interrupted, revert commits that are not in the history

97 + # of HEAD.

98 + test_expect_code 1 git revert -s --strategy=recursive -X patience -X ours --no-edit picked yetanotherpick &&

99 + test_path_is_dir .git/sequencer &&

100 + test_path_is_file .git/sequencer/head &&

101 + test_path_is_file .git/sequencer/todo &&

102 + test_path_is_file .git/sequencer/opts &&

103 + echo "true" >expect &&

104 + git config --file=.git/sequencer/opts --get-all options.signoff >actual &&

105 + test_cmp expect actual &&

106 + echo "recursive" >expect &&

107 + git config --file=.git/sequencer/opts --get-all options.strategy >actual &&

108 + test_cmp expect actual &&

109 + cat >expect <<-\EOF &&

110 + patience

111 + ours

112 + EOF

113 + git config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&

114 + test_cmp expect actual &&

115 + echo "false" >expect &&

116 + git config --file=.git/sequencer/opts --get-all options.edit >actual &&

117 test_cmp expect actual

118 '

119