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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2019, 19:14 UTC
Message-ID: <xmqqfthgb01m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <ebec2379207681152c6e5196a1418aca03da113a.1576746982.git.gitgitgadget@gmail.com>

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

Show 14 quoted lines

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

Show 10 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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