Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Dennis Kaarsemaker <dennis@kaarsemaker.net>]

On do, 2016-09-01 at 17:32 +0200, Johannes Schindelin wrote:
> Hi Dennis,
> 
> On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> 
> > 
> > On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> > > 
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 51c2f76..4c902e5 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -763,7 +763,8 @@ enum todo_command {
> > >         TODO_SQUASH,
> > >         TODO_EXEC,
> > >         TODO_NOOP,
> > > -       TODO_DROP
> > > +       TODO_DROP,
> > > +       TODO_COMMENT
> > >  };
> > (picking a random commit that touches this enum)
> > 
> > In a few places you now make comparisons like "< TODO_NOOP", so I
> > think
> > it would be good to have a comment near the definition of this enum
> > that says that ordering matters and why, so people don't attempt to
> > add
> > a new TODO_FOOBAR at the end.
> True.
> 
> It does not seem that we have a precedent for that. The closest is
> what I
> had in an early iteration of the fsck message IDs, and subsequently
> things
> were refactored so that it is not the order, but a flag, that
> determines
> what the command does.
> 
> Not sure how to do this elegantly. Maybe like this?
> 
> 	enum todo_command {
> 		TODO_PICK_COMMANDS = 0,
> 		TODO_PICK = TODO_PICK_COMMANDS,
> 		TODO_SQUASH,
> 
> 		TODO_NON_PICK_COMMANDS,
> 		TODO_EXEC = TODO_NON_PICK_COMMANDS,
> 
> 		TODO_NOOP_COMMANDS,
> 		TODO_NOOP = TODO_NOOP_COMMANDS,
> 		TODO_DROP
> 		TODO_DROP,
> 
> 		TODO_LAST_COMMAND,
> 		TODO_COMMENT = TODO_LAST_COMMAND
> 	};
> 
> But that is so god-awful to read.

Agreed, that sure is awful.

How about something like

/*
 * 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.
 */
enum todo_command {
	/* All commands that handle commits */
	TODO_PICK,
	...
	/* All commands that do something else than pick */
	TODO_EXEC,
	...
	/* All commands that do nothing but are counted for reporting progress */
	TODO_NOOP,
	...
	/* Comments, which are not counted
	TODO_COMMENT
}