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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 1, 2016, 15:32 UTC
Message-ID: <alpine.DEB.2.20.1609011720160.129229@virtualbox>
In-Reply-To: <1472718815.4680.21.camel@kaarsemaker.net>

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

Show 20 quoted lines

> 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.

Still unsure, Dscho