commit: display advice hints when commit fails

t/t7500-commit-template-squash-signoff.sh +9 -0

index 46a5cd4b73..3d76e8ebbd 100755
@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
 	)
 '
 
+test_expect_success 'commit without staging files fails and displays hints' '
+	echo "initial" >>file &&
+	git add file &&
+	git commit -m initial &&
+	echo "changes" >>file &&
+	test_must_fail git commit -m initial >actual &&
+	test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'
+
 test_done

From: Heba Waly <[email protected]>

Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g:

# [...]
# Changes not staged for commit:
# modified: builtin/commit.c
#

  # no changes added to commit

to:

# [...]
# Changes not staged for commit:
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
# modified: ../builtin/commit.c
#

  # no changes added to commit (use "git add" and/or "git commit -a")

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly <[email protected]>

---
 builtin/commit.c                          | 1 +
 t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
 2 files changed, 10 insertions(+)

builtin/commit.c +0 -0

index 2db2ad0de4..4439666465 100644

builtin/commit.c +0 -0

 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)

t/t7500-commit-template-squash-signoff.sh +9 -0

index 46a5cd4b73..3d76e8ebbd 100755
@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
 	)
 '
 
+test_expect_success 'commit without staging files fails and displays hints' '
+	echo "initial" >>file &&
+	git add file &&
+	git commit -m initial &&
+	echo "changes" >>file &&
+	test_must_fail git commit -m initial >actual &&
+	test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'
+
 test_done

Signature · 1 line

gitgitgadget

Quoted 41 lines · Heba Waly via GitGitGadget

From: Heba Waly <[email protected]>

Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g:

# [...]
# Changes not staged for commit:
# modified: builtin/commit.c
#

  # no changes added to commit

to:

# [...]
# Changes not staged for commit:
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
# modified: ../builtin/commit.c
#

  # no changes added to commit (use "git add" and/or "git commit -a")

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly <[email protected]>

---
 builtin/commit.c                          | 1 +
 t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
 2 files changed, 10 insertions(+)

builtin/commit.c +0 -0

index 2db2ad0de4..4439666465 100644

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 964

Quoted 4 lines

builtin/commit.c +1 -0

@@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
       */
      if (!committable && whence != FROM_MERGE && !allow_empty &&
          !(amend && is_a_merge(current_head))) {
+             s->hints = advice_status_hints;

Hm. This looks like it turns hints back on specifically for this case, but might not fix other places where ea9882bfc4 turned them off.

I think the intent of that commit was to not put hints into the editor, so does it make sense to instead wrap this guy:

/*
* Most hints are counter-productive when the commit has
* already started.
*/
s->hints = 0;

in "if (use_editor)"?

I didn't try it on my end. Maybe it won't help much, because we think we're going to use the editor right up until we realize it's not committable?

I wonder which other cases that commit got rid of hints for by accident.

- Emily

Quoted 11 lines

builtin/commit.c +0 -0

              s->display_comment_prefix = old_display_comment_prefix;
              run_status(stdout, index_file, prefix, 0, s);
              if (amend)

t/t7500-commit-template-squash-signoff.sh +0 -0

index 46a5cd4b73..3d76e8ebbd 100755
@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
      )
 '
 

Quoted 11 lines

t/t7500-commit-template-squash-signoff.sh +9 -1

+test_expect_success 'commit without staging files fails and displays hints' '
+     echo "initial" >>file &&
+     git add file &&
+     git commit -m initial &&
+     echo "changes" >>file &&
+     test_must_fail git commit -m initial >actual &&
+     test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'
+
 test_done
--

Quoted 1 line

gitgitgadget

From: Heba Waly <[email protected]>

Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g:

# [...]
# Changes not staged for commit:
# modified: builtin/commit.c
#

  # no changes added to commit

to:

# [...]
# Changes not staged for commit:
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
# modified: ../builtin/commit.c

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly <[email protected]>

---
 builtin/commit.c                          | 1 +
 t/t7500-commit-template-squash-signoff.sh | 9 +++++++++

Quoted 5 lines

From: Heba Waly <[email protected]>

Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g:

Wrap your commit messages at 72 characters.

Quoted 6 lines

# [...]
# Changes not staged for commit:
# modified: builtin/commit.c
#

  # no changes added to commit

Quoted 8 lines

to:

# [...]
# Changes not staged for commit:
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
# modified: ../builtin/commit.c

For tidiness, can this line also be "builtin/commit.c" (that is, without the "../" at the beginning) to match what's before "to:"?

Quoted 11 lines

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly <[email protected]>

