@matheusbsbs / matheus-bagliano / commits / e032abd5a0

rebase: fix rewritten list for failed pick

git rebase keeps a list that maps the OID of each commit before it was rebased to the OID of the equivalent commit after the rebase. This list is used to drive the "post-rewrite" hook that is called at the end of a successful rebase. When a rebase stops for the user to resolve merge conflicts the OID of the commit being picked is written to ".git/rebase-merge/stopped-sha". Then when the rebase is continued that OID is added to the list of rewritten commits. Unfortunately if a commit cannot be picked because it would overwrite an untracked file we still write the "stopped-sha1" file. This means that when the rebase is continued the commit is added into the list of rewritten commits even though it has not been picked yet. Fix this by not calling error_with_patch() for failed commands. The pick has failed so there is nothing to commit and therefore we do not want to set up the state files for committing staged changes when the rebase continues. This change means we no-longer write a patch for the failed command or display the error message printed by error_with_patch(). As the command has failed the patch isn't really useful and in any case the user can inspect the commit associated with the failed command by inspecting REBASE_HEAD. Unless the user has disabled it we already print an advice message that is more helpful than the message from error_with_patch() which the user will still see. Even if the advice is disabled the user will see the messages from the merge machinery detailing the problem. The code to add a failed command back into the todo list is duplicated between pick_one_commit() and the loop in pick_commits(). Both sites print advice about the command being rescheduled, decrement the current item and save the todo list. To avoid duplicating this code pick_one_commit() is modified to set a flag to indicate that the command should be rescheduled in the main loop. This simplifies things as only the remaining copy of the code needs to be modified to set REBASE_HEAD rather than calling error_with_patch(). 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 e032abd5a0bec40a700527f71073f3ba21fce7d6

4 files changed +63 -14

sequencer.c

+7 -12

