Re: [PATCH v2 11/15] git_config_set: do not use a state machine

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 9, 2018, 07:50 UTC
Message-ID: <nycvar.QRO.7.76.6.1804090945460.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180406212840.GC7870@sigill.intra.peff.net>

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

Show 23 quoted lines

> On Tue, Apr 03, 2018 at 06:28:42PM +0200, Johannes Schindelin wrote:
> 
> > While a neat theoretical construct, state machines are hard to read. In
> > this instance, it does not even make a whole lot of sense because we are
> > more interested in flags, anyway: has the section been seen? Has the key
> > been seen? Does the current section match the key we are looking for?
> > 
> > Besides, the state `SECTION_SEEN` was named in a misleading way: it did
> > not indicate that we saw the section matching the key we are looking
> > for, but it instead indicated that we are *currently* in that section.
> > 
> > Let's just replace the state machine logic by clear and obvious flags.
> > 
> > This will also make it easier to review the upcoming patches to use the
> > newly-introduced `event_fn` callback of the config parser.
> 
> I think this is probably a good direction. But one thing state machines
> can help with is keeping the state to a manageable size. With 3 bits of
> flags, we now have 8 possible states, up from the previous 4.
> 
> Clearly some of those are nonsensical (can you be in key_seen without
> section_seen? I'd think not), but it's up to the code to interpret and
> reset those manually.

That is true. On the other hand, it is easy to miss incorrect state transitions in state machines (or to miss unused states).

> I'll defer to your judgement, though, on this making things for the
> future patches more readable. You spend a lot more time poking at it
> than I have.

The original reason to get rid of the state machine was: I did not need the states any more in the end. Since the section name is set via the event stream we now know in the config_fn whether we are in the correct section or not.

I also liked the fact that it was much easier to reason about correct code: "Did I catch all the states that apply here?" is a hairier question than "Is this flag true?"

Thanks, Dscho