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

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

[Kaartic Sivaraam]

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

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.

 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);
+		}
 
 		/*
 		 * These should never fail because they come from our own
@@ -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
 
 		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>");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
 		cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
 	else if (!strcmp(cleanup_arg, "verbatim"))
-- 
2.11.0

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

[Junio C Hamano]

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.

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

[Kaartic Sivaraam]

On Mon, 2017-06-26 at 14:59 -0700, Junio C Hamano wrote:
> 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").
> 
Corrected it.

> 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?
> 
See comment below.

> 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.
> 
I thought it's not good to trade-off readability for vertical space as
the ultimate aim of the commit template (at least to me) is to convey
information to the user about the commit that he's going to make. For
which, I thought it made more sense to improve it's readability by
adding new lines between different sections rather than constrain the
output within a few lines.

In case it's not worth it, I'll revert it back which isn't that big an
issue, anyway. In case it's not suggested to revert this back, I'll add
the tests.

> We do not use // comment in most parts of our codebase that are
> supposed to be platform neutral (iow, compat/ is exempt).
> 
It seems to be a result of my ignorance that C allowed '//' comments
only recently.

> > @@ -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.
> 
I actually didn't think of modifying that in order to keep it in line
with the output of `git status`. Further, to me, adding *this* new line
before the "Changes not staged for commit" (or something in it's place)
seems to be wasting some vertical space with an added drawback that
it's not in line with `git status`.

> Having said that, to be quite honest, I think this "assuming --only"
> message outlive
> d 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 ;-)
> 
You're right! It is worth saving lines if it's not useful these days.

I have sent another mail to the mailing list to get any other possible
improvements to the status.
http://public-inbox.org/git/1498583779.2737.4.camel@gmail.com/

-- 
Regards,
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

>> 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.
>> 
> I thought it's not good to trade-off readability for vertical space as
> the ultimate aim of the commit template (at least to me) is to convey
> information to the user about the commit that he's going to make. For
> which, I thought it made more sense to improve it's readability by
> adding new lines between different sections rather than constrain the
> output within a few lines.

You have to be careful when making a trade-off argument.  It depends
on how familiar you already are with the presentation.  Those who
are/got used to the order of things that come, they will know there
is extra information when the block of lines are longer than usual
without reading every character and then their eyes are guided to
read what is extra, without having to waste precious screen real
estate.  Nobody will _stay_ a new user who is not yet familiar with
the everyday output.

>> 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.
>> 
> I actually didn't think of modifying that in order to keep it in line
> with the output of `git status`.

I was (and still am) assuming that if we make this change to "git
commit", we should make matching change to "git status" as a given.

> Further, to me, adding *this* new line
> before the "Changes not staged for commit" (or something in it's place)
> seems to be wasting some vertical space ...

I think it is in line with your original reasoning why you wanted
these extra blank lines to separate blocks of different kinds of
information:

 - "Please do this" instruction at the beginning
 - Make sure you know the default is --only, not --include
 - By the way you are committing for that person, not you
 - This change is being committed on that branch
 - Here are the changes that are already in the index
 - Here are the changes that are not in the index
 - Here are untracked files

Lack of a blank between the fourth block and the fifth block [*1*]
makes it somewhat inconsistent, doesn't it?


[Footnote]

*1* Yes, we should think about removing the optional second block,
    as I think that it outlived its usefulness; if we are to do so,
    these become the third and the fourth blocks.

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

[Kaartic Sivaraam]

On Tue, 2017-06-27 at 10:56 -0700, Junio C Hamano wrote:
> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> > I thought it's not good to trade-off readability for vertical space
> > as
> > the ultimate aim of the commit template (at least to me) is to
> > convey
> > information to the user about the commit that he's going to make.
> > For
> > which, I thought it made more sense to improve it's readability by
> > adding new lines between different sections rather than constrain
> > the
> > output within a few lines.
> 
> You have to be careful when making a trade-off argument.  It depends
> on how familiar you already are with the presentation.  Those who
> are/got used to the order of things that come, they will know there
> is extra information when the block of lines are longer than usual
> without reading every character and then their eyes are guided to
> read what is extra, without having to waste precious screen real
> estate.  Nobody will _stay_ a new user who is not yet familiar with
> the everyday output.
> 
You're right. I didn't consider the fact that experienced users would
be affected as a result of this change, sorry about that. I thought,
making this change would help the new users who would possibly find the
commit template to be congested and let experienced users to get
accustomed to this new output format. I thought this change would be a
win-win (at least after people get accustomed to the new formatting). 

In case screen real estate is considered more important here, no
issues. I'll drop that part of the change, happily.

> > I actually didn't think of modifying that in order to keep it in
> > line
> > with the output of `git status`.
> 
> I was (and still am) assuming that if we make this change to "git
> commit", we should make matching change to "git status" as a given.
I get it now. In that case, I don't think making the change would be a
good choice for the following reasons,

    * I think vertical spacing matters more in the output printed to a
    console.
    * I myself find it odd to add a new line below the branch
    information possibly because I'm too accustomed to it's current
    output.

I tried adding the new line, it seemed to be too spacious. It might be
just me in this case.

> > Further, to me, adding *this* new line
> > before the "Changes not staged for commit" (or something in it's
> > place)
> > seems to be wasting some vertical space ...
> 
> I think it is in line with your original reasoning why you wanted
> these extra blank lines to separate blocks of different kinds of
> information:
> 
>  - "Please do this" instruction at the beginning
>  - Make sure you know the default is --only, not --include
>  - By the way you are committing for that person, not you
>  - This change is being committed on that branch
>  - Here are the changes that are already in the index
>  - Here are the changes that are not in the index
>  - Here are untracked files
> 
> Lack of a blank between the fourth block and the fifth block [*1*]
> makes it somewhat inconsistent, doesn't it?
> 
It does, for the given set of blocks. I didn't find it inconsistent as
I thought the separate blocks as follows,

 - "Please do this" instruction at the beginning
 - Make sure you know the default is --only, not --include
 - By the way you are committing for that person, not you
 - Status of repository (git status)

> [Footnote]
> 
> *1* Yes, we should think about removing the optional second block,
>     as I think that it outlived its usefulness; if we are to do so,
>     these become the third and the fourth blocks.
If I interpreted your previous email correctly, I thought we were doing
it!

I'll send a "typical" patch as a follow-up of this mail.

-- 
Regards,
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

[PATCH] commit-template: improve readability of commit template

[Kaartic Sivaraam]

The commit template adds the optional parts without
a new line to distinguish them. This results in
difficulty in interpreting it's content. Add new
lines to separate the distinct parts of the template.

The warning about usage of 'explicit paths without
any corresponding options' has outlived it's purpose of
warning users about the usage '--only' as default rather
than '--include'. Remove it.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 builtin/commit.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..22d17e6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -139,7 +139,6 @@ static enum commit_whence whence;
 static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
-static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -841,9 +840,6 @@ 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)
-			status_printf_ln(s, GIT_COLOR_NORMAL,
-					"%s", only_include_assumed);
 
 		/*
 		 * These should never fail because they come from our own
@@ -877,8 +873,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 */
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
@@ -1208,8 +1203,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	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...");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
 		cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
 	else if (!strcmp(cleanup_arg, "verbatim"))
