Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command

Johannes Schindelin [off-list ref] writes:

+/*
+ * Note that ordering matters in this enum. Not only must it match the mapping
+ * below, it is also divided into several sections that matter.  When adding
+ * new commands, make sure you add it in the right section.
+ */

Good thinking.  Makes me wish C were a better language, though ;-)

 enum todo_command {
+	/* commands that handle commits */
 	TODO_PICK = 0,
-	TODO_REVERT
+	TODO_REVERT,
+	/* commands that do nothing but are counted for reporting progress */
+	TODO_NOOP
 };
 
 static const char *todo_command_strings[] = {
 	"pick",
-	"revert"
+	"revert",
+	"noop"
 };

I wonder if making this a switch() statement is easier to read in
the longer run.  The only thing at this point we are gaining by "not
only mapping and enum must match, the orders matter" is so that this
codepath can do the same thing for PICK and REVERT, but these two
would become more and more minority as we learn more words.

 		todo_list->current++;
 		if (res)
 			return res;

On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano [off-list ref] wrote:

Johannes Schindelin [off-list ref] writes:

quoted

+/*
+ * Note that ordering matters in this enum. Not only must it match the mapping
+ * below, it is also divided into several sections that matter.  When adding
+ * new commands, make sure you add it in the right section.
+ */

Good thinking.  Makes me wish C were a better language, though ;-)

Do this:

  static const char *todo_command_strings[] = {
      [TODO_PICK] = "pick",
      [TODO_REVERT] = "revert",
      [TODO_NOOP] = "noop:,
  };

which makes the array be order-independent. You still need to make
sure you fill in all the entries, of course, but it tends to avoid at
least one gotcha, and it makes it more obvious how the two are tied
together.

              Linus

Hi Junio,

On Tue, 13 Dec 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

@@ -1292,7 +1316,12 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 		struct todo_item *item = todo_list->items + todo_list->current;
 		if (save_todo(todo_list, opts))
 			return -1;
-		res = do_pick_commit(item->command, item->commit, opts);
+		if (item->command <= TODO_REVERT)
+			res = do_pick_commit(item->command, item->commit,
+					opts);
+		else if (item->command != TODO_NOOP)
+			return error(_("unknown command %d"), item->command);

I wonder if making this a switch() statement is easier to read in
the longer run.  The only thing at this point we are gaining by "not
only mapping and enum must match, the orders matter" is so that this
codepath can do the same thing for PICK and REVERT, but these two
would become more and more minority as we learn more words.

I doubt that this is easier to read. There are essentially three
categories we are handling: exec, comments, and everything else. IMO the
current code is the easiest to understand.

Ciao,
Dscho