Re: [PATCH RFC v2 03/16] sequencer: introduce new history editing mode

[Patrick Steinhardt <ps@xxxxxx>]

On Tue, Aug 26, 2025 at 08:55:13AM -0400, D. Ben Knoble wrote:
> On Sun, Aug 24, 2025 at 1:42 PM Patrick Steinhardt <ps@xxxxxx> wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index bff181df76..898ac1a2a8 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3052,17 +3087,29 @@ static int read_populate_todo(struct repository *r,
> >                 return error(_("no commits parsed."));
> >
> >         if (!is_rebase_i(opts)) {
> > -               enum todo_command valid =
> > -                       opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
> > +               enum todo_command valid;
> >                 int i;
> >
> > -               for (i = 0; i < todo_list->nr; i++)
> > +               switch (opts->action) {
> > +               case REPLAY_PICK:
> > +               case REPLAY_HISTORY_EDIT:
> > +                       valid = TODO_PICK;
> > +                       break;
> > +               default:
> > +                       valid = TODO_REVERT;
> > +                       break;
> > +               }
> 
> I think I see this hunk repeated in a few places—maybe some
> leftoverbits for a refactor?

Fair enough. I don't really see a strong reason why we shouldn't fix
this in the same patch though. We can for example do something like the
below patch.

Patrick

-- >8 --

diff --git a/sequencer.c b/sequencer.c
index 898ac1a2a8..9a66e7d128 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3063,6 +3063,19 @@ static void todo_list_write_total_nr(struct todo_list *todo_list)
 	}
 }
 
+static enum todo_command action_to_command(enum replay_action action)
+{
+	switch (action) {
+	case REPLAY_PICK:
+	case REPLAY_HISTORY_EDIT:
+		return TODO_PICK;
+	case REPLAY_REVERT:
+		return TODO_REVERT;
+	default:
+		BUG("unsupported action %d", action);
+	}
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
@@ -3087,19 +3100,9 @@ static int read_populate_todo(struct repository *r,
 		return error(_("no commits parsed."));
 
 	if (!is_rebase_i(opts)) {
-		enum todo_command valid;
+		enum todo_command valid = action_to_command(opts->action);
 		int i;
 
-		switch (opts->action) {
-		case REPLAY_PICK:
-		case REPLAY_HISTORY_EDIT:
-			valid = TODO_PICK;
-			break;
-		default:
-			valid = TODO_REVERT;
-			break;
-		}
-
 		for (i = 0; i < todo_list->nr; i++) {
 			if (valid == todo_list->items[i].command)
 				continue;
@@ -3408,16 +3411,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 	if (prepare_revs(opts))
 		return -1;
 
-	switch (opts->action) {
-	case REPLAY_PICK:
-	case REPLAY_HISTORY_EDIT:
-		command = TODO_PICK;
-		break;
-	default:
-		command = TODO_REVERT;
-		break;
-	}
-
+	command = action_to_command(opts->action);
 	command_string = todo_command_info[command].str;
 	encoding = get_log_output_encoding();
 
@@ -5578,15 +5572,7 @@ static int single_pick(struct repository *r,
 	int check_todo;
 	struct todo_item item;
 
-	switch (opts->action) {
-	case REPLAY_PICK:
-	case REPLAY_HISTORY_EDIT:
-		item.command = TODO_PICK;
-		break;
-	default:
-		item.command = TODO_REVERT;
-		break;
-	}
+	item.command = action_to_command(opts->action);
 	item.commit = cmit;
 
 	return do_pick_commit(r, &item, opts, 0, &check_todo);