Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

Hi Jialun,

On 2021-07-08 00:23:07+0800, Hu Jialun <hujialun@comp.nus.edu.sg> wrote:
> While the prefilled commit prompt is mostly the same for different
> cleanup modes, those are separately repeated, which violates the DRY
> principle and hinders maintainability.
> 
> Unify and reorder identical substrings to improve.
> 
> Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg>
> ---
>  builtin/commit.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43..815b408002 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -910,21 +910,24 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		}
>  
>  		fprintf(s->fp, "\n");
> -		if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)
> -			status_printf(s, GIT_COLOR_NORMAL,
> -				_("Please enter the commit message for your changes."
> -				  " Lines starting\nwith '%c' will be ignored, and an empty"
> -				  " message aborts the commit.\n"), comment_line_char);
> +		const char *msg_enter_prompt = _("Please enter the commit message for your changes.");
> +		const char *keep_char_prompt = _("Lines starting with '%c' will be kept;"
> +						 " you may remove them yourself if you want to.");
> +		const char *ignore_char_prompt = _("Lines starting with '%c' will be ignored.");
> +		const char *empty_msg_abort_prompt = _("An empty message aborts the commit.");

In Git project, it's enforced to have -Wdeclaration-after-statement,
IOW, move all declaration before statement.



> +		if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {
> +			status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);

builtin/commit.c:919:4: error: format not a string literal and no format arguments [-Werror=format-security]
  919 |    status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);

msg_enter_prompt will come from translator and may have '%' inside it.

We can solve it by inserting "%s" there.

However, I think we shouldn't take this route, because splitting likes this
will make a translation lego. I can't speak for Junio, but from my
observation, it's preferred to have 3 variables for 3 full-text, and
we will pick the suitable text in each if-leg.

> +			status_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);
> +			status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
> +		}
>  		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
>  			if (whence == FROM_COMMIT && !merge_contains_scissors)
>  				wt_status_add_cut_line(s->fp);
> -		} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> -			status_printf(s, GIT_COLOR_NORMAL,
> -				_("Please enter the commit message for your changes."
> -				  " Lines starting\n"
> -				  "with '%c' will be kept; you may remove them"
> -				  " yourself if you want to.\n"
> -				  "An empty message aborts the commit.\n"), comment_line_char);
> +		} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> +			status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
> +			status_printf_ln(s, GIT_COLOR_NORMAL, keep_char_prompt, comment_line_char);
> +			status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
> +		}

After changing those texts, the tests should be updated, too.
It's a customary service for the next developer, who needs to bisect
this project to have all test-cases pass on each changes.

With this change, t7500.50 and t7502.37 runs into failures.
Please fix them here, instead of next change.
>  
>  		/*
>  		 * These should never fail because they come from our own
> -- 
> 2.32.0
> 

-- 
Danh