Re: [PATCH 1/1] commit: display advice hints when commit fails

Jonathan Tan [off-list ref] writes:

quoted

From: Heba Waly <redacted>

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

  # [...]
  # 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

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

quoted

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

quoted

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

quoted

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

Thanks for a careful review.