Re: [PATCH 04/10] builtin/revert.c: refactor run_sequencer() return pattern

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 1, 2023, 04:25 UTC
Message-ID: <xmqqcz7yhoeq.fsf@gitster.g>
In-Reply-To: <patch-04.10-1e4e504c533-20221230T071741Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 6 quoted lines

> Refactor the return pattern in run_sequencer() to make it easier to
> insert a "replay_opts_release()" call between the "fn(...)" invocation
> and the eventual return.
>
> Usually we'd name the "cbfun" here "fn", but by using this name we'll
> nicely align all the "cmd == ?" comparisons.

If the new helper function sequencer_remove_branch_state() is used elsewhere in later steps of the series, it is great that it is extracted out of an existing code to handle the 'q(uit)' action.

However, I'd not appreciate this change from if/elseif/... cascade to ? : ? : cascade, mixed into a creation of the new helper function. Such a change forces all conceivable future command handlers to take only r and opts, and that is not a consistency we do not know we need (yet---YAGNI).

Then you do not even have to talk about cbfun vs fn ;-).

And if sequencer_remove_branch_state() is not reused elsewhere in the series, then simply dropping this step would be great.

Thanks.