---
 builtin/commit.c                          | 1 +
 t/t7500-commit-template-squash-signoff.sh | 9 +++++++++

I wondered if there was a better place to put the test, but I couldn't find one, so this is fine.

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 967

Quoted 8 lines

builtin/commit.c +1 -0

@@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
+		s->hints = advice_status_hints;
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)

I checked that this undoing of "s->hints = 0" is safe, because s is no longer used in this function nor in the calling function cmd_commit() (which is the one that declared s locally).

Still probably worth a comment, though. For example:

This status is to be printed to stdout, so hints will be useful to the user. Reset s->hints to what the user configured.

The corresponding comment on "s->hints = 0" might need to be tweaked, too, but I can't think of anything at the moment.

t/t7500-commit-template-squash-signoff.sh test_expect_success 'check commit with unstaged rename and copy' ' · line 390

Quoted 6 lines

t/t7500-commit-template-squash-signoff.sh +6 -0

+test_expect_success 'commit without staging files fails and displays hints' '
+	echo "initial" >>file &&
+	git add file &&
+	git commit -m initial &&
+	echo "changes" >>file &&
+	test_must_fail git commit -m initial >actual &&

Use another commit message for this, since this is no longer "initial". (Maybe "after initial" or something like that.)

Quoted 6 lines

From: Heba Waly <[email protected]>

Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g:

Wrap your commit messages at 72 characters.

Quoted 6 lines

# [...]
# Changes not staged for commit:
# modified: builtin/commit.c
#

  # no changes added to commit

Quoted 8 lines

to:

# [...]
# Changes not staged for commit:
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
# modified: ../builtin/commit.c

For tidiness, can this line also be "builtin/commit.c" (that is, without the "../" at the beginning) to match what's before "to:"?

Quoted 11 lines

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly <[email protected]>

---
 builtin/commit.c                          | 1 +
 t/t7500-commit-template-squash-signoff.sh | 9 +++++++++

I wondered if there was a better place to put the test, but I couldn't find one, so this is fine.

builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 967

Quoted 8 lines

builtin/commit.c +1 -0

@@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
       */
      if (!committable && whence != FROM_MERGE && !allow_empty &&
          !(amend && is_a_merge(current_head))) {
+             s->hints = advice_status_hints;
              s->display_comment_prefix = old_display_comment_prefix;
              run_status(stdout, index_file, prefix, 0, s);
              if (amend)

I checked that this undoing of "s->hints = 0" is safe, because s is no longer used in this function nor in the calling function cmd_commit() (which is the one that declared s locally).

Still probably worth a comment, though. For example:

This status is to be printed to stdout, so hints will be useful to the user. Reset s->hints to what the user configured.

The corresponding comment on "s->hints = 0" might need to be tweaked, too, but I can't think of anything at the moment.

t/t7500-commit-template-squash-signoff.sh test_expect_success 'check commit with unstaged rename and copy' ' · line 390

Quoted 6 lines

t/t7500-commit-template-squash-signoff.sh +6 -0

+test_expect_success 'commit without staging files fails and displays hints' '
+     echo "initial" >>file &&
+     git add file &&
+     git commit -m initial &&
+     echo "changes" >>file &&
+     test_must_fail git commit -m initial >actual &&

Use another commit message for this, since this is no longer "initial". (Maybe "after initial" or something like that.)

builtin/commit.c +0 -0

index e48c1fd90a..868c0d7819 100644

This fix was about "we do not want to unconditionally drop the advice messages when we reject the attempt to commit and show the output like 'git status'", wasn't it? The earlier single-liner fix in v1 that flips s->hints just before calling run_status() before rejecting the attempt to commit was a lot easier to reason about, as the fix was very focused and to the point. Why are we seeing this many (seemingly unrelated) changes?

t/t7500-commit-template-squash-signoff.sh +6 -0

+       git add file &&
+       git commit -m initial &&
+       echo "changes" >>file &&
+       test_must_fail git commit -m update >actual &&
+       test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'

t/t7500-commit-template-squash-signoff.sh test_expect_success 'check commit with unstaged rename and copy' ' · line 386

Quoted 5 lines · Junio C Hamano

In any case, here is what I tentatively have in my tree (with heavy rewrite to the proposed log message).

t/t7500-commit-template-squash-signoff.sh +2 -0

+test_expect_success 'commit without staging files fails and displays hints' '
+       echo "initial" >>file &&

The use of '>>' here rather than '>' feels wrong, especially when "initial" is used for both the file body and the commit message, causing a reader of the test to wonder if this test somehow depends upon earlier tests.

Quoted 7 lines

t/t7500-commit-template-squash-signoff.sh +6 -0

+       git add file &&
+       git commit -m initial &&
+       echo "changes" >>file &&
+       test_must_fail git commit -m update >actual &&
+       test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'

Quoted 7 lines · Junio C Hamano

This fix was about "we do not want to unconditionally drop the advice messages when we reject the attempt to commit and show the output like 'git status'", wasn't it? The earlier single-liner fix in v1 that flips s->hints just before calling run_status() before rejecting the attempt to commit was a lot easier to reason about, as the fix was very focused and to the point. Why are we seeing this many (seemingly unrelated) changes?

In any case, here is what I tentatively have in my tree (with heavy rewrite to the proposed log message).

-- >8 --
From: Heba Waly <[email protected]>
Date: Tue, 17 Dec 2019 09:17:22 +0000
Subject: [PATCH] commit: honor advice.statusHints when rejecting an empty
commit

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, because giving the hints in the "git status" like output in the commit message template are too late to be useful (they say things like "'git add' to stage", but that is only possible after aborting the current "git commit" session).

But there is one case that the hints can be useful: When the current attempt to commit is rejected because no change is recorded in the index. The message is given and "git commit" errors out, so the hints can immediately be followed by the user. Teach the codepath to honor the configuration variable.

Signed-off-by Heba Waly <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/commit.c                          | 1 +
 t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
 2 files changed, 10 insertions(+)

builtin/commit.c +1 -0

index e588bc6ad3..0078faf117 100644
@@ -944,6 +944,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
+		s->hints = advice_status_hints;
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)

