Jacob Keller <jacob.kel...@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).