@@ -4141,6 +4141,7 @@ static int do_merge(struct repository *r,

4141 if (ret < 0) {

4142 error(_("could not even attempt to merge '%.*s'"),

4143 merge_arg_len, arg);

4144 + unlink(git_path_merge_msg(r));

4145 goto leave_merge;

4146 }

4147 /*

@@ -4631,7 +4632,7 @@ N_("Could not execute the todo command\n"

4632 static int pick_one_commit(struct repository *r,

4633 struct todo_list *todo_list,

4634 struct replay_opts *opts,

4634 - int *check_todo)

4635 + int *check_todo, int* reschedule)

4636 {

4637 int res;

4638 struct todo_item *item = todo_list->items + todo_list->current;

@@ -4644,12 +4645,8 @@ static int pick_one_commit(struct repository *r,

4645 check_todo);

4646 if (is_rebase_i(opts) && res < 0) {

4647 /* Reschedule */

4647 - advise(_(rescheduled_advice),

4648 - get_item_line_length(todo_list, todo_list->current),

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

4650 - todo_list->current--;

4651 - if (save_todo(todo_list, opts))

4652 - return -1;

4648 + *reschedule = 1;

4649 + return -1;

4650 }

4651 if (item->command == TODO_EDIT) {

4652 struct commit *commit = item->commit;

@@ -4749,7 +4746,8 @@ static int pick_commits(struct repository *r,

4746 }

4747 }

4748 if (item->command <= TODO_SQUASH) {

4752 - res = pick_one_commit(r, todo_list, opts, &check_todo);

4749 + res = pick_one_commit(r, todo_list, opts, &check_todo,

4750 + &reschedule);

4751 if (!res && item->command == TODO_EDIT)

4752 return 0;

4753 } else if (item->command == TODO_EXEC) {

@@ -4803,10 +4801,7 @@ static int pick_commits(struct repository *r,

4801 if (save_todo(todo_list, opts))

4802 return -1;

4803 if (item->commit)

4806 - return error_with_patch(r,

4807 - item->commit,

4808 - arg, item->arg_len,

4809 - opts, res, 0);

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

4805 } else if (is_rebase_i(opts) && check_todo && !res &&

4806 reread_todo_if_changed(r, todo_list, opts)) {

4807 return -1;

t/t3404-rebase-interactive.sh

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

1287 >file6 &&

1288 test_must_fail git rebase --continue &&

1289 test_cmp_rev HEAD F &&

1290 + test_cmp_rev REBASE_HEAD I &&

1291 rm file6 &&

1292 + test_path_is_missing .git/rebase-merge/patch &&

1293 git rebase --continue &&

1294 test_cmp_rev HEAD I

1295 '

@@ -1305,7 +1307,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'

1307 >file6 &&

1308 test_must_fail git rebase --continue &&

1309 test_cmp_rev HEAD F &&

1310 + test_cmp_rev REBASE_HEAD I &&

1311 rm file6 &&

1312 + test_path_is_missing .git/rebase-merge/patch &&

1313 git rebase --continue &&

1314 test $(git cat-file commit HEAD | sed -ne \$p) = I &&

1315 git reset --hard original-branch2

@@ -1323,7 +1327,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '

1327 >file6 &&

1328 test_must_fail git rebase --continue &&

1329 test $(git cat-file commit HEAD | sed -ne \$p) = F &&

1330 + test_cmp_rev REBASE_HEAD I &&

1331 rm file6 &&

1332 + test_path_is_missing .git/rebase-merge/patch &&

1333 git rebase --continue &&

1334 test $(git cat-file commit HEAD | sed -ne \$p) = I

1335 '

t/t3430-rebase-merges.sh

+2 -2

@@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '

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

166 test_tick &&

167 test_must_fail git rebase -ir HEAD &&

168 + test_cmp_rev REBASE_HEAD H^0 &&

169 grep "^merge -C .* G$" .git/rebase-merge/done &&

170 grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&

170 - test_path_is_file .git/rebase-merge/patch &&

171 + test_path_is_missing .git/rebase-merge/patch &&

172

173 : fail because of merge conflict &&

173 - rm G.t .git/rebase-merge/patch &&

174 git reset --hard conflicting-G &&

175 test_must_fail git rebase --continue &&

176 ! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&

t/t5407-post-rewrite-hook.sh

+48

@@ -17,6 +17,12 @@ test_expect_success 'setup' '

17 git checkout A^0 &&

18 test_commit E bar E &&

19 test_commit F foo F &&

20 + git checkout B &&

21 + git merge E &&

22 + git tag merge-E &&

23 + test_commit G G &&

24 + test_commit H H &&

25 + test_commit I I &&

26 git checkout main &&

28 test_hook --setup post-rewrite <<-EOF

@@ -173,6 +179,48 @@ test_fail_interactive_rebase () {

179 )

180 }

181

182 +test_expect_success 'git rebase with failed pick' '

183 + clear_hook_input &&

184 + cat >todo <<-\EOF &&

185 + exec >bar

186 + merge -C merge-E E

187 + exec >G

188 + pick G

189 + exec >H 2>I

190 + pick H

191 + fixup I

192 + EOF

193 +

194 + (

195 + set_replace_editor todo &&

196 + test_must_fail git rebase -i D D 2>err

197 + ) &&

198 + grep "would be overwritten" err &&

199 + rm bar &&

200 +

201 + test_must_fail git rebase --continue 2>err &&

202 + grep "would be overwritten" err &&

203 + rm G &&

204 +

205 + test_must_fail git rebase --continue 2>err &&

206 + grep "would be overwritten" err &&

207 + rm H &&

208 +

209 + test_must_fail git rebase --continue 2>err &&

210 + grep "would be overwritten" err &&

211 + rm I &&

212 +

213 + git rebase --continue &&

214 + echo rebase >expected.args &&

215 + cat >expected.data <<-EOF &&

216 + $(git rev-parse merge-E) $(git rev-parse HEAD~2)

217 + $(git rev-parse G) $(git rev-parse HEAD~1)

218 + $(git rev-parse H) $(git rev-parse HEAD)

219 + $(git rev-parse I) $(git rev-parse HEAD)

220 + EOF

221 + verify_hook_input

222 +'

223 +

224 test_expect_success 'git rebase -i (unchanged)' '

225 git reset --hard D &&

226 clear_hook_input &&