t/t7500-commit-template-squash-signoff.sh +9 -0

index 46a5cd4b73..a8179e4074 100755
@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
 	)
 '
 
+test_expect_success 'commit without staging files fails and displays hints' '
+	echo "initial" >>file &&
+	git add file &&
+	git commit -m initial &&
+	echo "changes" >>file &&
+	test_must_fail git commit -m update >actual &&
+	test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'
+
 test_done

Signature · 1 line

2.24.1-732-ga9f9d4909c

Hm. I'm surprised to see this feedback come in the form of a local change when making the topic branch, rather than in a reply to the v1 patch. What's the reasoning? (Or is this scissors patch intended to be the feedback?)

I ask because out of all of us, it seems the Outreachy interns can benefit the most from advice on how and why to write their commit messages - that is, part of the point of an internship is to learn best practices and cultural norms in addition to coding practice. (Plus, I find being asked to rewrite a commit message tends to force me to understand my own change even better than before.)

I'll go ahead and look through the changes to the commit message so I can learn what you're looking for too :)

For me, I don't particularly see why we'd want to be rid of it - it sort of feels like "a picture is worth a thousand words" to include the actual use case in the commit message.

Quoted 4 lines · Emily Shaffer

Hm. I'm surprised to see this feedback come in the form of a local change when making the topic branch, rather than in a reply to the v1 patch. What's the reasoning? (Or is this scissors patch intended to be the feedback?)

You haven't seen a suggestion in the form of counter-proposal?

Quoted 7 lines

I ask because out of all of us, it seems the Outreachy interns can benefit the most from advice on how and why to write their commit messages - that is, part of the point of an internship is to learn best practices and cultural norms in addition to coding practice. (Plus, I find being asked to rewrite a commit message tends to force me to understand my own change even better than before.)

It's something Mentors can help doing (I do not necessarily have time for that myself), and you're welcome to use the "tenatively queued" version as an example.

Quoted 2 lines

I'll go ahead and look through the changes to the commit message so I can learn what you're looking for too :)

Nice.

One thing you missed in your review of the "tentatively queued" version is the reversal of the order of presentation. Instead of starting with "I decided to do this" without explanation, give the picture of status quo to set the stage, explain what issue exists in the current behaviour, and then describe what approach was chosen to solve the issue.

Quoted 4 lines

For me, I don't particularly see why we'd want to be rid of it - it sort of feels like "a picture is worth a thousand words" to include the actual use case in the commit message.

Output coming from commands and/or options that are used only in a bit more advanced workflow and the ones that are rarely seen, I do agree that showing example is a good way to illustrate exactly what you are talking about.

On the other hand, for behaviour of basic local commands like "git add", "git commit", "git diff", ..., I do not necessarily agree, as these should be obvious and clear to all the intended audiences, which would be "anybody who has used Git for say more than two weeks.

Quoted 7 lines

