Re: [PATCH v3 0/5] config-parse: create config parsing library

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 23, 2023, 19:34 UTC
Message-ID: <ZTbK3QTJYXxYj/M6@nand.local>
In-Reply-To: <xmqq34y9jho2.fsf@gitster.g>

On Tue, Oct 17, 2023 at 10:13:49AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Josh Steadmon <steadmon@google.com> writes:
>
> > Open questions:
> > - How do folks feel about the do_event() refactor in patches 2 & 3?
>
> I gave a quick re-read and found that the code after patch 2 made it
> easier to see how config.c::do_event() does its thing (even though
> the patch text of that exact step was somehow a bit hard to follow).
>
> However, the helper added by patch 3, do_event_and_flush(), that
> duplicates exactly what do_event() does, is hard to reason about, at
> least for me.  It returns early without setting .previous_type to
> EOF and the value returned from the helper signals if that is the
> case (the two early return points both return what flush_event()
> gave us), but the only caller of the helper does not even inspect
> the return value, unlike all the callers of do_event(), which also
> looks a bit fishy.

I had similar thoughts while reviewing.

But I am not sure that I agree that this series is moving us in the right direction necessarily. Or at least I am not convinced that shipping the intermediate state is worth doing before we have callers that could drop '#include "config.h"' for just the parser.

This feels like churn that does not yield a tangible pay-off, at least in the sense that the refactoring and code movement delivers us something that we can substantively use today.

I dunno.

Thanks, Taylor