-- 
2.11.0

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

[Kaartic Sivaraam]

I forgot to add the RFC in the subject in a hurry. Please consider it's
presence.

-- 
Regards,
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

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

[Kaartic Sivaraam]

I might have been ignorant about something about git in my reply in the
previous email (found below). In that case, please enlighten me.

On Wed, 2017-06-28 at 18:34 +0530, Kaartic Sivaraam wrote:
> On Tue, 2017-06-27 at 10:56 -0700, Junio C Hamano wrote:
> > Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> > > I thought it's not good to trade-off readability for vertical
> > > space
> > > as
> > > the ultimate aim of the commit template (at least to me) is to
> > > convey
> > > information to the user about the commit that he's going to make.
> > > For
> > > which, I thought it made more sense to improve it's readability
> > > by
> > > adding new lines between different sections rather than constrain
> > > the
> > > output within a few lines.
> > 
> > You have to be careful when making a trade-off argument.  It
> > depends
> > on how familiar you already are with the presentation.  Those who
> > are/got used to the order of things that come, they will know there
> > is extra information when the block of lines are longer than usual
> > without reading every character and then their eyes are guided to
> > read what is extra, without having to waste precious screen real
> > estate.  Nobody will _stay_ a new user who is not yet familiar with
> > the everyday output.
> > 
> 
> You're right. I didn't consider the fact that experienced users would
> be affected as a result of this change, sorry about that. I thought,
> making this change would help the new users who would possibly find
> the
> commit template to be congested and let experienced users to get
> accustomed to this new output format. I thought this change would be
> a
> win-win (at least after people get accustomed to the new
> formatting). 
> 
> In case screen real estate is considered more important here, no
> issues. I'll drop that part of the change, happily.
> 
> > > I actually didn't think of modifying that in order to keep it in
> > > line
> > > with the output of `git status`.
> > 
> > I was (and still am) assuming that if we make this change to "git
> > commit", we should make matching change to "git status" as a given.
> 
> I get it now. In that case, I don't think making the change would be
> a
> good choice for the following reasons,
> 
>     * I think vertical spacing matters more in the output printed to
> a
>     console.
>     * I myself find it odd to add a new line below the branch
>     information possibly because I'm too accustomed to it's current
>     output.
> 
> I tried adding the new line, it seemed to be too spacious. It might
> be
> just me in this case.
> 
> > > Further, to me, adding *this* new line
> > > before the "Changes not staged for commit" (or something in it's
> > > place)
> > > seems to be wasting some vertical space ...
> > 
> > I think it is in line with your original reasoning why you wanted
> > these extra blank lines to separate blocks of different kinds of
> > information:
> > 
> >  - "Please do this" instruction at the beginning
> >  - Make sure you know the default is --only, not --include
> >  - By the way you are committing for that person, not you
> >  - This change is being committed on that branch
> >  - Here are the changes that are already in the index
> >  - Here are the changes that are not in the index
> >  - Here are untracked files
> > 
> > Lack of a blank between the fourth block and the fifth block [*1*]
> > makes it somewhat inconsistent, doesn't it?
> > 
> 
> It does, for the given set of blocks. I didn't find it inconsistent
> as
> I thought the separate blocks as follows,
> 
>  - "Please do this" instruction at the beginning
>  - Make sure you know the default is --only, not --include
>  - By the way you are committing for that person, not you
>  - Status of repository (git status)
> 
> > [Footnote]
> > 
> > *1* Yes, we should think about removing the optional second block,
> >     as I think that it outlived its usefulness; if we are to do so,
> >     these become the third and the fourth blocks.
> 
> If I interpreted your previous email correctly, I thought we were
> doing
> it!
> 
> I'll send a "typical" patch as a follow-up of this mail.
> 

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> The commit template adds the optional parts without
> a new line to distinguish them. This results in
> difficulty in interpreting it's content. Add new
> lines to separate the distinct parts of the template.
>
> The warning about usage of 'explicit paths without
> any corresponding options' has outlived it's purpose of
> warning users about the usage '--only' as default rather
> than '--include'. Remove it.
>
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  builtin/commit.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)

