sequencer: factor out part of pick_commits()
This simplifies the next commit. If a pick fails we now return the error at the end of the loop body rather than returning early, a successful "edit" command continues to return early. There are three things to check to ensure that removing the early return for an error does not change the behavior of the code: (1) We could enter the block guarded by "if (reschedule)". This block is not entered because "reschedlue" is always zero when picking a commit. (2) We could enter the block guarded by "else if (is_rebase_i(opts) && check_todo && !res)". This block is not entered when returning an error because "res" is non-zero in that case. (3) todo_list->current could be incremented before returning. That is avoided by moving the increment which is of course a potential change in behavior itself. The move is safe because none of the callers look at todo_list after this function returns. Moving the increment makes it clear we only want to advance the current item if the command was successful. 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
f2b5f41eda52416d29fade2f823d3b5bd7aa2205
1 file changed
+71
-61
sequencer.c
+71
-61
@@ -4628,6 +4628,72 @@ N_("Could not execute the todo command\n"
4628
" git rebase --edit-todo\n"
4629
" git rebase --continue\n");
4630
4631
+static int pick_one_commit(struct repository *r,
4632
+ struct todo_list *todo_list,
4633
+ struct replay_opts *opts,
4634
+ int *check_todo)
4635
+{
4636
+ int res;
4637
+ struct todo_item *item = todo_list->items + todo_list->current;
4638
+ const char *arg = todo_item_get_arg(todo_list, item);
4639
+ if (is_rebase_i(opts))
4640
+ opts->reflog_message = reflog_message(
4641
+ opts, command_to_string(item->command), NULL);
4642
+
4643
+ res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
4644
+ check_todo);
4645
+ if (is_rebase_i(opts) && res < 0) {
4646
+ /* 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;
4653
+ }
4654
+ if (item->command == TODO_EDIT) {
4655
+ struct commit *commit = item->commit;
4656
+ if (!res) {
4657
+ if (!opts->verbose)
4658
+ term_clear_line();
4659
+ fprintf(stderr, _("Stopped at %s... %.*s\n"),
4660
+ short_commit_name(commit), item->arg_len, arg);
4661
+ }
4662
+ return error_with_patch(r, commit,
4663
+ arg, item->arg_len, opts, res, !res);
4664
+ }
4665
+ if (is_rebase_i(opts) && !res)
4666
+ record_in_rewritten(&item->commit->object.oid,
4667
+ peek_command(todo_list, 1));
4668
+ if (res && is_fixup(item->command)) {
4669
+ if (res == 1)
4670
+ intend_to_amend();
4671
+ return error_failed_squash(r, item->commit, opts,
4672
+ item->arg_len, arg);
4673
+ } else if (res && is_rebase_i(opts) && item->commit) {
4674
+ int to_amend = 0;
4675
+ struct object_id oid;
4676
+
4677
+ /*
4678
+ * If we are rewording and have either
4679
+ * fast-forwarded already, or are about to
4680
+ * create a new root commit, we want to amend,
4681
+ * otherwise we do not.
4682
+ */
4683
+ if (item->command == TODO_REWORD &&
4684
+ !repo_get_oid(r, "HEAD", &oid) &&
4685
+ (oideq(&item->commit->object.oid, &oid) ||
4686
+ (opts->have_squash_onto &&
4687
+ oideq(&opts->squash_onto, &oid))))
4688
+ to_amend = 1;
4689
+
4690
+ return res | error_with_patch(r, item->commit,
4691
+ arg, item->arg_len, opts,
4692
+ res, to_amend);
4693
+ }
4694
+ return res;
4695
+}
4696
+
4697
static int pick_commits(struct repository *r,
4698
struct todo_list *todo_list,
4699
struct replay_opts *opts)
@@ -4683,66 +4749,9 @@ static int pick_commits(struct repository *r,
4749
}
4750
}
4751
if (item->command <= TODO_SQUASH) {
4686
- if (is_rebase_i(opts))
4687
- opts->reflog_message = reflog_message(opts,
4688
- command_to_string(item->command), NULL);
4689
-
4690
- res = do_pick_commit(r, item, opts,
4691
- is_final_fixup(todo_list),
4692
- &check_todo);
4693
- if (is_rebase_i(opts) && res < 0) {
4694
- /* Reschedule */
4695
- advise(_(rescheduled_advice),
4696
- get_item_line_length(todo_list,
4697
- todo_list->current),
4698
- get_item_line(todo_list,
4699
- todo_list->current));
4700
- todo_list->current--;
4701
- if (save_todo(todo_list, opts))
4702
- return -1;
4703
- }
4704
- if (item->command == TODO_EDIT) {
4705
- struct commit *commit = item->commit;
4706
- if (!res) {
4707
- if (!opts->verbose)
4708
- term_clear_line();
4709
- fprintf(stderr,
4710
- _("Stopped at %s... %.*s\n"),
4711
- short_commit_name(commit),
4712
- item->arg_len, arg);
4713
- }
4714
- return error_with_patch(r, commit,
4715
- arg, item->arg_len, opts, res, !res);
4716
- }
4717
- if (is_rebase_i(opts) && !res)
4718
- record_in_rewritten(&item->commit->object.oid,
4719
- peek_command(todo_list, 1));
4720
- if (res && is_fixup(item->command)) {
4721
- if (res == 1)
4722
- intend_to_amend();
4723
- return error_failed_squash(r, item->commit, opts,
4724
- item->arg_len, arg);
4725
- } else if (res && is_rebase_i(opts) && item->commit) {
4726
- int to_amend = 0;
4727
- struct object_id oid;
4728
-
4729
- /*
4730
- * If we are rewording and have either
4731
- * fast-forwarded already, or are about to
4732
- * create a new root commit, we want to amend,
4733
- * otherwise we do not.
4734
- */
4735
- if (item->command == TODO_REWORD &&
4736
- !repo_get_oid(r, "HEAD", &oid) &&
4737
- (oideq(&item->commit->object.oid, &oid) ||
4738
- (opts->have_squash_onto &&
4739
- oideq(&opts->squash_onto, &oid))))
4740
- to_amend = 1;
4741
-
4742
- return res | error_with_patch(r, item->commit,
4743
- arg, item->arg_len, opts,
4744
- res, to_amend);
4745
- }
4752
+ res = pick_one_commit(r, todo_list, opts, &check_todo);
4753
+ if (!res && item->command == TODO_EDIT)
4754
+ return 0;
4755
} else if (item->command == TODO_EXEC) {
4756
char *end_of_arg = (char *)(arg + item->arg_len);
4757
int saved = *end_of_arg;
@@ -4803,9 +4812,10 @@ static int pick_commits(struct repository *r,
4812
return -1;
4813
}
4814
4806
- todo_list->current++;
4815
if (res)
4816
return res;
4817
+
4818
+ todo_list->current++;
4819
}
4820
4821
if (is_rebase_i(opts)) {