Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> > The code looks good, but I'm a little wary of adding bud which
> > hard-codes a specific label. I suppose it does grant a bit of
> > readability to the resulting script... ? It doesn't seem that
> > important compared to use using "reset onto"? At least when
> > documenting this it should be made clear that the "onto" label is
> > special.
> 
> I do not think we would mind "bud" too much in the end result, but
> the change in 1/8 is made harder to read than necessary with it.  It
> is the only thing that needs "a single-letter command name may now
> not have any argument after it" change to the parser among the three
> things being added here, and it also needs to be added to the list
> of special commands without arguments.
> 
> It would have been easier to reason about if addition of "bud" was
> in its own patch done after label and reset are added.  And if done
> as a separate step, perhaps it would have been easier to realize
> that it would be a more future-proof solution for handling the
> "special" ness of BUD to add a new "unsigned flags" word to
> todo_command_info[] structure and using a bit that says "this does
> not take an arg" than to hardcode "noop and bud are the commands
> without args" in the code.  That hardcode was good enough when there
> was only one thing in that special case.  Now it has two.

> In a similar way, the code to special case label and reset just like
> exec may also want to become more table driven, perhaps using
> another bit in the same new flags word to say "this does not refer
> to commit".  I think that can become [v2 1/N] while addition of "bud"
> can be [v2 2/N] (after all, "bud" just does the same do_reset() with
> hardcoded argument, so "label/reset" must come first).