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

[Junio C Hamano]

Johannes Schindelin  writes:

> On Tue, 13 Dec 2016, Junio C Hamano wrote:
>
>> Linus Torvalds  writes:
>> 
>> > On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano  wrote:
>> >> Johannes Schindelin  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.

I now realize that I was unclear about what "thinking" I found good
in my comment.  I do not particularly like defining two parallel
things and having to maintain them in sync.  The "Good thinking"
praise goes to whoever thought that this burdensome fact deserves a
clear comment in front of these two things.

And ...

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

... solves only one-half of the problem with the language I had.
The order of the entries in this array[] may become more flexible
in the source, but you still have to define enum separately.

I guess if we really want to, we need to resort to something "ugly
but workable" like what you did in fsck.c with FOREACH_MSG_ID(X).
That approach may be the least ugly way if we have to maintain two
or more parallel things in sync.

... and then realizes you wrote pretty much the same thing
... after writing all of the above ;-)

But it is way overkill for sequencer commands that are only handful.

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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  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

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

[Johannes Schindelin]

Hi,

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

> Linus Torvalds  writes:
> 
> > On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano  wrote:
> >> Johannes Schindelin  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.

> 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

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

[Junio C Hamano]

Linus Torvalds  writes:

> On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano  wrote:
>> Johannes Schindelin  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.  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.
>
>   Linus

Yes, I know.  But I do not think the variant of C we stick to is not
new enough to have that.

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

[Linus Torvalds]

On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano  wrote:
> Johannes Schindelin  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.  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.

  Linus

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

[Junio C Hamano]

Johannes Schindelin  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.  Makes me wish C were a better language, though ;-)

>  enum todo_command {
> + /* commands that handle commits */
>   TODO_PICK = 0,
> - TODO_REVERT
> + TODO_REVERT,
> + /* commands that do nothing but are counted for reporting progress */
> + TODO_NOOP
>  };
>  
>  static const char *todo_command_strings[] = {
>   "pick",
> - "revert"
> + "revert",
> + "noop"
>  };

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

>   todo_list->current++;
>   if (res)
>   return res;