Re: [PATCH v2 9/9] help: don't print "\n" before single-section output
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 23, 2022, 22:31 UTC
- Message-ID
- <xmqqh78pqkph.fsf@gitster.g>
- In-Reply-To
- <patch-v2-9.9-08dc693dc3e-20220221T193708Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 26 quoted lines
> @@ -124,7 +124,8 @@ static void print_cmd_by_category(const struct category_description *catdesc,
> uint32_t mask = catdesc[i].category;
> const char *desc = catdesc[i].desc;
>
> - putchar('\n');
> + if (i)
> + putchar('\n');
> puts(_(desc));
> print_command_list(cmds, mask, longest);
> }
> @@ -328,6 +329,7 @@ void list_commands(struct cmdnames *main_cmds, struct cmdnames *other_cmds)
> void list_common_cmds_help(void)
> {
> puts(_("These are common Git commands used in various situations:"));
> + putchar('\n');
> print_cmd_by_category(common_categories, NULL);
> }
>
> @@ -481,6 +483,7 @@ void list_all_cmds_help(int show_external_commands, int show_aliases)
> int longest;
>
> puts(_("See 'git help <command>' to read about a specific subcommand"));
> + putchar('\n');
> print_cmd_by_category(main_categories, &longest);
>
> if (show_external_commands)This patch is a good example to demonstrate that some changes cannot be reviewed without showing in the whole file what didn't get changed. Among three callers of print_cmd_by_category(), two of them we see here are *not* the first to speak, and do add a blank line before the call. The other caller that does not appear in the patch is what is being fixed, list_guides_help(), which has nobody to speak before it.
The correct pattern we want to follow in these messages is that before you say something, you add a blank line if somebody else has spoken before you. It might make sense to tell these print_cmd_by* helpers if they are the first to speak by passing the pointer to a "state" variable to indicate what has been emitted so far, instead of making the callers responsible to physically add blank lines, as the callers would stop knowing if nobody else has spoken before them if we further refactor them and introduce new direct callers.
In any case, the posted patch itself looks OK. After following these 9 steps, I still do not see enough justification for the earlier "use puts()" step, except for correcting the misused printf_ln().
Thanks, will queue the whole thing.
Show 12 quoted lines
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh > index 64321480c68..6c3e1f7159d 100755 > --- a/t/t0012-help.sh > +++ b/t/t0012-help.sh > @@ -226,7 +226,6 @@ test_expect_success "'git help -a' section spacing" ' > > test_expect_success "'git help -g' section spacing" ' > test_section_spacing_trailer git help -g <<-\EOF && > - > The Git concept guides are: > > EOF