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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 22 Jan 2018, Junio C Hamano wrote:

> 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.

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