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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2023, 17:13 UTC
Message-ID: <xmqq34y9jho2.fsf@gitster.g>
In-Reply-To: <cover.1695330852.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 5 quoted lines

> Config parsing no longer uses global state as of gc/config-context, so the
> natural next step for libification is to turn that into its own library.
> This series starts that process by moving config parsing into
> config-parse.[c|h] so that other programs can include this functionality
> without pulling in all of config.[c|h].

This has been in list archive collecting dust. It is unfortunate that not many people appear to be interested in reviewing others' patches?

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

Thanks.