| Message ID | patch-4.6-e4bc7e57a6d-20210908T151949Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | help: fix usage nits & bugs, completion shellscript->C | expand |
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.
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.
On Wed, Sep 08 2021, Junio C Hamano wrote: > 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. What do you two think of the end-state of this in 6/6? I went back & forth a bit with whether to keep this similar to pre-4/6 logic, or the switch statement in 6/6, and then for that switch statement whether to have the fallthrough case or not. Perhaps 6/6 with no fallthrough (but a bit of duplication) is the best?
diff --git a/builtin/help.c b/builtin/help.c index 0f9dc31c40f..0737b22069b 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_config) { int for_human = show_config == 1; - 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")); + return 0; }
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> --- builtin/help.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)