Hi Junio, On Mon, 22 Jan 2018, Junio C Hamano wrote:
> 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. I dropped the `bud` command. It did come in handy when I truly recreated branch structure from a way-too-long topic branch, but that is probably a rare use case, and not worth spending so much air time on. > 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). The downside of such a table-driven approach is readability, of course. It becomes *less* readable because all of a sudden you have to jump back and forth between the parsing code and the table (and then you also have to keep the table header in sight). I have had enough problems with such table-driven approaches in the past, even in Git's own source code. Exhibit A: t0027. I do not wish upon my worst enemies having to investigate problems in that script, for in those tables despair awaits ye who dare enter. So I respectfully decline to go into that direction in the sequencer. Ciao, Dscho
