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

[Toon Claes <toon@iotcl.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> That's fair, and the result is easier to write. But is it really easier
>> to read?

You're bringing up a very valid point there, and not directly in what
you're saying, but how it makes me reconsider.

So we're comparing:

    pick_regular_commit(revs->repo, commit, replayed_commits,
                        mode == REPLAY_MODE_REVERT ? last_commit : onto,
                        &merge_opt, &result, mode, opts->empty);

with:

    pick_regular_commit(revs->repo, commit, replayed_commits,
                        reverse ? last_commit : onto,
                        &merge_opt, &result, reverse, opts->empty);

You can argue which of both is easier to read, but the problem isn't
really whether it's a bool or an enum, but the ternary operator in this
lengthy function call is. That is the problem I was trying to solve, and
converting enum to bool isn't really the solution.

>> And what if we ever have to create a third mode going forward?

Personally I find this weak argument. As far as I know we most of the
time do not write code in a way so "it will be ready to add X in the
future". In my personal experience, I'm always wrong in predicting what
might be added in the future. Although I must say this case is
different, because we're not adding something new, no this commit was
dumbing down something existing. So I'll revisit this commit in the next
iteration.

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

That's also a very valid argument, which I didn't take in mind.

> 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 for bringing in this illustrative example. Point made, I'll
revisit.

-- 
Cheers,
Toon