I think I can agree with both of the changes, but this commit does
two things that may be better done in two separate commits.  If I
were doing this patch, I probably would make [PATCH 1/2] about
removing the only_include_assumed (which is all except for the hunk
at -877,8) and then [PATCH 2/2] about giving a separating blank line
before the "git status" output.


> The warning about usage of 'explicit paths without
> any corresponding options' has outlived it's purpose of
> warning users about the usage '--only' as default rather
> than '--include'. Remove it.

With a bit more digging of the history:

    The notice that "git commit <paths>" default to "git commit
    --only <paths>" was there since 756e3ee0 ("Merge branch
    'jc/commit'", 2006-02-14).  Back then, existing users of Git
    expected the command doing "git commit --include <paths>", and
    after we changed the behaviour of the command to align with
    other people's "$scm commit <paths>", we added the text to help
    them transition their expectations.  Remove the message that now
    has outlived its usefulness.

perhaps.

Thanks.


> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8d1cac062..22d17e6f2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -139,7 +139,6 @@ static enum commit_whence whence;
>  static int sequencer_in_use;
>  static int use_editor = 1, include_status = 1;
>  static int show_ignored_in_status, have_option_m;
> -static const char *only_include_assumed;
>  static struct strbuf message = STRBUF_INIT;
>  
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
> @@ -841,9 +840,6 @@ 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)
> -			status_printf_ln(s, GIT_COLOR_NORMAL,
> -					"%s", only_include_assumed);
>  
>  		/*
>  		 * These should never fail because they come from our own
> @@ -877,8 +873,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 */
>  
>  		saved_color_setting = s->use_color;
>  		s->use_color = 0;
> @@ -1208,8 +1203,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
>  	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...");
>  	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
>  		cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
>  	else if (!strcmp(cleanup_arg, "verbatim"))

