Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
Hi Dennis,
On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> /*
> * 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
> }
I like it! Changed accordingly.
Thanks!
Dscho
Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
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
}
Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
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.
Still unsure,
Dscho
Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
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.
D.
[PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'
In the upcoming patch, we will support rebase -i's progress
reporting. The progress skips comments but counts 'noop's.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
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
};
static struct {
@@ -778,12 +779,13 @@ static struct {
{ 's', "squash" },
{ 'x', "exec" },
{ 0, "noop" },
- { 'd', "drop" }
+ { 'd', "drop" },
+ { 0, NULL }
};
static const char *command_to_string(const enum todo_command command)
{
- if (command < ARRAY_SIZE(todo_command_info))
+ if (command < TODO_COMMENT)
return todo_command_info[command].str;
die("Unknown command: %d", command);
}
@@ -1235,14 +1237,14 @@ static int parse_insn_line(struct todo_item *item,
const char *bol, char *eol)
bol += strspn(bol, " \t");
if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
- item->command = TODO_NOOP;
+ item->command = TODO_COMMENT;
item->commit = NULL;
item->arg = bol;
item->arg_len = eol - bol;
return 0;
}
- for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
+ for (i = 0; i < TODO_COMMENT; i++)
if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
item->command = i;
break;
@@ -1252,7 +1254,7 @@ static int parse_insn_line(struct todo_item *item, const
char *bol, char *eol)
item->command = i;
break;
}
- if (i >= ARRAY_SIZE(todo_command_info))
+ if (i >= TODO_COMMENT)
return -1;
if (item->command == TODO_NOOP) {
--
2.10.0.rc2.102.g5c102ec
