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

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
> 
> 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 <heba.waly@gmail.com>
> ---
>  builtin/commit.c                          | 1 +
>  t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2db2ad0de4..4439666465 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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

>  		s->display_comment_prefix = old_display_comment_prefix;
>  		run_status(stdout, index_file, prefix, 0, s);
>  		if (amend)
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 46a5cd4b73..3d76e8ebbd 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -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
> -- 
> gitgitgadget