@matheusbsbs / matheus-bagliano / commits / 203573b024

rebase -i: fix adding failed command to the todo list

When rebasing commands are moved from the todo list in "git-rebase-todo" to the "done" file (which is used by "git status" to show the recently executed commands) just before they are executed. This means that if a command fails because it would overwrite an untracked file it has to be added back into the todo list before the rebase stops for the user to fix the problem. Unfortunately when a failed command is added back into the todo list the command preceding it is erroneously appended to the "done" file. This means that when rebase stops after "pick B" fails the "done" file contains pick A pick B pick A instead of pick A pick B This happens because save_todo() updates the "done" file with the previous command whenever "git-rebase-todo" is updated. When we add the failed pick back into "git-rebase-todo" we do not want to update "done". Fix this by adding a "reschedule" parameter to save_todo() which prevents the "done" file from being updated when adding a failed command back into the "git-rebase-todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file. Reported-by: Stefan Haller <lists@haller-berlin.de> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Sep 6, 2023 at 15:22 UTC 203573b024610d7e50cc83a6d06867880ed8cc4f

3 files changed +40 -23

sequencer.c

+6 -6

@@ -3380,7 +3380,8 @@ give_advice:

3380 return -1;

3381 }

3382

3383 -static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)

3383 +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,

3384 + int reschedule)

3385 {

3386 struct lock_file todo_lock = LOCK_INIT;

3387 const char *todo_path = get_todo_path(opts);

@@ -3390,7 +3391,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)

3391 * rebase -i writes "git-rebase-todo" without the currently executing

3392 * command, appending it to "done" instead.

3393 */

3393 - if (is_rebase_i(opts))

3394 + if (is_rebase_i(opts) && !reschedule)

3395 next++;

3396

3397 fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);

@@ -3403,7 +3404,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)

3404 if (commit_lock_file(&todo_lock) < 0)

3405 return error(_("failed to finalize '%s'"), todo_path);

3406

3406 - if (is_rebase_i(opts) && next > 0) {

3407 + if (is_rebase_i(opts) && !reschedule && next > 0) {

3408 const char *done = rebase_path_done();

3409 int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);

3410 int ret = 0;

@@ -4716,7 +4717,7 @@ static int pick_commits(struct repository *r,

4717 const char *arg = todo_item_get_arg(todo_list, item);

4718 int check_todo = 0;

4719

4719 - if (save_todo(todo_list, opts))

4720 + if (save_todo(todo_list, opts, reschedule))

4721 return -1;

4722 if (is_rebase_i(opts)) {

4723 if (item->command != TODO_COMMENT) {

@@ -4797,8 +4798,7 @@ static int pick_commits(struct repository *r,

4798 get_item_line_length(todo_list,

4799 todo_list->current),

4800 get_item_line(todo_list, todo_list->current));

4800 - todo_list->current--;

4801 - if (save_todo(todo_list, opts))

4801 + if (save_todo(todo_list, opts, reschedule))

4802 return -1;

4803 if (item->commit)

4804 write_rebase_head(&item->commit->object.oid);

t/t3404-rebase-interactive.sh

+18 -11

@@ -1277,19 +1277,24 @@ test_expect_success 'todo count' '

1277 '

1278

1279 test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '

1280 - git checkout --force branch2 &&

1280 + git checkout --force A &&

1281 git clean -f &&

1282 + cat >todo <<-EOF &&

1283 + exec >file2

1284 + pick $(git rev-parse B) B

1285 + pick $(git rev-parse C) C

1286 + pick $(git rev-parse D) D

1287 + exec cat .git/rebase-merge/done >actual

1288 + EOF

1289 (

1283 - set_fake_editor &&

1284 - FAKE_LINES="edit 1 2" git rebase -i A

1290 + set_replace_editor todo &&

1291 + test_must_fail git rebase -i A

1292 ) &&

1286 - test_cmp_rev HEAD F &&

1287 - test_path_is_missing file6 &&

1288 - >file6 &&

1289 - test_must_fail git rebase --continue &&

1290 - test_cmp_rev HEAD F &&

1291 - test_cmp_rev REBASE_HEAD I &&

1292 - rm file6 &&

1293 + test_cmp_rev HEAD B &&

1294 + test_cmp_rev REBASE_HEAD C &&

1295 + head -n3 todo >expect &&

1296 + test_cmp expect .git/rebase-merge/done &&

1297 + rm file2 &&

1298 test_path_is_missing .git/rebase-merge/patch &&

1299 echo changed >file1 &&

1300 git add file1 &&

@@ -1297,7 +1302,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '

1302 grep "error: you have staged changes in your working tree" err &&

1303 git reset --hard HEAD &&

1304 git rebase --continue &&

1300 - test_cmp_rev HEAD I

1305 + test_cmp_rev HEAD D &&

1306 + tail -n3 todo >>expect &&

1307 + test_cmp expect actual

1308 '

1309

1310 test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '

t/t3430-rebase-merges.sh

+16 -6

@@ -128,14 +128,24 @@ test_expect_success 'generate correct todo list' '

128 '

129

130 test_expect_success '`reset` refuses to overwrite untracked files' '

131 - git checkout -b refuse-to-reset &&

131 + git checkout B &&

132 test_commit dont-overwrite-untracked &&

133 - git checkout @{-1} &&

134 - : >dont-overwrite-untracked.t &&

135 - echo "reset refs/tags/dont-overwrite-untracked" >script-from-scratch &&

133 + cat >script-from-scratch <<-EOF &&

134 + exec >dont-overwrite-untracked.t

135 + pick $(git rev-parse B) B

136 + reset refs/tags/dont-overwrite-untracked

137 + pick $(git rev-parse C) C

138 + exec cat .git/rebase-merge/done >actual

139 + EOF

140 test_config sequence.editor \""$PWD"/replace-editor.sh\" &&

137 - test_must_fail git rebase -ir HEAD &&

138 - git rebase --abort

141 + test_must_fail git rebase -ir A &&

142 + test_cmp_rev HEAD B &&

143 + head -n3 script-from-scratch >expect &&

144 + test_cmp expect .git/rebase-merge/done &&

145 + rm dont-overwrite-untracked.t &&

146 + git rebase --continue &&

147 + tail -n3 script-from-scratch >>expect &&

148 + test_cmp expect actual

149 '

150

151 test_expect_success '`reset` rejects trees' '