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

[Junio C Hamano <gitster@pobox.com>]

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

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