Re: [PATCH v4 1/3] replay: refactor enum replay_mode into a bool

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 22, 2026, 15:43 UTC
Message-ID: <xmqq7bnq37jm.fsf@gitster.g>
In-Reply-To: <ajk-YQxLWfspNWIm@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 27 quoted lines

> On Mon, Jun 22, 2026 at 02:41:55PM +0200, Toon Claes wrote:
>> In 2760ee4983 (replay: add --revert mode to reverse commit changes,
>> 2026-03-26) the enum `replay_mode` was introduced. This has two possible
>> values:
>> 
>>  - The value `REPLAY_MODE_REVERT` is used when option `--revert` is
>>    passed to git-replay(1). When using this value the commits are
>>    processed in reverse order and the inverse of the changes are
>>    applied.
>> 
>>  - The value `REPLAY_MODE_PICK` is used when either option `--onto` or
>>    `--advance` is used. In both cases the commits are processed in
>>    normal order, and the changes are applied as-is.
>> 
>> Since there are only two possible values of this enum, simplify the code
>> by converting the enum into a bool. This avoids adding code paths that
>> check for invalid values of the enum, and shortens code where the value
>> is checked with a ternary operator.
>
> That's fair, and the result is easier to write. But is it really easier
> to read? And what if we ever have to create a third mode going forward?
>
> I'm generally no fan of booleans as parameters as they basically give
> you no information at all at the callsite, except if you're lucky and
> you already have an aptly-named variable available that you can pass.
> Which seems to be the case here, but I'm still not sure whether this
> change really improves the code.

I tend to agree with you on both counts. The "what happens when somebody else wants a third choice?" is a quesiton I would ask the first thing as the maintainer of a project.

Even if the boolean parameter is so obviously named, the callsite can only say "true" or "false", unlike some other popular languages that lets you say

	my_function(use_revert_mode=true, verbose=false);

and you cannot tell what effect the author wanted out of that "true" if all you can write were

	my_function(true, false);

Of course, we could go ultra verbose, like

	my_function(true, /* use_revert_mode */
		    false, /* verbose */);

but then we are often better off writing:

	my_function(REPLAY_MODE_REVERT, REPLAY_QUIET);

Thanks.