[PATCH 1/2] commit-template: remove outdated notice about explicit paths

[Kaartic Sivaraam]

The notice that "git commit <paths>" default to "git commit
--only <paths>" was there since 756e3ee0 ("Merge branch
'jc/commit'", 2006-02-14).  Back then, existing users of Git
expected the command doing "git commit --include <paths>", and
after we changed the behaviour of the command to align with
other people's "$scm commit <paths>", we added the text to help
them transition their expectations.  Remove the message that now
has outlived its usefulness.
---
 builtin/commit.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..64701c8f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -139,7 +139,6 @@ static enum commit_whence whence;
 static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
-static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -841,9 +840,6 @@ 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)
-			status_printf_ln(s, GIT_COLOR_NORMAL,
-					"%s", only_include_assumed);
 
 		/*
 		 * These should never fail because they come from our own
@@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	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...");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
 		cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
 	else if (!strcmp(cleanup_arg, "verbatim"))
-- 
2.11.0

[PATCH 2/2] commit-template: add new line before status information

[Kaartic Sivaraam]

The commit template adds the optional parts without
a new line to distinguish them. This results in
difficulty in interpreting it's content, specifically
for inexperienced users.

Add new lines to separate the distinct parts of the
template.
---
 I tried writing tests to ensure that the new line is added
 but as it seems to require checking multi-line, special 
 options of grep were required to check. I tried the following,

   test_expect_success 'new line found before status message' '
    ! (GIT_EDITOR="cat >editor-input" git commit) &&
    grep -Pz "#\n# On branch" editor-input
   '

 It worked well locally but seems to make the build with 
 GETTEXT_POISON=YesPlease to fail. So, I removed it.
 Not sure how to write a good test for this change, sorry :(

 builtin/commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 64701c8f4..22d17e6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -873,8 +873,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 */
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
-- 
2.11.0

