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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 19, 2016, 12:51 UTC
Message-ID: <alpine.DEB.2.20.1612191237400.54750@virtualbox>
In-Reply-To: <xmqqtwa7ze9i.fsf@gitster.mtv.corp.google.com>

Hi,

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

Show 12 quoted lines

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Johannes Schindelin <johannes.schindelin@gmx.de> 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.

Not my thinking... This was in direct response to a suggestion by Dennis Kaarsemaker, I cannot take credit for the idea.

Show 17 quoted lines

> 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.
> 
> Yes, I know.  But I do not think the variant of C we stick to is not
> new enough to have that.

Let me try to express this without double negation: our coding guidelines state very explicitly that we do not use C99 initializers, and it also explains why: we want to support a broader range of compilers. For details, see:

https://github.com/git/git/blob/v2.11.0/Documentation/CodingGuidelines#L179-L181

TBH I briefly considered going the same route as I did for the fsck warnings (taking a lot of heat for the ugliness):

https://github.com/git/git/blob/v2.11.0/fsck.c#L17-L85

It would have looked somewhat like this:

	#define FOREACH_TODO(FUNC) \
		FUNC(PICK, 'p', "pick") \
		FUNC(REVERT, 0, "revert") \
		FUNC(EDIT, 'e', "edit") \
		...
		FUNC(COMMENT, 0, NULL)

	#define TODO(id, short, long) TODO_##id,
	enum todo_command {
		FOREACH_TODO(TODO)
		TODO_MAX
	};
	#undef TODO

	#define TODO(id, short, long) { short, long },
	static struct {
		char c;
		const char *str;
	} todo_command_info[] = {
		FOREACH_TODO(TODO)
		{ 0, NULL }
	};
	#undef TODO

I have to admit that even I prefer the version I provided in my contribution over the "fsck method" outlined above.

Ciao, Dscho