Eric Sunshine [off-list ref] writes:
On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
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 <redacted>
---
@@ -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 [off-list ref] writes:
quoted
On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
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 <redacted>
---
@@ -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?