Re: [PATCH 2/2] commit-template: add new line before status information

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> The commit template adds the optional parts without
> a new line to distinguish them. This results in
> difficulty in interpreting it's content, specifically
> for inexperienced users.
>
> Add new lines to separate the distinct parts of the
> template.
> ---
>  I tried writing tests to ensure that the new line is added
>  but as it seems to require checking multi-line, special 
>  options of grep were required to check. I tried the following,
>
>    test_expect_success 'new line found before status message' '
>     ! (GIT_EDITOR="cat >editor-input" git commit) &&
>     grep -Pz "#\n# On branch" editor-input
>    '
>
>  It worked well locally but seems to make the build with 
>  GETTEXT_POISON=YesPlease to fail. So, I removed it.
>  Not sure how to write a good test for this change, sorry :(

The above is a good way to capture the input to the editor, but the
test with "grep -P" which is not portable would not work well.  You
however should be able to prepare an expected output with

	cat >expect <<\-EOF &&
	... expected contents to editor-input here ...
	EOF

and do "test_i18ncmp expect editor-input" instead of "grep -P".

Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> The notice that "git commit <paths>" default to "git commit
> --only <paths>" was there since 756e3ee0 ("Merge branch
> 'jc/commit'", 2006-02-14).  Back then, existing users of Git
> expected the command doing "git commit --include <paths>", and
> after we changed the behaviour of the command to align with
> other people's "$scm commit <paths>", we added the text to help
> them transition their expectations.  Remove the message that now
> has outlived its usefulness.
> ---
>  builtin/commit.c | 6 ------
>  1 file changed, 6 deletions(-)

When I said "I would have ... if I were doing this", I merely meant
exactly that---as I weren't doing it, I left it up to you.  But you
did it the way anyways, which is very nice of you ;-).

Looks good.  Should we consider this signed-off by you?

Thanks.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8d1cac062..64701c8f4 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -139,7 +139,6 @@ static enum commit_whence whence;
>  static int sequencer_in_use;
>  static int use_editor = 1, include_status = 1;
>  static int show_ignored_in_status, have_option_m;
> -static const char *only_include_assumed;
>  static struct strbuf message = STRBUF_INIT;
>  
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
> @@ -841,9 +840,6 @@ 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)
> -			status_printf_ln(s, GIT_COLOR_NORMAL,
> -					"%s", only_include_assumed);
>  
>  		/*
>  		 * These should never fail because they come from our own
> @@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
>  	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...");
>  	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
>  		cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
>  	else if (!strcmp(cleanup_arg, "verbatim"))

Re: [PATCH 2/2] commit-template: add new line before status information

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> The commit template adds the optional parts without
> a new line to distinguish them. This results in
> difficulty in interpreting it's content, specifically
> for inexperienced users.
>
> Add new lines to separate the distinct parts of the
> template.

The rationale of this has changed in this final version, hasn't it,
especially with the removal of the "include/only warning" bit?

We used to add a blank line to separate the "we are committing for
somebody else", which is an optional part, from the status output,
only when we have the optional part.  Now with your change, we
unconditionally have a blank before the part that would have been
shown by "git status" to separate the two parts out.



> ---
>  I tried writing tests to ensure that the new line is added
>  but as it seems to require checking multi-line, special 
>  options of grep were required to check. I tried the following,
>
>    test_expect_success 'new line found before status message' '
>     ! (GIT_EDITOR="cat >editor-input" git commit) &&
>     grep -Pz "#\n# On branch" editor-input
>    '
>
>  It worked well locally but seems to make the build with 
>  GETTEXT_POISON=YesPlease to fail. So, I removed it.
>  Not sure how to write a good test for this change, sorry :(
>
>  builtin/commit.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 64701c8f4..22d17e6f2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -873,8 +873,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 */
>  
>  		saved_color_setting = s->use_color;
>  		s->use_color = 0;

Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths

[Kaartic Sivaraam]

On Thu, 2017-06-29 at 10:56 -0700, Junio C Hamano wrote:
> When I said "I would have ... if I were doing this", I merely meant
> exactly that---as I weren't doing it, I left it up to you.  But you
> did it the way anyways, which is very nice of you ;-).
> 
It made a *lot* of sense, that's why. :)

> Looks good.  Should we consider this signed-off by you?
> 
Yeah, forgot that. Will follow-up with a patch.

Re: [PATCH 2/2] commit-template: add new line before status information

[Kaartic Sivaraam]

On Thu, 2017-06-29 at 11:17 -0700, Junio C Hamano wrote:
> The rationale of this has changed in this final version, hasn't it,
> especially with the removal of the "include/only warning" bit?
> 
> We used to add a blank line to separate the "we are committing for
> somebody else", which is an optional part, from the status output,
> only when we have the optional part.  Now with your change, we
> unconditionally have a blank before the part that would have been
> shown by "git status" to separate the two parts out.
> 
Yes, modified the commit message accordingly hoping that's what you
meant.