This fix was about "we do not want to unconditionally drop the advice messages when we reject the attempt to commit and show the output like 'git status'", wasn't it? The earlier single-liner fix in v1 that flips s->hints just before calling run_status() before rejecting the attempt to commit was a lot easier to reason about, as the fix was very focused and to the point. Why are we seeing this many (seemingly unrelated) changes?

In any case, here is what I tentatively have in my tree (with heavy rewrite to the proposed log message).

I think the intent of that commit was to not put hints into the editor, so does it make sense to instead wrap this guy:

/*
* Most hints are counter-productive when the commit has
* already started.
*/
s->hints = 0;

in "if (use_editor)"?

I didn't try it on my end. Maybe it won't help much, because we think we're going to use the editor right up until we realize it's not committable?

Quoted 2 lines

In any case, here is what I tentatively have in my tree (with heavy rewrite to the proposed log message).

Junio, what are your plans over what you have in your tree? If you'd like to hear Heba's opinion on it, then she can chime in; if you'd like a review, then I think it's good to go in.

Quoted 3 lines

I didn't try it on my end. Maybe it won't help much, because we think we're going to use the editor right up until we realize it's not committable?

And I think the answer to that is "s" is used throughout the function in various ways (in particular, used to print statuses both to stdout and to the message template) so any wrapping or corralling of scope would just make things more complicated. In particular, the way Heba did it in v2 is more unclear - at the time of setting s->hints = 0, it's done

within a "if (use_editor && include_status)" block, but (as far as I can tell) the commit message template might also be used when there is no editor - for example, as input to a hook. And more importantly, when s->hints is reset to the config, we don't know at that point that the next status is going to stdout. So I think it's better just to use the v1 way.

The second area of discussion I see is in the commit message. Commit messages have to balance brevity and comprehensiveness, and this can be a subjective matter, but I think Junio's strikes a good balance.

Quoted 3 lines

Junio, what are your plans over what you have in your tree? If you'd like to hear Heba's opinion on it, then she can chime in; if you'd like a review, then I think it's good to go in.

On hold until anything like those happens ;-)

A random reviewer mentioning something on a patch (either in a line-by-line critique form or "how about doing it this way instead" counterproposal form) without getting followed up by others (including the original author) is a stall review thread, and it does not change the equation if the random reviewer happens to be me.

Quoted 5 lines

And I think the answer to that is "s" is used throughout the function in various ways (in particular, used to print statuses both to stdout and to the message template) so any wrapping or corralling of scope would just make things more complicated. In particular, the way Heba did it in v2 is more unclear - at the time of setting s->hints = 0, it's done

You mean "less clear" (just double checking if I got the negation right)?

I think I've merged it to 'next' yesterday, but it does not mean that much as we are in -rc and it is not such an urgent "oops we broke it in this cycle, let's fix it" issue. If we see a v3 that improves it, I do not mind at all reverting what I merged to 'next' and use the updated one instead (either way, it will be in 'master' during the next cycle at the earliest).

@@ -19,16 +19,16 @@
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
- # modified: ../builtin/commit.c
+ # modified: /builtin/commit.c

Quoted 6 lines · Heba Waly via GitGitGadget

@@ -19,16 +19,16 @@
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
- # modified: ../builtin/commit.c
+ # modified: /builtin/commit.c

Really?

Quoted 10 lines · Junio C Hamano

Quoted 6 lines · Heba Waly via GitGitGadget

@@ -19,16 +19,16 @@
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
- # modified: ../builtin/commit.c
+ # modified: /builtin/commit.c

Really?

It's hard to know what this cryptic comment means.. :)

This was a recommended change:
https://lore.kernel.org/git/[email protected]

Since other changes were being made at the same time, I personally don't mind a little nit fix in the commit message.

Or, do you mean that "now it looks like the file is at the filesystem root, which is wrong"? It is indeed wrong now when it wasn't before. But I, for one, can't tell what you mean by just the one word.

Quoted 20 lines · Emily Shaffer

Quoted 10 lines · Junio C Hamano

Quoted 6 lines · Heba Waly via GitGitGadget

@@ -19,16 +19,16 @@
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
- # modified: ../builtin/commit.c
+ # modified: /builtin/commit.c

Really?

It's hard to know what this cryptic comment means.. :)

This was a recommended change:
https://lore.kernel.org/git/[email protected]

Since other changes were being made at the same time, I personally don't mind a little nit fix in the commit message.

Quoted 3 lines

Or, do you mean that "now it looks like the file is at the filesystem root, which is wrong"? It is indeed wrong now when it wasn't before. But I, for one, can't tell what you mean by just the one word.

That is exactly the point. I am not in the business of spoon feeding answers. I want my contributors to *think*.

