On Tue, Oct 17, 2023 at 10:13:49AM -0700, Junio C Hamano wrote: > Josh Steadmon <steadmon@xxxxxxxxxx> 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