[PATCH 1/2] commit-template: remove outdated notice about explicit paths

[Kaartic Sivaraam]

The notice that "git commit <paths>" default to "git commit
--only <paths>" was there since 756e3ee0 ("Merge branch
'jc/commit'", 2006-02-14).  Back then, existing users of Git
expected the command doing "git commit --include <paths>", and
after the behaviour of the command was changed to align with
other people's "$scm commit <paths>", the text was added to help
them transition their expectations.

Remove the message that now has outlived its usefulness.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Only commit message has been changed. Modified it a little
 to remove the self referencing words that didn't seem to suit
 well with the commit's author (that's just me :))
 
 builtin/commit.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..64701c8f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -139,7 +139,6 @@ static enum commit_whence whence;
 static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
-static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -841,9 +840,6 @@ 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)
-			status_printf_ln(s, GIT_COLOR_NORMAL,
-					"%s", only_include_assumed);
 
 		/*
 		 * These should never fail because they come from our own
@@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	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...");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
 		cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
 	else if (!strcmp(cleanup_arg, "verbatim"))
-- 
2.11.0

[PATCH 2/2] commit-template: distinguish status information unconditionally

[Kaartic Sivaraam]

The commit template adds the status information without
adding a new line to distinguish them in the absence
of optional parts. This results in difficulty in interpreting
it's content, specifically for inexperienced users.

Unconditionally, add new lines to separate the status message
from the other parts of the commit-template to make it more
readable.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 In trying to make the test independent of the previous test,
 it got a bit long. Hope it's not doing anything that shouldn't
 be done.

 In case you would wonder, the 'git reset' line is to remove 
 the files that were staged in previous test. I've done it in
 the same motive that lead to a bigger test.

 travis-ci builds passed

 builtin/commit.c  |  3 +--
 t/t7500-commit.sh | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 64701c8f4..22d17e6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -873,8 +873,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 */
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 116885a26..5739d3ed2 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -329,4 +329,27 @@ test_expect_success 'invalid message options when using --fixup' '
 	test_must_fail git commit --fixup HEAD~1 -F log
 '
 
+cat >expected-template <<EOF
+
+# Please enter the commit message for your changes. Lines starting
+# with '#' will be ignored, and an empty message aborts the commit.
+#
+# Author:    A U Thor <author@example.com>
+#
+# On branch commit-template-check
+# Changes to be committed:
+#	new file:   commit-template-check
+#
+# Untracked files not listed
+EOF
+
+test_expect_success 'new line found before status message in commit template' '
+	git checkout -b commit-template-check &&
+	git reset --hard HEAD &&
+	touch commit-template-check &&
+	git add commit-template-check &&
+	GIT_EDITOR="cat >editor-input" git commit --untracked-files=no --allow-empty-message &&
+	test_i18ncmp expected-template editor-input
+'
+
 test_done
-- 
2.11.0

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Junio C Hamano]

Thanks, both looks good.  Will queue.

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Kaartic Sivaraam]

On Fri, 2017-06-30 at 07:52 -0700, Junio C Hamano wrote:
> Thanks, both looks good.  Will queue.
You're welcome :)

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 01 2017, Kaartic Sivaraam jotted:

> On Fri, 2017-06-30 at 07:52 -0700, Junio C Hamano wrote:
>> Thanks, both looks good.Will queue.
> You're welcome :)

Just as someone reading this from the sidelines, very nice to have
someone working this part of the UI, but it would be much easier to
review if you included before/after examples of changes, e.g. (for this
hypothetical change):

    
    Before we'd say:
    
        # Please enter the commit message for your changes. Lines starting
        # with '#' will be ignored, and an empty message aborts the commit.
        #
        # Date:      <date>
        #
        # On branch master
        # Your branch is up-to-date with 'origin/master'.
    
    Now:
    
        # Please enter the commit message for your changes. Lines starting
        # with '#' will be ignored, and an empty message aborts the commit.
        #
        # Date:      <date>
        #
        # On branch master
        # Your current branch is up-to-date with 'origin/master'.
    
    And as a word-diff:
    
        [...]
        # Your {+current+} branch is up-to-date with 'origin/master'.

