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

From: Junio C Hamano <[email-masked]>
Date: Mon, 22 Jan 2018 13:25:32 -0800

Jacob Keller <[email-masked]> 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).