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
}