Or something like that, much easier to read something like that than
read the code and mentally glue together what it's going to change.

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Kaartic Sivaraam]

On Sat, 2017-07-01 at 13:44 +0200, Ævar Arnfjörð Bjarmason wrote:
> Just as someone reading this from the sidelines, very nice to have
> someone working this part of the UI, but it would be much easier to
> review if you included before/after examples of changes

That's a good comment. Will try to follow that in future. :)

-- 
Regards,
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Junio C Hamano]

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

> Just as someone reading this from the sidelines, very nice to have
> someone working this part of the UI, but it would be much easier to
> review if you included before/after examples of changes, e.g. (for this
> hypothetical change):
> 
>     
>     Before we'd say:
>     
>         # Please enter the commit message for your changes. Lines starting
>         # with '#' will be ignored, and an empty message aborts the commit.
>         #
>         # Date:      <date>
>         #
>         # On branch master
>         # Your branch is up-to-date with 'origin/master'.
>     
>     Now:
>     
>         # Please enter the commit message for your changes. Lines starting
>         # with '#' will be ignored, and an empty message aborts the commit.
>         #
>         # Date:      <date>
>         #
>         # On branch master
>         # Your current branch is up-to-date with 'origin/master'.
>     
>     And as a word-diff:
>     
>         [...]
>         # Your {+current+} branch is up-to-date with 'origin/master'.
>
> Or something like that, much easier to read something like that than
> read the code and mentally glue together what it's going to change.

I think you gave an example that is different from what was done on
purpose, so that Kaartic can respond with "I see what you mean; what
I did is different from your example but this.", but it seems that
the attempt failed X-<.

I do not think the patch changes the output in a situation where the
above "Before" would be shown.  Instead, when the extra "Date: <date>"
(or "Author: <author>") is not shown, the Before picture would look
like:

         # Please enter the commit message for your changes. Lines starting
         # with '#' will be ignored, and an empty message aborts the commit.
         # On branch master

and the update makes it look like:

         # Please enter the commit message for your changes. Lines starting
         # with '#' will be ignored, and an empty message aborts the commit.
         #
         # On branch master

Hope that helps.

Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths

[Kaartic Sivaraam]

On Fri, 2017-06-30 at 17:42 +0530, Kaartic Sivaraam wrote:
> The notice that "git commit <paths>" default to "git commit
> --only <paths>" was there since 756e3ee0 ("Merge branch
> 'jc/commit'", 2006-02-14).  Back then, existing users of Git
> expected the command doing "git commit --include <paths>", and
> after the behaviour of the command was changed to align with
> other people's "$scm commit <paths>", the text was added to help
> them transition their expectations.
> 
> Remove the message that now has outlived its usefulness.
> 
It just recently dawned on me that the message,

    Explicit paths specified without -i or -o; assuming --only paths..

is translated and "git grep" shows it's presence in the files present
in the 'po' directory. What should be done to them? Should the
translators be notified? 

BTW what does the word 'po' stand for in the first place? I digged a
bit but couldn't find much from the log.

-- 
Kaartic

Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> It just recently dawned on me that the message,
>
>     Explicit paths specified without -i or -o; assuming --only paths..
>
> is translated and "git grep" shows it's presence in the files present
> in the 'po' directory. What should be done to them?

Leave them as they are.  They are catalogued by the messages in the
code and gives us the translated equivalent.  If you removed a message,
the only effect is that the corresponding message will not be asked
for translation.

Near the end of a development cycle, the internationalization
coordinator will update the master message catalog and he'll remove
it when notices that the message no longer is used (even if he fails
to spot the obsolete and unused message, no harm is done).

Thanks.