Re: [PATCH v2 9/9] help: don't print "\n" before single-section output

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -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.


> 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