Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

[tl;dr: the version in your repo is fine, and there's a trivial fix below if we want to silence the warning in the meantime] On Mon, Oct 17, 2016 at 10:37:52AM +0200, Johannes Schindelin wrote: > > I'm not sure I agree. IIRC, Assigning values outside the range of an enum > > has > > always

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Hi Peff, On Sun, 16 Oct 2016, Jeff King wrote: > On Sun, Oct 16, 2016 at 10:09:12AM +0200, Johannes Schindelin wrote: > > > > Good catch. It technically needs to check the lower bound, too. In > > > theory, if somebody wanted to add an enum value that is negative, you'd > > > use a signed cast

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

On Sun, Oct 16, 2016 at 10:09:12AM +0200, Johannes Schindelin wrote: > > Good catch. It technically needs to check the lower bound, too. In > > theory, if somebody wanted to add an enum value that is negative, you'd > > use a signed cast and check against both 0 and ARRAY_SIZE(). In > > practice,

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Hi, On Sat, 15 Oct 2016, Jeff King wrote: > On Sat, Oct 15, 2016 at 07:40:15PM +0200, Torsten Bögershausen wrote: > > > > I wonder if: > > > > > > if ((int)command < ARRAY_SIZE(todo_command_strings)) > > > > > > silences the warning (I suppose size_t is probably an even better type, > > >

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

On Sat, Oct 15, 2016 at 07:40:15PM +0200, Torsten Bögershausen wrote: > > I wonder if: > > > > if ((int)command < ARRAY_SIZE(todo_command_strings)) > > > > silences the warning (I suppose size_t is probably an even better type, > > though obviously it does not matter in practice). > > > Both

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

On 15.10.16 19:19, Jeff King wrote: > On Sat, Oct 15, 2016 at 07:03:46PM +0200, Torsten Bögershausen wrote: > >> sequencer.c:633:14: warning: comparison of constant 2 with expression of >> type 'const enum todo_command' is always true >> [-Wtautological-constant-out-of-range-compare] >>

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

On Sat, Oct 15, 2016 at 07:03:46PM +0200, Torsten Bögershausen wrote: > sequencer.c:633:14: warning: comparison of constant 2 with expression of type > 'const enum todo_command' is always true > [-Wtautological-constant-out-of-range-compare] > if (command <

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Not sure is this has been reported before: sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare] if (command < ARRAY_SIZE(todo_command_strings)) ~~~ ^

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Johannes Schindelin writes: > In the end, I decided to actually *not* use strbuf_add_unique_abbrev() > here because it really makes the code very much too ugly after the > introduction of short_commit_name(): It's perfectly fine not to use that function when it does

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Johannes Schindelin writes: >> I am personally fine with this line; two things come to mind: >> >> - This would work just fine as-is with Linus's change to turn >>DEFAULT_ABBREV to -1. >> >> - It appears that it is more fashionable to use >>

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Hi Junio, On Tue, 11 Oct 2016, Johannes Schindelin wrote: > On Tue, 11 Oct 2016, Johannes Schindelin wrote: > > > -- snipsnap -- > > @@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list > > *todo_list, > > item->command = command; > >

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Hi Junio, On Tue, 11 Oct 2016, Johannes Schindelin wrote: > -- snipsnap -- > @@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list > *todo_list, > item->command = command; > item->commit = commit; > item->offset_in_buf =

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Hi Junio, On Mon, 10 Oct 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > if (parent && parse_commit(parent) < 0) > > - /* TRANSLATORS: The first %s will be "revert" or > > - "cherry-pick", the second %s a SHA1 */ > > +

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

Johannes Schindelin writes: > Let's just bite the bullet and rewrite the entire parser; the code now > ... > In particular, we choose to maintain the list of commands in an array > instead of a linked list: this is flexible enough to allow us later on to > even

[PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

When we came up with the "sequencer" idea, we really wanted to have kind of a plumbing equivalent of the interactive rebase. Hence the choice of words: the "todo" script, a "pick", etc. However, when it came time to implement the entire shebang, somehow this idea got lost and the sequencer was