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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

>> 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:
>
> Wrap your commit messages at 72 characters.
>
>>   # [...]
>>   # 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:"?
>
>> 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 +++++++++
>
> I wondered if there was a better place to put the test, but I couldn't
> find one, so this is fine.
>
>> @@ -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.
>
>> +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.