Re: [PATCH 4/6] help: refactor "for_human" control flow in cmd_help()

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> Instead of having two lines that call list_config_help(for_human)
>>> let's setup the pager and print the trailer conditionally. This makes
>>> it clearer at a glance how the two differ in behavior.
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>> -               if (!for_human) {
>>> -                       list_config_help(for_human);
>>> -                       return 0;
>>> -               }
>>> -               setup_pager();
>>> +               if (for_human)
>>> +                       setup_pager();
>>>                 list_config_help(for_human);
>>> -               printf("\n%s\n", _("'git help config' for more information"));
>>> +               if (for_human)
>>> +                       printf("\n%s\n", _("'git help config' for more information"));
>>> +
>>
>> For what it's worth, I find the original logic easier to reason about
>> since it gets the "simple" case out of the way early, thus I don't
>> have to keep as much (or any) state in mind as I'm reading the rest of
>> the code. However, it's highly subjective, of course; just one
>> person's opinion.
>
> FWIW, it makes two of us ;-)
>
> Quite honestly, I do not see much commonality in the code above that
> targets two different audiences, so whether you handle simple one or
> complex one first, a single big switch upfront that gives clearly
> separate control flow to two distinct cases is easier to follow than
> "as the middle step that calls list_config_help() is the same, let's
> have two conditionals before and after and serve these two audiences
> with a single code path that is slightly tweaked", which is the
> result of this patch.