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

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

Hi Junio,

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

Show 18 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > @@ -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