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

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

"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index e48c1fd90a..868c0d7819 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -811,12 +811,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	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;
> -

Hmm.

> @@ -837,6 +831,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		int saved_color_setting;
>  		struct ident_split ci, ai;
>  
> +		/*
> +		 * Most hints are counter-productive when displayed in
> +		 * the commit message editor.
> +		 */
> +		s->hints = 0;
> +

We no longer drop s->hints when we are not using editor and not
including status (i.e. the "else" side) because these lines are
moved inside "if".  As this change is not about that "no editor"
side, I am not 100% convinced that this is a good change.

> @@ -912,6 +912,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		saved_color_setting = s->use_color;
>  		s->use_color = 0;
>  		committable = run_status(s->fp, index_file, prefix, 1, s);
> +		if(!committable)

Style: SP between "if" and "(".

> +			/*
> +			 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;

The "if" side has been changed to flip s->hints to the configured
advice hints value when !committable here.  The "else" side
(i.e. when we are not using editor and not including status) does
not do anything to s->hints after finding out if committable after
this change.  Because "s->hints = 0" was moved to "if" with this
patch, the "else" side no longer drops s->hints at all.

So the final run_status() called when the attempt to commit is
rejected will feed s->hints that is not cleared with this change.

Is that intended?  Is the updated behaviour checked with a test?

>  		s->use_color = saved_color_setting;
>  		string_list_clear(&s->change, 1);
>  	} else {

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?

Puzzled...