Re: [PATCH 4/6] help: refactor "for_human" control flow in cmd_help()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 8, 2021, 17:02 UTC
- Message-ID
- <xmqqsfyf3srv.fsf@gitster.g>
- In-Reply-To
- <CAPig+cQVsFvkLMXrm7d3mCUoJKj7x+0PNcAYJ0-uFHaqbRw0ag@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 27 quoted lines
> 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.