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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2017, 21:59 UTC
Message-ID: <xmqqefu64dgz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170626172433.19785-1-kaarticsivaraam91196@gmail.com>

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Show 9 quoted lines

> * 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").

Show 6 quoted lines

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

Show 17 quoted lines

>  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?

Show 7 quoted lines

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

Show 8 quoted lines

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