That is exactly the point. I am not in the business of spoon feeding answers.

I want my contributors to *think*.

Any contributor working on this topic should be competent enough to realize/notice it once it is pointed out---even though lack of proof-reading before sending may cause such a mistake by carelessness.

Quoted 4 lines

Any contributor working on this topic should be competent enough to realize/notice it once it is pointed out---even though lack of proof-reading before sending may cause such a mistake by carelessness.

I have no reason not to believe that this is crossing the line. No matter how much a reviewer disagrees with the proposed changes, I'd appreciate
keeping a mutually respectful discussion during the review process.

Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g:

. [...] . Changes not staged for commit: . modified: builtin/commit.c . .
no changes added to commit

to:

. [...] . Changes not staged for commit: . (use "git add ..." to update
what will be committed) . (use "git checkout -- ..." to discard changes in working directory) . . modified: ../builtin/commit.c . . no changes added to commit (use "git add" and/or "git commit -a")

In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly [email protected] [[email protected]]

Heba Waly (1):
commit: display advice hints when commit fails

 builtin/commit.c                          | 18 ++++++++++++------
 t/t7500-commit-template-squash-signoff.sh |  9 +++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

base-commit: 12029dc57db23baef008e77db1909367599210ee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-495%2FHebaWaly%2Fhints-for-unsuccessful-commit-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-495/HebaWaly/hints-for-unsuccessful-commit-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/495

Range-diff vs v1:

1: f23477c5a3 ! 1: ebec237920 commit: display advice hints when commit fails
@@ -2,9 +2,9 @@

commit: display advice hints when commit fails

- Display hints to the user when trying to commit without staging the modified
- files first (when advice.statusHints is set to true). Change the output of the
- unsuccessful commit from e.g:
+ Display hints to the user when trying to commit without staging the
+ modified files first (when advice.statusHints is set to true). Change
+ the output of the unsuccessful commit from e.g:

# [...]
# Changes not staged for commit:
@@ -19,16 +19,16 @@
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working directory)
#
- # modified: ../builtin/commit.c
+ # modified: /builtin/commit.c
#
# no changes added to commit (use "git add" and/or "git commit -a")

- In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,
- 2013-09-12) the intent was to disable status hints when writing to
- COMMIT_EDITMSG, but in fact the implementation disabled status messages in
- more locations, e.g in case the commit wasn't successful, status hints
- will still be disabled and no hints will be displayed to the user although
- advice.statusHints is set to true.
+ In ea9882bfc4 (commit: disable status hints when writing to
+ COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when
+ writing to COMMIT_EDITMSG, but in fact the implementation disabled
+ status messages in more locations, e.g in case the commit wasn't
+ successful, status hints will still be disabled and no hints will be
+ displayed to the user although advice.statusHints is set to true.

Signed-off-by Heba Waly <[email protected]>

@@ -36,13 +36,44 @@
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@
- */
- if (!committable && whence != FROM_MERGE && !allow_empty &&
- !(amend && is_a_merge(current_head))) {
-+ s->hints = advice_status_hints;
- s->display_comment_prefix = old_display_comment_prefix;
- run_status(stdout, index_file, prefix, 0, s);
- if (amend)
+ old_display_comment_prefix = s->display_comment_prefix;
+ s->display_comment_prefix = 1;
+
+- /*
+- * Most hints are counter-productive when the commit has +- * already started.
+- */
+- s->hints = 0;
+-
+ if (clean_message_contents)
+ strbuf_stripspace(&sb, 0);
+
+@@
+ int saved_color_setting;
+ struct ident_split ci, ai;
+
++ /*
++ * Most hints are counter-productive when displayed in
++ * the commit message editor.
++ */
++ s->hints = 0;
++
+ if (whence != FROM_COMMIT) {
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+ !merge_contains_scissors)
+@@
+ saved_color_setting = s->use_color;
+ s->use_color = 0;
+ committable = run_status(s->fp, index_file, prefix, 1, s);
++ if(!committable)
++ /*
++ Status is to be printed to stdout, so hints will be useful to the
++ user. Reset s->hints to what the user configured
++ */
++ s->hints = advice_status_hints;
+ s->use_color = saved_color_setting;
+ string_list_clear(&s->change, 1);
+ } else {

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh --- a/t/t7500-commit-template-squash-signoff.sh
@@ -56,7 +87,7 @@
+ git add file &&
+ git commit -m initial &&
+ echo "changes" >>file &&
-+ test_must_fail git commit -m initial >actual &&
++ test_must_fail git commit -m update >actual &&
+ test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual +'
+

--
gitgitgadget