Re: [PATCH/RFC] commit-template: improve readability of commit template

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

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> * Previously the commit template didn't separate the
>   distinct messages shown. This resulted in difficulty
>   in interpreting it's content. Add new lines to separate
>   the distinct parts of the template.
>
> * Previously the warning about usage of explicit paths
>   without any options wasn't clear. Make it more clear
>   so user gets what it's trying to say.
>

We don't usually make a bullet list in log message.  Please stick to
a plain prose.  

"Previously" is superflous.  Say what it does (e.g. "The commit
template adds optional parts without extra blank lines to its normal
output") in present tense and explain the ramifications of it
(e.g. "I personally find that this makes it harder to find the
optional bit").

> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  I've tried to improve the message specified in the commit. Hope
>  it works correctly.
>
>  Local test passed.

Perhaps you would want to ensure that this change (if you find it
valuable) will not get broken by other people in the future by
writing a new test that ensures that these extra blank lines are
always there when you think they are needed?

I personally do not find these new blank lines are necessary, and
this change wastes vertical screen real estate which is a limited
resource, but that may be just me.  I on the other hand do not think
the result of this patch is overly worse than the status quo, either.



>  builtin/commit.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8d1cac062..0a5676b76 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -841,9 +841,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				  "with '%c' will be kept; you may remove them"
>  				  " yourself if you want to.\n"
>  				  "An empty message aborts the commit.\n"), comment_line_char);
> -		if (only_include_assumed)
> +		if (only_include_assumed) {
> +			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); // Add new line for clarity
>  			status_printf_ln(s, GIT_COLOR_NORMAL,
>  					"%s", only_include_assumed);
> +		}

We do not use // comment in most parts of our codebase that are
supposed to be platform neutral (iow, compat/ is exempt).

But more importantly, wouldn't

		if (only_include_assumed)
			status_printf_ln(s, GIT_COLOR_NORMAL,
-					"%s", only_include_asssumed);
+					"\n%s", only_include_asssumed);

be sufficient?

> @@ -877,8 +879,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				(int)(ci.name_end - ci.name_begin), ci.name_begin,
>  				(int)(ci.mail_end - ci.mail_begin), ci.mail_begin);
>  
> -		if (ident_shown)
> -			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
> +		status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); // Add new line for clarity

This does ensure that an extra blank line appears after the optional
section (either after the "only/include assumed" message, or "writing
for somebody else" message).

If we were to go with this sparser output, I think we also should
give an extra blank line before and after the "HEAD detached from
cafebabe" message you would see:

	$ git checkout HEAD^0
	$ git commit --allow-empty -o

or "On branch blah" if you are on a branch.  I think your change
adds a blank before, but it does not have a separation before
"Changes not staged for commit" line.

>  		saved_color_setting = s->use_color;
>  		s->use_color = 0;
> @@ -1209,7 +1210,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  	if (argc == 0 && (also || (only && !amend && !allow_empty)))
>  		die(_("No paths with --include/--only does not make sense."));
>  	if (argc > 0 && !also && !only)
> -		only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
> +		only_include_assumed = _("Explicit paths (<paths>) specified without -i or -o; assuming --only <paths>");

I think "paths (<paths>)" is excessive.  If you are using <token> to
hint that they refer to "commit -h" or "commit --help" output, then

    Explicit <paths> specified without -i or -o; assumign --only <paths>

should be sufficient.

Having said that, to be quite honest, I think this "assuming --only"
message outlived its usefulness.  This was necessary in very early
days of Git because originally "git commit foo" did "git add foo &&
git commit" (i.e. "-i" was the default) and then later when we made
"--only" the new default in order to match everybody else's SCM, we
needed to remind users of older versions of Git that "git commit foo"
now means "git commit --only foo", not "git commit -i foo" which they
may have been used to.  These days, hopefully nobody expects the "-i"
semantics when they do "git commit foo", so perhaps it may be a better
change to _remove_ the message altogether.

And with that done, I wouldn't have reservations on this change
(i.e. "is it worth wasting extra screen real estate, especially in
the vertical direction?"), as instead of wasting 2 lines to give a
message that is no longer useful in today's world, it will be
removing one line ;-)

Thanks.