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

2016-09-02 Thread Johannes Schindelin
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'

2016-09-01 Thread Dennis Kaarsemaker
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'

2016-09-01 Thread Johannes Schindelin
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'

2016-09-01 Thread Dennis Kaarsemaker
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'

2016-08-31 Thread Johannes Schindelin
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