From: Junio C Hamano Date: Wed, 08 Sep 2021 17:02:12 GMT Subject: Re: [PATCH 4/6] help: refactor "for_human" control flow in cmd_help() Message-ID: In-Reply-To: Eric Sunshine writes: > On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason > 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 >> --- >> @@ -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.